Re: [PATCH v3] migration: Support gtree migration

[Peter Xu]

On Fri, Oct 04, 2019 at 01:20:25PM +0200, Eric Auger 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>

Mostly looks sane to me (with Juan's comment fixed).  Some more
trivial comments below.

> +/*
> + * For migrating a GTree whose key is a pointer to _key_type and the
> + * value, a pointer to _val_type
> + * The target tree must have been properly initialized
> + * _vmsd: Start address of the 2 element array containing the key vmsd
> + *        and the data vmsd
> + * _key_type: type of the key
> + * _val_type: type of the value
> + */
> +#define VMSTATE_GTREE_V(_field, _state, _version, _vmsd,                     
>   \
> +                        _key_type, _val_type)                                
>   \
> +{                                                                            
>   \
> +    .name         = (stringify(_field)),                                     
>   \
> +    .version_id   = (_version),                                              
>   \
> +    .vmsd         = (_vmsd),                                                 
>   \
> +    .info         = &vmstate_info_gtree,                                     
>   \
> +    .start        = sizeof(_key_type),                                       
>   \

Nitpick: Are we reusing the "start" field to store the size just to
avoid defining new field in VMStateField?  If so, not sure whether we
can start to use unions to both keep VMStateField small while keep the
code clean.  Like:

  union {
    struct {
      size_t key_size;
      size_t value_size;
    };
    struct {
      size_t start;
      size_t size;
    };
  }

?

This can of course also be done on top of this patch no matter what.

[...]

> +static gboolean put_gtree_elem(gpointer key, gpointer value, gpointer data)
> +{
> +    struct put_gtree_data *capsule = (struct put_gtree_data *)data;
> +    QEMUFile *f = capsule->f;
> +    int ret;
> +
> +    qemu_put_byte(f, true);
> +
> +    /* put the key */
> +    if (!capsule->key_vmsd) {
> +        qemu_put_be32(f, GPOINTER_TO_UINT(key)); /* direct key */

This is special code path for direct key case.  Can we simply define
VMSTATE_GTREE_DIRECT_KEY_V() somehow better so that it just uses the
VMSTATE_UINT32_V() as the key vmsd?  Then iiuc vmstate_save_state()
could work well with that too.

Also, should we avoid using UINT in all cases?  But of course if we
start to use VMSTATE_UINT32_V then we don't have this issue.

Thanks,

> +    } else {
> +        ret = vmstate_save_state(f, capsule->key_vmsd, key, capsule->vmdesc);
> +        if (ret) {
> +            capsule->ret = ret;
> +            return true;
> +        }
> +    }
> +
> +    /* put the data */
> +    ret = vmstate_save_state(f, capsule->val_vmsd, value, capsule->vmdesc);
> +    if (ret) {
> +        capsule->ret = ret;
> +        return true;
> +    }
> +    return false;
> +}

-- 
Peter Xu