Rule 13.1: Initializer lists shall not contain persistent side effects

The assignment operation in:

.irq = rc = uart->irq,

is a persistent side effect in a struct initializer list.

This patch avoids rc assignment and directly uses uart->irq
in the following if statement.

No functional changes.

Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- avoid assignment of rc;
- drop changes in vcpu_yield(void).
---
 xen/drivers/char/ns16550.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index ddf2a48be6..644a3192bb 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -445,11 +445,13 @@ 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
             };
 
-            if ( rc > 0 )
+            rc = 0;
+
+            if ( uart->irq > 0 )
             {
                 struct msi_desc *msi_desc = NULL;
 
-- 
2.34.1

On 24.11.2023 18:29, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
> 
> The assignment operation in:
> 
> .irq = rc = uart->irq,
> 
> is a persistent side effect in a struct initializer list.
> 
> This patch avoids rc assignment and directly uses uart->irq
> in the following if statement.
> 
> No functional changes.
> 
> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Who's the author of this patch? (Either the order of the SoB is wrong, or
there's a From: tag missing.)

> ---
> Changes in v2:
> - avoid assignment of rc;
> - drop changes in vcpu_yield(void).
> ---
>  xen/drivers/char/ns16550.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

This warrants a more specific subject prefix. Also there's only a single
violation being dealt with here.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -445,11 +445,13 @@ 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
>              };
>  
> -            if ( rc > 0 )
> +            rc = 0;
> +
> +            if ( uart->irq > 0 )
>              {
>                  struct msi_desc *msi_desc = NULL;

The fact that there's no functional change here isn't really obvious.
Imo you want to prove that to a reasonable degree in the description.

Jan

On Mon, 27 Nov 2023, Jan Beulich wrote:
> On 24.11.2023 18:29, Simone Ballarin wrote:
> > Rule 13.1: Initializer lists shall not contain persistent side effects
> > 
> > The assignment operation in:
> > 
> > .irq = rc = uart->irq,
> > 
> > is a persistent side effect in a struct initializer list.
> > 
> > This patch avoids rc assignment and directly uses uart->irq
> > in the following if statement.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> Who's the author of this patch? (Either the order of the SoB is wrong, or
> there's a From: tag missing.)
> 
> > ---
> > Changes in v2:
> > - avoid assignment of rc;
> > - drop changes in vcpu_yield(void).
> > ---
> >  xen/drivers/char/ns16550.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> This warrants a more specific subject prefix. Also there's only a single
> violation being dealt with here.
> 
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -445,11 +445,13 @@ 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
> >              };
> >  
> > -            if ( rc > 0 )
> > +            rc = 0;
> > +
> > +            if ( uart->irq > 0 )
> >              {
> >                  struct msi_desc *msi_desc = NULL;
> 
> The fact that there's no functional change here isn't really obvious.
> Imo you want to prove that to a reasonable degree in the description.
 
Agreed. Only reading this chunk, wouldn't it be better to do:

    };

    rc = uart->irq;

    if ( rc > 0 )

at least it would be obvious?

On 02.12.2023 04:22, Stefano Stabellini wrote:
> On Mon, 27 Nov 2023, Jan Beulich wrote:
>> On 24.11.2023 18:29, Simone Ballarin wrote:
>>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>>
>>> The assignment operation in:
>>>
>>> .irq = rc = uart->irq,
>>>
>>> is a persistent side effect in a struct initializer list.
>>>
>>> This patch avoids rc assignment and directly uses uart->irq
>>> in the following if statement.
>>>
>>> No functional changes.
>>>
>>> Signed-off-by: Maria Celeste Cesario  <maria.celeste.cesario@bugseng.com>
>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> Who's the author of this patch? (Either the order of the SoB is wrong, or
>> there's a From: tag missing.)
>>
>>> ---
>>> Changes in v2:
>>> - avoid assignment of rc;
>>> - drop changes in vcpu_yield(void).
>>> ---
>>>  xen/drivers/char/ns16550.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> This warrants a more specific subject prefix. Also there's only a single
>> violation being dealt with here.
>>
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -445,11 +445,13 @@ 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
>>>              };
>>>  
>>> -            if ( rc > 0 )
>>> +            rc = 0;
>>> +
>>> +            if ( uart->irq > 0 )
>>>              {
>>>                  struct msi_desc *msi_desc = NULL;
>>
>> The fact that there's no functional change here isn't really obvious.
>> Imo you want to prove that to a reasonable degree in the description.
>  
> Agreed. Only reading this chunk, wouldn't it be better to do:
> 
>     };
> 
>     rc = uart->irq;
> 
>     if ( rc > 0 )
> 
> at least it would be obvious?

Indeed.

Jan