Re: [Qemu-devel] [PATCH 3/3] Add dbus-vmstate object

[Juan Quintela]

Marc-André Lureau <address@hidden> wrote:
> When instanciated, this object will connect to the given D-Bus
> bus. During migration, it will take the data from org.qemu.VMState1
> instances.
>
> See documentation for further details.
>
> Signed-off-by: Marc-André Lureau <address@hidden>


> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index 981e8e122f..dbbe12b225 100644
> --- a/backends/Makefile.objs
> +++ b/backends/Makefile.objs
> @@ -17,3 +17,7 @@ endif
>  common-obj-$(call land,$(CONFIG_VHOST_USER),$(CONFIG_VIRTIO)) += vhost-user.o
>  
>  common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
> +
> +common-obj-$(CONFIG_GIO) += dbus-vmstate.o
> +dbus-vmstate.o-cflags = $(GIO_CFLAGS)
> +dbus-vmstate.o-libs = $(GIO_LIBS)

I assume this only deppends of GIO, the variable that this patch
introduces is GDBUS_CODEGEN in configure.  I think it would be a good
idea to be able to disable this "feature" entirely.

Before anything, I have no clue about DBUS and not a lot about glib, so
don't expect anything else that questions on that front.

Several questions:

> +#define DBUS_VMSTATE_SIZE_LIMIT (1 << 20) /* 1mb */

What size do you expect in real life?  I am assuming that you have an
user case in mind.

> +#define DBUS_VMSTATE_SECTION 0x00

A section name of 0x00 is asking for trouble,  I would try a magic
number here?

> +#define DBUS_VMSTATE_EOF 0xff

You are doing:
- registering a savevm live subsection
  I don't understand why.  If this list is small enough, we would like
  probably preffer a normal device, and if it is big, are we expecting
  that the dbus source handle duplicates, etc, etc.

> +static GHashTable *
> +get_id_list_set(DBusVMState *self)
> +{
> +    char **ids;
> +    GHashTable *set;
> +    int i;
> +
> +    if (!self->id_list) {
> +        return NULL;
> +    }
> +
> +    ids = g_strsplit(self->id_list, ",", -1);
> +    set = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL);
> +    for (i = 0; ids[i]; i++) {
> +        g_hash_table_add(set, ids[i]);
> +        ids[i] = NULL;
> +    }

Why are we using a hash table here?

