Marc-André Lureau <marcandre.lur...@redhat.com> 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 <marcandre.lur...@redhat.com>
> 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.
