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

[Blue Swirl]

On Sun, Sep 5, 2010 at 5:57 PM, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>> ---
>>  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
>