[PULL,18/20] hw/i386/pc: Unify vmport=auto handling

From: Kamil Szczęk <kamil@szczek.dev>

The code which translates vmport=auto to on/off is currently separate
for each PC machine variant, while being functionally equivalent.
This moves the translation into a shared initialization function, while
also tightening the enum assertion.

Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
Reviewed-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <v8pz1uwgIYWkidgZK-o8H-qJvnSyl0641XVmNO43Qls307AA3QRPuad_py6xGe0JAxB6yDEe76oZ8tau_n-2Y6sJBCKzCujNbEUUFhd-ahI=@szczek.dev>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc.c      | 5 +++++
 hw/i386/pc_piix.c | 5 -----
 hw/i386/pc_q35.c  | 5 -----
 3 files changed, 5 insertions(+), 10 deletions(-)

Hi Kamil,

On 20/8/24 00:51, Philippe Mathieu-Daudé wrote:
> From: Kamil Szczęk <kamil@szczek.dev>
> 
> The code which translates vmport=auto to on/off is currently separate
> for each PC machine variant, while being functionally equivalent.
> This moves the translation into a shared initialization function, while
> also tightening the enum assertion.
> 
> Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
> Reviewed-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <v8pz1uwgIYWkidgZK-o8H-qJvnSyl0641XVmNO43Qls307AA3QRPuad_py6xGe0JAxB6yDEe76oZ8tau_n-2Y6sJBCKzCujNbEUUFhd-ahI=@szczek.dev>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc.c      | 5 +++++
>   hw/i386/pc_piix.c | 5 -----
>   hw/i386/pc_q35.c  | 5 -----
>   3 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c74931d577..72229a24ff 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1217,6 +1217,11 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>           isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
>       }
>   
> +    assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX);

Coverity reported:

 >>>     CID 1559533:  Integer handling issues  (CONSTANT_EXPRESSION_RESULT)
 >>>     "pcms->vmport >= 0" is always true regardless of the values of 
its operands. This occurs as the logical first operand of "&&".

QAPI enums are unsigned because they start at 0, see:
https://www.qemu.org/docs/master/devel/qapi-code-gen.html#enumeration-types

   The generated C enumeration constants have values 0, 1, …, N-1
   (in QAPI schema order), where N is the number of values. There
   is an additional enumeration constant PREFIX__MAX with value N.

Could you post a patch to address this issue?

Thanks,

Phil.

> +    if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> +        pcms->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
> +    }
> +
>       /* Super I/O */
>       pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>                       pcms->vmport != ON_OFF_AUTO_ON);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index d9e69243b4..347afa4c37 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -310,11 +310,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
>   
>       pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
>   
> -    assert(pcms->vmport != ON_OFF_AUTO__MAX);
> -    if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> -        pcms->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
> -    }
> -
>       /* init basic PC hardware */
>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
>                            !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 9d108b194e..f2d8edfa84 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -276,11 +276,6 @@ static void pc_q35_init(MachineState *machine)
>           x86_register_ferr_irq(x86ms->gsi[13]);
>       }
>   
> -    assert(pcms->vmport != ON_OFF_AUTO__MAX);
> -    if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> -        pcms->vmport = ON_OFF_AUTO_ON;
> -    }
> -
>       /* init basic PC hardware */
>       pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc, !mc->no_floppy,
>                            0xff0104);

On Tuesday, August 20th, 2024 at 21:48, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> 
> Hi Kamil,
> 
> On 20/8/24 00:51, Philippe Mathieu-Daudé wrote:
> 
> > From: Kamil Szczęk kamil@szczek.dev
> > 
> > The code which translates vmport=auto to on/off is currently separate
> > for each PC machine variant, while being functionally equivalent.
> > This moves the translation into a shared initialization function, while
> > also tightening the enum assertion.
> > 
> > Signed-off-by: Kamil Szczęk kamil@szczek.dev
> > Reviewed-by: Bernhard Beschow shentey@gmail.com
> > Reviewed-by: Philippe Mathieu-Daudé philmd@linaro.org
> > Message-ID: v8pz1uwgIYWkidgZK-o8H-qJvnSyl0641XVmNO43Qls307AA3QRPuad_py6xGe0JAxB6yDEe76oZ8tau_n-2Y6sJBCKzCujNbEUUFhd-ahI=@szczek.dev
> > Signed-off-by: Philippe Mathieu-Daudé philmd@linaro.org
> > ---
> > hw/i386/pc.c | 5 +++++
> > hw/i386/pc_piix.c | 5 -----
> > hw/i386/pc_q35.c | 5 -----
> > 3 files changed, 5 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c74931d577..72229a24ff 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1217,6 +1217,11 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> > isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
> > }
> > 
> > + assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX);
> 
> 
> Coverity reported:
> 
> > CID 1559533: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
> > "pcms->vmport >= 0" is always true regardless of the values of
> > its operands. This occurs as the logical first operand of "&&".
> 
> QAPI enums are unsigned because they start at 0, see:
> https://www.qemu.org/docs/master/devel/qapi-code-gen.html#enumeration-types
> 
> The generated C enumeration constants have values 0, 1, …, N-1
> (in QAPI schema order), where N is the number of values. There
> is an additional enumeration constant PREFIX__MAX with value N.

