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