Re: [Qemu-devel] [Qemu-trivial] [PATCH] migration: Fix compiler warning ('caps' may be used uninitialized)

[Stefan Weil <sw@weilnetz.de>]

Am 29.09.2013 22:13, schrieb Michael Tokarev:
> 29.09.2013 19:41, Stefan Weil wrote:
>> The QEMU buildbot default_i386_debian_6_0 shows this warning:
>>
>>    CC    migration.o
>> migration.c: In function 'qmp_query_migrate_capabilities':
>> migration.c:149: warning:
>>   'caps' may be used uninitialized in this function
>
> Gah, how disgusting.  The code is correct, yet gcc complains
> needlessly...

That's not the first time where we help the compiler by modifying the code.

>
>> While changing this code, I also replaced g_malloc0
>> by the type safe g_new0.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>
>> Buildbot URL:
>> http://buildbot.b1-systems.de/qemu/builders/default_i386_debian_6_0/builds/775/steps/compile/logs/stdio
>>
>>
>>   migration.c |   12 +++++-------
>>   1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 200d404..8dcb6ce 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -145,17 +145,15 @@ uint64_t migrate_max_downtime(void)
>>
>>   MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error
>> **errp)
>>   {
>> -    MigrationCapabilityStatusList *head = NULL;
>> -    MigrationCapabilityStatusList *caps;
>> +    MigrationCapabilityStatusList *head =
>> +        g_new0(MigrationCapabilityStatusList, 1);
>> +    MigrationCapabilityStatusList *caps = head;
>>       MigrationState *s = migrate_get_current();
>>       int i;
>>
>>       for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
>> -        if (head == NULL) {
>> -            head = g_malloc0(sizeof(*caps));
>> -            caps = head;
>> -        } else {
>> -            caps->next = g_malloc0(sizeof(*caps));
>> +        if (i > 0) {
>> +            caps->next = g_new0(MigrationCapabilityStatusList, i);
>>               caps = caps->next;
>>           }
>
> But this assumes that MIGRATION_CAPABILITY_MAX > 0 ! ;)
> Ie, that there's at least one entry (head) in the list.
> Do we care?

I thought of MIGRATION_CAPABILITY_MAX == 0, too. In current QEMU it's
defined in

qapi-types.h:    MIGRATION_CAPABILITY_MAX = 4,

If there will be no MigrationCapability at all in the future,
we should care about removing the code, not handle the 0 case.
But of course we could add an assertion (feel free to add one,
or I send an update).

Should we fix the callers, too? Today, they assume that the function
might return NULL (= no MigrationCapabilityexists).

>
> Previous code was more natural, so to say...
>
> I'd just initialize `caps' to the same NULL as `head'
> initially...  Or maybe change for() into do..while()..
> Dunno, somehow I don't like the new code much, either ;)
>
> Thanks,
>
> /mjt