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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 22/07/23 01:06, Stefano Stabellini wrote:
> 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
Ok


> > -/* 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".
Ok

> >       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?
sched_id is also a common parameter name used by functions declared 
where the declaration of function 'sched_id' is also visible, so it's 
entirely equivalent whether to change parameter names or the function 
identifier, but since there's just one usage of the function (in 
'xen/common/sysctl.c') I preferred to make the minimal change in the 
patch. If you prefer this done the other way around, no problem.
> >           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
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)