Re: [PATCH v3] 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>

Overal I like the patch.  I think that I found a minor error.


> +static int put_gtree(QEMUFile *f, void *pv, size_t unused_size,
> +                     const VMStateField *field, QJSON *vmdesc)
> +{
> +    bool direct_key = (!field->start);
> +    const VMStateDescription *key_vmsd = direct_key ? NULL : &field->vmsd[0];
> +    const VMStateDescription *val_vmsd = direct_key ? &field->vmsd[0] :
> +                                                      &field->vmsd[1];
> +    struct put_gtree_data capsule = {f, key_vmsd, val_vmsd, 0};

Please, consider change the last line to:

       struct put_gtree_data capsule = {
           .f = f,
           .key_vmsd = key_vmsd,
           .val_vmsd = val_vmsd,
           .ret = 0};

It makes much easier make changes on the future.

Once here, I think that you are missing on the middle a:

          .vmdesc = vmdesc,

No?  At least you use it on put_gtree_elem, and I don't see any place
where you assign it.  When I did the change is when I noticed that it
was missing.

> +    GTree **pval = pv;
> +    GTree *tree = *pval;
> +    int ret;
> +
> +    qemu_put_be32(f, g_tree_nnodes(tree));
> +    g_tree_foreach(tree, put_gtree_elem, (gpointer)&capsule);
> +    qemu_put_byte(f, false);
> +    ret = capsule.ret;
> +    if (ret) {
> +        error_report("%s : failed to save gtree (%d)", field->name, ret);
> +    }
> +    return ret;
> +}

As said before, with this changes you have my reviewed-by.

Later, Juan.