Re: [PATCH v3] migration: Support gtree migration

[Auger Eric]

Hi Juan,

On 10/5/19 12:34 AM, Juan Quintela wrote:
> Eric Auger <address@hidden> 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 <address@hidden>
> 
> 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.

You are completely right. Thank you for catching this.

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