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

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

[Stefan Weil]

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

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;
         }
         caps->value =
-- 
1.7.10.4

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

[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...

> 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?

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

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

[Stefan Weil]

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

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

[Markus Armbruster]

Stefan Weil <sw@weilnetz.de> writes:

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

It's also not the first time where attempting to "help" the compiler
made code less readable, or even less correct.  So let's be just as
careful as with "real" changes.

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

[Stefan Weil]

Am 30.09.2013 11:59, schrieb Markus Armbruster:
> Stefan Weil <sw@weilnetz.de> writes:
>> 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.
> It's also not the first time where attempting to "help" the compiler
> made code less readable, or even less correct.  So let's be just as
> careful as with "real" changes.

Well, I try to do my best. ;-)

Is there anything wrong with my patch? I think the code looks cleaner
than before.

If there is a better way to fix the problem that's fine, too.

The problem withthe buildbot showing a compiler warning exists and we
should fix it somehow.

Regards,
Stefan

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

[Markus Armbruster]

Stefan Weil <sw@weilnetz.de> writes:

> Am 30.09.2013 11:59, schrieb Markus Armbruster:
>> Stefan Weil <sw@weilnetz.de> writes:
>>> 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.
>> It's also not the first time where attempting to "help" the compiler
>> made code less readable, or even less correct.  So let's be just as
>> careful as with "real" changes.
>
> Well, I try to do my best. ;-)
>
> Is there anything wrong with my patch? I think the code looks cleaner
> than before.

Michael disagrees, and I can see his points.

> If there is a better way to fix the problem that's fine, too.
>
> The problem withthe buildbot showing a compiler warning exists and we
> should fix it somehow.

The warning is bogus, and current compilers don't seem to emit it.  If
you can remove the warning by improving the code (or at least not making
it worse), then go right ahead anyway.

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

[Michael Tokarev]

How about this:

diff --git a/migration.c b/migration.c
index b4f8462..6066ab4 100644
--- a/migration.c
+++ b/migration.c
@@ -146,22 +146,16 @@ uint64_t migrate_max_downtime(void)
  MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
  {
      MigrationCapabilityStatusList *head = NULL;
-    MigrationCapabilityStatusList *caps;
+    MigrationCapabilityStatusList **capp = &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));
-            caps = caps->next;
-        }
-        caps->value =
-            g_malloc(sizeof(*caps->value));
-        caps->value->capability = i;
-        caps->value->state = s->enabled_capabilities[i];
+        *capp = g_malloc0(sizeof(*head));
+        (*capp)->value = g_malloc(sizeof(*head->value));
+        (*capp)->value->capability = i;
+        (*capp)->value->state = s->enabled_capabilities[i];
+       capp = &(*capp)->next;
      }

      return head;


This is what I had in mind at the very beginnig, but only now tried
to make a patch...

Thanks,

/mjt
Thanks,

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

[Stefan Weil]

Am 02.10.2013 21:02, schrieb Michael Tokarev:
> How about this:
>
> diff --git a/migration.c b/migration.c
> index b4f8462..6066ab4 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -146,22 +146,16 @@ uint64_t migrate_max_downtime(void)
>  MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error
> **errp)
>  {
>      MigrationCapabilityStatusList *head = NULL;
> -    MigrationCapabilityStatusList *caps;
> +    MigrationCapabilityStatusList **capp = &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));
> -            caps = caps->next;
> -        }
> -        caps->value =
> -            g_malloc(sizeof(*caps->value));
> -        caps->value->capability = i;
> -        caps->value->state = s->enabled_capabilities[i];
> +        *capp = g_malloc0(sizeof(*head));
> +        (*capp)->value = g_malloc(sizeof(*head->value));
> +        (*capp)->value->capability = i;
> +        (*capp)->value->state = s->enabled_capabilities[i];
> +       capp = &(*capp)->next;
>      }
>
>      return head;
>
>
> This is what I had in mind at the very beginnig, but only now tried
> to make a patch...
>
> Thanks,
>
> /mjt
> Thanks,


That's a possible solution. Paolo also sent a sketch of a similar
solution. And here are two more:

MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
{
    MigrationCapabilityStatusList *head = NULL;
    MigrationState *s = migrate_get_current();
    MigrationCapability i = MIGRATION_CAPABILITY_MAX;

    do {
        MigrationCapabilityStatusList *caps =
            g_new(MigrationCapabilityStatusList, 1);
        i--;
        caps->next = head;
        caps->value = g_new(MigrationCapabilityStatus, 1);
        caps->value->capability = i;
        caps->value->state = s->enabled_capabilities[i];
        head = caps;
    } while (i > 0);

    return head;
}

MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
{
    MigrationCapabilityStatusList *head = NULL;
    MigrationCapabilityStatusList *prev = NULL;
    MigrationState *s = migrate_get_current();
    MigrationCapability i;

    for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
        MigrationCapabilityStatusList *caps =
            g_new(MigrationCapabilityStatusList, 1);
        if (prev == NULL) {
            head = caps;
        } else {
            prev->next = caps;
            prev = caps;
        }
        caps->value = g_new(MigrationCapabilityStatus, 1);
        caps->value->capability = i;
        caps->value->state = s->enabled_capabilities[i];
    }

    return head;
}

Which one do we take? Any correct solution which fixes the compiler
warning is fine for me (although I prefer g_new instead of g_malloc as
you might have guessed). :-)

Regards,
Stefan

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

[Paolo Bonzini]

Il 02/10/2013 22:24, Stefan Weil ha scritto:
> Am 02.10.2013 21:02, schrieb Michael Tokarev:
> MigrationCapabilityStatusList *qmp_query_migrate_capabilities(Error **errp)
> {
>     MigrationCapabilityStatusList *head = NULL;
>     MigrationCapabilityStatusList *prev = NULL;
>     MigrationState *s = migrate_get_current();
>     MigrationCapability i;
> 
>     for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
>         MigrationCapabilityStatusList *caps =
>             g_new(MigrationCapabilityStatusList, 1);
>         if (prev == NULL) {
>             head = caps;
>         } else {
>             prev->next = caps;
>             prev = caps;
>         }
>         caps->value = g_new(MigrationCapabilityStatus, 1);
>         caps->value->capability = i;
>         caps->value->state = s->enabled_capabilities[i];
>     }
> 
>     return head;
> }

I dislike having head initialized to NULL.

> Which one do we take? Any correct solution which fixes the compiler
> warning is fine for me (although I prefer g_new instead of g_malloc as
> you might have guessed). :-)

Mine uses g_new0 so it should work for you as well? :)

Paolo

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

[Michael Tokarev]

Okay.  This takes just too long and too many people
are affected.  I'll just set the variable in question
(caps) to NULL at entry for now, -- it is not a critical
path and the current code is correct anyway.  This is
becoming ridiculous, when there are so many different
opinions about such a trivial thing, with the result
being that nothing is fixed.  Setting it to NULL at
least will let a buildbot to be fixed for now.

Thanks,

/mjt

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

[Michael Tokarev]

05.10.2013 13:15, Michael Tokarev пишет:
> Okay.  This takes just too long and too many people
> are affected.  I'll just set the variable in question
> (caps) to NULL at entry for now, -- it is not a critical
> path and the current code is correct anyway.  This is
> becoming ridiculous, when there are so many different
> opinions about such a trivial thing, with the result
> being that nothing is fixed.  Setting it to NULL at
> least will let a buildbot to be fixed for now.

Forgot the patch itself.  It is the shortest from all
already proposed ;)

Author: Michael Tokarev <mjt@tls.msk.ru>
Date:   Sat Oct 5 13:18:28 2013 +0400

     migration: Fix compiler warning ('caps' may be used uninitialized)

     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

diff --git a/migration.c b/migration.c
index b4f8462..2b1ab20 100644
--- a/migration.c
+++ b/migration.c
@@ -150,6 +150,7 @@ MigrationCapabilityStatusList *qmp_query_migrate_capabilitie
      MigrationState *s = migrate_get_current();
      int i;

+    caps = NULL; /* silence compiler warning */
      for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
          if (head == NULL) {
              head = g_malloc0(sizeof(*caps));


Thanks,

/mjt

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

[Stefan Weil]

Am 05.10.2013 11:19, schrieb Michael Tokarev:
>
> Author: Michael Tokarev <mjt@tls.msk.ru>
> Date:   Sat Oct 5 13:18:28 2013 +0400
>
>     migration: Fix compiler warning ('caps' may be used uninitialized)
>
>     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>
> diff --git a/migration.c b/migration.c
> index b4f8462..2b1ab20 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -150,6 +150,7 @@ MigrationCapabilityStatusList
> *qmp_query_migrate_capabilitie
>      MigrationState *s = migrate_get_current();
>      int i;
>
> +    caps = NULL; /* silence compiler warning */
>      for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
>          if (head == NULL) {
>              head = g_malloc0(sizeof(*caps));
>
>
> Thanks,
>
> /mjt
>


Reviewed-by: Stefan Weil <sw@weilnetz.de>

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

[Paolo Bonzini]

Il 29/09/2013 17:41, Stefan Weil ha scritto:
> 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
> 
> 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;
>          }
>          caps->value =
> 

What about

    MigrationCapabilityStatusList *head = NULL;
    MigrationCapabilityStatusList **p_tail = &head;
    MigrationState *s = migrate_get_current();
    int i;

    for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
        MigrationCapabilityStatusList *caps = g_malloc0(sizeof(*caps));
        caps->value =
            ...
        *p_next = caps;
        p_next = &caps->next;
    }