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

How about this:



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

Thanks,

/mjt
Thanks,

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

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

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