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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 21 Jul 2023, 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)

nit: code style, now the line is over 80 chars, could be fixed on commit


>  {
>      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();
> @@ -2040,8 +2040,8 @@ long do_set_timer_op(s_time_t timeout)
>      return 0;
>  }
>  
> -/* sched_id - fetch ID of current scheduler */
> -int sched_id(void)
> +/* scheduler_id - fetch ID of current scheduler */
> +int scheduler_id(void)
>  {
>      return ops.sched_id;
>  }
> @@ -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;

We don't typically use "exec" especially in the context of softirqs.
I would just change it to "softirq".


>      unsigned int          cpu = smp_processor_id();
>  
>      ASSERT_NOT_IN_ATOMIC();
> @@ -2604,7 +2604,7 @@ static void cf_check sched_slave(void)
>              return;
>          }
>  
> -        do_softirq = true;
> +        exec_softirq = true;
>      }
>  
>      if ( !prev->rendezvous_in_cnt )
> @@ -2614,7 +2614,7 @@ static void cf_check sched_slave(void)
>          rcu_read_unlock(&sched_res_rculock);
>  
>          /* Check for failed forced context switch. */
> -        if ( do_softirq )
> +        if ( exec_softirq )
>              raise_softirq(SCHEDULE_SOFTIRQ);
>  
>          return;
> diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
> index 87a1e31ee9..aba51a7963 100644
> --- 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;

Nice catch! This is almost a bug fix.


>          /* We need the lock to scan the runqueue. */
>          spin_lock(&rqd->lock);
> @@ -3901,7 +3901,7 @@ csched2_dump(const struct scheduler *ops)
>  
>              if ( svc )
>              {
> -                printk("\t%3d: ", loop++);
> +                printk("\t%3d: ", it++);
>                  csched2_dump_unit(prv, svc);
>              }
>          }
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 0cbfe8bd44..7cabfb0230 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -71,7 +71,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>          break;
>  
>      case XEN_SYSCTL_sched_id:
> -        op->u.sched_id.sched_id = sched_id();
> +        op->u.sched_id.sched_id = scheduler_id();

I am confused about this one. There is no global variable or no other
global function named "sched_id". Why do we need to rename sched_id to
scheduler_id?


>          break;
>  
>      case XEN_SYSCTL_getdomaininfolist:
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 854f3e32c0..bfe714d2e2 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -791,7 +791,7 @@ int  sched_init_domain(struct domain *d, unsigned int 
> poolid);
>  void sched_destroy_domain(struct domain *d);
>  long sched_adjust(struct domain *, struct xen_domctl_scheduler_op *);
>  long sched_adjust_global(struct xen_sysctl_scheduler_op *);
> -int  sched_id(void);
> +int  scheduler_id(void);
>  
>  /*
>   * sched_get_id_by_name - retrieves a scheduler id given a scheduler name
> -- 
> 2.34.1
>