Re: [PATCH v4] migration: Support gtree migration

[Juan Quintela <quintela@redhat.com>]

Eric Auger <eric.auger@redhat.com> wrote:
> Introduce support for GTree migration. A custom save/restore
> is implemented. Each item is made of a key and a data.
>
> If the key is a pointer to an object, 2 VMSDs are passed into
> the GTree VMStateField.
>
> When putting the items, the tree is traversed in sorted order by
> g_tree_foreach.
>
> On the get() path, gtrees must be allocated using the proper
> key compare, key destroy and value destroy. This must be handled
> beforehand, for example in a pre_load method.
>
> Tests are added to test save/dump of structs containing gtrees
> including the virtio-iommu domain/mappings scenario.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

I found a bug, you have to respin, go to BUG
(here was a reviewed-by)

But, ....

I didn't noticed on the 1st look

> +    const VMStateDescription *key_vmsd = direct_key ? NULL : &field->vmsd[0];
> +    const VMStateDescription *val_vmsd = direct_key ? &field->vmsd[0] :
> +                                                      &field->vmsd[1];
> +    const char *key_vmsd_name = direct_key ? "direct" : key_vmsd->name;

This is ugly as hell.
We are using a pointer to pass to mean an array, and abuse it.

But once told that, it is not trivial to do this in a proper way.

On the other hand, if you have to respin for any other reason, please
consider changing the meaning of vmsd[0] and [1].

vmsd[0] -> val_t
vmsd[1] -> key_t

if (vmsd[1] == NULL)
   direct_key = true;

    const VMStateDescription *val_vmsd = &field->vmsd[0];
    const VMStateDescription *key_vmsd = &field->vmsd[1]
    const char *key_vmsd_name = key_vmsd ? "direct" : key_vmsd->name;

Same for get_gtree().


> +        if (direct_key) {
> +            key = (void *)(uintptr_t)qemu_get_be64(f);

no g_malloc().

> +        } else {
> +            key = g_malloc0(key_size);
> +            ret = vmstate_load_state(f, key_vmsd, key, version_id);
> +            if (ret) {
> +                error_report("%s : failed to load %s (%d)",
> +                             field->name, key_vmsd->name, ret);
> +                g_free(key);
> +                return ret;
> +            }
> +        }
> +        val = g_malloc0(val_size);
> +        ret = vmstate_load_state(f, val_vmsd, val, version_id);
> +        if (ret) {
> +            error_report("%s : failed to load %s (%d)",
> +                         field->name, val_vmsd->name, ret);
> +            g_free(key);

BUG: Allways free.  This need to be protected by a direct_key(), no?

> +            g_free(val);
> +            return ret;
> +        }
> +        g_tree_insert(tree, key, val);
> +    }
> +    if (count != nnodes) {
> +        error_report("%s inconsistent stream when loading the gtree",
> +                     field->name);

BUG2:  we need to return an error here, right?

> +    }
> +    trace_get_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
> +    return ret;
> +}
> +

Later, Juan.