> +
> +    g_strfreev(ids);
> +    return set;
> +}
> +
> +static GHashTable *
> +dbus_get_proxies(DBusVMState *self, GError **err)
> +{
> +    GError *local_err = NULL;
> +    GHashTable *proxies = NULL;
> +    GVariant *result = NULL;
> +    char **names = NULL;
> +    char **left = NULL;
> +    GHashTable *ids = get_id_list_set(self);
> +    size_t i;
> +
> +    proxies = g_hash_table_new_full(g_str_hash, g_str_equal,
> +                                    g_free, g_object_unref);
> +
> +    names = dbus_get_vmstate1_names(self, &local_err);
> +    if (!names) {
> +        if (g_error_matches(local_err,
> +                            G_DBUS_ERROR, G_DBUS_ERROR_NAME_HAS_NO_OWNER)) {
> +            return proxies;
> +        }
> +        g_propagate_error(err, local_err);
> +        goto err;
> +    }
> +
> +    for (i = 0; names[i]; i++) {
> +        GDBusProxy *proxy;
> +        char *id;
> +        size_t size;
> +
> +        proxy = g_dbus_proxy_new_sync(self->bus, G_DBUS_PROXY_FLAGS_NONE,
> +                    (GDBusInterfaceInfo *) &vmstate1_interface_info,
> +                    names[i],
> +                    "/org/qemu/VMState1",
> +                    "org.qemu.VMState1",
> +                    NULL, err);
> +        if (!proxy) {
> +            goto err;
> +        }
> +
> +        result = g_dbus_proxy_get_cached_property(proxy, "Id");
> +        if (!result) {
> +            g_set_error_literal(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> +                                "VMState Id property is missing.");
> +            g_clear_object(&proxy);
> +            goto err;
> +        }
> +
> +        id = g_variant_dup_string(result, &size);
> +        if (ids && !g_hash_table_remove(ids, id)) {
> +            g_free(id);
> +            g_clear_object(&proxy);
> +            continue;
> +        }
> +        if (size == 0 || size >= 256) {

Why are we using 256 char limit here?  Is that a dbus thing, or it is
about this implementation?

> +            g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> +                        "VMState Id '%s' is invalid.", id);

                        "VMState Id '%s' has invalid size '%d", id, size); ??

> +            g_free(id);
> +            g_clear_object(&proxy);
> +            goto err;
> +        }
> +
> +        if (!g_hash_table_insert(proxies, id, proxy)) {
> +            g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> +                        "Duplicated VMState Id '%s'", id);
> +            goto err;
> +        }
> +
> +        g_clear_pointer(&result, g_variant_unref);
> +    }
> +
> +    if (ids) {
> +        left = (char **)g_hash_table_get_keys_as_array(ids, NULL);
> +        if (*left) {
> +            char *leftids = g_strjoinv(",", left);
> +            g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> +                        "Required VMState Id are missing: %s", leftids);
> +            g_free(leftids);
> +            goto err;
> +        }
> +        g_free(left);
> +    }
> +
> +    g_clear_pointer(&ids, g_hash_table_unref);
> +    g_strfreev(names);
> +    return proxies;
> +
> +err:
> +    g_free(left);
> +    g_clear_pointer(&proxies, g_hash_table_unref);
> +    g_clear_pointer(&result, g_variant_unref);
> +    g_clear_pointer(&ids, g_hash_table_unref);
> +    g_strfreev(names);
> +    return NULL;
> +}
> +
> +static int
> +dbus_load_state_proxy(GDBusProxy *proxy, const uint8_t *data, size_t size)
> +{
> +    GError *err = NULL;
> +    GVariant *value, *result;
> +    int ret = -1;
> +
> +    value = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE,
> +                                      data, size, sizeof(char));
> +    result = g_dbus_proxy_call_sync(proxy, "Load",
> +                                    g_variant_new("(@ay)", value),
> +                                    G_DBUS_CALL_FLAGS_NO_AUTO_START,
> +                                    -1, NULL, &err);
> +    if (!resultp) {
> +        error_report("Failed to Load: %s", err->message);
> +        goto end;
> +    }
> +
> +    ret = 0;
> +
> +end:
> +    g_clear_pointer(&result, g_variant_unref);
> +    g_clear_error(&err);
> +    return ret;
> +}
> +
> +static int
> +dbus_load_state(QEMUFile *f, void *opaque, int version_id)
> +{
> +    DBusVMState *self = DBUS_VMSTATE(opaque);
> +    GError *err = NULL;
> +    GHashTable *proxies = NULL;
> +    uint8_t *data = NULL;
> +    int ret = -1;
> +
> +    proxies = dbus_get_proxies(self, &err);
> +    if (!proxies) {
> +        error_report("Failed to get proxies: %s", err->message);
> +        goto end;
> +    }
> +
> +    while (qemu_get_byte(f) != DBUS_VMSTATE_EOF) {

I know that there are other examples in qemu that uses this pattern:

while (more items)
  proccess aonther item;

But we are thing to move to a system that is:

for (numef of items)
  process item

I would expect that it would be better that you put a header with:
- a version
- number of elements
- total size

We would want at some point in the future to be able to introspect
inside the migration stream, that kind of things help.  

What do you think?


> +        GDBusProxy *proxy = NULL;
> +        char id[256];
> +        unsigned int size;
> +
> +        if (qemu_get_counted_string(f, id) == 0) {
> +            error_report("Invalid vmstate Id");
               error_report("Invalid vmstate Id: '%s'", id); ??

> +            goto end;
> +        }
> +
> +        proxy = g_hash_table_lookup(proxies, id);
> +        if (!proxy) {
> +            error_report("Failed to find proxy Id '%s'", id);
> +            goto end;
> +        }
> +
> +        size = qemu_get_be32(f);
> +        if (size > DBUS_VMSTATE_SIZE_LIMIT) {
> +            error_report("Invalid vmstate size: %u", size);
> +            goto end;
> +        }
> +
> +        data = g_malloc(size);
> +        if (qemu_get_buffer(f, data, size) != size) {
> +            error_report("Failed to read %u bytes", size);
> +            goto end;
> +        }
> +
> +        if (dbus_load_state_proxy(proxy, data, size) < 0) {
> +            error_report("Failed to restore Id '%s'", id);
> +            goto end;
> +        }
> +
> +        g_clear_pointer(&data, g_free);
> +        g_hash_table_remove(proxies, id);
> +    }
> +
> +    if (g_hash_table_size(proxies) != 0) {
> +        error_report("Missing DBus states from migration stream.");
> +        goto end;
> +    }
> +
> +    ret = 0;
> +
> +end:
> +    g_clear_pointer(&proxies, g_hash_table_unref);
> +    g_clear_pointer(&data, g_free);
> +    g_clear_error(&err);
> +    return ret;
> +}
> +
> +static void
> +dbus_save_state_proxy(gpointer key,
> +                      gpointer value,
> +                      gpointer user_data)
> +{
> +    QEMUFile *f = user_data;
> +    const char *id = key;
> +    GDBusProxy *proxy = value;
> +    GVariant *result = NULL, *child = NULL;
> +    const uint8_t *data;
> +    size_t size;
> +    GError *err = NULL;
> +
> +    result = g_dbus_proxy_call_sync(proxy, "Save",
> +                                    NULL, G_DBUS_CALL_FLAGS_NO_AUTO_START,
> +                                    -1, NULL, &err);
> +    if (!result) {
> +        error_report("Failed to Save: %s", err->message);
> +        g_clear_error(&err);
> +        goto end;
> +    }
> +
> +    child = g_variant_get_child_value(result, 0);
> +    data = g_variant_get_fixed_array(child, &size, sizeof(char));
> +    if (!data) {
> +        error_report("Failed to Save: not a byte array");
> +        goto end;
> +    }
> +    if (size > DBUS_VMSTATE_SIZE_LIMIT) {
> +        error_report("Too much vmstate data to save: %lu", size);
> +        goto end;
> +    }
> +
> +    qemu_put_byte(f, DBUS_VMSTATE_SECTION);
> +    qemu_put_counted_string(f, id);
> +    qemu_put_be32(f, size);
> +    qemu_put_buffer(f, data, size);

We are just using vmstate to send an opaque chunk of data.  Normally we
are against this, we need a good explanation about why we want to put
this kind of handlers there.

> +
> +end:
> +    g_clear_pointer(&child, g_variant_unref);
> +    g_clear_pointer(&result, g_variant_unref);
> +}
> +
> +static void
> +dbus_save_state(QEMUFile *f, void *opaque)
> +{
> +    DBusVMState *self = DBUS_VMSTATE(opaque);
> +    GHashTable *proxies;
> +    GError *err = NULL;
> +
> +    proxies = dbus_get_proxies(self, &err);
> +    if (!proxies) {
> +        error_report("Failed to get proxies: %s", err->message);
> +        goto end;
> +    }
> +
> +    /* TODO: how to report an error and cancel? */

This is basically where I would like to have some kind of header.

> +    g_hash_table_foreach(proxies, dbus_save_state_proxy, f);
> +    qemu_put_byte(f, DBUS_VMSTATE_EOF);
> +
> +end:
> +    g_clear_error(&err);
> +    g_clear_pointer(&proxies, g_hash_table_unref);
> +}
> +
> +static const SaveVMHandlers savevm_handlers = {
> +    .save_state = dbus_save_state,
> +    .load_state = dbus_load_state,
> +};
> +
> +static void
> +dbus_vmstate_complete(UserCreatable *uc, Error **errp)
> +{
> +    DBusVMState *self = DBUS_VMSTATE(uc);
> +    GError *err = NULL;
> +    GDBusConnection *bus;
> +
> +    if (!object_resolve_path_type("", TYPE_DBUS_VMSTATE, NULL)) {
> +        error_setg(errp, "There is already an instance of %s",
> +                   TYPE_DBUS_VMSTATE);
> +        return;
> +    }
> +
> +    if (!self->dbus_addr) {
> +        error_setg(errp, QERR_MISSING_PARAMETER, "addr");
> +        return;
> +    }
> +
> +    if (register_savevm_live(NULL, TYPE_DBUS_VMSTATE, 0, 0,
> +                             &savevm_handlers, self) < 0) {
> +        error_setg(errp, "Failed to register savevm handler");
> +        return;
> +    }
> +
> +    bus = g_dbus_connection_new_for_address_sync(self->dbus_addr,
> +             G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT |
> +             G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION,
> +             NULL, NULL, &err);
> +    if (err) {
> +        error_setg(errp, "failed to connect to DBus: '%s'", err->message);
> +        g_clear_error(&err);
> +    }
> +
> +    self->bus = bus;
> +}
> +
> +static void
> +dbus_vmstate_finalize(Object *o)
> +{
> +    DBusVMState *self = DBUS_VMSTATE(o);
> +
> +    unregister_savevm(NULL, TYPE_DBUS_VMSTATE, self);
> +    g_clear_object(&self->bus);
> +    g_free(self->dbus_addr);
> +    g_free(self->id_list);
> +}
> +

