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

From: Simone Ballarin <simone.ballarin@bugseng.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org, consulting@bugseng.com,
	George Dunlap <george.dunlap@citrix.com>,
	Dario Faggioli <dfaggioli@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jan Beulich <jbeulich@suse.com>, Julien Grall <julien@xen.org>,
	Wei Liu <wl@xen.org>
Subject: Re: [XEN PATCH 4/4] xen: address violations of MISRA C:2012 Rule 13.1
Date: Thu, 19 Oct 2023 11:18:16 +0200	[thread overview]
Message-ID: <66049700-c16b-4086-b14f-154285f3ebf0@bugseng.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2310181803351.965337@ubuntu-linux-20-04-desktop>

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@bugseng.com>
>> ---
>>   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)