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

2013-10-05 Thread 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)

2013-10-05 Thread 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 m...@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 m...@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)

2013-10-05 Thread Stefan Weil 
Am 05.10.2013 11:19, schrieb Michael Tokarev:

 Author: Michael Tokarev m...@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 m...@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 s...@weilnetz.de

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

2013-10-03 Thread 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)

2013-10-02 Thread 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)

2013-10-02 Thread 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)

2013-10-01 Thread Markus Armbruster 
Stefan Weil s...@weilnetz.de writes:

 Am 30.09.2013 11:59, schrieb Markus Armbruster:
 Stefan Weil s...@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:

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

2013-09-30 Thread Markus Armbruster 
Stefan Weil s...@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:

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

2013-09-30 Thread Stefan Weil 
Am 30.09.2013 11:59, schrieb Markus Armbruster:
 Stefan Weil s...@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:

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

2013-09-29 Thread Michael Tokarev 

29.09.2013 19:41, Stefan Weil wrote:

The QEMU buildbot default_i386_debian_6_0 shows this warning:

   CCmigration.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 s...@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)

2013-09-29 Thread 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:

CCmigration.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 s...@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