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