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 "&&".

Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
Reported-By: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7779c88a91..523dfe3f3f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1221,7 +1221,7 @@ 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);
+    assert(pcms->vmport < ON_OFF_AUTO__MAX);
     if (pcms->vmport == ON_OFF_AUTO_AUTO) {
         pcms->vmport = (xen_enabled() || !pcms->i8042_enabled)
             ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
--
2.45.0

On 8/21/24 08:52, Kamil Szczęk wrote:
> 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 "&&".
> 
> Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
> Reported-By: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7779c88a91..523dfe3f3f 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1221,7 +1221,7 @@ 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);
> +    assert(pcms->vmport < ON_OFF_AUTO__MAX);

Given the usage here

>       if (pcms->vmport == ON_OFF_AUTO_AUTO) {

and later, here

>     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
>                     pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);

absolutely nothing goes off the rails no matter the value of vmport.

It is not used to index an array, which might be out of bounds.
It it not a security issue.
There's no need or benefit for the assert at all.


r~

On Wednesday, August 21st, 2024 at 00:59, Richard Henderson <richard.henderson@linaro.org> wrote:
> On 8/21/24 08:52, Kamil Szczęk wrote:
> 
> > 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 "&&".
> > 
> > Signed-off-by: Kamil Szczęk kamil@szczek.dev
> > Reported-By: Philippe Mathieu-Daudé philmd@linaro.org
> > ---
> > hw/i386/pc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 7779c88a91..523dfe3f3f 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1221,7 +1221,7 @@ 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);
> > + assert(pcms->vmport < ON_OFF_AUTO__MAX);
> 
> 
> Given the usage here
> 
> > if (pcms->vmport == ON_OFF_AUTO_AUTO) {
> 
> 
> and later, here
> 
> > pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
> > pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
> 
> 
> absolutely nothing goes off the rails no matter the value of vmport.
> 
> It is not used to index an array, which might be out of bounds.
> It it not a security issue.
> There's no need or benefit for the assert at all.
> 

Agreed, v2 posted.