| Message ID | 20240819225116.17928-19-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/20] hw/mips/loongson3_virt: Store core_iocsr into LoongsonMachineState | expand |
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.
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); + 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);