Re: [XEN PATCH 4/4] xen: address violations of MISRA C:2012 Rule 13.1

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

On 19/10/23 03:06, Stefano Stabellini wrote:
> On Wed, 18 Oct 2023, Simone Ballarin wrote:
> > Rule 13.1: Initializer lists shall not contain persistent side effects
> >
> > This patch moves expressions with side-effects outside the initializer lists.
> >
> > No functional changes.
> >
> > Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
> > ---
> >   xen/common/sched/core.c    | 16 ++++++++++++----
> >   xen/drivers/char/ns16550.c |  4 +++-
> >   2 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > index 12deefa745..519884f989 100644
> > --- a/xen/common/sched/core.c
> > +++ b/xen/common/sched/core.c
> > @@ -1504,6 +1504,8 @@ long vcpu_yield(void)
> >   {
> >       struct vcpu * v=current;
> >       spinlock_t *lock;
> > +    domid_t domain_id;
> > +    int vcpu_id;
> >   
> >       rcu_read_lock(&sched_res_rculock);
> >   
> > @@ -1515,7 +1517,9 @@ long vcpu_yield(void)
> >   
> >       SCHED_STAT_CRANK(vcpu_yield);
> >   
> > -    TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
> > +    domain_id = current->domain->domain_id;
> > +    vcpu_id = current->vcpu_id;
> > +    TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id);
> Also here it looks like accessing the current pointer is considered a
> side effect. Why? This is a just a global variable that is only accessed
> for reading.
>
>
> >       raise_softirq(SCHEDULE_SOFTIRQ);
> >       return 0;
> >   }
> > @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> > arg)
> >       case SCHEDOP_shutdown:
> >       {
> >           struct sched_shutdown sched_shutdown;
> > +        domid_t domain_id;
> > +        int vcpu_id;
> >   
> >           ret = -EFAULT;
> >           if ( copy_from_guest(&sched_shutdown, arg, 1) )
> >               break;
> >   
> > +        domain_id = current->domain->domain_id;
> > +        vcpu_id = current->vcpu_id;
> >           TRACE_3D(TRC_SCHED_SHUTDOWN,
> > -                 current->domain->domain_id, current->vcpu_id,
> > -                 sched_shutdown.reason);
> > +                 domain_id, vcpu_id, sched_shutdown.reason);
> >           ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason);
> >   
> >           break;
> > @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> > arg)
> >       {
> >           struct sched_shutdown sched_shutdown;
> >           struct domain *d = current->domain;
> > +        int vcpu_id = current->vcpu_id;
> >   
> >           ret = -EFAULT;
> >           if ( copy_from_guest(&sched_shutdown, arg, 1) )
> >               break;
> >   
> >           TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
> > -                 d->domain_id, current->vcpu_id, sched_shutdown.reason);
> > +                 d->domain_id, vcpu_id, sched_shutdown.reason);
> >   
> >           spin_lock(&d->shutdown_lock);
> >           if ( d->shutdown_code == SHUTDOWN_CODE_INVALID )
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > index 28ddedd50d..0b3d8b2a30 100644
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -445,10 +445,12 @@ static void __init cf_check ns16550_init_postirq(struct 
> > serial_port *port)
> >               struct msi_info msi = {
> >                   .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> >                                    uart->ps_bdf[2]),
> > -                .irq = rc = uart->irq,
> > +                .irq = uart->irq,
> >                   .entry_nr = 1
> >               };
> >   
> > +            rc = uart->irq;
> What's the side effect here? If the side effect is rc = uart->irq, why
> is it considered a side effect?
Yes, rc = uart->irq is the side-effect: it writes a variable
declared outside the context of the expression.

Consider the following example:

int rc;

struct S s = {
  .x = rc = 2,
  .y = rc = 3
};

What's the value of rc?


--
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)