Hi Juan,
On 10/11/19 12:18 PM, 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>
>
> 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.
I agree. My first attempt was using subsections to pass the second vmsd
but it was similarly ugly.
>
> 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
OK. I chose that order since usually the pair is expressed as key/value
and I found it more logical from the qemu user perspective. But I have
no strong preference.
>
> 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().
OK
>
>
>> + 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?
ouch yes
>
>> + 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?
yep
>
>> + }
>> + trace_get_gtree_end(field->name, key_vmsd_name, val_vmsd->name, ret);
>> + return ret;
>> +}
>> +
>
> Later, Juan.
>
Thanks for the review
Eric