Documentation is nice.


> diff --git a/docs/interop/dbus-vmstate.rst b/docs/interop/dbus-vmstate.rst
> new file mode 100644
> index 0000000000..32d3475315
> --- /dev/null
> +++ b/docs/interop/dbus-vmstate.rst
> @@ -0,0 +1,64 @@
> +============
> +DBus VMState
> +============
> +
> +Introduction
> +============
> +
> +Helper processes may have their state migrated with the help of the
> +QEMU object "dbus-vmstate".
> +
> +Upon migration, QEMU will go through the queue of "org.qemu.VMState1"
> +DBus name owners and query their "Id". It must be unique among the
> +helpers.
> +
> +It will then save arbitrary data of each Id to be transferred in the
> +migration stream and restored/loaded at the corresponding destination
> +helper.
> +
> +The data amount to be transferred is limited to 1Mb. The state must be
> +saved quickly (a few seconds maximum). (DBus imposes a time limit on
> +reply anyway, and migration would fail if data isn't given quickly
> +enough)

Seconds?  here?  We expect that the part of save_live to be clearly
under a second except if configured otherwise.

> +dbus-vmstate object can be configured with the expected list of
> +helpers by setting its "id-list" property, with a coma-separated Id
> +list.

Documentation is ok.

Test looks ok.

Later, Juan.