Re: [PATCH v3] migration: Support gtree migration

[Peter Xu]

On Thu, Oct 10, 2019 at 09:57:01AM +0200, Auger Eric wrote:
> >> +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.
> if the key_vmsd is a VMSTATE_UINT32_V then I understand
> vmstate_save_state(f, capsule->key_vmsd, key, capsule->vmdesc)
> expects key to be a pointer to a uint32. But in that case of direct key,
> it is a uint32. I don't figure out how to use vmstate_save_state in your
> proposal.

Ah I see the point.  And indeed I can't think of a better way now
(e.g., maybe I will always try to use GTrees with malloc()ed keys to
be simple when I want to migrate a gtree, but yeah that's not a reason
to refuse this patch :).

Though we should be very careful on defining vmsds for GTrees in the
future with the help of this patch, and we must have the type (either
direct or not) to match the real usage of the tree otherwise QEMU can
potentially unreference the constant as a pointer.

> 
> > 
> > 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.
> Depending on the clarification of above point, maybe I can rename
> VMSTATE_GTREE_DIRECT_KEY_V into VMSTATE_GTREE_DIRECT_UINT_KEY_V
> 
> direct keys seem to be more common for hash tables actually.
> https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-new-full
> 
> There are stand conversion macros to/from int, uint, size
> https://developer.gnome.org/glib/stable/glib-Type-Conversion-Macros.html

Yeh it's fine to use direct keys.  Though my question was more about
"unsigned int" - here when we put, we cast a pointer into unsigned
int, which can be 2/4 bytes, IIUC.  I'm thinking whether at least we
should use direct cast (e.g., (uint32_t)ptr) to replace
GPOINTER_TO_UINT() to make sure it's 4 bytes.  Futher, maybe we should
start with uint64_t here in the migration stream, otherwise we should
probably drop the high 32 bits if we migrate a gtree whose key is 64
bits long (and since we're working with migration we won't be able to
change that in the future for compatibility reasons...).

Summary:

Maybe we can replace:

    qemu_put_be32(f, GPOINTER_TO_UINT(key)); /* direct key */

To:

    qemu_put_be64(f, (uint64_t)key); /* direct key */

And apply similar thing to get() side?

Thanks,

-- 
Peter Xu