[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4] migration: Support gtree migration
From: |
Juan Quintela |
Subject: |
Re: [PATCH v4] migration: Support gtree migration |
Date: |
Fri, 11 Oct 2019 12:18:13 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
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>
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.