[Qemu-devel] [PATCH 08/15] blkdebug: fix enum comparison

[Qemu-devel] [PATCH 08/15] blkdebug: fix enum comparison

[Blue Swirl]

The signedness of enum types depend on the compiler implementation.
Therefore the check for negative values may or may not be meaningful.

Fix by explicitly casting to a signed integer.

Since the values are also checked earlier against event_names
table, this is an internal error. Change the 'if' to 'assert'.

This also fixes a warning with GCC flag -Wtype-limits.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 block/blkdebug.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 2a63df9..4d6ff0a 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
*bs, BlkDebugEvent event)
     struct BlkdebugRule *rule;
     BlkdebugVars old_vars = s->vars;

-    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
-        return;
-    }
+    assert((int)event >= 0 && event < BLKDBG_EVENT_MAX);

     QLIST_FOREACH(rule, &s->rules[event], next) {
         process_rule(bs, rule, &old_vars);
-- 
1.6.2.4

[Qemu-devel] Re: [PATCH 08/15] blkdebug: fix enum comparison

[Michael S. Tsirkin]

On Sun, Sep 05, 2010 at 03:06:32PM +0000, Blue Swirl wrote:
> The signedness of enum types depend on the compiler implementation.
> Therefore the check for negative values may or may not be meaningful.
> 
> Fix by explicitly casting to a signed integer.
> 
> Since the values are also checked earlier against event_names
> table, this is an internal error. Change the 'if' to 'assert'.
> 
> This also fixes a warning with GCC flag -Wtype-limits.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  block/blkdebug.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 2a63df9..4d6ff0a 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
> *bs, BlkDebugEvent event)
>      struct BlkdebugRule *rule;
>      BlkdebugVars old_vars = s->vars;
> 
> -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
> -        return;
> -    }
> +    assert((int)event >= 0 && event < BLKDBG_EVENT_MAX);

I am not sure all compilers must generate a negative value from
a very large unsigned integer cast to int.

assert((unsigned)event < BLKDBG_EVENT_MAX);

will do the same but without integer overflow.

> 
>      QLIST_FOREACH(rule, &s->rules[event], next) {
>          process_rule(bs, rule, &old_vars);
> -- 
> 1.6.2.4

[Qemu-devel] Re: [PATCH 08/15] blkdebug: fix enum comparison

[Blue Swirl]

On Sun, Sep 5, 2010 at 5:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 05, 2010 at 03:06:32PM +0000, Blue Swirl wrote:
>> The signedness of enum types depend on the compiler implementation.
>> Therefore the check for negative values may or may not be meaningful.
>>
>> Fix by explicitly casting to a signed integer.
>>
>> Since the values are also checked earlier against event_names
>> table, this is an internal error. Change the 'if' to 'assert'.
>>
>> This also fixes a warning with GCC flag -Wtype-limits.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  block/blkdebug.c |    4 +---
>>  1 files changed, 1 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 2a63df9..4d6ff0a 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
>> *bs, BlkDebugEvent event)
>>      struct BlkdebugRule *rule;
>>      BlkdebugVars old_vars = s->vars;
>>
>> -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>> -        return;
>> -    }
>> +    assert((int)event >= 0 && event < BLKDBG_EVENT_MAX);
>
> I am not sure all compilers must generate a negative value from
> a very large unsigned integer cast to int.

The enum rules seem to be vague. The type of enums may also be signed
(on GCC when the enum set includes negative values, on other compilers
in other cases). Do any machines or compilers exist (on which QEMU
runs) where this could happen?

> assert((unsigned)event < BLKDBG_EVENT_MAX);
>
> will do the same but without integer overflow.

It's not the same if BLKDBG_EVENT_MAX >= 0x80000000 and the type of
the BlkDebugEvent is unsigned. It's probably more correct, though.

>>
>>      QLIST_FOREACH(rule, &s->rules[event], next) {
>>          process_rule(bs, rule, &old_vars);
>> --
>> 1.6.2.4
>

[Qemu-devel] Re: [PATCH 08/15] blkdebug: fix enum comparison'

[Michael S. Tsirkin]

On Sun, Sep 05, 2010 at 07:37:54PM +0000, Blue Swirl wrote:
> On Sun, Sep 5, 2010 at 5:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 05, 2010 at 03:06:32PM +0000, Blue Swirl wrote:
> >> The signedness of enum types depend on the compiler implementation.
> >> Therefore the check for negative values may or may not be meaningful.
> >>
> >> Fix by explicitly casting to a signed integer.
> >>
> >> Since the values are also checked earlier against event_names
> >> table, this is an internal error. Change the 'if' to 'assert'.
> >>
> >> This also fixes a warning with GCC flag -Wtype-limits.
> >>
> >> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> >> ---
> >>  block/blkdebug.c |    4 +---
> >>  1 files changed, 1 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/block/blkdebug.c b/block/blkdebug.c
> >> index 2a63df9..4d6ff0a 100644
> >> --- a/block/blkdebug.c
> >> +++ b/block/blkdebug.c
> >> @@ -439,9 +439,7 @@ static void blkdebug_debug_event(BlockDriverState
> >> *bs, BlkDebugEvent event)
> >>      struct BlkdebugRule *rule;
> >>      BlkdebugVars old_vars = s->vars;
> >>
> >> -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
> >> -        return;
> >> -    }
> >> +    assert((int)event >= 0 && event < BLKDBG_EVENT_MAX);
> >
> > I am not sure all compilers must generate a negative value from
> > a very large unsigned integer cast to int.
> 
> The enum rules seem to be vague. The type of enums may also be signed
> (on GCC when the enum set includes negative values, on other compilers
> in other cases). Do any machines or compilers exist (on which QEMU
> runs) where this could happen?

I remember reading that GCC sometimes assumes signed integers don't overflow,
and generates code behaves incorrectly if they do.
No idea whether this is ever the case for casts.

-- 
MST