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

[Kamil Szczęk]

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