Oh, and here I thought I was being smart with modifying this assert :D

> 
> Could you post a patch to address this issue?
> 

Will do shortly. Although, I've looked around the codebase and found a few more instances of this pattern.

"assert\(.*>= *0.*__MAX" yields the following results:

job.c
> assert(s1 >= 0 && s1 < JOB_STATUS__MAX);
> assert(verb >= 0 && verb < JOB_VERB__MAX);

blkdebug.c
> assert((int)event >= 0 && event < BLKDBG__MAX);

pc.c
> assert(pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX);

options.c
> assert(mode >= 0 && mode < MIG_MODE__MAX);

savevm.c
> assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX);

Does coverity also complain about those? If so, should I address all of them or keep it minimal?

Also, just as a test I added a single line of code before the assert:

pcms->vmport = -1;

And, to my surprise, it compiled successfully without any warning and as expected, aborted on the assert:

qemu-system-x86_64: ../hw/i386/pc.c:1225: pc_basic_device_init: Assertion 'pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX' failed.

Is this expected behavior?

> Thanks,
> 
> Phil.
> 
> > + if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> > + pcms->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
> > + }
> > +
> > /* Super I/O */
> > pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
> > pcms->vmport != ON_OFF_AUTO_ON);
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index d9e69243b4..347afa4c37 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -310,11 +310,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
> > 
> > pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
> > 
> > - assert(pcms->vmport != ON_OFF_AUTO__MAX);
> > - if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> > - pcms->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
> > - }
> > -
> > /* init basic PC hardware */
> > pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
> > !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 9d108b194e..f2d8edfa84 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -276,11 +276,6 @@ static void pc_q35_init(MachineState *machine)
> > x86_register_ferr_irq(x86ms->gsi[13]);
> > }
> > 
> > - assert(pcms->vmport != ON_OFF_AUTO__MAX);
> > - if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> > - pcms->vmport = ON_OFF_AUTO_ON;
> > - }
> > -
> > /* init basic PC hardware */
> > pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc, !mc->no_floppy,
> > 0xff0104);

On 8/21/24 06:32, Kamil Szczęk wrote:
> Also, just as a test I added a single line of code before the assert:
> 
> pcms->vmport = -1;
> 
> And, to my surprise, it compiled successfully without any warning and as expected, aborted on the assert:
> 
> qemu-system-x86_64: ../hw/i386/pc.c:1225: pc_basic_device_init: Assertion 'pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX' failed.
> 
> Is this expected behavior?

Yes.

The underlying integral type for enum in C is implementation defined.
It can and does vary between compilers, leading to this sort of thing.

The only reasonable fix is

   (unsigned)foo < max

But you could also question whether the assert is really useful.


r~

On Wednesday, August 21st, 2024 at 00:45, Richard Henderson <richard.henderson@linaro.org> wrote:
> On 8/21/24 06:32, Kamil Szczęk wrote:
> 
> > Also, just as a test I added a single line of code before the assert:
> > 
> > pcms->vmport = -1;
> > 
> > And, to my surprise, it compiled successfully without any warning and as expected, aborted on the assert:
> > 
> > qemu-system-x86_64: ../hw/i386/pc.c:1225: pc_basic_device_init: Assertion 'pcms->vmport >= 0 && pcms->vmport < ON_OFF_AUTO__MAX' failed.
> > 
> > Is this expected behavior?
> 
> 
> Yes.
> 
> The underlying integral type for enum in C is implementation defined.
> It can and does vary between compilers, leading to this sort of thing.
> 
> The only reasonable fix is
> 
> (unsigned)foo < max

Fair enough, just posted a patch.