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

[Philippe Mathieu-Daudé]

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);