[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-trivial] [PATCH] migration: Fix compiler warning
From: |
Stefan Weil |
Subject: |
Re: [Qemu-devel] [Qemu-trivial] [PATCH] migration: Fix compiler warning ('caps' may be used uninitialized) |
Date: |
Sun, 29 Sep 2013 22:33:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.0 |
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 <address@hidden>
>> ---
>>
>> 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