Re: [XEN PATCH] xen/sched: mechanical renaming to address MISRA C:2012 Rule 5.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.07.2023 17:31, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
> 
> The renaming s/sched_id/scheduler_id of the function defined in
> 'xen/common/sched/core.c' prevents any hiding of that function
> by the many instances of omonymous function parameters.
> 
> Similarly, the renames
> - s/ops/operations
> - s/do_softirq/exec_softirq
> - s/loop/it
> are introduced for parameter names, to avoid any conflict
> with the homonymous variable or function defined in an enclosing
> scope.
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
>  xen/common/sched/core.c    | 18 +++++++++---------
>  xen/common/sched/credit2.c |  4 ++--
>  xen/common/sysctl.c        |  2 +-
>  xen/include/xen/sched.h    |  2 +-
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 022f548652..e74b1208bd 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -99,13 +99,13 @@ static void sched_set_affinity(
>      struct sched_unit *unit, const cpumask_t *hard, const cpumask_t *soft);
>  
>  static struct sched_resource *cf_check
> -sched_idle_res_pick(const struct scheduler *ops, const struct sched_unit 
> *unit)
> +sched_idle_res_pick(const struct scheduler *operations, const struct 
> sched_unit *unit)
>  {
>      return unit->res;
>  }
>  
>  static void *cf_check
> -sched_idle_alloc_udata(const struct scheduler *ops, struct sched_unit *unit,
> +sched_idle_alloc_udata(const struct scheduler *operations, struct sched_unit 
> *unit,
>                         void *dd)
>  {
>      /* Any non-NULL pointer is fine here. */
> @@ -113,12 +113,12 @@ sched_idle_alloc_udata(const struct scheduler *ops, 
> struct sched_unit *unit,
>  }
>  
>  static void cf_check
> -sched_idle_free_udata(const struct scheduler *ops, void *priv)
> +sched_idle_free_udata(const struct scheduler *operations, void *priv)
>  {
>  }
>  
>  static void cf_check sched_idle_schedule(
> -    const struct scheduler *ops, struct sched_unit *unit, s_time_t now,
> +    const struct scheduler *operations, struct sched_unit *unit, s_time_t 
> now,
>      bool tasklet_work_scheduled)
>  {
>      const unsigned int cpu = smp_processor_id();

These renames bring the idle scheduler out of sync with all others. I
think it wants considering to rename the file scope variable instead.

> @@ -2579,7 +2579,7 @@ static void cf_check sched_slave(void)
>      struct sched_unit    *prev = vprev->sched_unit, *next;
>      s_time_t              now;
>      spinlock_t           *lock;
> -    bool                  do_softirq = false;
> +    bool                  exec_softirq = false;

As an alternative to Stefano's suggestion, maybe consider "need_softirq"?

> --- a/xen/common/sched/credit2.c
> +++ b/xen/common/sched/credit2.c
> @@ -3884,7 +3884,7 @@ csched2_dump(const struct scheduler *ops)
>      list_for_each_entry ( rqd, &prv->rql, rql )
>      {
>          struct list_head *iter, *runq = &rqd->runq;
> -        int loop = 0;
> +        int it = 0;

Instead of renaming, why can't we just drop this second variable, re-using
the outer scope one here (and at the same time doing away with a not really
appropriate use of plain "int")? (This may then want accompanying by ...

> @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
>  
>              if ( svc )
>              {
> -                printk("\t%3d: ", loop++);
> +                printk("\t%3d: ", it++);

... switching to %3u here.)

Jan