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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?