* [PATCH v6 0/8] Add dbus-vmstate
@ 2019-12-11 13:44 Marc-André Lureau
2019-12-11 13:44 ` [PATCH v6 1/8] vmstate: add qom interface to get id Marc-André Lureau
` (7 more replies)
0 siblings, 8 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:44 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
Hi,
With external processes or helpers participating to the VM support, it
becomes necessary to handle their migration. Various options exist to
transfer their state:
1) as the VM memory, RAM or devices (we could say that's how
vhost-user devices can be handled today, they are expected to
restore from ring state)
2) other "vmstate" (as with TPM emulator state blobs)
3) left to be handled by management layer
1) is not practical, since an external processes may legitimatelly
need arbitrary state date to back a device or a service, or may not
even have an associated device.
2) needs ad-hoc code for each helper, but is simple and working
3) is complicated for management layer, QEMU has the migration timing
The proposed "dbus-vmstate" object will connect to a given D-Bus
address, and save/load from org.qemu.VMState1 owners on migration.
Thus helpers can easily have their state migrated with QEMU, without
implementing ad-hoc support (such as done for TPM emulation)
D-Bus is ubiquitous on Linux (it is systemd IPC), and can be made to
work on various other OSes. There are several implementations and good
bindings for various languages. (the tests/dbus-vmstate-test.c is a
good example of how simple the implementation of services can be, even
in C)
dbus-vmstate is put into use by the libvirt series "[PATCH 00/23] Use
a slirp helper process".
v6:
- rebased (minor change in patch 2)
v5:
- trying to fix patchew/ci: install dbus-daemon in containers, skip
test if unavailable
v4:
- add Daniel security scenarios to the D-Bus document
- misc doc improvements
- add "util: add dbus helper unit" patch, with
qemu_dbus_get_queued_owners()
- add "configure: add GDBUS_CODEGEN", explaining why gio-unix is
required when available
- silence the expected failing tests
- update copyright headers, MAINTAINERS
- add r-b/a-b tags
- rebased
(Note: patchew dbus test fails for unclear reasons, but I can't
reproduce locally nor on travis)
v3:
- after various discussions on helper processes, we settled on a
preference for having a bus for communications. This version is
actually v1 updated.
- added a dbus.rst document to describe D-Bus recommendations for QEMU
- added dbus-vmstate-daemon.sh to play with the dbus-daemon configuration
(although it is not very useful in the context of a single UID)
- added a new vmstate interface, so that any object can implement
VMStateDescription, and converted dbus-vmstate
- added "migration: fix vmdesc leak on vmstate_save() error"
- convert to g_auto
v2:
- D-Bus is most common and practical through a bus, but it requires a
daemon to be running. I argue that the benefits outweight the cost
of running an extra daemon in v1 in the context of multi-process
qemu, but it is also possible to connect in p2p mode as done in this
new version.
Marc-André Lureau (8):
vmstate: add qom interface to get id
vmstate: replace DeviceState with VMStateIf
docs: start a document to describe D-Bus usage
util: add dbus helper unit
Add dbus-vmstate object
configure: add GDBUS_CODEGEN
dockerfiles: add dbus-daemon to some of latest distributions
tests: add dbus-vmstate-test
MAINTAINERS | 12 +
backends/Makefile.objs | 4 +
backends/dbus-vmstate.c | 496 +++++++++++++++++++++++
configure | 7 +
docs/interop/dbus-vmstate.rst | 74 ++++
docs/interop/dbus.rst | 104 +++++
docs/interop/index.rst | 2 +
hw/block/onenand.c | 2 +-
hw/core/Makefile.objs | 1 +
hw/core/qdev.c | 21 +-
hw/core/vmstate-if.c | 23 ++
hw/ide/cmd646.c | 2 +-
hw/ide/isa.c | 2 +-
hw/ide/piix.c | 2 +-
hw/ide/via.c | 2 +-
hw/misc/max111x.c | 2 +-
hw/net/eepro100.c | 4 +-
hw/net/virtio-net.c | 3 +-
hw/nvram/eeprom93xx.c | 4 +-
hw/ppc/spapr_drc.c | 9 +-
hw/ppc/spapr_iommu.c | 4 +-
hw/s390x/s390-skeys.c | 2 +-
include/hw/vmstate-if.h | 40 ++
include/migration/register.h | 4 +-
include/migration/vmstate.h | 10 +-
include/qemu/dbus.h | 18 +
migration/savevm.c | 20 +-
stubs/vmstate.c | 4 +-
tests/Makefile.include | 23 +-
tests/dbus-vmstate-daemon.sh | 95 +++++
tests/dbus-vmstate-test.c | 399 ++++++++++++++++++
tests/dbus-vmstate1.xml | 12 +
tests/docker/dockerfiles/centos7.docker | 1 +
tests/docker/dockerfiles/debian10.docker | 1 +
tests/docker/dockerfiles/fedora.docker | 1 +
tests/docker/dockerfiles/ubuntu.docker | 1 +
util/Makefile.objs | 3 +
util/dbus.c | 55 +++
38 files changed, 1430 insertions(+), 39 deletions(-)
create mode 100644 backends/dbus-vmstate.c
create mode 100644 docs/interop/dbus-vmstate.rst
create mode 100644 docs/interop/dbus.rst
create mode 100644 hw/core/vmstate-if.c
create mode 100644 include/hw/vmstate-if.h
create mode 100644 include/qemu/dbus.h
create mode 100755 tests/dbus-vmstate-daemon.sh
create mode 100644 tests/dbus-vmstate-test.c
create mode 100644 tests/dbus-vmstate1.xml
create mode 100644 util/dbus.c
--
2.24.0.308.g228f53135a
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v6 1/8] vmstate: add qom interface to get id
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
@ 2019-12-11 13:44 ` Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 2/8] vmstate: replace DeviceState with VMStateIf Marc-André Lureau
` (6 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:44 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
Add an interface to get the instance id, instead of depending on
Device and qdev_get_dev_path().
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
MAINTAINERS | 2 ++
hw/core/Makefile.objs | 1 +
hw/core/qdev.c | 14 +++++++++++++
hw/core/vmstate-if.c | 23 +++++++++++++++++++++
include/hw/vmstate-if.h | 40 ++++++++++++++++++++++++++++++++++++
include/migration/register.h | 2 ++
include/migration/vmstate.h | 2 ++
tests/Makefile.include | 1 +
8 files changed, 85 insertions(+)
create mode 100644 hw/core/vmstate-if.c
create mode 100644 include/hw/vmstate-if.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 5e5e3e52d6..525b4740e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2189,6 +2189,8 @@ Migration
M: Juan Quintela <quintela@redhat.com>
M: Dr. David Alan Gilbert <dgilbert@redhat.com>
S: Maintained
+F: hw/core/vmstate-if.c
+F: include/hw/vmstate-if.h
F: include/migration/
F: migration/
F: scripts/vmstate-static-checker.py
diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index fd0550d1d9..0edd9e635d 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -9,6 +9,7 @@ common-obj-y += hotplug.o
common-obj-$(CONFIG_SOFTMMU) += nmi.o
common-obj-$(CONFIG_SOFTMMU) += vm-change-state-handler.o
common-obj-y += cpu.o
+common-obj-y += vmstate-if.o
common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
common-obj-$(CONFIG_XILINX_AXI) += stream.o
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cf1ba28fe3..add43d460e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1089,9 +1089,18 @@ static void device_unparent(Object *obj)
}
}
+static char *
+device_vmstate_if_get_id(VMStateIf *obj)
+{
+ DeviceState *dev = DEVICE(obj);
+
+ return qdev_get_dev_path(dev);
+}
+
static void device_class_init(ObjectClass *class, void *data)
{
DeviceClass *dc = DEVICE_CLASS(class);
+ VMStateIfClass *vc = VMSTATE_IF_CLASS(class);
class->unparent = device_unparent;
@@ -1103,6 +1112,7 @@ static void device_class_init(ObjectClass *class, void *data)
*/
dc->hotpluggable = true;
dc->user_creatable = true;
+ vc->get_id = device_vmstate_if_get_id;
}
void device_class_set_parent_reset(DeviceClass *dc,
@@ -1160,6 +1170,10 @@ static const TypeInfo device_type_info = {
.class_init = device_class_init,
.abstract = true,
.class_size = sizeof(DeviceClass),
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_VMSTATE_IF },
+ { }
+ }
};
static void qdev_register_types(void)
diff --git a/hw/core/vmstate-if.c b/hw/core/vmstate-if.c
new file mode 100644
index 0000000000..bf453620fe
--- /dev/null
+++ b/hw/core/vmstate-if.c
@@ -0,0 +1,23 @@
+/*
+ * VMState interface
+ *
+ * Copyright (c) 2009-2019 Red Hat Inc
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/vmstate-if.h"
+
+static const TypeInfo vmstate_if_info = {
+ .name = TYPE_VMSTATE_IF,
+ .parent = TYPE_INTERFACE,
+ .class_size = sizeof(VMStateIfClass),
+};
+
+static void vmstate_register_types(void)
+{
+ type_register_static(&vmstate_if_info);
+}
+
+type_init(vmstate_register_types);
diff --git a/include/hw/vmstate-if.h b/include/hw/vmstate-if.h
new file mode 100644
index 0000000000..8ff7f0f292
--- /dev/null
+++ b/include/hw/vmstate-if.h
@@ -0,0 +1,40 @@
+/*
+ * VMState interface
+ *
+ * Copyright (c) 2009-2019 Red Hat Inc
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef VMSTATE_IF_H
+#define VMSTATE_IF_H
+
+#include "qom/object.h"
+
+#define TYPE_VMSTATE_IF "vmstate-if"
+
+#define VMSTATE_IF_CLASS(klass) \
+ OBJECT_CLASS_CHECK(VMStateIfClass, (klass), TYPE_VMSTATE_IF)
+#define VMSTATE_IF_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(VMStateIfClass, (obj), TYPE_VMSTATE_IF)
+#define VMSTATE_IF(obj) \
+ INTERFACE_CHECK(VMStateIf, (obj), TYPE_VMSTATE_IF)
+
+typedef struct VMStateIf VMStateIf;
+
+typedef struct VMStateIfClass {
+ InterfaceClass parent_class;
+
+ char * (*get_id)(VMStateIf *obj);
+} VMStateIfClass;
+
+static inline char *vmstate_if_get_id(VMStateIf *vmif)
+{
+ if (!vmif) {
+ return NULL;
+ }
+
+ return VMSTATE_IF_GET_CLASS(vmif)->get_id(vmif);
+}
+
+#endif /* VMSTATE_IF_H */
diff --git a/include/migration/register.h b/include/migration/register.h
index a13359a08d..73149c9a01 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -14,6 +14,8 @@
#ifndef MIGRATION_REGISTER_H
#define MIGRATION_REGISTER_H
+#include "hw/vmstate-if.h"
+
typedef struct SaveVMHandlers {
/* This runs inside the iothread lock. */
SaveStateHandler *save_state;
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index ac4f46a67d..f546f61c5e 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -27,6 +27,8 @@
#ifndef QEMU_VMSTATE_H
#define QEMU_VMSTATE_H
+#include "hw/vmstate-if.h"
+
typedef struct VMStateInfo VMStateInfo;
typedef struct VMStateField VMStateField;
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 8566f5f119..e2ab8ba334 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -581,6 +581,7 @@ tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
hw/core/irq.o \
hw/core/fw-path-provider.o \
hw/core/reset.o \
+ hw/core/vmstate-if.o \
$(test-qapi-obj-y)
tests/test-vmstate$(EXESUF): tests/test-vmstate.o \
migration/vmstate.o migration/vmstate-types.o migration/qemu-file.o \
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 2/8] vmstate: replace DeviceState with VMStateIf
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
2019-12-11 13:44 ` [PATCH v6 1/8] vmstate: add qom interface to get id Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 3/8] docs: start a document to describe D-Bus usage Marc-André Lureau
` (5 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
Replace DeviceState dependency with VMStateIf on vmstate API.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Acked-by: Halil Pasic <pasic@linux.ibm.com>
---
hw/block/onenand.c | 2 +-
hw/core/qdev.c | 7 ++++---
hw/ide/cmd646.c | 2 +-
hw/ide/isa.c | 2 +-
hw/ide/piix.c | 2 +-
hw/ide/via.c | 2 +-
hw/misc/max111x.c | 2 +-
hw/net/eepro100.c | 4 ++--
hw/net/virtio-net.c | 3 ++-
hw/nvram/eeprom93xx.c | 4 ++--
hw/ppc/spapr_drc.c | 9 +++++----
hw/ppc/spapr_iommu.c | 4 ++--
hw/s390x/s390-skeys.c | 2 +-
include/migration/register.h | 2 +-
include/migration/vmstate.h | 8 ++++----
migration/savevm.c | 20 ++++++++++----------
stubs/vmstate.c | 4 ++--
17 files changed, 41 insertions(+), 38 deletions(-)
diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index fcc5a69b90..9c233c12e4 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -822,7 +822,7 @@ static void onenand_realize(DeviceState *dev, Error **errp)
onenand_mem_setup(s);
sysbus_init_irq(sbd, &s->intr);
sysbus_init_mmio(sbd, &s->container);
- vmstate_register(dev,
+ vmstate_register(VMSTATE_IF(dev),
((s->shift & 0x7f) << 24)
| ((s->id.man & 0xff) << 16)
| ((s->id.dev & 0xff) << 8)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index add43d460e..fadc01f622 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -890,7 +890,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
dev->canonical_path = object_get_canonical_path(OBJECT(dev));
if (qdev_get_vmsd(dev)) {
- if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
+ if (vmstate_register_with_alias_id(VMSTATE_IF(dev),
+ -1, qdev_get_vmsd(dev), dev,
dev->instance_id_alias,
dev->alias_required_for_version,
&local_err) < 0) {
@@ -925,7 +926,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
local_errp);
}
if (qdev_get_vmsd(dev)) {
- vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+ vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
}
if (dc->unrealize) {
local_errp = local_err ? NULL : &local_err;
@@ -949,7 +950,7 @@ child_realize_fail:
}
if (qdev_get_vmsd(dev)) {
- vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
+ vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
}
post_realize_fail:
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 19984d2af9..3f9be968d1 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -302,7 +302,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
}
g_free(irq);
- vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
+ vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
qemu_register_reset(cmd646_reset, d);
}
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 7b6e283679..9c7f88b2d5 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -75,7 +75,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
isa_init_irq(isadev, &s->irq, s->isairq);
ide_init2(&s->bus, s->irq);
- vmstate_register(dev, 0, &vmstate_ide_isa, s);
+ vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
ide_register_restart_cb(&s->bus);
}
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index db313dd3b1..bc575b4d70 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -156,7 +156,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
bmdma_setup_bar(d);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
- vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
+ vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
pci_piix_init_ports(d);
}
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 053622bd82..096de8dba0 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -190,7 +190,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
bmdma_setup_bar(d);
pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
- vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
+ vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_pci, d);
for (i = 0; i < 2; i++) {
ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
index a713149f16..211008ce02 100644
--- a/hw/misc/max111x.c
+++ b/hw/misc/max111x.c
@@ -146,7 +146,7 @@ static int max111x_init(SSISlave *d, int inputs)
s->input[7] = 0x80;
s->com = 0;
- vmstate_register(dev, -1, &vmstate_max111x, s);
+ vmstate_register(VMSTATE_IF(dev), -1, &vmstate_max111x, s);
return 0;
}
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index cc2dd8b1c9..cc71a7a036 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1815,7 +1815,7 @@ static void pci_nic_uninit(PCIDevice *pci_dev)
{
EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
- vmstate_unregister(&pci_dev->qdev, s->vmstate, s);
+ vmstate_unregister(VMSTATE_IF(&pci_dev->qdev), s->vmstate, s);
g_free(s->vmstate);
eeprom93xx_free(&pci_dev->qdev, s->eeprom);
qemu_del_nic(s->nic);
@@ -1874,7 +1874,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
s->vmstate = g_memdup(&vmstate_eepro100, sizeof(vmstate_eepro100));
s->vmstate->name = qemu_get_queue(s->nic)->model;
- vmstate_register(&pci_dev->qdev, -1, s->vmstate, s);
+ vmstate_register(VMSTATE_IF(&pci_dev->qdev), -1, s->vmstate, s);
}
static void eepro100_instance_init(Object *obj)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index db3d7c38e6..777d62d3c8 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2853,7 +2853,8 @@ static void virtio_net_handle_migration_primary(VirtIONet *n,
if (migration_in_setup(s) && !should_be_hidden) {
if (failover_unplug_primary(n)) {
- vmstate_unregister(n->primary_dev, qdev_get_vmsd(n->primary_dev),
+ vmstate_unregister(VMSTATE_IF(n->primary_dev),
+ qdev_get_vmsd(n->primary_dev),
n->primary_dev);
qapi_event_send_unplug_primary(n->primary_device_id);
atomic_set(&n->primary_should_be_hidden, true);
diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 5b01b9b03f..07f09549ed 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -321,7 +321,7 @@ eeprom_t *eeprom93xx_new(DeviceState *dev, uint16_t nwords)
/* Output DO is tristate, read results in 1. */
eeprom->eedo = 1;
logout("eeprom = 0x%p, nwords = %u\n", eeprom, nwords);
- vmstate_register(dev, 0, &vmstate_eeprom, eeprom);
+ vmstate_register(VMSTATE_IF(dev), 0, &vmstate_eeprom, eeprom);
return eeprom;
}
@@ -329,7 +329,7 @@ void eeprom93xx_free(DeviceState *dev, eeprom_t *eeprom)
{
/* Destroy EEPROM. */
logout("eeprom = 0x%p\n", eeprom);
- vmstate_unregister(dev, &vmstate_eeprom, eeprom);
+ vmstate_unregister(VMSTATE_IF(dev), &vmstate_eeprom, eeprom);
g_free(eeprom);
}
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 62f1a42592..17aeac3801 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -511,7 +511,7 @@ static void realize(DeviceState *d, Error **errp)
error_propagate(errp, err);
return;
}
- vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
+ vmstate_register(VMSTATE_IF(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
drc);
trace_spapr_drc_realize_complete(spapr_drc_index(drc));
}
@@ -523,7 +523,7 @@ static void unrealize(DeviceState *d, Error **errp)
gchar *name;
trace_spapr_drc_unrealize(spapr_drc_index(drc));
- vmstate_unregister(DEVICE(drc), &vmstate_spapr_drc, drc);
+ vmstate_unregister(VMSTATE_IF(drc), &vmstate_spapr_drc, drc);
root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
name = g_strdup_printf("%x", spapr_drc_index(drc));
object_property_del(root_container, name, errp);
@@ -619,7 +619,8 @@ static void realize_physical(DeviceState *d, Error **errp)
return;
}
- vmstate_register(DEVICE(drcp), spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
+ vmstate_register(VMSTATE_IF(drcp),
+ spapr_drc_index(SPAPR_DR_CONNECTOR(drcp)),
&vmstate_spapr_drc_physical, drcp);
qemu_register_reset(drc_physical_reset, drcp);
}
@@ -635,7 +636,7 @@ static void unrealize_physical(DeviceState *d, Error **errp)
return;
}
- vmstate_unregister(DEVICE(drcp), &vmstate_spapr_drc_physical, drcp);
+ vmstate_unregister(VMSTATE_IF(drcp), &vmstate_spapr_drc_physical, drcp);
qemu_unregister_reset(drc_physical_reset, drcp);
}
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 3d3bcc8649..5704fe6051 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -317,7 +317,7 @@ static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
- vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
+ vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
tcet);
}
@@ -420,7 +420,7 @@ static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
{
SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev);
- vmstate_unregister(DEVICE(tcet), &vmstate_spapr_tce_table, tcet);
+ vmstate_unregister(VMSTATE_IF(tcet), &vmstate_spapr_tce_table, tcet);
QLIST_REMOVE(tcet, list);
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index bd37f39120..5da6e5292f 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -392,7 +392,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
register_savevm_live(TYPE_S390_SKEYS, 0, 1,
&savevm_s390_storage_keys, ss);
} else {
- unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss);
+ unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
}
}
diff --git a/include/migration/register.h b/include/migration/register.h
index 73149c9a01..00c38ebe9f 100644
--- a/include/migration/register.h
+++ b/include/migration/register.h
@@ -76,6 +76,6 @@ int register_savevm_live(const char *idstr,
const SaveVMHandlers *ops,
void *opaque);
-void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque);
+void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque);
#endif
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index f546f61c5e..4aef72c426 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1158,22 +1158,22 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
/* Returns: 0 on success, -1 on failure */
-int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
+int vmstate_register_with_alias_id(VMStateIf *obj, int instance_id,
const VMStateDescription *vmsd,
void *base, int alias_id,
int required_for_version,
Error **errp);
/* Returns: 0 on success, -1 on failure */
-static inline int vmstate_register(DeviceState *dev, int instance_id,
+static inline int vmstate_register(VMStateIf *obj, int instance_id,
const VMStateDescription *vmsd,
void *opaque)
{
- return vmstate_register_with_alias_id(dev, instance_id, vmsd,
+ return vmstate_register_with_alias_id(obj, instance_id, vmsd,
opaque, -1, 0, NULL);
}
-void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
+void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
void *opaque);
struct MemoryRegion;
diff --git a/migration/savevm.c b/migration/savevm.c
index a71b930b91..59efc1981d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -760,17 +760,17 @@ int register_savevm_live(const char *idstr,
return 0;
}
-void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
+void unregister_savevm(VMStateIf *obj, const char *idstr, void *opaque)
{
SaveStateEntry *se, *new_se;
char id[256] = "";
- if (dev) {
- char *path = qdev_get_dev_path(dev);
- if (path) {
- pstrcpy(id, sizeof(id), path);
+ if (obj) {
+ char *oid = vmstate_if_get_id(obj);
+ if (oid) {
+ pstrcpy(id, sizeof(id), oid);
pstrcat(id, sizeof(id), "/");
- g_free(path);
+ g_free(oid);
}
}
pstrcat(id, sizeof(id), idstr);
@@ -784,7 +784,7 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque)
}
}
-int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
+int vmstate_register_with_alias_id(VMStateIf *obj, int instance_id,
const VMStateDescription *vmsd,
void *opaque, int alias_id,
int required_for_version,
@@ -802,8 +802,8 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
se->vmsd = vmsd;
se->alias_id = alias_id;
- if (dev) {
- char *id = qdev_get_dev_path(dev);
+ if (obj) {
+ char *id = vmstate_if_get_id(obj);
if (id) {
if (snprintf(se->idstr, sizeof(se->idstr), "%s/", id) >=
sizeof(se->idstr)) {
@@ -834,7 +834,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
return 0;
}
-void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd,
+void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
void *opaque)
{
SaveStateEntry *se, *new_se;
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
index e1e89b87f0..6951d9fdc5 100644
--- a/stubs/vmstate.c
+++ b/stubs/vmstate.c
@@ -3,7 +3,7 @@
const VMStateDescription vmstate_dummy = {};
-int vmstate_register_with_alias_id(DeviceState *dev,
+int vmstate_register_with_alias_id(VMStateIf *obj,
int instance_id,
const VMStateDescription *vmsd,
void *base, int alias_id,
@@ -13,7 +13,7 @@ int vmstate_register_with_alias_id(DeviceState *dev,
return 0;
}
-void vmstate_unregister(DeviceState *dev,
+void vmstate_unregister(VMStateIf *obj,
const VMStateDescription *vmsd,
void *opaque)
{
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 3/8] docs: start a document to describe D-Bus usage
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
2019-12-11 13:44 ` [PATCH v6 1/8] vmstate: add qom interface to get id Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 2/8] vmstate: replace DeviceState with VMStateIf Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-12 11:36 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 4/8] util: add dbus helper unit Marc-André Lureau
` (4 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
MAINTAINERS | 5 +++
docs/interop/dbus.rst | 99 ++++++++++++++++++++++++++++++++++++++++++
docs/interop/index.rst | 1 +
3 files changed, 105 insertions(+)
create mode 100644 docs/interop/dbus.rst
diff --git a/MAINTAINERS b/MAINTAINERS
index 525b4740e8..19faa0e868 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2199,6 +2199,11 @@ F: tests/migration-test.c
F: docs/devel/migration.rst
F: qapi/migration.json
+D-Bus
+M: Marc-André Lureau <marcandre.lureau@redhat.com>
+S: Maintained
+F: docs/interop/dbus.rst
+
Seccomp
M: Eduardo Otubo <otubo@redhat.com>
S: Supported
diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst
new file mode 100644
index 0000000000..3d760e4882
--- /dev/null
+++ b/docs/interop/dbus.rst
@@ -0,0 +1,99 @@
+=====
+D-Bus
+=====
+
+Introduction
+============
+
+QEMU may be running with various helper processes involved:
+ - vhost-user* processes (gpu, virtfs, input, etc...)
+ - TPM emulation (or other devices)
+ - user networking (slirp)
+ - network services (DHCP/DNS, samba/ftp etc)
+ - background tasks (compression, streaming etc)
+ - client UI
+ - admin & cli
+
+Having several processes allows stricter security rules, as well as
+greater modularity.
+
+While QEMU itself uses QMP as primary IPC (and Spice/VNC for remote
+display), D-Bus is the de facto IPC of choice on Unix systems. The
+wire format is machine friendly, good bindings exist for various
+languages, and there are various tools available.
+
+Using a bus, helper processes can discover and communicate with each
+other easily, without going through QEMU. The bus topology is also
+easier to apprehend and debug than a mesh. However, it is wise to
+consider the security aspects of it.
+
+Security
+========
+
+A QEMU D-Bus bus should be private to a single VM. Thus, only
+cooperative tasks are running on the same bus to serve the VM.
+
+D-Bus, the protocol and standard, doesn't have mechanisms to enforce
+security between peers once the connection is established. Peers may
+have additional mechanisms to enforce security rules, based for
+example on UNIX credentials. However, because the daemon has
+controlled who can send/recv messages to who, doesn't magically make
+this secure. The semantics of the actual methods implemented using
+D-Bus are just as critical. Peers need to carefully validate any
+information they received from a peer with a different trust level.
+
+dbus-daemon policy
+------------------
+
+dbus-daemon can enforce various policies based on the UID/GID of the
+processes that are connected to it. It is thus a good idea to run
+helpers as different UID from QEMU and set appropriate policies.
+
+Depending on the use case, you may choose different scenarios:
+
+ - Everything the same UID
+
+ - Convenient for developers
+ - Improved reliability - crash of one part doens't take
+ out entire VM
+ - No security benefit over traditional QEMU
+
+ - Two UIDs, one for QEMU, one for dbus & helpers
+
+ - Moderately improved security isolation
+
+ - Many UIDs, one for QEMU one for dbus and one for each helpers
+
+ - Best security isolation
+ - Complex to manager distinct UIDs needed for each VM
+
+For example, to allow only ``qemu`` user to talk to ``qemu-helper``
+``org.qemu.Helper1`` service, a dbus-daemon policy may contain:
+
+.. code:: xml
+
+ <policy user="qemu">
+ <allow send_destination="org.qemu.Helper1"/>
+ <allow receive_sender="org.qemu.Helper1"/>
+ </policy>
+
+ <policy user="qemu-helper">
+ <allow own="org.qemu.Helper1"/>
+ </policy>
+
+
+dbus-daemon can also perfom SELinux checks based on the security
+context of the source and the target. For example, ``virtiofs_t``
+could be allowed to send a message to ``svirt_t``, but ``virtiofs_t``
+wouldn't be allowed to send a message to ``virtiofs_t``.
+
+See dbus-daemon man page for details.
+
+Guidelines
+==========
+
+When implementing new D-Bus interfaces, it is recommended to follow
+the "D-Bus API Design Guidelines":
+https://dbus.freedesktop.org/doc/dbus-api-design.html
+
+The "org.qemu.*" prefix is reserved for the QEMU project.
diff --git a/docs/interop/index.rst b/docs/interop/index.rst
index 3e33fb5933..ded134ea75 100644
--- a/docs/interop/index.rst
+++ b/docs/interop/index.rst
@@ -13,6 +13,7 @@ Contents:
:maxdepth: 2
bitmaps
+ dbus
live-block-operations
pr-helper
qemu-ga
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 4/8] util: add dbus helper unit
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
` (2 preceding siblings ...)
2019-12-11 13:45 ` [PATCH v6 3/8] docs: start a document to describe D-Bus usage Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-12 11:38 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 5/8] Add dbus-vmstate object Marc-André Lureau
` (3 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
MAINTAINERS | 2 ++
include/qemu/dbus.h | 18 +++++++++++++++
util/Makefile.objs | 3 +++
util/dbus.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 78 insertions(+)
create mode 100644 include/qemu/dbus.h
create mode 100644 util/dbus.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 19faa0e868..f08fb4f24e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2202,6 +2202,8 @@ F: qapi/migration.json
D-Bus
M: Marc-André Lureau <marcandre.lureau@redhat.com>
S: Maintained
+F: util/dbus.c
+F: include/qemu/dbus.h
F: docs/interop/dbus.rst
Seccomp
diff --git a/include/qemu/dbus.h b/include/qemu/dbus.h
new file mode 100644
index 0000000000..efd74bef96
--- /dev/null
+++ b/include/qemu/dbus.h
@@ -0,0 +1,18 @@
+/*
+ * Helpers for using D-Bus
+ *
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef DBUS_H
+#define DBUS_H
+
+#include <gio/gio.h>
+
+GStrv qemu_dbus_get_queued_owners(GDBusConnection *connection,
+ const char *name);
+
+#endif /* DBUS_H */
diff --git a/util/Makefile.objs b/util/Makefile.objs
index df124af1c5..80b76352cd 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -55,5 +55,8 @@ util-obj-$(CONFIG_INOTIFY1) += filemonitor-inotify.o
util-obj-$(CONFIG_LINUX) += vfio-helpers.o
util-obj-$(CONFIG_POSIX) += drm.o
util-obj-y += guest-random.o
+util-obj-$(CONFIG_GIO) += dbus.o
+dbus.o-cflags = $(GIO_CFLAGS)
+dbus.o-libs = $(GIO_LIBS)
stub-obj-y += filemonitor-stub.o
diff --git a/util/dbus.c b/util/dbus.c
new file mode 100644
index 0000000000..bb51870e54
--- /dev/null
+++ b/util/dbus.c
@@ -0,0 +1,55 @@
+/*
+ * Helpers for using D-Bus
+ *
+ * Copyright (C) 2019 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/dbus.h"
+#include "qemu/error-report.h"
+
+/*
+ * qemu_dbus_get_queued_owners() - return the list of queued unique names
+ * @connection: A GDBusConnection
+ * @name: a service name
+ *
+ * Return: a GStrv of unique names, or NULL on failure.
+ */
+GStrv
+qemu_dbus_get_queued_owners(GDBusConnection *connection, const char *name)
+{
+ g_autoptr(GDBusProxy) proxy = NULL;
+ g_autoptr(GVariant) result = NULL;
+ g_autoptr(GVariant) child = NULL;
+ g_autoptr(GError) err = NULL;
+
+ proxy = g_dbus_proxy_new_sync(connection, G_DBUS_PROXY_FLAGS_NONE, NULL,
+ "org.freedesktop.DBus",
+ "/org/freedesktop/DBus",
+ "org.freedesktop.DBus",
+ NULL, &err);
+ if (!proxy) {
+ error_report("Failed to create DBus proxy: %s", err->message);
+ return NULL;
+ }
+
+ result = g_dbus_proxy_call_sync(proxy, "ListQueuedOwners",
+ g_variant_new("(s)", name),
+ G_DBUS_CALL_FLAGS_NO_AUTO_START,
+ -1, NULL, &err);
+ if (!result) {
+ if (g_error_matches(err,
+ G_DBUS_ERROR,
+ G_DBUS_ERROR_NAME_HAS_NO_OWNER)) {
+ return g_new0(char *, 1);
+ }
+ error_report("Failed to call ListQueuedOwners: %s", err->message);
+ return NULL;
+ }
+
+ child = g_variant_get_child_value(result, 0);
+ return g_variant_dup_strv(child, NULL);
+}
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 5/8] Add dbus-vmstate object
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
` (3 preceding siblings ...)
2019-12-11 13:45 ` [PATCH v6 4/8] util: add dbus helper unit Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-12 12:03 ` Daniel P. Berrangé
2019-12-13 16:32 ` Dr. David Alan Gilbert
2019-12-11 13:45 ` [PATCH v6 6/8] configure: add GDBUS_CODEGEN Marc-André Lureau
` (2 subsequent siblings)
7 siblings, 2 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
When instantiated, this object will connect to the given D-Bus bus
"addr". During migration, it will take/restore the data from
org.qemu.VMState1 instances. See documentation for details.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
MAINTAINERS | 2 +
backends/Makefile.objs | 4 +
backends/dbus-vmstate.c | 496 ++++++++++++++++++++++++++++++++++
docs/interop/dbus-vmstate.rst | 74 +++++
docs/interop/dbus.rst | 5 +
docs/interop/index.rst | 1 +
6 files changed, 582 insertions(+)
create mode 100644 backends/dbus-vmstate.c
create mode 100644 docs/interop/dbus-vmstate.rst
diff --git a/MAINTAINERS b/MAINTAINERS
index f08fb4f24e..7af80d0c1d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2202,9 +2202,11 @@ F: qapi/migration.json
D-Bus
M: Marc-André Lureau <marcandre.lureau@redhat.com>
S: Maintained
+F: backends/dbus-vmstate.c
F: util/dbus.c
F: include/qemu/dbus.h
F: docs/interop/dbus.rst
+F: docs/interop/dbus-vmstate.rst
Seccomp
M: Eduardo Otubo <otubo@redhat.com>
diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index f0691116e8..28a847cd57 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)
diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
new file mode 100644
index 0000000000..059dd420b8
--- /dev/null
+++ b/backends/dbus-vmstate.c
@@ -0,0 +1,496 @@
+/*
+ * QEMU dbus-vmstate
+ *
+ * Copyright (C) 2019 Red Hat Inc
+ *
+ * Authors:
+ * Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qemu/dbus.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "qapi/qmp/qerror.h"
+#include "migration/vmstate.h"
+
+typedef struct DBusVMState DBusVMState;
+typedef struct DBusVMStateClass DBusVMStateClass;
+
+#define TYPE_DBUS_VMSTATE "dbus-vmstate"
+#define DBUS_VMSTATE(obj) \
+ OBJECT_CHECK(DBusVMState, (obj), TYPE_DBUS_VMSTATE)
+#define DBUS_VMSTATE_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(DBusVMStateClass, (obj), TYPE_DBUS_VMSTATE)
+#define DBUS_VMSTATE_CLASS(klass) \
+ OBJECT_CLASS_CHECK(DBusVMStateClass, (klass), TYPE_DBUS_VMSTATE)
+
+struct DBusVMStateClass {
+ ObjectClass parent_class;
+};
+
+struct DBusVMState {
+ Object parent;
+
+ GDBusConnection *bus;
+ char *dbus_addr;
+ char *id_list;
+
+ uint32_t data_size;
+ uint8_t *data;
+};
+
+static const GDBusPropertyInfo vmstate_property_info[] = {
+ { -1, (char *) "Id", (char *) "s",
+ G_DBUS_PROPERTY_INFO_FLAGS_READABLE, NULL },
+};
+
+static const GDBusPropertyInfo * const vmstate_property_info_pointers[] = {
+ &vmstate_property_info[0],
+ NULL
+};
+
+static const GDBusInterfaceInfo vmstate1_interface_info = {
+ -1,
+ (char *) "org.qemu.VMState1",
+ (GDBusMethodInfo **) NULL,
+ (GDBusSignalInfo **) NULL,
+ (GDBusPropertyInfo **) &vmstate_property_info_pointers,
+ NULL,
+};
+
+#define DBUS_VMSTATE_SIZE_LIMIT (1 * MiB)
+
+static GHashTable *
+get_id_list_set(DBusVMState *self)
+{
+ g_auto(GStrv) ids = NULL;
+ g_autoptr(GHashTable) set = NULL;
+ 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;
+ }
+
+ return g_steal_pointer(&set);
+}
+
+static GHashTable *
+dbus_get_proxies(DBusVMState *self, GError **err)
+{
+ g_autoptr(GHashTable) proxies = NULL;
+ g_autoptr(GHashTable) ids = NULL;
+ g_auto(GStrv) names = NULL;
+ size_t i;
+
+ ids = get_id_list_set(self);
+ proxies = g_hash_table_new_full(g_str_hash, g_str_equal,
+ g_free, g_object_unref);
+
+ names = qemu_dbus_get_queued_owners(self->bus, "org.qemu.VMState1");
+ if (!names) {
+ return NULL;
+ }
+
+ for (i = 0; names[i]; i++) {
+ g_autoptr(GDBusProxy) proxy = NULL;
+ g_autoptr(GVariant) result = NULL;
+ g_autofree char *id = NULL;
+ 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) {
+ return NULL;
+ }
+
+ 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.");
+ return NULL;
+ }
+
+ id = g_variant_dup_string(result, &size);
+ if (ids && !g_hash_table_remove(ids, id)) {
+ g_clear_pointer(&id, g_free);
+ g_clear_object(&proxy);
+ continue;
+ }
+ if (size == 0 || size >= 256) {
+ g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
+ "VMState Id '%s' is invalid.", id);
+ return NULL;
+ }
+
+ if (!g_hash_table_insert(proxies, id, proxy)) {
+ g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
+ "Duplicated VMState Id '%s'", id);
+ return NULL;
+ }
+ id = NULL;
+ proxy = NULL;
+
+ g_clear_pointer(&result, g_variant_unref);
+ }
+
+ if (ids) {
+ g_autofree char **left = NULL;
+
+ left = (char **)g_hash_table_get_keys_as_array(ids, NULL);
+ if (*left) {
+ g_autofree char *leftids = g_strjoinv(",", left);
+ g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
+ "Required VMState Id are missing: %s", leftids);
+ return NULL;
+ }
+ }
+
+ return g_steal_pointer(&proxies);
+}
+
+static int
+dbus_load_state_proxy(GDBusProxy *proxy, const uint8_t *data, size_t size)
+{
+ g_autoptr(GError) err = NULL;
+ g_autoptr(GVariant) result = NULL;
+ g_autoptr(GVariant) value = NULL;
+
+ 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)",
+ g_steal_pointer(&value)),
+ G_DBUS_CALL_FLAGS_NO_AUTO_START,
+ -1, NULL, &err);
+ if (!result) {
+ error_report("Failed to Load: %s", err->message);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int dbus_vmstate_post_load(void *opaque, int version_id)
+{
+ DBusVMState *self = DBUS_VMSTATE(opaque);
+ g_autoptr(GInputStream) m = NULL;
+ g_autoptr(GDataInputStream) s = NULL;
+ g_autoptr(GError) err = NULL;
+ g_autoptr(GHashTable) proxies = NULL;
+ uint32_t nelem;
+
+ proxies = dbus_get_proxies(self, &err);
+ if (!proxies) {
+ error_report("Failed to get proxies: %s", err->message);
+ return -1;
+ }
+
+ m = g_memory_input_stream_new_from_data(self->data, self->data_size, NULL);
+ s = g_data_input_stream_new(m);
+ g_data_input_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
+
+ nelem = g_data_input_stream_read_uint32(s, NULL, &err);
+ if (err) {
+ goto error;
+ }
+
+ while (nelem > 0) {
+ GDBusProxy *proxy = NULL;
+ uint32_t len;
+ gsize bytes_read, avail;
+ char id[256];
+
+ len = g_data_input_stream_read_uint32(s, NULL, &err);
+ if (err) {
+ goto error;
+ }
+ if (len >= 256) {
+ error_report("Invalid DBus vmstate proxy name %u", len);
+ return -1;
+ }
+ if (!g_input_stream_read_all(G_INPUT_STREAM(s), id, len,
+ &bytes_read, NULL, &err)) {
+ goto error;
+ }
+ g_return_val_if_fail(bytes_read == len, -1);
+ id[len] = 0;
+
+ proxy = g_hash_table_lookup(proxies, id);
+ if (!proxy) {
+ error_report("Failed to find proxy Id '%s'", id);
+ return -1;
+ }
+
+ len = g_data_input_stream_read_uint32(s, NULL, &err);
+ avail = g_buffered_input_stream_get_available(
+ G_BUFFERED_INPUT_STREAM(s));
+
+ if (len > DBUS_VMSTATE_SIZE_LIMIT || len > avail) {
+ error_report("Invalid vmstate size: %u", len);
+ return -1;
+ }
+
+ if (dbus_load_state_proxy(proxy,
+ g_buffered_input_stream_peek_buffer(G_BUFFERED_INPUT_STREAM(s),
+ NULL),
+ len) < 0) {
+ error_report("Failed to restore Id '%s'", id);
+ return -1;
+ }
+
+ if (!g_seekable_seek(G_SEEKABLE(s), len, G_SEEK_CUR, NULL, &err)) {
+ goto error;
+ }
+
+ nelem -= 1;
+ }
+
+ return 0;
+
+error:
+ error_report("Failed to read from stream: %s", err->message);
+ return -1;
+}
+
+static void
+dbus_save_state_proxy(gpointer key,
+ gpointer value,
+ gpointer user_data)
+{
+ GDataOutputStream *s = user_data;
+ const char *id = key;
+ GDBusProxy *proxy = value;
+ g_autoptr(GVariant) result = NULL;
+ g_autoptr(GVariant) child = NULL;
+ g_autoptr(GError) err = NULL;
+ const uint8_t *data;
+ gsize size;
+
+ 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);
+ return;
+ }
+
+ 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");
+ return;
+ }
+ if (size > DBUS_VMSTATE_SIZE_LIMIT) {
+ error_report("Too large vmstate data to save: %" G_GSIZE_FORMAT, size);
+ return;
+ }
+
+ if (!g_data_output_stream_put_uint32(s, strlen(id), NULL, &err) ||
+ !g_data_output_stream_put_string(s, id, NULL, &err) ||
+ !g_data_output_stream_put_uint32(s, size, NULL, &err) ||
+ !g_output_stream_write_all(G_OUTPUT_STREAM(s),
+ data, size, NULL, NULL, &err)) {
+ error_report("Failed to write to stream: %s", err->message);
+ }
+}
+
+static int dbus_vmstate_pre_save(void *opaque)
+{
+ DBusVMState *self = DBUS_VMSTATE(opaque);
+ g_autoptr(GOutputStream) m = NULL;
+ g_autoptr(GDataOutputStream) s = NULL;
+ g_autoptr(GHashTable) proxies = NULL;
+ g_autoptr(GError) err = NULL;
+
+ proxies = dbus_get_proxies(self, &err);
+ if (!proxies) {
+ error_report("Failed to get proxies: %s", err->message);
+ return -1;
+ }
+
+ m = g_memory_output_stream_new_resizable();
+ s = g_data_output_stream_new(m);
+ g_data_output_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
+
+ if (!g_data_output_stream_put_uint32(s, g_hash_table_size(proxies),
+ NULL, &err)) {
+ error_report("Failed to write to stream: %s", err->message);
+ return -1;
+ }
+
+ g_hash_table_foreach(proxies, dbus_save_state_proxy, s);
+
+ if (g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m))
+ > UINT32_MAX) {
+ error_report("DBus vmstate buffer is too large");
+ return -1;
+ }
+
+ if (!g_output_stream_close(G_OUTPUT_STREAM(m), NULL, &err)) {
+ error_report("Failed to close stream: %s", err->message);
+ return -1;
+ }
+
+ g_free(self->data);
+ self->data_size =
+ g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m));
+ self->data =
+ g_memory_output_stream_steal_data(G_MEMORY_OUTPUT_STREAM(m));
+
+ return 0;
+}
+
+static const VMStateDescription dbus_vmstate = {
+ .name = TYPE_DBUS_VMSTATE,
+ .version_id = 0,
+ .pre_save = dbus_vmstate_pre_save,
+ .post_load = dbus_vmstate_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(data_size, DBusVMState),
+ VMSTATE_VBUFFER_ALLOC_UINT32(data, DBusVMState, 0, 0, data_size),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+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;
+ }
+
+ 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;
+
+ if (vmstate_register(VMSTATE_IF(self), -1, &dbus_vmstate, self) < 0) {
+ error_setg(errp, "Failed to register vmstate");
+ }
+}
+
+static void
+dbus_vmstate_finalize(Object *o)
+{
+ DBusVMState *self = DBUS_VMSTATE(o);
+
+ vmstate_unregister(VMSTATE_IF(self), &dbus_vmstate, self);
+
+ g_clear_object(&self->bus);
+ g_free(self->dbus_addr);
+ g_free(self->id_list);
+ g_free(self->data);
+}
+
+static char *
+get_dbus_addr(Object *o, Error **errp)
+{
+ DBusVMState *self = DBUS_VMSTATE(o);
+
+ return g_strdup(self->dbus_addr);
+}
+
+static void
+set_dbus_addr(Object *o, const char *str, Error **errp)
+{
+ DBusVMState *self = DBUS_VMSTATE(o);
+
+ g_free(self->dbus_addr);
+ self->dbus_addr = g_strdup(str);
+}
+
+static char *
+get_id_list(Object *o, Error **errp)
+{
+ DBusVMState *self = DBUS_VMSTATE(o);
+
+ return g_strdup(self->id_list);
+}
+
+static void
+set_id_list(Object *o, const char *str, Error **errp)
+{
+ DBusVMState *self = DBUS_VMSTATE(o);
+
+ g_free(self->id_list);
+ self->id_list = g_strdup(str);
+}
+
+static char *
+dbus_vmstate_get_id(VMStateIf *vmif)
+{
+ return g_strdup(TYPE_DBUS_VMSTATE);
+}
+
+static void
+dbus_vmstate_class_init(ObjectClass *oc, void *data)
+{
+ UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+ VMStateIfClass *vc = VMSTATE_IF_CLASS(oc);
+
+ ucc->complete = dbus_vmstate_complete;
+ vc->get_id = dbus_vmstate_get_id;
+
+ object_class_property_add_str(oc, "addr",
+ get_dbus_addr, set_dbus_addr,
+ &error_abort);
+ object_class_property_add_str(oc, "id-list",
+ get_id_list, set_id_list,
+ &error_abort);
+}
+
+static const TypeInfo dbus_vmstate_info = {
+ .name = TYPE_DBUS_VMSTATE,
+ .parent = TYPE_OBJECT,
+ .instance_size = sizeof(DBusVMState),
+ .instance_finalize = dbus_vmstate_finalize,
+ .class_size = sizeof(DBusVMStateClass),
+ .class_init = dbus_vmstate_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_USER_CREATABLE },
+ { TYPE_VMSTATE_IF },
+ { }
+ }
+};
+
+static void
+register_types(void)
+{
+ type_register_static(&dbus_vmstate_info);
+}
+
+type_init(register_types);
diff --git a/docs/interop/dbus-vmstate.rst b/docs/interop/dbus-vmstate.rst
new file mode 100644
index 0000000000..8693891640
--- /dev/null
+++ b/docs/interop/dbus-vmstate.rst
@@ -0,0 +1,74 @@
+=============
+D-Bus VMState
+=============
+
+Introduction
+============
+
+The QEMU dbus-vmstate object's aim is to migrate helpers' data running
+on a QEMU D-Bus bus. (refer to the :doc:`dbus` document for
+recommendation on D-Bus usage)
+
+Upon migration, QEMU will go through the queue of
+``org.qemu.VMState1`` D-Bus 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). (D-Bus imposes a time limit on
+reply anyway, and migration would fail if data isn't given quickly
+enough.)
+
+dbus-vmstate object can be configured with the expected list of
+helpers by setting its ``id-list`` property, with a comma-separated
+``Id`` list.
+
+Interface
+=========
+
+On object path ``/org/qemu/VMState1``, the following
+``org.qemu.VMState1`` interface should be implemented:
+
+.. code:: xml
+
+ <interface name="org.qemu.VMState1">
+ <property name="Id" type="s" access="read"/>
+ <method name="Load">
+ <arg type="ay" name="data" direction="in"/>
+ </method>
+ <method name="Save">
+ <arg type="ay" name="data" direction="out"/>
+ </method>
+ </interface>
+
+"Id" property
+-------------
+
+A string that identifies the helper uniquely. (maximum 256 bytes
+including terminating NUL byte)
+
+.. note::
+
+ The helper ID namespace is a separate namespace. In particular, it is not
+ related to QEMU "id" used in -object/-device objects.
+
+Load(in u8[] bytes) method
+--------------------------
+
+The method called on destination with the state to restore.
+
+The helper may be initially started in a waiting state (with
+an --incoming argument for example), and it may resume on success.
+
+An error may be returned to the caller.
+
+Save(out u8[] bytes) method
+---------------------------
+
+The method called on the source to get the current state to be
+migrated. The helper should continue to run normally.
+
+An error may be returned to the caller.
diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst
index 3d760e4882..d9af608dc9 100644
--- a/docs/interop/dbus.rst
+++ b/docs/interop/dbus.rst
@@ -97,3 +97,8 @@ the "D-Bus API Design Guidelines":
https://dbus.freedesktop.org/doc/dbus-api-design.html
The "org.qemu.*" prefix is reserved for the QEMU project.
+
+QEMU Interfaces
+===============
+
+:doc:`dbus-vmstate`
diff --git a/docs/interop/index.rst b/docs/interop/index.rst
index ded134ea75..049387ac6d 100644
--- a/docs/interop/index.rst
+++ b/docs/interop/index.rst
@@ -14,6 +14,7 @@ Contents:
bitmaps
dbus
+ dbus-vmstate
live-block-operations
pr-helper
qemu-ga
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 6/8] configure: add GDBUS_CODEGEN
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
` (4 preceding siblings ...)
2019-12-11 13:45 ` [PATCH v6 5/8] Add dbus-vmstate object Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-12 12:05 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 8/8] tests: add dbus-vmstate-test Marc-André Lureau
7 siblings, 1 reply; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
gdbus-codegen generated code requires gio-unix on Unix, so add it to
GIO libs/cflags.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
configure | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/configure b/configure
index 6099be1d84..68a7705df7 100755
--- a/configure
+++ b/configure
@@ -3720,10 +3720,16 @@ if $pkg_config --atleast-version=$glib_req_ver gio-2.0; then
gio=yes
gio_cflags=$($pkg_config --cflags gio-2.0)
gio_libs=$($pkg_config --libs gio-2.0)
+ gdbus_codegen=$($pkg_config --variable=gdbus_codegen gio-2.0)
else
gio=no
fi
+if $pkg_config --atleast-version=$glib_req_ver gio-unix-2.0; then
+ gio_cflags="$gio_cflags $($pkg_config --cflags gio-unix-2.0)"
+ gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
+fi
+
# Sanity check that the current size_t matches the
# size that glib thinks it should be. This catches
# problems on multi-arch where people try to build
@@ -6949,6 +6955,7 @@ if test "$gio" = "yes" ; then
echo "CONFIG_GIO=y" >> $config_host_mak
echo "GIO_CFLAGS=$gio_cflags" >> $config_host_mak
echo "GIO_LIBS=$gio_libs" >> $config_host_mak
+ echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
fi
echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
if test "$gnutls" = "yes" ; then
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
` (5 preceding siblings ...)
2019-12-11 13:45 ` [PATCH v6 6/8] configure: add GDBUS_CODEGEN Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-12 12:06 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 8/8] tests: add dbus-vmstate-test Marc-André Lureau
7 siblings, 1 reply; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
To get dbus-vmstate test covered.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
tests/docker/dockerfiles/centos7.docker | 1 +
tests/docker/dockerfiles/debian10.docker | 1 +
tests/docker/dockerfiles/fedora.docker | 1 +
tests/docker/dockerfiles/ubuntu.docker | 1 +
4 files changed, 4 insertions(+)
diff --git a/tests/docker/dockerfiles/centos7.docker b/tests/docker/dockerfiles/centos7.docker
index 953637065c..562d65be9e 100644
--- a/tests/docker/dockerfiles/centos7.docker
+++ b/tests/docker/dockerfiles/centos7.docker
@@ -8,6 +8,7 @@ ENV PACKAGES \
bzip2-devel \
ccache \
csnappy-devel \
+ dbus-daemon \
flex \
gcc-c++ \
gcc \
diff --git a/tests/docker/dockerfiles/debian10.docker b/tests/docker/dockerfiles/debian10.docker
index dad498b52e..5de79ae552 100644
--- a/tests/docker/dockerfiles/debian10.docker
+++ b/tests/docker/dockerfiles/debian10.docker
@@ -21,6 +21,7 @@ RUN apt update && \
build-essential \
ca-certificates \
clang \
+ dbus \
flex \
gettext \
git \
diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker
index 4ddc7dd112..4fb1215520 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -9,6 +9,7 @@ ENV PACKAGES \
ccache \
clang \
cyrus-sasl-devel \
+ dbus-daemon \
device-mapper-multipath-devel \
findutils \
flex \
diff --git a/tests/docker/dockerfiles/ubuntu.docker b/tests/docker/dockerfiles/ubuntu.docker
index f486492224..2182edadea 100644
--- a/tests/docker/dockerfiles/ubuntu.docker
+++ b/tests/docker/dockerfiles/ubuntu.docker
@@ -13,6 +13,7 @@ FROM ubuntu:19.04
ENV PACKAGES flex bison \
ccache \
clang \
+ dbus \
gcc \
gettext \
git \
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v6 8/8] tests: add dbus-vmstate-test
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
` (6 preceding siblings ...)
2019-12-11 13:45 ` [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions Marc-André Lureau
@ 2019-12-11 13:45 ` Marc-André Lureau
2019-12-12 12:11 ` Daniel P. Berrangé
2019-12-13 18:20 ` Dr. David Alan Gilbert
7 siblings, 2 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-11 13:45 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, quintela, mprivozn, dgilbert, Marc-André Lureau, pbonzini
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
MAINTAINERS | 1 +
tests/Makefile.include | 22 +-
tests/dbus-vmstate-daemon.sh | 95 +++++++++
tests/dbus-vmstate-test.c | 399 +++++++++++++++++++++++++++++++++++
tests/dbus-vmstate1.xml | 12 ++
5 files changed, 528 insertions(+), 1 deletion(-)
create mode 100755 tests/dbus-vmstate-daemon.sh
create mode 100644 tests/dbus-vmstate-test.c
create mode 100644 tests/dbus-vmstate1.xml
diff --git a/MAINTAINERS b/MAINTAINERS
index 7af80d0c1d..cc34e027f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2203,6 +2203,7 @@ D-Bus
M: Marc-André Lureau <marcandre.lureau@redhat.com>
S: Maintained
F: backends/dbus-vmstate.c
+F: tests/dbus-vmstate*
F: util/dbus.c
F: include/qemu/dbus.h
F: docs/interop/dbus.rst
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e2ab8ba334..000021d752 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -160,12 +160,17 @@ check-qtest-generic-$(CONFIG_MODULES) += tests/modules-test$(EXESUF)
check-qtest-generic-y += tests/device-introspect-test$(EXESUF)
check-qtest-generic-y += tests/cdrom-test$(EXESUF)
+DBUS_DAEMON := $(shell which dbus-daemon 2>/dev/null)
+ifneq ($(GDBUS_CODEGEN),)
+ifneq ($(DBUS_DAEMON),)
+check-qtest-pci-$(CONFIG_GIO) += tests/dbus-vmstate-test$(EXESUF)
+endif
+endif
check-qtest-pci-$(CONFIG_RTL8139_PCI) += tests/rtl8139-test$(EXESUF)
check-qtest-pci-$(CONFIG_VGA) += tests/display-vga-test$(EXESUF)
check-qtest-pci-$(CONFIG_HDA) += tests/intel-hda-test$(EXESUF)
check-qtest-pci-$(CONFIG_IVSHMEM_DEVICE) += tests/ivshmem-test$(EXESUF)
-
check-qtest-i386-$(CONFIG_ISA_TESTDEV) = tests/endianness-test$(EXESUF)
check-qtest-i386-y += tests/fdc-test$(EXESUF)
check-qtest-i386-y += tests/ide-test$(EXESUF)
@@ -636,6 +641,19 @@ tests/qapi-schema/doc-good.test.texi: $(SRC_PATH)/tests/qapi-schema/doc-good.jso
@mv tests/qapi-schema/doc-good-qapi-doc.texi $@
@rm -f tests/qapi-schema/doc-good-qapi-*.[ch] tests/qapi-schema/doc-good-qmp-*.[ch]
+tests/dbus-vmstate1.h tests/dbus-vmstate1.c: tests/dbus-vmstate1-gen-timestamp ;
+tests/dbus-vmstate1-gen-timestamp: $(SRC_PATH)/tests/dbus-vmstate1.xml
+ $(call quiet-command,$(GDBUS_CODEGEN) $< \
+ --interface-prefix org.qemu --generate-c-code tests/dbus-vmstate1, \
+ "GEN","$(@:%-timestamp=%)")
+ @>$@
+
+tests/dbus-vmstate-test.o-cflags := -DSRCDIR="$(SRC_PATH)"
+tests/dbus-vmstate1.o-cflags := $(GIO_CFLAGS)
+tests/dbus-vmstate1.o-libs := $(GIO_LIBS)
+
+tests/dbus-vmstate-test.o: tests/dbus-vmstate1.h
+
tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $(test-qapi-obj-y)
tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(test-qapi-obj-y)
tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y) tests/test-qapi-events.o
@@ -839,6 +857,7 @@ tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
tests/test-filter-redirector$(EXESUF): tests/test-filter-redirector.o $(qtest-obj-y)
tests/test-x86-cpuid-compat$(EXESUF): tests/test-x86-cpuid-compat.o $(qtest-obj-y)
tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y) $(libqos-spapr-obj-y)
+tests/dbus-vmstate-test$(EXESUF): tests/dbus-vmstate-test.o tests/dbus-vmstate1.o $(libqos-pc-obj-y) $(libqos-spapr-obj-y)
tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o $(test-util-obj-y) libvhost-user.a
tests/test-uuid$(EXESUF): tests/test-uuid.o $(test-util-obj-y)
tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
@@ -1200,6 +1219,7 @@ check-clean:
rm -rf $(check-unit-y) tests/*.o $(QEMU_IOTESTS_HELPERS-y)
rm -rf $(sort $(foreach target,$(SYSEMU_TARGET_LIST), $(check-qtest-$(target)-y)) $(check-qtest-generic-y))
rm -f tests/test-qapi-gen-timestamp
+ rm -f tests/dbus-vmstate1-gen-timestamp
rm -rf $(TESTS_VENV_DIR) $(TESTS_RESULTS_DIR)
clean: check-clean
diff --git a/tests/dbus-vmstate-daemon.sh b/tests/dbus-vmstate-daemon.sh
new file mode 100755
index 0000000000..c4394ac918
--- /dev/null
+++ b/tests/dbus-vmstate-daemon.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+# dbus-daemon wrapper script for dbus-vmstate testing
+#
+# This script allows to tweak the dbus-daemon policy during the test
+# to test different configurations.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, see <http://www.gnu.org/licenses/>.
+#
+# Copyright (C) 2019 Red Hat, Inc.
+
+write_config()
+{
+ CONF="$1"
+ cat > "$CONF" <<EOF
+<busconfig>
+ <type>session</type>
+ <listen>unix:tmpdir=/tmp</listen>
+
+ <policy context="default">
+ <!-- Holes must be punched in service configuration files for
+ name ownership and sending method calls -->
+ <deny own="*"/>
+ <deny send_type="method_call"/>
+
+ <!-- Signals and reply messages (method returns, errors) are allowed
+ by default -->
+ <allow send_type="signal"/>
+ <allow send_requested_reply="true" send_type="method_return"/>
+ <allow send_requested_reply="true" send_type="error"/>
+
+ <!-- All messages may be received by default -->
+ <allow receive_type="method_call"/>
+ <allow receive_type="method_return"/>
+ <allow receive_type="error"/>
+ <allow receive_type="signal"/>
+
+ <!-- Allow anyone to talk to the message bus -->
+ <allow send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus" />
+ <allow send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus.Introspectable"/>
+ <allow send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus.Properties"/>
+ <!-- But disallow some specific bus services -->
+ <deny send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus"
+ send_member="UpdateActivationEnvironment"/>
+ <deny send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.DBus.Debug.Stats"/>
+ <deny send_destination="org.freedesktop.DBus"
+ send_interface="org.freedesktop.systemd1.Activator"/>
+
+ <allow own="org.qemu.VMState1"/>
+ <allow send_destination="org.qemu.VMState1"/>
+ <allow receive_sender="org.qemu.VMState1"/>
+
+ </policy>
+
+ <include if_selinux_enabled="yes"
+ selinux_root_relative="yes">contexts/dbus_contexts</include>
+
+</busconfig>
+EOF
+}
+
+ARGS=
+for arg in "$@"
+do
+ case $arg in
+ --config-file=*)
+ CONF="${arg#*=}"
+ write_config "$CONF"
+ ARGS="$ARGS $1"
+ shift
+ ;;
+ *)
+ ARGS="$ARGS $1"
+ shift
+ ;;
+ esac
+done
+
+exec dbus-daemon $ARGS
diff --git a/tests/dbus-vmstate-test.c b/tests/dbus-vmstate-test.c
new file mode 100644
index 0000000000..34818c1e79
--- /dev/null
+++ b/tests/dbus-vmstate-test.c
@@ -0,0 +1,399 @@
+#include "qemu/osdep.h"
+#include <glib/gstdio.h>
+#include <gio/gio.h>
+#include "libqtest.h"
+#include "qemu-common.h"
+#include "dbus-vmstate1.h"
+
+static char *workdir;
+
+typedef struct TestServerId {
+ const char *name;
+ const char *data;
+ size_t size;
+} TestServerId;
+
+static const TestServerId idA = {
+ "idA", "I'am\0idA!", sizeof("I'am\0idA!")
+};
+
+static const TestServerId idB = {
+ "idB", "I'am\0idB!", sizeof("I'am\0idB!")
+};
+
+typedef struct TestServer {
+ const TestServerId *id;
+ bool save_called;
+ bool load_called;
+} TestServer;
+
+typedef struct Test {
+ const char *id_list;
+ bool migrate_fail;
+ bool without_dst_b;
+ TestServer srcA;
+ TestServer dstA;
+ TestServer srcB;
+ TestServer dstB;
+ GMainLoop *loop;
+ QTestState *src_qemu;
+} Test;
+
+static gboolean
+vmstate_load(VMState1 *object, GDBusMethodInvocation *invocation,
+ const gchar *arg_data, gpointer user_data)
+{
+ TestServer *h = user_data;
+ g_autoptr(GVariant) var = NULL;
+ GVariant *args;
+ const uint8_t *data;
+ size_t size;
+
+ args = g_dbus_method_invocation_get_parameters(invocation);
+ var = g_variant_get_child_value(args, 0);
+ data = g_variant_get_fixed_array(var, &size, sizeof(char));
+ g_assert_cmpuint(size, ==, h->id->size);
+ g_assert(!memcmp(data, h->id->data, h->id->size));
+ h->load_called = true;
+
+ g_dbus_method_invocation_return_value(invocation, g_variant_new("()"));
+ return TRUE;
+}
+
+static gboolean
+vmstate_save(VMState1 *object, GDBusMethodInvocation *invocation,
+ gpointer user_data)
+{
+ TestServer *h = user_data;
+ GVariant *var;
+
+ var = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE,
+ h->id->data, h->id->size, sizeof(char));
+ g_dbus_method_invocation_return_value(invocation,
+ g_variant_new("(@ay)", var));
+ h->save_called = true;
+
+ return TRUE;
+}
+
+static gboolean
+wait_for_migration_complete(gpointer user_data)
+{
+ Test *test = user_data;
+ QDict *rsp_return;
+ bool stop = false;
+ const char *status;
+
+ qtest_qmp_send(test->src_qemu, "{ 'execute': 'query-migrate' }");
+ rsp_return = qtest_qmp_receive_success(test->src_qemu, NULL, NULL);
+ status = qdict_get_str(rsp_return, "status");
+ if (g_str_equal(status, "completed") || g_str_equal(status, "failed")) {
+ stop = true;
+ g_assert_cmpstr(status, ==,
+ test->migrate_fail ? "failed" : "completed");
+ }
+ qobject_unref(rsp_return);
+
+ if (stop) {
+ g_main_loop_quit(test->loop);
+ }
+ return stop ? G_SOURCE_REMOVE : G_SOURCE_CONTINUE;
+}
+
+static void migrate(QTestState *who, const char *uri)
+{
+ QDict *args, *rsp;
+
+ args = qdict_new();
+ qdict_put_str(args, "uri", uri);
+
+ rsp = qtest_qmp(who, "{ 'execute': 'migrate', 'arguments': %p }", args);
+
+ g_assert(qdict_haskey(rsp, "return"));
+ qobject_unref(rsp);
+}
+
+typedef struct WaitNamed {
+ GMainLoop *loop;
+ bool named;
+} WaitNamed;
+
+static void
+named_cb(GDBusConnection *connection,
+ const gchar *name,
+ gpointer user_data)
+{
+ WaitNamed *t = user_data;
+
+ t->named = true;
+ g_main_loop_quit(t->loop);
+}
+
+static GDBusConnection *
+get_connection(Test *test, guint *ownid)
+{
+ g_autofree gchar *addr = NULL;
+ WaitNamed *wait;
+ GError *err = NULL;
+ GDBusConnection *c;
+
+ wait = g_new0(WaitNamed, 1);
+ wait->loop = test->loop;
+ addr = g_dbus_address_get_for_bus_sync(G_BUS_TYPE_SESSION, NULL, &err);
+ g_assert_no_error(err);
+
+ c = g_dbus_connection_new_for_address_sync(
+ addr,
+ G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION |
+ G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT,
+ NULL, NULL, &err);
+ g_assert_no_error(err);
+ *ownid = g_bus_own_name_on_connection(c, "org.qemu.VMState1",
+ G_BUS_NAME_OWNER_FLAGS_NONE,
+ named_cb, named_cb, wait, g_free);
+ if (!wait->named) {
+ g_main_loop_run(wait->loop);
+ }
+
+ return c;
+}
+
+static GDBusObjectManagerServer *
+get_server(GDBusConnection *conn, TestServer *s, const TestServerId *id)
+{
+ g_autoptr(GDBusObjectSkeleton) sk = NULL;
+ g_autoptr(VMState1Skeleton) v = NULL;
+ GDBusObjectManagerServer *os;
+
+ s->id = id;
+ os = g_dbus_object_manager_server_new("/org/qemu");
+ sk = g_dbus_object_skeleton_new("/org/qemu/VMState1");
+
+ v = VMSTATE1_SKELETON(vmstate1_skeleton_new());
+ g_object_set(v, "id", id->name, NULL);
+
+ g_signal_connect(v, "handle-load", G_CALLBACK(vmstate_load), s);
+ g_signal_connect(v, "handle-save", G_CALLBACK(vmstate_save), s);
+
+ g_dbus_object_skeleton_add_interface(sk, G_DBUS_INTERFACE_SKELETON(v));
+ g_dbus_object_manager_server_export(os, sk);
+ g_dbus_object_manager_server_set_connection(os, conn);
+
+ return os;
+}
+
+static void
+set_id_list(Test *test, QTestState *s)
+{
+ if (!test->id_list) {
+ return;
+ }
+
+ g_assert(!qmp_rsp_is_err(qtest_qmp(s,
+ "{ 'execute': 'qom-set', 'arguments': "
+ "{ 'path': '/objects/dv', 'property': 'id-list', 'value': %s } }",
+ test->id_list)));
+}
+static void
+test_dbus_vmstate(Test *test)
+{
+ g_autofree char *src_qemu_args = NULL;
+ g_autofree char *dst_qemu_args = NULL;
+ g_autoptr(GTestDBus) srcbus = NULL;
+ g_autoptr(GTestDBus) dstbus = NULL;
+ g_autoptr(GDBusConnection) srcconnA = NULL;
+ g_autoptr(GDBusConnection) srcconnB = NULL;
+ g_autoptr(GDBusConnection) dstconnA = NULL;
+ g_autoptr(GDBusConnection) dstconnB = NULL;
+ g_autoptr(GDBusObjectManagerServer) srcserverA = NULL;
+ g_autoptr(GDBusObjectManagerServer) srcserverB = NULL;
+ g_autoptr(GDBusObjectManagerServer) dstserverA = NULL;
+ g_autoptr(GDBusObjectManagerServer) dstserverB = NULL;
+ g_auto(GStrv) srcaddr = NULL;
+ g_auto(GStrv) dstaddr = NULL;
+ g_autofree char *uri = NULL;
+ QTestState *src_qemu = NULL, *dst_qemu = NULL;
+ guint ownsrcA, ownsrcB, owndstA, owndstB;
+
+ uri = g_strdup_printf("unix:%s/migsocket", workdir);
+
+ test->loop = g_main_loop_new(NULL, TRUE);
+
+ srcbus = g_test_dbus_new(G_TEST_DBUS_NONE);
+ g_test_dbus_up(srcbus);
+ srcconnA = get_connection(test, &ownsrcA);
+ srcserverA = get_server(srcconnA, &test->srcA, &idA);
+ srcconnB = get_connection(test, &ownsrcB);
+ srcserverB = get_server(srcconnB, &test->srcB, &idB);
+
+ /* remove ,guid=foo part */
+ srcaddr = g_strsplit(g_test_dbus_get_bus_address(srcbus), ",", 2);
+ src_qemu_args =
+ g_strdup_printf("-object dbus-vmstate,id=dv,addr=%s", srcaddr[0]);
+
+ dstbus = g_test_dbus_new(G_TEST_DBUS_NONE);
+ g_test_dbus_up(dstbus);
+ dstconnA = get_connection(test, &owndstA);
+ dstserverA = get_server(dstconnA, &test->dstA, &idA);
+ if (!test->without_dst_b) {
+ dstconnB = get_connection(test, &owndstB);
+ dstserverB = get_server(dstconnB, &test->dstB, &idB);
+ }
+
+ dstaddr = g_strsplit(g_test_dbus_get_bus_address(dstbus), ",", 2);
+ dst_qemu_args =
+ g_strdup_printf("-object dbus-vmstate,id=dv,addr=%s -incoming %s",
+ dstaddr[0], uri);
+
+ src_qemu = qtest_init(src_qemu_args);
+ dst_qemu = qtest_init(dst_qemu_args);
+ set_id_list(test, src_qemu);
+ set_id_list(test, dst_qemu);
+
+ migrate(src_qemu, uri);
+ test->src_qemu = src_qemu;
+ g_timeout_add_seconds(1, wait_for_migration_complete, test);
+
+ g_main_loop_run(test->loop);
+ g_main_loop_unref(test->loop);
+
+ if (test->migrate_fail) {
+ qtest_set_expected_status(dst_qemu, 1);
+ }
+ qtest_quit(dst_qemu);
+ qtest_quit(src_qemu);
+ g_bus_unown_name(ownsrcA);
+ g_bus_unown_name(ownsrcB);
+ g_bus_unown_name(owndstA);
+ if (!test->without_dst_b) {
+ g_bus_unown_name(owndstB);
+ }
+}
+
+static void
+check_not_migrated(TestServer *s, TestServer *d)
+{
+ assert(!s->save_called);
+ assert(!s->load_called);
+ assert(!d->save_called);
+ assert(!d->load_called);
+}
+
+static void
+check_migrated(TestServer *s, TestServer *d)
+{
+ assert(s->save_called);
+ assert(!s->load_called);
+ assert(!d->save_called);
+ assert(d->load_called);
+}
+
+static void
+test_dbus_vmstate_without_list(void)
+{
+ Test test = { 0, };
+
+ test_dbus_vmstate(&test);
+
+ check_migrated(&test.srcA, &test.dstA);
+ check_migrated(&test.srcB, &test.dstB);
+}
+
+static void
+test_dbus_vmstate_with_list(void)
+{
+ Test test = { .id_list = "idA,idB" };
+
+ test_dbus_vmstate(&test);
+
+ check_migrated(&test.srcA, &test.dstA);
+ check_migrated(&test.srcB, &test.dstB);
+}
+
+static void
+test_dbus_vmstate_only_a(void)
+{
+ Test test = { .id_list = "idA" };
+
+ test_dbus_vmstate(&test);
+
+ check_migrated(&test.srcA, &test.dstA);
+ check_not_migrated(&test.srcB, &test.dstB);
+}
+
+static void
+test_dbus_vmstate_missing_src(void)
+{
+ Test test = { .id_list = "idA,idC", .migrate_fail = true };
+
+ /* run in subprocess to silence QEMU error reporting */
+ if (g_test_subprocess()) {
+ test_dbus_vmstate(&test);
+ check_not_migrated(&test.srcA, &test.dstA);
+ check_not_migrated(&test.srcB, &test.dstB);
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+}
+
+static void
+test_dbus_vmstate_missing_dst(void)
+{
+ Test test = { .id_list = "idA,idB",
+ .without_dst_b = true,
+ .migrate_fail = true };
+
+ /* run in subprocess to silence QEMU error reporting */
+ if (g_test_subprocess()) {
+ test_dbus_vmstate(&test);
+ assert(test.srcA.save_called);
+ assert(test.srcB.save_called);
+ assert(!test.dstB.save_called);
+ return;
+ }
+
+ g_test_trap_subprocess(NULL, 0, 0);
+ g_test_trap_assert_passed();
+}
+
+int
+main(int argc, char **argv)
+{
+ GError *err = NULL;
+ g_autofree char *dbus_daemon = NULL;
+ int ret;
+
+ dbus_daemon = g_build_filename(G_STRINGIFY(SRCDIR),
+ "tests",
+ "dbus-vmstate-daemon.sh",
+ NULL);
+ g_setenv("G_TEST_DBUS_DAEMON", dbus_daemon, true);
+
+ g_test_init(&argc, &argv, NULL);
+
+ workdir = g_dir_make_tmp("dbus-vmstate-test-XXXXXX", &err);
+ if (!workdir) {
+ g_error("Unable to create temporary dir: %s\n", err->message);
+ exit(1);
+ }
+
+ qtest_add_func("/dbus-vmstate/without-list",
+ test_dbus_vmstate_without_list);
+ qtest_add_func("/dbus-vmstate/with-list",
+ test_dbus_vmstate_with_list);
+ qtest_add_func("/dbus-vmstate/only-a",
+ test_dbus_vmstate_only_a);
+ qtest_add_func("/dbus-vmstate/missing-src",
+ test_dbus_vmstate_missing_src);
+ qtest_add_func("/dbus-vmstate/missing-dst",
+ test_dbus_vmstate_missing_dst);
+
+ ret = g_test_run();
+
+ rmdir(workdir);
+ g_free(workdir);
+
+ return ret;
+}
diff --git a/tests/dbus-vmstate1.xml b/tests/dbus-vmstate1.xml
new file mode 100644
index 0000000000..cc8563be4c
--- /dev/null
+++ b/tests/dbus-vmstate1.xml
@@ -0,0 +1,12 @@
+<?xml version="1.0"?>
+<node name="/" xmlns:doc="http://www.freedesktop.org/dbus/1.0/doc.dtd">
+ <interface name="org.qemu.VMState1">
+ <property name="Id" type="s" access="read"/>
+ <method name="Load">
+ <arg type="ay" name="data" direction="in"/>
+ </method>
+ <method name="Save">
+ <arg type="ay" name="data" direction="out"/>
+ </method>
+ </interface>
+</node>
--
2.24.0.308.g228f53135a
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v6 3/8] docs: start a document to describe D-Bus usage
2019-12-11 13:45 ` [PATCH v6 3/8] docs: start a document to describe D-Bus usage Marc-André Lureau
@ 2019-12-12 11:36 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-12 11:36 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, qemu-devel, dgilbert, quintela
On Wed, Dec 11, 2019 at 05:45:01PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MAINTAINERS | 5 +++
> docs/interop/dbus.rst | 99 ++++++++++++++++++++++++++++++++++++++++++
> docs/interop/index.rst | 1 +
> 3 files changed, 105 insertions(+)
> create mode 100644 docs/interop/dbus.rst
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 525b4740e8..19faa0e868 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2199,6 +2199,11 @@ F: tests/migration-test.c
> F: docs/devel/migration.rst
> F: qapi/migration.json
>
> +D-Bus
> +M: Marc-André Lureau <marcandre.lureau@redhat.com>
> +S: Maintained
> +F: docs/interop/dbus.rst
> +
> Seccomp
> M: Eduardo Otubo <otubo@redhat.com>
> S: Supported
> diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst
> new file mode 100644
> index 0000000000..3d760e4882
> --- /dev/null
> +++ b/docs/interop/dbus.rst
> @@ -0,0 +1,99 @@
> +=====
> +D-Bus
> +=====
> +
> +Introduction
> +============
> +
> +QEMU may be running with various helper processes involved:
> + - vhost-user* processes (gpu, virtfs, input, etc...)
> + - TPM emulation (or other devices)
> + - user networking (slirp)
> + - network services (DHCP/DNS, samba/ftp etc)
> + - background tasks (compression, streaming etc)
> + - client UI
> + - admin & cli
> +
> +Having several processes allows stricter security rules, as well as
> +greater modularity.
> +
> +While QEMU itself uses QMP as primary IPC (and Spice/VNC for remote
> +display), D-Bus is the de facto IPC of choice on Unix systems. The
> +wire format is machine friendly, good bindings exist for various
> +languages, and there are various tools available.
> +
> +Using a bus, helper processes can discover and communicate with each
> +other easily, without going through QEMU. The bus topology is also
> +easier to apprehend and debug than a mesh. However, it is wise to
> +consider the security aspects of it.
> +
> +Security
> +========
> +
> +A QEMU D-Bus bus should be private to a single VM. Thus, only
> +cooperative tasks are running on the same bus to serve the VM.
> +
> +D-Bus, the protocol and standard, doesn't have mechanisms to enforce
> +security between peers once the connection is established. Peers may
> +have additional mechanisms to enforce security rules, based for
> +example on UNIX credentials.
However, because the daemon has
> +controlled who can send/recv messages to who, doesn't magically make
> +this secure.
This reads a little awkwardly, instead:
The daemon can control which peers can send/recv messages using various
metadata attributes, however, this is alone is not generally sufficient
to make the deployment secure.
> The semantics of the actual methods implemented using
> +D-Bus are just as critical. Peers need to carefully validate any
> +information they received from a peer with a different trust level.
> +
> +dbus-daemon policy
> +------------------
> +
> +dbus-daemon can enforce various policies based on the UID/GID of the
> +processes that are connected to it. It is thus a good idea to run
> +helpers as different UID from QEMU and set appropriate policies.
> +
> +Depending on the use case, you may choose different scenarios:
> +
> + - Everything the same UID
> +
> + - Convenient for developers
> + - Improved reliability - crash of one part doens't take
> + out entire VM
> + - No security benefit over traditional QEMU
In the last point add
', unless additional unless additional controls
such as SELinux or AppArmor are applied'.
> +
> + - Two UIDs, one for QEMU, one for dbus & helpers
> +
> + - Moderately improved security isolation
s/improved/improved user based/
> +
> + - Many UIDs, one for QEMU one for dbus and one for each helpers
> +
> + - Best security isolation
s/Best/Best user based/
> + - Complex to manager distinct UIDs needed for each VM
> +
> +For example, to allow only ``qemu`` user to talk to ``qemu-helper``
> +``org.qemu.Helper1`` service, a dbus-daemon policy may contain:
> +
> +.. code:: xml
> +
> + <policy user="qemu">
> + <allow send_destination="org.qemu.Helper1"/>
> + <allow receive_sender="org.qemu.Helper1"/>
> + </policy>
> +
> + <policy user="qemu-helper">
> + <allow own="org.qemu.Helper1"/>
> + </policy>
> +
> +
> +dbus-daemon can also perfom SELinux checks based on the security
> +context of the source and the target. For example, ``virtiofs_t``
> +could be allowed to send a message to ``svirt_t``, but ``virtiofs_t``
> +wouldn't be allowed to send a message to ``virtiofs_t``.
> +
> +See dbus-daemon man page for details.
> +
> +Guidelines
> +==========
> +
> +When implementing new D-Bus interfaces, it is recommended to follow
> +the "D-Bus API Design Guidelines":
> +https://dbus.freedesktop.org/doc/dbus-api-design.html
> +
> +The "org.qemu.*" prefix is reserved for the QEMU project.
Perhaps change the last few words to
'for services implemented & distributed by the QEMU project'
> diff --git a/docs/interop/index.rst b/docs/interop/index.rst
> index 3e33fb5933..ded134ea75 100644
> --- a/docs/interop/index.rst
> +++ b/docs/interop/index.rst
> @@ -13,6 +13,7 @@ Contents:
> :maxdepth: 2
>
> bitmaps
> + dbus
> live-block-operations
> pr-helper
> qemu-ga
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 4/8] util: add dbus helper unit
2019-12-11 13:45 ` [PATCH v6 4/8] util: add dbus helper unit Marc-André Lureau
@ 2019-12-12 11:38 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-12 11:38 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, qemu-devel, dgilbert, quintela
On Wed, Dec 11, 2019 at 05:45:02PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MAINTAINERS | 2 ++
> include/qemu/dbus.h | 18 +++++++++++++++
> util/Makefile.objs | 3 +++
> util/dbus.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 78 insertions(+)
> create mode 100644 include/qemu/dbus.h
> create mode 100644 util/dbus.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 19faa0e868..f08fb4f24e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2202,6 +2202,8 @@ F: qapi/migration.json
> D-Bus
> M: Marc-André Lureau <marcandre.lureau@redhat.com>
> S: Maintained
> +F: util/dbus.c
> +F: include/qemu/dbus.h
> F: docs/interop/dbus.rst
>
> Seccomp
> diff --git a/include/qemu/dbus.h b/include/qemu/dbus.h
> new file mode 100644
> index 0000000000..efd74bef96
> --- /dev/null
> +++ b/include/qemu/dbus.h
> @@ -0,0 +1,18 @@
> +/*
> + * Helpers for using D-Bus
> + *
> + * Copyright (C) 2019 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef DBUS_H
> +#define DBUS_H
> +
> +#include <gio/gio.h>
> +
> +GStrv qemu_dbus_get_queued_owners(GDBusConnection *connection,
> + const char *name);
> +
> +#endif /* DBUS_H */
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index df124af1c5..80b76352cd 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -55,5 +55,8 @@ util-obj-$(CONFIG_INOTIFY1) += filemonitor-inotify.o
> util-obj-$(CONFIG_LINUX) += vfio-helpers.o
> util-obj-$(CONFIG_POSIX) += drm.o
> util-obj-y += guest-random.o
> +util-obj-$(CONFIG_GIO) += dbus.o
> +dbus.o-cflags = $(GIO_CFLAGS)
> +dbus.o-libs = $(GIO_LIBS)
>
> stub-obj-y += filemonitor-stub.o
> diff --git a/util/dbus.c b/util/dbus.c
> new file mode 100644
> index 0000000000..bb51870e54
> --- /dev/null
> +++ b/util/dbus.c
> @@ -0,0 +1,55 @@
> +/*
> + * Helpers for using D-Bus
> + *
> + * Copyright (C) 2019 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/dbus.h"
> +#include "qemu/error-report.h"
> +
> +/*
> + * qemu_dbus_get_queued_owners() - return the list of queued unique names
> + * @connection: A GDBusConnection
> + * @name: a service name
> + *
> + * Return: a GStrv of unique names, or NULL on failure.
> + */
> +GStrv
> +qemu_dbus_get_queued_owners(GDBusConnection *connection, const char *name)
> +{
> + g_autoptr(GDBusProxy) proxy = NULL;
> + g_autoptr(GVariant) result = NULL;
> + g_autoptr(GVariant) child = NULL;
> + g_autoptr(GError) err = NULL;
> +
> + proxy = g_dbus_proxy_new_sync(connection, G_DBUS_PROXY_FLAGS_NONE, NULL,
> + "org.freedesktop.DBus",
> + "/org/freedesktop/DBus",
> + "org.freedesktop.DBus",
> + NULL, &err);
> + if (!proxy) {
> + error_report("Failed to create DBus proxy: %s", err->message);
> + return NULL;
> + }
> +
> + result = g_dbus_proxy_call_sync(proxy, "ListQueuedOwners",
> + g_variant_new("(s)", name),
> + G_DBUS_CALL_FLAGS_NO_AUTO_START,
> + -1, NULL, &err);
> + if (!result) {
> + if (g_error_matches(err,
> + G_DBUS_ERROR,
> + G_DBUS_ERROR_NAME_HAS_NO_OWNER)) {
> + return g_new0(char *, 1);
> + }
> + error_report("Failed to call ListQueuedOwners: %s", err->message);
IMHO, helper code shouldn't be callling error_report, there should be
an Error **errp output parameter.
> + return NULL;
> + }
> +
> + child = g_variant_get_child_value(result, 0);
> + return g_variant_dup_strv(child, NULL);
> +}
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 5/8] Add dbus-vmstate object
2019-12-11 13:45 ` [PATCH v6 5/8] Add dbus-vmstate object Marc-André Lureau
@ 2019-12-12 12:03 ` Daniel P. Berrangé
2019-12-16 7:44 ` Marc-André Lureau
2019-12-13 16:32 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-12 12:03 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, qemu-devel, dgilbert, quintela
On Wed, Dec 11, 2019 at 05:45:03PM +0400, Marc-André Lureau wrote:
> When instantiated, this object will connect to the given D-Bus bus
> "addr". During migration, it will take/restore the data from
> org.qemu.VMState1 instances. See documentation for details.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MAINTAINERS | 2 +
> backends/Makefile.objs | 4 +
> backends/dbus-vmstate.c | 496 ++++++++++++++++++++++++++++++++++
> docs/interop/dbus-vmstate.rst | 74 +++++
> docs/interop/dbus.rst | 5 +
> docs/interop/index.rst | 1 +
> 6 files changed, 582 insertions(+)
> create mode 100644 backends/dbus-vmstate.c
> create mode 100644 docs/interop/dbus-vmstate.rst
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f08fb4f24e..7af80d0c1d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2202,9 +2202,11 @@ F: qapi/migration.json
> D-Bus
> M: Marc-André Lureau <marcandre.lureau@redhat.com>
> S: Maintained
> +F: backends/dbus-vmstate.c
> F: util/dbus.c
> F: include/qemu/dbus.h
> F: docs/interop/dbus.rst
> +F: docs/interop/dbus-vmstate.rst
>
> Seccomp
> M: Eduardo Otubo <otubo@redhat.com>
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index f0691116e8..28a847cd57 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)
> diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> new file mode 100644
> index 0000000000..059dd420b8
> --- /dev/null
> +++ b/backends/dbus-vmstate.c
> @@ -0,0 +1,496 @@
> +/*
> + * QEMU dbus-vmstate
> + *
> + * Copyright (C) 2019 Red Hat Inc
> + *
> + * Authors:
> + * Marc-André Lureau <marcandre.lureau@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qemu/dbus.h"
> +#include "qemu/error-report.h"
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "qapi/qmp/qerror.h"
> +#include "migration/vmstate.h"
> +
> +typedef struct DBusVMState DBusVMState;
> +typedef struct DBusVMStateClass DBusVMStateClass;
> +
> +#define TYPE_DBUS_VMSTATE "dbus-vmstate"
> +#define DBUS_VMSTATE(obj) \
> + OBJECT_CHECK(DBusVMState, (obj), TYPE_DBUS_VMSTATE)
> +#define DBUS_VMSTATE_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(DBusVMStateClass, (obj), TYPE_DBUS_VMSTATE)
> +#define DBUS_VMSTATE_CLASS(klass) \
> + OBJECT_CLASS_CHECK(DBusVMStateClass, (klass), TYPE_DBUS_VMSTATE)
> +
> +struct DBusVMStateClass {
> + ObjectClass parent_class;
> +};
> +
Not an objection to your patch here. This just reminds me that we
ought to follow GLib's lead and implement some helper macros to
automate all this tedious boilerplate. So we can just do something
simple like:
QOM_DECLARE_FINAL_TYPE(DBusVMState, dbus_vmstate, DBUS_VMSATE, Object)
and an equiv to do the TypeInfo declaration & registration.
> +struct DBusVMState {
> + Object parent;
> +
> + GDBusConnection *bus;
> + char *dbus_addr;
> + char *id_list;
> +
> + uint32_t data_size;
> + uint8_t *data;
> +};
> +
> +static const GDBusPropertyInfo vmstate_property_info[] = {
> + { -1, (char *) "Id", (char *) "s",
> + G_DBUS_PROPERTY_INFO_FLAGS_READABLE, NULL },
> +};
> +
> +static const GDBusPropertyInfo * const vmstate_property_info_pointers[] = {
> + &vmstate_property_info[0],
> + NULL
> +};
> +
> +static const GDBusInterfaceInfo vmstate1_interface_info = {
> + -1,
> + (char *) "org.qemu.VMState1",
> + (GDBusMethodInfo **) NULL,
> + (GDBusSignalInfo **) NULL,
> + (GDBusPropertyInfo **) &vmstate_property_info_pointers,
> + NULL,
> +};
> +
> +#define DBUS_VMSTATE_SIZE_LIMIT (1 * MiB)
> +
> +static GHashTable *
> +get_id_list_set(DBusVMState *self)
> +{
> + g_auto(GStrv) ids = NULL;
> + g_autoptr(GHashTable) set = NULL;
> + 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;
> + }
> +
> + return g_steal_pointer(&set);
> +}
> +
> +static GHashTable *
> +dbus_get_proxies(DBusVMState *self, GError **err)
> +{
> + g_autoptr(GHashTable) proxies = NULL;
> + g_autoptr(GHashTable) ids = NULL;
> + g_auto(GStrv) names = NULL;
> + size_t i;
> +
> + ids = get_id_list_set(self);
> + proxies = g_hash_table_new_full(g_str_hash, g_str_equal,
> + g_free, g_object_unref);
> +
> + names = qemu_dbus_get_queued_owners(self->bus, "org.qemu.VMState1");
> + if (!names) {
> + return NULL;
> + }
> +
> + for (i = 0; names[i]; i++) {
> + g_autoptr(GDBusProxy) proxy = NULL;
> + g_autoptr(GVariant) result = NULL;
> + g_autofree char *id = NULL;
> + 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) {
> + return NULL;
> + }
> +
> + 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.");
> + return NULL;
> + }
> +
> + id = g_variant_dup_string(result, &size);
> + if (ids && !g_hash_table_remove(ids, id)) {
> + g_clear_pointer(&id, g_free);
> + g_clear_object(&proxy);
> + continue;
> + }
> + if (size == 0 || size >= 256) {
> + g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> + "VMState Id '%s' is invalid.", id);
> + return NULL;
> + }
> +
> + if (!g_hash_table_insert(proxies, id, proxy)) {
> + g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> + "Duplicated VMState Id '%s'", id);
> + return NULL;
> + }
> + id = NULL;
> + proxy = NULL;
> +
> + g_clear_pointer(&result, g_variant_unref);
> + }
> +
> + if (ids) {
> + g_autofree char **left = NULL;
> +
> + left = (char **)g_hash_table_get_keys_as_array(ids, NULL);
> + if (*left) {
> + g_autofree char *leftids = g_strjoinv(",", left);
> + g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> + "Required VMState Id are missing: %s", leftids);
> + return NULL;
> + }
> + }
> +
> + return g_steal_pointer(&proxies);
> +}
> +
> +static int
> +dbus_load_state_proxy(GDBusProxy *proxy, const uint8_t *data, size_t size)
> +{
> + g_autoptr(GError) err = NULL;
> + g_autoptr(GVariant) result = NULL;
> + g_autoptr(GVariant) value = NULL;
> +
> + 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)",
> + g_steal_pointer(&value)),
> + G_DBUS_CALL_FLAGS_NO_AUTO_START,
> + -1, NULL, &err);
> + if (!result) {
> + error_report("Failed to Load: %s", err->message);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int dbus_vmstate_post_load(void *opaque, int version_id)
> +{
> + DBusVMState *self = DBUS_VMSTATE(opaque);
> + g_autoptr(GInputStream) m = NULL;
> + g_autoptr(GDataInputStream) s = NULL;
> + g_autoptr(GError) err = NULL;
> + g_autoptr(GHashTable) proxies = NULL;
> + uint32_t nelem;
> +
> + proxies = dbus_get_proxies(self, &err);
> + if (!proxies) {
> + error_report("Failed to get proxies: %s", err->message);
> + return -1;
> + }
> +
> + m = g_memory_input_stream_new_from_data(self->data, self->data_size, NULL);
> + s = g_data_input_stream_new(m);
> + g_data_input_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
> +
> + nelem = g_data_input_stream_read_uint32(s, NULL, &err);
> + if (err) {
> + goto error;
> + }
> +
> + while (nelem > 0) {
> + GDBusProxy *proxy = NULL;
> + uint32_t len;
> + gsize bytes_read, avail;
> + char id[256];
> +
> + len = g_data_input_stream_read_uint32(s, NULL, &err);
> + if (err) {
> + goto error;
> + }
> + if (len >= 256) {
> + error_report("Invalid DBus vmstate proxy name %u", len);
> + return -1;
> + }
> + if (!g_input_stream_read_all(G_INPUT_STREAM(s), id, len,
> + &bytes_read, NULL, &err)) {
> + goto error;
> + }
> + g_return_val_if_fail(bytes_read == len, -1);
> + id[len] = 0;
> +
> + proxy = g_hash_table_lookup(proxies, id);
> + if (!proxy) {
> + error_report("Failed to find proxy Id '%s'", id);
> + return -1;
> + }
> +
> + len = g_data_input_stream_read_uint32(s, NULL, &err);
> + avail = g_buffered_input_stream_get_available(
> + G_BUFFERED_INPUT_STREAM(s));
> +
> + if (len > DBUS_VMSTATE_SIZE_LIMIT || len > avail) {
> + error_report("Invalid vmstate size: %u", len);
> + return -1;
> + }
> +
> + if (dbus_load_state_proxy(proxy,
> + g_buffered_input_stream_peek_buffer(G_BUFFERED_INPUT_STREAM(s),
> + NULL),
> + len) < 0) {
> + error_report("Failed to restore Id '%s'", id);
> + return -1;
> + }
> +
> + if (!g_seekable_seek(G_SEEKABLE(s), len, G_SEEK_CUR, NULL, &err)) {
> + goto error;
> + }
> +
> + nelem -= 1;
> + }
> +
> + return 0;
> +
> +error:
> + error_report("Failed to read from stream: %s", err->message);
> + return -1;
> +}
> +
> +static void
> +dbus_save_state_proxy(gpointer key,
> + gpointer value,
> + gpointer user_data)
> +{
> + GDataOutputStream *s = user_data;
> + const char *id = key;
> + GDBusProxy *proxy = value;
> + g_autoptr(GVariant) result = NULL;
> + g_autoptr(GVariant) child = NULL;
> + g_autoptr(GError) err = NULL;
> + const uint8_t *data;
> + gsize size;
> +
> + 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);
> + return;
> + }
> +
> + 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");
> + return;
> + }
> + if (size > DBUS_VMSTATE_SIZE_LIMIT) {
> + error_report("Too large vmstate data to save: %" G_GSIZE_FORMAT, size);
> + return;
> + }
> +
> + if (!g_data_output_stream_put_uint32(s, strlen(id), NULL, &err) ||
> + !g_data_output_stream_put_string(s, id, NULL, &err) ||
> + !g_data_output_stream_put_uint32(s, size, NULL, &err) ||
> + !g_output_stream_write_all(G_OUTPUT_STREAM(s),
> + data, size, NULL, NULL, &err)) {
> + error_report("Failed to write to stream: %s", err->message);
> + }
This is a bit of a bike-shed comment, but I'm curious if you
considered using GVariant for the serializing data vs the
data output stream ? I feel like GVariant is enforcing more
structure & safety on the data serialization process, which
could be appealing.
> +static int dbus_vmstate_pre_save(void *opaque)
> +{
> + DBusVMState *self = DBUS_VMSTATE(opaque);
> + g_autoptr(GOutputStream) m = NULL;
> + g_autoptr(GDataOutputStream) s = NULL;
> + g_autoptr(GHashTable) proxies = NULL;
> + g_autoptr(GError) err = NULL;
> +
> + proxies = dbus_get_proxies(self, &err);
> + if (!proxies) {
> + error_report("Failed to get proxies: %s", err->message);
> + return -1;
> + }
> +
> + m = g_memory_output_stream_new_resizable();
> + s = g_data_output_stream_new(m);
> + g_data_output_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
> +
> + if (!g_data_output_stream_put_uint32(s, g_hash_table_size(proxies),
> + NULL, &err)) {
> + error_report("Failed to write to stream: %s", err->message);
> + return -1;
> + }
> +
> + g_hash_table_foreach(proxies, dbus_save_state_proxy, s);
> +
> + if (g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m))
> + > UINT32_MAX) {
> + error_report("DBus vmstate buffer is too large");
> + return -1;
> + }
> +
> + if (!g_output_stream_close(G_OUTPUT_STREAM(m), NULL, &err)) {
> + error_report("Failed to close stream: %s", err->message);
> + return -1;
> + }
> +
> + g_free(self->data);
> + self->data_size =
> + g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m));
> + self->data =
> + g_memory_output_stream_steal_data(G_MEMORY_OUTPUT_STREAM(m));
> +
> + return 0;
> +}
> +
> +static const VMStateDescription dbus_vmstate = {
> + .name = TYPE_DBUS_VMSTATE,
> + .version_id = 0,
> + .pre_save = dbus_vmstate_pre_save,
> + .post_load = dbus_vmstate_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(data_size, DBusVMState),
> + VMSTATE_VBUFFER_ALLOC_UINT32(data, DBusVMState, 0, 0, data_size),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void
> +dbus_vmstate_complete(UserCreatable *uc, Error **errp)
> +{
> + DBusVMState *self = DBUS_VMSTATE(uc);
> + GError *err = NULL;
Can use g_autoptr for this
> + 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;
> + }
> +
> + 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);
Why not just use self->bus directly.
> + if (err) {
> + error_setg(errp, "failed to connect to DBus: '%s'", err->message);
> + g_clear_error(&err);
Missing return here, as we don't want to register vmstate handler
if we fail.
> + }
> +
> + self->bus = bus;
> +
> + if (vmstate_register(VMSTATE_IF(self), -1, &dbus_vmstate, self) < 0) {
> + error_setg(errp, "Failed to register vmstate");
> + }
> +}
> diff --git a/docs/interop/dbus-vmstate.rst b/docs/interop/dbus-vmstate.rst
> new file mode 100644
> index 0000000000..8693891640
> --- /dev/null
> +++ b/docs/interop/dbus-vmstate.rst
> @@ -0,0 +1,74 @@
> +=============
> +D-Bus VMState
> +=============
> +
> +Introduction
> +============
> +
> +The QEMU dbus-vmstate object's aim is to migrate helpers' data running
> +on a QEMU D-Bus bus. (refer to the :doc:`dbus` document for
> +recommendation on D-Bus usage)
> +
> +Upon migration, QEMU will go through the queue of
> +``org.qemu.VMState1`` D-Bus 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). (D-Bus imposes a time limit on
A few seconds is too long IMHO. I think the expectation ought to
be a small fraction of a second.
Anything longer than that suggests there is some extra synchronization
work needing beyond serailizing state, which might suggest the need
for a separate DBus call. eg a way to tell the backend to "quiesce"
itself perhaps
For now we can keep it simple and just say that this method should
not do anything except seriailize state in a fraction of a second.
> +reply anyway, and migration would fail if data isn't given quickly
> +enough.)
> +
> +dbus-vmstate object can be configured with the expected list of
> +helpers by setting its ``id-list`` property, with a comma-separated
> +``Id`` list.
> +
> +Interface
> +=========
> +
> +On object path ``/org/qemu/VMState1``, the following
> +``org.qemu.VMState1`` interface should be implemented:
> +
> +.. code:: xml
> +
> + <interface name="org.qemu.VMState1">
> + <property name="Id" type="s" access="read"/>
> + <method name="Load">
> + <arg type="ay" name="data" direction="in"/>
> + </method>
> + <method name="Save">
> + <arg type="ay" name="data" direction="out"/>
> + </method>
> + </interface>
> +
> +"Id" property
> +-------------
> +
> +A string that identifies the helper uniquely. (maximum 256 bytes
> +including terminating NUL byte)
> +
> +.. note::
> +
> + The helper ID namespace is a separate namespace. In particular, it is not
> + related to QEMU "id" used in -object/-device objects.
Are there any expectations here on a scheme ? I feel leaving it
unspecified is probably a mistake. Should it follow a DBUs like
reverse.domain.name.style ?
> +
> +Load(in u8[] bytes) method
> +--------------------------
> +
> +The method called on destination with the state to restore.
> +
> +The helper may be initially started in a waiting state (with
> +an --incoming argument for example), and it may resume on success.
> +
> +An error may be returned to the caller.
> +
> +Save(out u8[] bytes) method
> +---------------------------
> +
> +The method called on the source to get the current state to be
> +migrated. The helper should continue to run normally.
> +
> +An error may be returned to the caller.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 6/8] configure: add GDBUS_CODEGEN
2019-12-11 13:45 ` [PATCH v6 6/8] configure: add GDBUS_CODEGEN Marc-André Lureau
@ 2019-12-12 12:05 ` Daniel P. Berrangé
2019-12-19 12:54 ` Marc-André Lureau
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-12 12:05 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, qemu-devel, dgilbert, quintela
On Wed, Dec 11, 2019 at 05:45:04PM +0400, Marc-André Lureau wrote:
> gdbus-codegen generated code requires gio-unix on Unix, so add it to
> GIO libs/cflags.
What is the situation on Windows, is it still supported ?
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> configure | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/configure b/configure
> index 6099be1d84..68a7705df7 100755
> --- a/configure
> +++ b/configure
> @@ -3720,10 +3720,16 @@ if $pkg_config --atleast-version=$glib_req_ver gio-2.0; then
> gio=yes
> gio_cflags=$($pkg_config --cflags gio-2.0)
> gio_libs=$($pkg_config --libs gio-2.0)
> + gdbus_codegen=$($pkg_config --variable=gdbus_codegen gio-2.0)
> else
> gio=no
> fi
>
> +if $pkg_config --atleast-version=$glib_req_ver gio-unix-2.0; then
> + gio_cflags="$gio_cflags $($pkg_config --cflags gio-unix-2.0)"
> + gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
> +fi
> +
> # Sanity check that the current size_t matches the
> # size that glib thinks it should be. This catches
> # problems on multi-arch where people try to build
> @@ -6949,6 +6955,7 @@ if test "$gio" = "yes" ; then
> echo "CONFIG_GIO=y" >> $config_host_mak
> echo "GIO_CFLAGS=$gio_cflags" >> $config_host_mak
> echo "GIO_LIBS=$gio_libs" >> $config_host_mak
> + echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
> fi
> echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
> if test "$gnutls" = "yes" ; then
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions
2019-12-11 13:45 ` [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions Marc-André Lureau
@ 2019-12-12 12:06 ` Daniel P. Berrangé
2019-12-19 12:23 ` Marc-André Lureau
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-12 12:06 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, qemu-devel, dgilbert, quintela
On Wed, Dec 11, 2019 at 05:45:05PM +0400, Marc-André Lureau wrote:
> To get dbus-vmstate test covered.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/docker/dockerfiles/centos7.docker | 1 +
> tests/docker/dockerfiles/debian10.docker | 1 +
> tests/docker/dockerfiles/fedora.docker | 1 +
> tests/docker/dockerfiles/ubuntu.docker | 1 +
> 4 files changed, 4 insertions(+)
For docker
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Does it need adding to travis, gitlab, shippable, etc CI configs
too ?
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 8/8] tests: add dbus-vmstate-test
2019-12-11 13:45 ` [PATCH v6 8/8] tests: add dbus-vmstate-test Marc-André Lureau
@ 2019-12-12 12:11 ` Daniel P. Berrangé
2019-12-13 18:20 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-12 12:11 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, qemu-devel, dgilbert, quintela
On Wed, Dec 11, 2019 at 05:45:06PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MAINTAINERS | 1 +
> tests/Makefile.include | 22 +-
> tests/dbus-vmstate-daemon.sh | 95 +++++++++
> tests/dbus-vmstate-test.c | 399 +++++++++++++++++++++++++++++++++++
> tests/dbus-vmstate1.xml | 12 ++
> 5 files changed, 528 insertions(+), 1 deletion(-)
> create mode 100755 tests/dbus-vmstate-daemon.sh
> create mode 100644 tests/dbus-vmstate-test.c
> create mode 100644 tests/dbus-vmstate1.xml
>
> +write_config()
> +{
> + CONF="$1"
> + cat > "$CONF" <<EOF
> +<busconfig>
> + <type>session</type>
> + <listen>unix:tmpdir=/tmp</listen>
I'd expect us to make it create any files in the builddir.
If we must use a global dir, then it should be a unique
temp dir created just for this test - can we subsitute
in the temp dir we create later on in this patch
> +
> + <policy context="default">
> + <!-- Holes must be punched in service configuration files for
> + name ownership and sending method calls -->
> + <deny own="*"/>
> + <deny send_type="method_call"/>
> +
> + <!-- Signals and reply messages (method returns, errors) are allowed
> + by default -->
> + <allow send_type="signal"/>
> + <allow send_requested_reply="true" send_type="method_return"/>
> + <allow send_requested_reply="true" send_type="error"/>
> +
> + <!-- All messages may be received by default -->
> + <allow receive_type="method_call"/>
> + <allow receive_type="method_return"/>
> + <allow receive_type="error"/>
> + <allow receive_type="signal"/>
> +
> + <!-- Allow anyone to talk to the message bus -->
> + <allow send_destination="org.freedesktop.DBus"
> + send_interface="org.freedesktop.DBus" />
> + <allow send_destination="org.freedesktop.DBus"
> + send_interface="org.freedesktop.DBus.Introspectable"/>
> + <allow send_destination="org.freedesktop.DBus"
> + send_interface="org.freedesktop.DBus.Properties"/>
> + <!-- But disallow some specific bus services -->
> + <deny send_destination="org.freedesktop.DBus"
> + send_interface="org.freedesktop.DBus"
> + send_member="UpdateActivationEnvironment"/>
> + <deny send_destination="org.freedesktop.DBus"
> + send_interface="org.freedesktop.DBus.Debug.Stats"/>
> + <deny send_destination="org.freedesktop.DBus"
> + send_interface="org.freedesktop.systemd1.Activator"/>
> +
> + <allow own="org.qemu.VMState1"/>
> + <allow send_destination="org.qemu.VMState1"/>
> + <allow receive_sender="org.qemu.VMState1"/>
> +
> + </policy>
> +
> + <include if_selinux_enabled="yes"
> + selinux_root_relative="yes">contexts/dbus_contexts</include>
> +
> +</busconfig>
> +EOF
> +}
> +
> +ARGS=
> +for arg in "$@"
> +do
> + case $arg in
> + --config-file=*)
> + CONF="${arg#*=}"
> + write_config "$CONF"
> + ARGS="$ARGS $1"
> + shift
> + ;;
> + *)
> + ARGS="$ARGS $1"
> + shift
> + ;;
> + esac
> +done
> +
> +exec dbus-daemon $ARGS
> diff --git a/tests/dbus-vmstate-test.c b/tests/dbus-vmstate-test.c
> new file mode 100644
> index 0000000000..34818c1e79
> --- /dev/null
> +++ b/tests/dbus-vmstate-test.c
> @@ -0,0 +1,399 @@
> +#include "qemu/osdep.h"
> +#include <glib/gstdio.h>
> +#include <gio/gio.h>
> +#include "libqtest.h"
> +#include "qemu-common.h"
> +#include "dbus-vmstate1.h"
> +
> +static char *workdir;
> +
> +typedef struct TestServerId {
> + const char *name;
> + const char *data;
> + size_t size;
> +} TestServerId;
> +
> +static const TestServerId idA = {
> + "idA", "I'am\0idA!", sizeof("I'am\0idA!")
> +};
> +
> +static const TestServerId idB = {
> + "idB", "I'am\0idB!", sizeof("I'am\0idB!")
> +};
> +
> +typedef struct TestServer {
> + const TestServerId *id;
> + bool save_called;
> + bool load_called;
> +} TestServer;
> +
> +typedef struct Test {
> + const char *id_list;
> + bool migrate_fail;
> + bool without_dst_b;
> + TestServer srcA;
> + TestServer dstA;
> + TestServer srcB;
> + TestServer dstB;
> + GMainLoop *loop;
> + QTestState *src_qemu;
> +} Test;
> +
> +static gboolean
> +vmstate_load(VMState1 *object, GDBusMethodInvocation *invocation,
> + const gchar *arg_data, gpointer user_data)
> +{
> + TestServer *h = user_data;
> + g_autoptr(GVariant) var = NULL;
> + GVariant *args;
> + const uint8_t *data;
> + size_t size;
> +
> + args = g_dbus_method_invocation_get_parameters(invocation);
> + var = g_variant_get_child_value(args, 0);
> + data = g_variant_get_fixed_array(var, &size, sizeof(char));
> + g_assert_cmpuint(size, ==, h->id->size);
> + g_assert(!memcmp(data, h->id->data, h->id->size));
> + h->load_called = true;
> +
> + g_dbus_method_invocation_return_value(invocation, g_variant_new("()"));
> + return TRUE;
> +}
> +
> +static gboolean
> +vmstate_save(VMState1 *object, GDBusMethodInvocation *invocation,
> + gpointer user_data)
> +{
> + TestServer *h = user_data;
> + GVariant *var;
> +
> + var = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE,
> + h->id->data, h->id->size, sizeof(char));
> + g_dbus_method_invocation_return_value(invocation,
> + g_variant_new("(@ay)", var));
> + h->save_called = true;
> +
> + return TRUE;
> +}
> +
> +static gboolean
> +wait_for_migration_complete(gpointer user_data)
> +{
> + Test *test = user_data;
> + QDict *rsp_return;
> + bool stop = false;
> + const char *status;
> +
> + qtest_qmp_send(test->src_qemu, "{ 'execute': 'query-migrate' }");
> + rsp_return = qtest_qmp_receive_success(test->src_qemu, NULL, NULL);
> + status = qdict_get_str(rsp_return, "status");
> + if (g_str_equal(status, "completed") || g_str_equal(status, "failed")) {
> + stop = true;
> + g_assert_cmpstr(status, ==,
> + test->migrate_fail ? "failed" : "completed");
> + }
> + qobject_unref(rsp_return);
> +
> + if (stop) {
> + g_main_loop_quit(test->loop);
> + }
> + return stop ? G_SOURCE_REMOVE : G_SOURCE_CONTINUE;
> +}
> +
> +static void migrate(QTestState *who, const char *uri)
> +{
> + QDict *args, *rsp;
> +
> + args = qdict_new();
> + qdict_put_str(args, "uri", uri);
> +
> + rsp = qtest_qmp(who, "{ 'execute': 'migrate', 'arguments': %p }", args);
> +
> + g_assert(qdict_haskey(rsp, "return"));
> + qobject_unref(rsp);
> +}
> +
> +typedef struct WaitNamed {
> + GMainLoop *loop;
> + bool named;
> +} WaitNamed;
> +
> +static void
> +named_cb(GDBusConnection *connection,
> + const gchar *name,
> + gpointer user_data)
> +{
> + WaitNamed *t = user_data;
> +
> + t->named = true;
> + g_main_loop_quit(t->loop);
> +}
> +
> +static GDBusConnection *
> +get_connection(Test *test, guint *ownid)
> +{
> + g_autofree gchar *addr = NULL;
> + WaitNamed *wait;
> + GError *err = NULL;
> + GDBusConnection *c;
> +
> + wait = g_new0(WaitNamed, 1);
> + wait->loop = test->loop;
> + addr = g_dbus_address_get_for_bus_sync(G_BUS_TYPE_SESSION, NULL, &err);
> + g_assert_no_error(err);
> +
> + c = g_dbus_connection_new_for_address_sync(
> + addr,
> + G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION |
> + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT,
> + NULL, NULL, &err);
> + g_assert_no_error(err);
> + *ownid = g_bus_own_name_on_connection(c, "org.qemu.VMState1",
> + G_BUS_NAME_OWNER_FLAGS_NONE,
> + named_cb, named_cb, wait, g_free);
> + if (!wait->named) {
> + g_main_loop_run(wait->loop);
> + }
> +
> + return c;
> +}
> +
> +static GDBusObjectManagerServer *
> +get_server(GDBusConnection *conn, TestServer *s, const TestServerId *id)
> +{
> + g_autoptr(GDBusObjectSkeleton) sk = NULL;
> + g_autoptr(VMState1Skeleton) v = NULL;
> + GDBusObjectManagerServer *os;
> +
> + s->id = id;
> + os = g_dbus_object_manager_server_new("/org/qemu");
> + sk = g_dbus_object_skeleton_new("/org/qemu/VMState1");
> +
> + v = VMSTATE1_SKELETON(vmstate1_skeleton_new());
> + g_object_set(v, "id", id->name, NULL);
> +
> + g_signal_connect(v, "handle-load", G_CALLBACK(vmstate_load), s);
> + g_signal_connect(v, "handle-save", G_CALLBACK(vmstate_save), s);
> +
> + g_dbus_object_skeleton_add_interface(sk, G_DBUS_INTERFACE_SKELETON(v));
> + g_dbus_object_manager_server_export(os, sk);
> + g_dbus_object_manager_server_set_connection(os, conn);
> +
> + return os;
> +}
> +
> +static void
> +set_id_list(Test *test, QTestState *s)
> +{
> + if (!test->id_list) {
> + return;
> + }
> +
> + g_assert(!qmp_rsp_is_err(qtest_qmp(s,
> + "{ 'execute': 'qom-set', 'arguments': "
> + "{ 'path': '/objects/dv', 'property': 'id-list', 'value': %s } }",
> + test->id_list)));
> +}
> +static void
> +test_dbus_vmstate(Test *test)
> +{
> + g_autofree char *src_qemu_args = NULL;
> + g_autofree char *dst_qemu_args = NULL;
> + g_autoptr(GTestDBus) srcbus = NULL;
> + g_autoptr(GTestDBus) dstbus = NULL;
> + g_autoptr(GDBusConnection) srcconnA = NULL;
> + g_autoptr(GDBusConnection) srcconnB = NULL;
> + g_autoptr(GDBusConnection) dstconnA = NULL;
> + g_autoptr(GDBusConnection) dstconnB = NULL;
> + g_autoptr(GDBusObjectManagerServer) srcserverA = NULL;
> + g_autoptr(GDBusObjectManagerServer) srcserverB = NULL;
> + g_autoptr(GDBusObjectManagerServer) dstserverA = NULL;
> + g_autoptr(GDBusObjectManagerServer) dstserverB = NULL;
> + g_auto(GStrv) srcaddr = NULL;
> + g_auto(GStrv) dstaddr = NULL;
> + g_autofree char *uri = NULL;
> + QTestState *src_qemu = NULL, *dst_qemu = NULL;
> + guint ownsrcA, ownsrcB, owndstA, owndstB;
> +
> + uri = g_strdup_printf("unix:%s/migsocket", workdir);
> +
> + test->loop = g_main_loop_new(NULL, TRUE);
> +
> + srcbus = g_test_dbus_new(G_TEST_DBUS_NONE);
> + g_test_dbus_up(srcbus);
> + srcconnA = get_connection(test, &ownsrcA);
> + srcserverA = get_server(srcconnA, &test->srcA, &idA);
> + srcconnB = get_connection(test, &ownsrcB);
> + srcserverB = get_server(srcconnB, &test->srcB, &idB);
> +
> + /* remove ,guid=foo part */
> + srcaddr = g_strsplit(g_test_dbus_get_bus_address(srcbus), ",", 2);
> + src_qemu_args =
> + g_strdup_printf("-object dbus-vmstate,id=dv,addr=%s", srcaddr[0]);
> +
> + dstbus = g_test_dbus_new(G_TEST_DBUS_NONE);
> + g_test_dbus_up(dstbus);
> + dstconnA = get_connection(test, &owndstA);
> + dstserverA = get_server(dstconnA, &test->dstA, &idA);
> + if (!test->without_dst_b) {
> + dstconnB = get_connection(test, &owndstB);
> + dstserverB = get_server(dstconnB, &test->dstB, &idB);
> + }
> +
> + dstaddr = g_strsplit(g_test_dbus_get_bus_address(dstbus), ",", 2);
> + dst_qemu_args =
> + g_strdup_printf("-object dbus-vmstate,id=dv,addr=%s -incoming %s",
> + dstaddr[0], uri);
> +
> + src_qemu = qtest_init(src_qemu_args);
> + dst_qemu = qtest_init(dst_qemu_args);
> + set_id_list(test, src_qemu);
> + set_id_list(test, dst_qemu);
> +
> + migrate(src_qemu, uri);
> + test->src_qemu = src_qemu;
> + g_timeout_add_seconds(1, wait_for_migration_complete, test);
> +
> + g_main_loop_run(test->loop);
> + g_main_loop_unref(test->loop);
> +
> + if (test->migrate_fail) {
> + qtest_set_expected_status(dst_qemu, 1);
> + }
> + qtest_quit(dst_qemu);
> + qtest_quit(src_qemu);
> + g_bus_unown_name(ownsrcA);
> + g_bus_unown_name(ownsrcB);
> + g_bus_unown_name(owndstA);
> + if (!test->without_dst_b) {
> + g_bus_unown_name(owndstB);
> + }
> +}
> +
> +static void
> +check_not_migrated(TestServer *s, TestServer *d)
> +{
> + assert(!s->save_called);
> + assert(!s->load_called);
> + assert(!d->save_called);
> + assert(!d->load_called);
> +}
> +
> +static void
> +check_migrated(TestServer *s, TestServer *d)
> +{
> + assert(s->save_called);
> + assert(!s->load_called);
> + assert(!d->save_called);
> + assert(d->load_called);
> +}
> +
> +static void
> +test_dbus_vmstate_without_list(void)
> +{
> + Test test = { 0, };
> +
> + test_dbus_vmstate(&test);
> +
> + check_migrated(&test.srcA, &test.dstA);
> + check_migrated(&test.srcB, &test.dstB);
> +}
> +
> +static void
> +test_dbus_vmstate_with_list(void)
> +{
> + Test test = { .id_list = "idA,idB" };
> +
> + test_dbus_vmstate(&test);
> +
> + check_migrated(&test.srcA, &test.dstA);
> + check_migrated(&test.srcB, &test.dstB);
> +}
> +
> +static void
> +test_dbus_vmstate_only_a(void)
> +{
> + Test test = { .id_list = "idA" };
> +
> + test_dbus_vmstate(&test);
> +
> + check_migrated(&test.srcA, &test.dstA);
> + check_not_migrated(&test.srcB, &test.dstB);
> +}
> +
> +static void
> +test_dbus_vmstate_missing_src(void)
> +{
> + Test test = { .id_list = "idA,idC", .migrate_fail = true };
> +
> + /* run in subprocess to silence QEMU error reporting */
> + if (g_test_subprocess()) {
> + test_dbus_vmstate(&test);
> + check_not_migrated(&test.srcA, &test.dstA);
> + check_not_migrated(&test.srcB, &test.dstB);
> + return;
> + }
> +
> + g_test_trap_subprocess(NULL, 0, 0);
> + g_test_trap_assert_passed();
> +}
> +
> +static void
> +test_dbus_vmstate_missing_dst(void)
> +{
> + Test test = { .id_list = "idA,idB",
> + .without_dst_b = true,
> + .migrate_fail = true };
> +
> + /* run in subprocess to silence QEMU error reporting */
> + if (g_test_subprocess()) {
> + test_dbus_vmstate(&test);
> + assert(test.srcA.save_called);
> + assert(test.srcB.save_called);
> + assert(!test.dstB.save_called);
> + return;
> + }
> +
> + g_test_trap_subprocess(NULL, 0, 0);
> + g_test_trap_assert_passed();
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> + GError *err = NULL;
> + g_autofree char *dbus_daemon = NULL;
> + int ret;
> +
> + dbus_daemon = g_build_filename(G_STRINGIFY(SRCDIR),
> + "tests",
> + "dbus-vmstate-daemon.sh",
> + NULL);
> + g_setenv("G_TEST_DBUS_DAEMON", dbus_daemon, true);
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + workdir = g_dir_make_tmp("dbus-vmstate-test-XXXXXX", &err);
> + if (!workdir) {
> + g_error("Unable to create temporary dir: %s\n", err->message);
> + exit(1);
> + }
> +
> + qtest_add_func("/dbus-vmstate/without-list",
> + test_dbus_vmstate_without_list);
> + qtest_add_func("/dbus-vmstate/with-list",
> + test_dbus_vmstate_with_list);
> + qtest_add_func("/dbus-vmstate/only-a",
> + test_dbus_vmstate_only_a);
> + qtest_add_func("/dbus-vmstate/missing-src",
> + test_dbus_vmstate_missing_src);
> + qtest_add_func("/dbus-vmstate/missing-dst",
> + test_dbus_vmstate_missing_dst);
> +
> + ret = g_test_run();
> +
> + rmdir(workdir);
> + g_free(workdir);
> +
> + return ret;
> +}
> diff --git a/tests/dbus-vmstate1.xml b/tests/dbus-vmstate1.xml
> new file mode 100644
> index 0000000000..cc8563be4c
> --- /dev/null
> +++ b/tests/dbus-vmstate1.xml
> @@ -0,0 +1,12 @@
> +<?xml version="1.0"?>
> +<node name="/" xmlns:doc="http://www.freedesktop.org/dbus/1.0/doc.dtd">
> + <interface name="org.qemu.VMState1">
> + <property name="Id" type="s" access="read"/>
> + <method name="Load">
> + <arg type="ay" name="data" direction="in"/>
> + </method>
> + <method name="Save">
> + <arg type="ay" name="data" direction="out"/>
> + </method>
> + </interface>
> +</node>
> --
> 2.24.0.308.g228f53135a
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 5/8] Add dbus-vmstate object
2019-12-11 13:45 ` [PATCH v6 5/8] Add dbus-vmstate object Marc-André Lureau
2019-12-12 12:03 ` Daniel P. Berrangé
@ 2019-12-13 16:32 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-12-13 16:32 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, berrange, qemu-devel, quintela
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
<snip>
Generally from the migration side I'm OK; I don't know that
much glib stuff as you're using, so I'll leave that to Dan.
> + if (!result) {
> + error_report("Failed to Load: %s", err->message);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int dbus_vmstate_post_load(void *opaque, int version_id)
> +{
> + DBusVMState *self = DBUS_VMSTATE(opaque);
> + g_autoptr(GInputStream) m = NULL;
> + g_autoptr(GDataInputStream) s = NULL;
> + g_autoptr(GError) err = NULL;
> + g_autoptr(GHashTable) proxies = NULL;
> + uint32_t nelem;
> +
> + proxies = dbus_get_proxies(self, &err);
> + if (!proxies) {
> + error_report("Failed to get proxies: %s", err->message);
Generally can you put either the function name or something in the error
to point us in the right direction; then if a user gets an error and it
says dbus_vmstate_post_load: Failed to get proxies (e.g. by using %s:
...__func__) then any random qemu dev will be able to resolve the blame
pointer quickly and not trying to guess which proxies we're talking
about.
You might also like to add some trace_ calls to watch what is going on.
Dave
> + return -1;
> + }
> +
> + m = g_memory_input_stream_new_from_data(self->data, self->data_size, NULL);
> + s = g_data_input_stream_new(m);
> + g_data_input_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
> +
> + nelem = g_data_input_stream_read_uint32(s, NULL, &err);
> + if (err) {
> + goto error;
> + }
> +
> + while (nelem > 0) {
> + GDBusProxy *proxy = NULL;
> + uint32_t len;
> + gsize bytes_read, avail;
> + char id[256];
> +
> + len = g_data_input_stream_read_uint32(s, NULL, &err);
> + if (err) {
> + goto error;
> + }
> + if (len >= 256) {
> + error_report("Invalid DBus vmstate proxy name %u", len);
> + return -1;
> + }
> + if (!g_input_stream_read_all(G_INPUT_STREAM(s), id, len,
> + &bytes_read, NULL, &err)) {
> + goto error;
> + }
> + g_return_val_if_fail(bytes_read == len, -1);
> + id[len] = 0;
> +
> + proxy = g_hash_table_lookup(proxies, id);
> + if (!proxy) {
> + error_report("Failed to find proxy Id '%s'", id);
> + return -1;
> + }
> +
> + len = g_data_input_stream_read_uint32(s, NULL, &err);
> + avail = g_buffered_input_stream_get_available(
> + G_BUFFERED_INPUT_STREAM(s));
> +
> + if (len > DBUS_VMSTATE_SIZE_LIMIT || len > avail) {
> + error_report("Invalid vmstate size: %u", len);
> + return -1;
> + }
> +
> + if (dbus_load_state_proxy(proxy,
> + g_buffered_input_stream_peek_buffer(G_BUFFERED_INPUT_STREAM(s),
> + NULL),
> + len) < 0) {
> + error_report("Failed to restore Id '%s'", id);
> + return -1;
> + }
> +
> + if (!g_seekable_seek(G_SEEKABLE(s), len, G_SEEK_CUR, NULL, &err)) {
> + goto error;
> + }
> +
> + nelem -= 1;
> + }
> +
> + return 0;
> +
> +error:
> + error_report("Failed to read from stream: %s", err->message);
> + return -1;
> +}
> +
> +static void
> +dbus_save_state_proxy(gpointer key,
> + gpointer value,
> + gpointer user_data)
> +{
> + GDataOutputStream *s = user_data;
> + const char *id = key;
> + GDBusProxy *proxy = value;
> + g_autoptr(GVariant) result = NULL;
> + g_autoptr(GVariant) child = NULL;
> + g_autoptr(GError) err = NULL;
> + const uint8_t *data;
> + gsize size;
> +
> + 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);
> + return;
> + }
> +
> + 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");
> + return;
> + }
> + if (size > DBUS_VMSTATE_SIZE_LIMIT) {
> + error_report("Too large vmstate data to save: %" G_GSIZE_FORMAT, size);
> + return;
> + }
> +
> + if (!g_data_output_stream_put_uint32(s, strlen(id), NULL, &err) ||
> + !g_data_output_stream_put_string(s, id, NULL, &err) ||
> + !g_data_output_stream_put_uint32(s, size, NULL, &err) ||
> + !g_output_stream_write_all(G_OUTPUT_STREAM(s),
> + data, size, NULL, NULL, &err)) {
> + error_report("Failed to write to stream: %s", err->message);
> + }
> +}
> +
> +static int dbus_vmstate_pre_save(void *opaque)
> +{
> + DBusVMState *self = DBUS_VMSTATE(opaque);
> + g_autoptr(GOutputStream) m = NULL;
> + g_autoptr(GDataOutputStream) s = NULL;
> + g_autoptr(GHashTable) proxies = NULL;
> + g_autoptr(GError) err = NULL;
> +
> + proxies = dbus_get_proxies(self, &err);
> + if (!proxies) {
> + error_report("Failed to get proxies: %s", err->message);
> + return -1;
> + }
> +
> + m = g_memory_output_stream_new_resizable();
> + s = g_data_output_stream_new(m);
> + g_data_output_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
> +
> + if (!g_data_output_stream_put_uint32(s, g_hash_table_size(proxies),
> + NULL, &err)) {
> + error_report("Failed to write to stream: %s", err->message);
> + return -1;
> + }
> +
> + g_hash_table_foreach(proxies, dbus_save_state_proxy, s);
> +
> + if (g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m))
> + > UINT32_MAX) {
> + error_report("DBus vmstate buffer is too large");
> + return -1;
> + }
> +
> + if (!g_output_stream_close(G_OUTPUT_STREAM(m), NULL, &err)) {
> + error_report("Failed to close stream: %s", err->message);
> + return -1;
> + }
> +
> + g_free(self->data);
> + self->data_size =
> + g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m));
> + self->data =
> + g_memory_output_stream_steal_data(G_MEMORY_OUTPUT_STREAM(m));
> +
> + return 0;
> +}
> +
> +static const VMStateDescription dbus_vmstate = {
> + .name = TYPE_DBUS_VMSTATE,
> + .version_id = 0,
> + .pre_save = dbus_vmstate_pre_save,
> + .post_load = dbus_vmstate_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(data_size, DBusVMState),
> + VMSTATE_VBUFFER_ALLOC_UINT32(data, DBusVMState, 0, 0, data_size),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +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;
> + }
> +
> + 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;
> +
> + if (vmstate_register(VMSTATE_IF(self), -1, &dbus_vmstate, self) < 0) {
> + error_setg(errp, "Failed to register vmstate");
> + }
> +}
> +
> +static void
> +dbus_vmstate_finalize(Object *o)
> +{
> + DBusVMState *self = DBUS_VMSTATE(o);
> +
> + vmstate_unregister(VMSTATE_IF(self), &dbus_vmstate, self);
> +
> + g_clear_object(&self->bus);
> + g_free(self->dbus_addr);
> + g_free(self->id_list);
> + g_free(self->data);
> +}
> +
> +static char *
> +get_dbus_addr(Object *o, Error **errp)
> +{
> + DBusVMState *self = DBUS_VMSTATE(o);
> +
> + return g_strdup(self->dbus_addr);
> +}
> +
> +static void
> +set_dbus_addr(Object *o, const char *str, Error **errp)
> +{
> + DBusVMState *self = DBUS_VMSTATE(o);
> +
> + g_free(self->dbus_addr);
> + self->dbus_addr = g_strdup(str);
> +}
> +
> +static char *
> +get_id_list(Object *o, Error **errp)
> +{
> + DBusVMState *self = DBUS_VMSTATE(o);
> +
> + return g_strdup(self->id_list);
> +}
> +
> +static void
> +set_id_list(Object *o, const char *str, Error **errp)
> +{
> + DBusVMState *self = DBUS_VMSTATE(o);
> +
> + g_free(self->id_list);
> + self->id_list = g_strdup(str);
> +}
> +
> +static char *
> +dbus_vmstate_get_id(VMStateIf *vmif)
> +{
> + return g_strdup(TYPE_DBUS_VMSTATE);
> +}
> +
> +static void
> +dbus_vmstate_class_init(ObjectClass *oc, void *data)
> +{
> + UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> + VMStateIfClass *vc = VMSTATE_IF_CLASS(oc);
> +
> + ucc->complete = dbus_vmstate_complete;
> + vc->get_id = dbus_vmstate_get_id;
> +
> + object_class_property_add_str(oc, "addr",
> + get_dbus_addr, set_dbus_addr,
> + &error_abort);
> + object_class_property_add_str(oc, "id-list",
> + get_id_list, set_id_list,
> + &error_abort);
> +}
> +
> +static const TypeInfo dbus_vmstate_info = {
> + .name = TYPE_DBUS_VMSTATE,
> + .parent = TYPE_OBJECT,
> + .instance_size = sizeof(DBusVMState),
> + .instance_finalize = dbus_vmstate_finalize,
> + .class_size = sizeof(DBusVMStateClass),
> + .class_init = dbus_vmstate_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_USER_CREATABLE },
> + { TYPE_VMSTATE_IF },
> + { }
> + }
> +};
> +
> +static void
> +register_types(void)
> +{
> + type_register_static(&dbus_vmstate_info);
> +}
> +
> +type_init(register_types);
> diff --git a/docs/interop/dbus-vmstate.rst b/docs/interop/dbus-vmstate.rst
> new file mode 100644
> index 0000000000..8693891640
> --- /dev/null
> +++ b/docs/interop/dbus-vmstate.rst
> @@ -0,0 +1,74 @@
> +=============
> +D-Bus VMState
> +=============
> +
> +Introduction
> +============
> +
> +The QEMU dbus-vmstate object's aim is to migrate helpers' data running
> +on a QEMU D-Bus bus. (refer to the :doc:`dbus` document for
> +recommendation on D-Bus usage)
> +
> +Upon migration, QEMU will go through the queue of
> +``org.qemu.VMState1`` D-Bus 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). (D-Bus imposes a time limit on
> +reply anyway, and migration would fail if data isn't given quickly
> +enough.)
> +
> +dbus-vmstate object can be configured with the expected list of
> +helpers by setting its ``id-list`` property, with a comma-separated
> +``Id`` list.
> +
> +Interface
> +=========
> +
> +On object path ``/org/qemu/VMState1``, the following
> +``org.qemu.VMState1`` interface should be implemented:
> +
> +.. code:: xml
> +
> + <interface name="org.qemu.VMState1">
> + <property name="Id" type="s" access="read"/>
> + <method name="Load">
> + <arg type="ay" name="data" direction="in"/>
> + </method>
> + <method name="Save">
> + <arg type="ay" name="data" direction="out"/>
> + </method>
> + </interface>
> +
> +"Id" property
> +-------------
> +
> +A string that identifies the helper uniquely. (maximum 256 bytes
> +including terminating NUL byte)
> +
> +.. note::
> +
> + The helper ID namespace is a separate namespace. In particular, it is not
> + related to QEMU "id" used in -object/-device objects.
> +
> +Load(in u8[] bytes) method
> +--------------------------
> +
> +The method called on destination with the state to restore.
> +
> +The helper may be initially started in a waiting state (with
> +an --incoming argument for example), and it may resume on success.
> +
> +An error may be returned to the caller.
> +
> +Save(out u8[] bytes) method
> +---------------------------
> +
> +The method called on the source to get the current state to be
> +migrated. The helper should continue to run normally.
> +
> +An error may be returned to the caller.
> diff --git a/docs/interop/dbus.rst b/docs/interop/dbus.rst
> index 3d760e4882..d9af608dc9 100644
> --- a/docs/interop/dbus.rst
> +++ b/docs/interop/dbus.rst
> @@ -97,3 +97,8 @@ the "D-Bus API Design Guidelines":
> https://dbus.freedesktop.org/doc/dbus-api-design.html
>
> The "org.qemu.*" prefix is reserved for the QEMU project.
> +
> +QEMU Interfaces
> +===============
> +
> +:doc:`dbus-vmstate`
> diff --git a/docs/interop/index.rst b/docs/interop/index.rst
> index ded134ea75..049387ac6d 100644
> --- a/docs/interop/index.rst
> +++ b/docs/interop/index.rst
> @@ -14,6 +14,7 @@ Contents:
>
> bitmaps
> dbus
> + dbus-vmstate
> live-block-operations
> pr-helper
> qemu-ga
> --
> 2.24.0.308.g228f53135a
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 8/8] tests: add dbus-vmstate-test
2019-12-11 13:45 ` [PATCH v6 8/8] tests: add dbus-vmstate-test Marc-André Lureau
2019-12-12 12:11 ` Daniel P. Berrangé
@ 2019-12-13 18:20 ` Dr. David Alan Gilbert
2019-12-16 9:58 ` Daniel P. Berrangé
1 sibling, 1 reply; 22+ messages in thread
From: Dr. David Alan Gilbert @ 2019-12-13 18:20 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: mprivozn, pbonzini, berrange, qemu-devel, quintela
* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> +static gboolean
> +vmstate_save(VMState1 *object, GDBusMethodInvocation *invocation,
> + gpointer user_data)
> +{
> + TestServer *h = user_data;
> + GVariant *var;
> +
> + var = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE,
> + h->id->data, h->id->size, sizeof(char));
> + g_dbus_method_invocation_return_value(invocation,
> + g_variant_new("(@ay)", var));
> + h->save_called = true;
> +
> + return TRUE;
> +}
> +
> +static gboolean
> +wait_for_migration_complete(gpointer user_data)
It's a shame we don't have a way to share this with migration-test.c;
we occasionally add more debug/cases in there.
Dave
> +{
> + Test *test = user_data;
> + QDict *rsp_return;
> + bool stop = false;
> + const char *status;
> +
> + qtest_qmp_send(test->src_qemu, "{ 'execute': 'query-migrate' }");
> + rsp_return = qtest_qmp_receive_success(test->src_qemu, NULL, NULL);
> + status = qdict_get_str(rsp_return, "status");
> + if (g_str_equal(status, "completed") || g_str_equal(status, "failed")) {
> + stop = true;
> + g_assert_cmpstr(status, ==,
> + test->migrate_fail ? "failed" : "completed");
> + }
> + qobject_unref(rsp_return);
> +
> + if (stop) {
> + g_main_loop_quit(test->loop);
> + }
> + return stop ? G_SOURCE_REMOVE : G_SOURCE_CONTINUE;
> +}
> +
> +static void migrate(QTestState *who, const char *uri)
> +{
> + QDict *args, *rsp;
> +
> + args = qdict_new();
> + qdict_put_str(args, "uri", uri);
> +
> + rsp = qtest_qmp(who, "{ 'execute': 'migrate', 'arguments': %p }", args);
> +
> + g_assert(qdict_haskey(rsp, "return"));
> + qobject_unref(rsp);
> +}
> +
> +typedef struct WaitNamed {
> + GMainLoop *loop;
> + bool named;
> +} WaitNamed;
> +
> +static void
> +named_cb(GDBusConnection *connection,
> + const gchar *name,
> + gpointer user_data)
> +{
> + WaitNamed *t = user_data;
> +
> + t->named = true;
> + g_main_loop_quit(t->loop);
> +}
> +
> +static GDBusConnection *
> +get_connection(Test *test, guint *ownid)
> +{
> + g_autofree gchar *addr = NULL;
> + WaitNamed *wait;
> + GError *err = NULL;
> + GDBusConnection *c;
> +
> + wait = g_new0(WaitNamed, 1);
> + wait->loop = test->loop;
> + addr = g_dbus_address_get_for_bus_sync(G_BUS_TYPE_SESSION, NULL, &err);
> + g_assert_no_error(err);
> +
> + c = g_dbus_connection_new_for_address_sync(
> + addr,
> + G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION |
> + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT,
> + NULL, NULL, &err);
> + g_assert_no_error(err);
> + *ownid = g_bus_own_name_on_connection(c, "org.qemu.VMState1",
> + G_BUS_NAME_OWNER_FLAGS_NONE,
> + named_cb, named_cb, wait, g_free);
> + if (!wait->named) {
> + g_main_loop_run(wait->loop);
> + }
> +
> + return c;
> +}
> +
> +static GDBusObjectManagerServer *
> +get_server(GDBusConnection *conn, TestServer *s, const TestServerId *id)
> +{
> + g_autoptr(GDBusObjectSkeleton) sk = NULL;
> + g_autoptr(VMState1Skeleton) v = NULL;
> + GDBusObjectManagerServer *os;
> +
> + s->id = id;
> + os = g_dbus_object_manager_server_new("/org/qemu");
> + sk = g_dbus_object_skeleton_new("/org/qemu/VMState1");
> +
> + v = VMSTATE1_SKELETON(vmstate1_skeleton_new());
> + g_object_set(v, "id", id->name, NULL);
> +
> + g_signal_connect(v, "handle-load", G_CALLBACK(vmstate_load), s);
> + g_signal_connect(v, "handle-save", G_CALLBACK(vmstate_save), s);
> +
> + g_dbus_object_skeleton_add_interface(sk, G_DBUS_INTERFACE_SKELETON(v));
> + g_dbus_object_manager_server_export(os, sk);
> + g_dbus_object_manager_server_set_connection(os, conn);
> +
> + return os;
> +}
> +
> +static void
> +set_id_list(Test *test, QTestState *s)
> +{
> + if (!test->id_list) {
> + return;
> + }
> +
> + g_assert(!qmp_rsp_is_err(qtest_qmp(s,
> + "{ 'execute': 'qom-set', 'arguments': "
> + "{ 'path': '/objects/dv', 'property': 'id-list', 'value': %s } }",
> + test->id_list)));
> +}
> +static void
> +test_dbus_vmstate(Test *test)
> +{
> + g_autofree char *src_qemu_args = NULL;
> + g_autofree char *dst_qemu_args = NULL;
> + g_autoptr(GTestDBus) srcbus = NULL;
> + g_autoptr(GTestDBus) dstbus = NULL;
> + g_autoptr(GDBusConnection) srcconnA = NULL;
> + g_autoptr(GDBusConnection) srcconnB = NULL;
> + g_autoptr(GDBusConnection) dstconnA = NULL;
> + g_autoptr(GDBusConnection) dstconnB = NULL;
> + g_autoptr(GDBusObjectManagerServer) srcserverA = NULL;
> + g_autoptr(GDBusObjectManagerServer) srcserverB = NULL;
> + g_autoptr(GDBusObjectManagerServer) dstserverA = NULL;
> + g_autoptr(GDBusObjectManagerServer) dstserverB = NULL;
> + g_auto(GStrv) srcaddr = NULL;
> + g_auto(GStrv) dstaddr = NULL;
> + g_autofree char *uri = NULL;
> + QTestState *src_qemu = NULL, *dst_qemu = NULL;
> + guint ownsrcA, ownsrcB, owndstA, owndstB;
> +
> + uri = g_strdup_printf("unix:%s/migsocket", workdir);
> +
> + test->loop = g_main_loop_new(NULL, TRUE);
> +
> + srcbus = g_test_dbus_new(G_TEST_DBUS_NONE);
> + g_test_dbus_up(srcbus);
> + srcconnA = get_connection(test, &ownsrcA);
> + srcserverA = get_server(srcconnA, &test->srcA, &idA);
> + srcconnB = get_connection(test, &ownsrcB);
> + srcserverB = get_server(srcconnB, &test->srcB, &idB);
> +
> + /* remove ,guid=foo part */
> + srcaddr = g_strsplit(g_test_dbus_get_bus_address(srcbus), ",", 2);
> + src_qemu_args =
> + g_strdup_printf("-object dbus-vmstate,id=dv,addr=%s", srcaddr[0]);
> +
> + dstbus = g_test_dbus_new(G_TEST_DBUS_NONE);
> + g_test_dbus_up(dstbus);
> + dstconnA = get_connection(test, &owndstA);
> + dstserverA = get_server(dstconnA, &test->dstA, &idA);
> + if (!test->without_dst_b) {
> + dstconnB = get_connection(test, &owndstB);
> + dstserverB = get_server(dstconnB, &test->dstB, &idB);
> + }
> +
> + dstaddr = g_strsplit(g_test_dbus_get_bus_address(dstbus), ",", 2);
> + dst_qemu_args =
> + g_strdup_printf("-object dbus-vmstate,id=dv,addr=%s -incoming %s",
> + dstaddr[0], uri);
> +
> + src_qemu = qtest_init(src_qemu_args);
> + dst_qemu = qtest_init(dst_qemu_args);
> + set_id_list(test, src_qemu);
> + set_id_list(test, dst_qemu);
> +
> + migrate(src_qemu, uri);
> + test->src_qemu = src_qemu;
> + g_timeout_add_seconds(1, wait_for_migration_complete, test);
> +
> + g_main_loop_run(test->loop);
> + g_main_loop_unref(test->loop);
> +
> + if (test->migrate_fail) {
> + qtest_set_expected_status(dst_qemu, 1);
> + }
> + qtest_quit(dst_qemu);
> + qtest_quit(src_qemu);
> + g_bus_unown_name(ownsrcA);
> + g_bus_unown_name(ownsrcB);
> + g_bus_unown_name(owndstA);
> + if (!test->without_dst_b) {
> + g_bus_unown_name(owndstB);
> + }
> +}
> +
> +static void
> +check_not_migrated(TestServer *s, TestServer *d)
> +{
> + assert(!s->save_called);
> + assert(!s->load_called);
> + assert(!d->save_called);
> + assert(!d->load_called);
> +}
> +
> +static void
> +check_migrated(TestServer *s, TestServer *d)
> +{
> + assert(s->save_called);
> + assert(!s->load_called);
> + assert(!d->save_called);
> + assert(d->load_called);
> +}
> +
> +static void
> +test_dbus_vmstate_without_list(void)
> +{
> + Test test = { 0, };
> +
> + test_dbus_vmstate(&test);
> +
> + check_migrated(&test.srcA, &test.dstA);
> + check_migrated(&test.srcB, &test.dstB);
> +}
> +
> +static void
> +test_dbus_vmstate_with_list(void)
> +{
> + Test test = { .id_list = "idA,idB" };
> +
> + test_dbus_vmstate(&test);
> +
> + check_migrated(&test.srcA, &test.dstA);
> + check_migrated(&test.srcB, &test.dstB);
> +}
> +
> +static void
> +test_dbus_vmstate_only_a(void)
> +{
> + Test test = { .id_list = "idA" };
> +
> + test_dbus_vmstate(&test);
> +
> + check_migrated(&test.srcA, &test.dstA);
> + check_not_migrated(&test.srcB, &test.dstB);
> +}
> +
> +static void
> +test_dbus_vmstate_missing_src(void)
> +{
> + Test test = { .id_list = "idA,idC", .migrate_fail = true };
> +
> + /* run in subprocess to silence QEMU error reporting */
> + if (g_test_subprocess()) {
> + test_dbus_vmstate(&test);
> + check_not_migrated(&test.srcA, &test.dstA);
> + check_not_migrated(&test.srcB, &test.dstB);
> + return;
> + }
> +
> + g_test_trap_subprocess(NULL, 0, 0);
> + g_test_trap_assert_passed();
> +}
> +
> +static void
> +test_dbus_vmstate_missing_dst(void)
> +{
> + Test test = { .id_list = "idA,idB",
> + .without_dst_b = true,
> + .migrate_fail = true };
> +
> + /* run in subprocess to silence QEMU error reporting */
> + if (g_test_subprocess()) {
> + test_dbus_vmstate(&test);
> + assert(test.srcA.save_called);
> + assert(test.srcB.save_called);
> + assert(!test.dstB.save_called);
> + return;
> + }
> +
> + g_test_trap_subprocess(NULL, 0, 0);
> + g_test_trap_assert_passed();
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> + GError *err = NULL;
> + g_autofree char *dbus_daemon = NULL;
> + int ret;
> +
> + dbus_daemon = g_build_filename(G_STRINGIFY(SRCDIR),
> + "tests",
> + "dbus-vmstate-daemon.sh",
> + NULL);
> + g_setenv("G_TEST_DBUS_DAEMON", dbus_daemon, true);
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + workdir = g_dir_make_tmp("dbus-vmstate-test-XXXXXX", &err);
> + if (!workdir) {
> + g_error("Unable to create temporary dir: %s\n", err->message);
> + exit(1);
> + }
> +
> + qtest_add_func("/dbus-vmstate/without-list",
> + test_dbus_vmstate_without_list);
> + qtest_add_func("/dbus-vmstate/with-list",
> + test_dbus_vmstate_with_list);
> + qtest_add_func("/dbus-vmstate/only-a",
> + test_dbus_vmstate_only_a);
> + qtest_add_func("/dbus-vmstate/missing-src",
> + test_dbus_vmstate_missing_src);
> + qtest_add_func("/dbus-vmstate/missing-dst",
> + test_dbus_vmstate_missing_dst);
> +
> + ret = g_test_run();
> +
> + rmdir(workdir);
> + g_free(workdir);
> +
> + return ret;
> +}
> diff --git a/tests/dbus-vmstate1.xml b/tests/dbus-vmstate1.xml
> new file mode 100644
> index 0000000000..cc8563be4c
> --- /dev/null
> +++ b/tests/dbus-vmstate1.xml
> @@ -0,0 +1,12 @@
> +<?xml version="1.0"?>
> +<node name="/" xmlns:doc="http://www.freedesktop.org/dbus/1.0/doc.dtd">
> + <interface name="org.qemu.VMState1">
> + <property name="Id" type="s" access="read"/>
> + <method name="Load">
> + <arg type="ay" name="data" direction="in"/>
> + </method>
> + <method name="Save">
> + <arg type="ay" name="data" direction="out"/>
> + </method>
> + </interface>
> +</node>
> --
> 2.24.0.308.g228f53135a
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 5/8] Add dbus-vmstate object
2019-12-12 12:03 ` Daniel P. Berrangé
@ 2019-12-16 7:44 ` Marc-André Lureau
0 siblings, 0 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-16 7:44 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Michal Privoznik, Juan Quintela, QEMU, Dr. David Alan Gilbert,
Paolo Bonzini
Hi
On Thu, Dec 12, 2019 at 4:03 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Dec 11, 2019 at 05:45:03PM +0400, Marc-André Lureau wrote:
> > When instantiated, this object will connect to the given D-Bus bus
> > "addr". During migration, it will take/restore the data from
> > org.qemu.VMState1 instances. See documentation for details.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > MAINTAINERS | 2 +
> > backends/Makefile.objs | 4 +
> > backends/dbus-vmstate.c | 496 ++++++++++++++++++++++++++++++++++
> > docs/interop/dbus-vmstate.rst | 74 +++++
> > docs/interop/dbus.rst | 5 +
> > docs/interop/index.rst | 1 +
> > 6 files changed, 582 insertions(+)
> > create mode 100644 backends/dbus-vmstate.c
> > create mode 100644 docs/interop/dbus-vmstate.rst
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f08fb4f24e..7af80d0c1d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2202,9 +2202,11 @@ F: qapi/migration.json
> > D-Bus
> > M: Marc-André Lureau <marcandre.lureau@redhat.com>
> > S: Maintained
> > +F: backends/dbus-vmstate.c
> > F: util/dbus.c
> > F: include/qemu/dbus.h
> > F: docs/interop/dbus.rst
> > +F: docs/interop/dbus-vmstate.rst
> >
> > Seccomp
> > M: Eduardo Otubo <otubo@redhat.com>
> > diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> > index f0691116e8..28a847cd57 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)
> > diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> > new file mode 100644
> > index 0000000000..059dd420b8
> > --- /dev/null
> > +++ b/backends/dbus-vmstate.c
> > @@ -0,0 +1,496 @@
> > +/*
> > + * QEMU dbus-vmstate
> > + *
> > + * Copyright (C) 2019 Red Hat Inc
> > + *
> > + * Authors:
> > + * Marc-André Lureau <marcandre.lureau@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/units.h"
> > +#include "qemu/dbus.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/error.h"
> > +#include "qom/object_interfaces.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "migration/vmstate.h"
> > +
> > +typedef struct DBusVMState DBusVMState;
> > +typedef struct DBusVMStateClass DBusVMStateClass;
> > +
> > +#define TYPE_DBUS_VMSTATE "dbus-vmstate"
> > +#define DBUS_VMSTATE(obj) \
> > + OBJECT_CHECK(DBusVMState, (obj), TYPE_DBUS_VMSTATE)
> > +#define DBUS_VMSTATE_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(DBusVMStateClass, (obj), TYPE_DBUS_VMSTATE)
> > +#define DBUS_VMSTATE_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(DBusVMStateClass, (klass), TYPE_DBUS_VMSTATE)
> > +
> > +struct DBusVMStateClass {
> > + ObjectClass parent_class;
> > +};
> > +
>
> Not an objection to your patch here. This just reminds me that we
> ought to follow GLib's lead and implement some helper macros to
> automate all this tedious boilerplate. So we can just do something
> simple like:
>
> QOM_DECLARE_FINAL_TYPE(DBusVMState, dbus_vmstate, DBUS_VMSATE, Object)
>
> and an equiv to do the TypeInfo declaration & registration.
>
> > +struct DBusVMState {
> > + Object parent;
> > +
> > + GDBusConnection *bus;
> > + char *dbus_addr;
> > + char *id_list;
> > +
> > + uint32_t data_size;
> > + uint8_t *data;
> > +};
> > +
> > +static const GDBusPropertyInfo vmstate_property_info[] = {
> > + { -1, (char *) "Id", (char *) "s",
> > + G_DBUS_PROPERTY_INFO_FLAGS_READABLE, NULL },
> > +};
> > +
> > +static const GDBusPropertyInfo * const vmstate_property_info_pointers[] = {
> > + &vmstate_property_info[0],
> > + NULL
> > +};
> > +
> > +static const GDBusInterfaceInfo vmstate1_interface_info = {
> > + -1,
> > + (char *) "org.qemu.VMState1",
> > + (GDBusMethodInfo **) NULL,
> > + (GDBusSignalInfo **) NULL,
> > + (GDBusPropertyInfo **) &vmstate_property_info_pointers,
> > + NULL,
> > +};
> > +
> > +#define DBUS_VMSTATE_SIZE_LIMIT (1 * MiB)
> > +
> > +static GHashTable *
> > +get_id_list_set(DBusVMState *self)
> > +{
> > + g_auto(GStrv) ids = NULL;
> > + g_autoptr(GHashTable) set = NULL;
> > + 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;
> > + }
> > +
> > + return g_steal_pointer(&set);
> > +}
> > +
> > +static GHashTable *
> > +dbus_get_proxies(DBusVMState *self, GError **err)
> > +{
> > + g_autoptr(GHashTable) proxies = NULL;
> > + g_autoptr(GHashTable) ids = NULL;
> > + g_auto(GStrv) names = NULL;
> > + size_t i;
> > +
> > + ids = get_id_list_set(self);
> > + proxies = g_hash_table_new_full(g_str_hash, g_str_equal,
> > + g_free, g_object_unref);
> > +
> > + names = qemu_dbus_get_queued_owners(self->bus, "org.qemu.VMState1");
> > + if (!names) {
> > + return NULL;
> > + }
> > +
> > + for (i = 0; names[i]; i++) {
> > + g_autoptr(GDBusProxy) proxy = NULL;
> > + g_autoptr(GVariant) result = NULL;
> > + g_autofree char *id = NULL;
> > + 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) {
> > + return NULL;
> > + }
> > +
> > + 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.");
> > + return NULL;
> > + }
> > +
> > + id = g_variant_dup_string(result, &size);
> > + if (ids && !g_hash_table_remove(ids, id)) {
> > + g_clear_pointer(&id, g_free);
> > + g_clear_object(&proxy);
> > + continue;
> > + }
> > + if (size == 0 || size >= 256) {
> > + g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> > + "VMState Id '%s' is invalid.", id);
> > + return NULL;
> > + }
> > +
> > + if (!g_hash_table_insert(proxies, id, proxy)) {
> > + g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> > + "Duplicated VMState Id '%s'", id);
> > + return NULL;
> > + }
> > + id = NULL;
> > + proxy = NULL;
> > +
> > + g_clear_pointer(&result, g_variant_unref);
> > + }
> > +
> > + if (ids) {
> > + g_autofree char **left = NULL;
> > +
> > + left = (char **)g_hash_table_get_keys_as_array(ids, NULL);
> > + if (*left) {
> > + g_autofree char *leftids = g_strjoinv(",", left);
> > + g_set_error(err, G_IO_ERROR, G_IO_ERROR_FAILED,
> > + "Required VMState Id are missing: %s", leftids);
> > + return NULL;
> > + }
> > + }
> > +
> > + return g_steal_pointer(&proxies);
> > +}
> > +
> > +static int
> > +dbus_load_state_proxy(GDBusProxy *proxy, const uint8_t *data, size_t size)
> > +{
> > + g_autoptr(GError) err = NULL;
> > + g_autoptr(GVariant) result = NULL;
> > + g_autoptr(GVariant) value = NULL;
> > +
> > + 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)",
> > + g_steal_pointer(&value)),
> > + G_DBUS_CALL_FLAGS_NO_AUTO_START,
> > + -1, NULL, &err);
> > + if (!result) {
> > + error_report("Failed to Load: %s", err->message);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int dbus_vmstate_post_load(void *opaque, int version_id)
> > +{
> > + DBusVMState *self = DBUS_VMSTATE(opaque);
> > + g_autoptr(GInputStream) m = NULL;
> > + g_autoptr(GDataInputStream) s = NULL;
> > + g_autoptr(GError) err = NULL;
> > + g_autoptr(GHashTable) proxies = NULL;
> > + uint32_t nelem;
> > +
> > + proxies = dbus_get_proxies(self, &err);
> > + if (!proxies) {
> > + error_report("Failed to get proxies: %s", err->message);
> > + return -1;
> > + }
> > +
> > + m = g_memory_input_stream_new_from_data(self->data, self->data_size, NULL);
> > + s = g_data_input_stream_new(m);
> > + g_data_input_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
> > +
> > + nelem = g_data_input_stream_read_uint32(s, NULL, &err);
> > + if (err) {
> > + goto error;
> > + }
> > +
> > + while (nelem > 0) {
> > + GDBusProxy *proxy = NULL;
> > + uint32_t len;
> > + gsize bytes_read, avail;
> > + char id[256];
> > +
> > + len = g_data_input_stream_read_uint32(s, NULL, &err);
> > + if (err) {
> > + goto error;
> > + }
> > + if (len >= 256) {
> > + error_report("Invalid DBus vmstate proxy name %u", len);
> > + return -1;
> > + }
> > + if (!g_input_stream_read_all(G_INPUT_STREAM(s), id, len,
> > + &bytes_read, NULL, &err)) {
> > + goto error;
> > + }
> > + g_return_val_if_fail(bytes_read == len, -1);
> > + id[len] = 0;
> > +
> > + proxy = g_hash_table_lookup(proxies, id);
> > + if (!proxy) {
> > + error_report("Failed to find proxy Id '%s'", id);
> > + return -1;
> > + }
> > +
> > + len = g_data_input_stream_read_uint32(s, NULL, &err);
> > + avail = g_buffered_input_stream_get_available(
> > + G_BUFFERED_INPUT_STREAM(s));
> > +
> > + if (len > DBUS_VMSTATE_SIZE_LIMIT || len > avail) {
> > + error_report("Invalid vmstate size: %u", len);
> > + return -1;
> > + }
> > +
> > + if (dbus_load_state_proxy(proxy,
> > + g_buffered_input_stream_peek_buffer(G_BUFFERED_INPUT_STREAM(s),
> > + NULL),
> > + len) < 0) {
> > + error_report("Failed to restore Id '%s'", id);
> > + return -1;
> > + }
> > +
> > + if (!g_seekable_seek(G_SEEKABLE(s), len, G_SEEK_CUR, NULL, &err)) {
> > + goto error;
> > + }
> > +
> > + nelem -= 1;
> > + }
> > +
> > + return 0;
> > +
> > +error:
> > + error_report("Failed to read from stream: %s", err->message);
> > + return -1;
> > +}
> > +
> > +static void
> > +dbus_save_state_proxy(gpointer key,
> > + gpointer value,
> > + gpointer user_data)
> > +{
> > + GDataOutputStream *s = user_data;
> > + const char *id = key;
> > + GDBusProxy *proxy = value;
> > + g_autoptr(GVariant) result = NULL;
> > + g_autoptr(GVariant) child = NULL;
> > + g_autoptr(GError) err = NULL;
> > + const uint8_t *data;
> > + gsize size;
> > +
> > + 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);
> > + return;
> > + }
> > +
> > + 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");
> > + return;
> > + }
> > + if (size > DBUS_VMSTATE_SIZE_LIMIT) {
> > + error_report("Too large vmstate data to save: %" G_GSIZE_FORMAT, size);
> > + return;
> > + }
> > +
> > + if (!g_data_output_stream_put_uint32(s, strlen(id), NULL, &err) ||
> > + !g_data_output_stream_put_string(s, id, NULL, &err) ||
> > + !g_data_output_stream_put_uint32(s, size, NULL, &err) ||
> > + !g_output_stream_write_all(G_OUTPUT_STREAM(s),
> > + data, size, NULL, NULL, &err)) {
> > + error_report("Failed to write to stream: %s", err->message);
> > + }
>
> This is a bit of a bike-shed comment, but I'm curious if you
> considered using GVariant for the serializing data vs the
> data output stream ? I feel like GVariant is enforcing more
> structure & safety on the data serialization process, which
> could be appealing.
>
No I haven't thought about it. I don't know if it's worth it for a string + u32.
> > +static int dbus_vmstate_pre_save(void *opaque)
> > +{
> > + DBusVMState *self = DBUS_VMSTATE(opaque);
> > + g_autoptr(GOutputStream) m = NULL;
> > + g_autoptr(GDataOutputStream) s = NULL;
> > + g_autoptr(GHashTable) proxies = NULL;
> > + g_autoptr(GError) err = NULL;
> > +
> > + proxies = dbus_get_proxies(self, &err);
> > + if (!proxies) {
> > + error_report("Failed to get proxies: %s", err->message);
> > + return -1;
> > + }
> > +
> > + m = g_memory_output_stream_new_resizable();
> > + s = g_data_output_stream_new(m);
> > + g_data_output_stream_set_byte_order(s, G_DATA_STREAM_BYTE_ORDER_BIG_ENDIAN);
> > +
> > + if (!g_data_output_stream_put_uint32(s, g_hash_table_size(proxies),
> > + NULL, &err)) {
> > + error_report("Failed to write to stream: %s", err->message);
> > + return -1;
> > + }
> > +
> > + g_hash_table_foreach(proxies, dbus_save_state_proxy, s);
> > +
> > + if (g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m))
> > + > UINT32_MAX) {
> > + error_report("DBus vmstate buffer is too large");
> > + return -1;
> > + }
> > +
> > + if (!g_output_stream_close(G_OUTPUT_STREAM(m), NULL, &err)) {
> > + error_report("Failed to close stream: %s", err->message);
> > + return -1;
> > + }
> > +
> > + g_free(self->data);
> > + self->data_size =
> > + g_memory_output_stream_get_size(G_MEMORY_OUTPUT_STREAM(m));
> > + self->data =
> > + g_memory_output_stream_steal_data(G_MEMORY_OUTPUT_STREAM(m));
> > +
> > + return 0;
> > +}
> > +
> > +static const VMStateDescription dbus_vmstate = {
> > + .name = TYPE_DBUS_VMSTATE,
> > + .version_id = 0,
> > + .pre_save = dbus_vmstate_pre_save,
> > + .post_load = dbus_vmstate_post_load,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_UINT32(data_size, DBusVMState),
> > + VMSTATE_VBUFFER_ALLOC_UINT32(data, DBusVMState, 0, 0, data_size),
> > + VMSTATE_END_OF_LIST()
> > + }
> > +};
> > +
> > +static void
> > +dbus_vmstate_complete(UserCreatable *uc, Error **errp)
> > +{
> > + DBusVMState *self = DBUS_VMSTATE(uc);
> > + GError *err = NULL;
>
> Can use g_autoptr for this
yes, thanks
>
> > + 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;
> > + }
> > +
> > + 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);
>
> Why not just use self->bus directly.
That works too, thanks
>
> > + if (err) {
> > + error_setg(errp, "failed to connect to DBus: '%s'", err->message);
> > + g_clear_error(&err);
>
> Missing return here, as we don't want to register vmstate handler
> if we fail.
ok
>
> > + }
> > +
> > + self->bus = bus;
> > +
> > + if (vmstate_register(VMSTATE_IF(self), -1, &dbus_vmstate, self) < 0) {
> > + error_setg(errp, "Failed to register vmstate");
> > + }
> > +}
>
>
> > diff --git a/docs/interop/dbus-vmstate.rst b/docs/interop/dbus-vmstate.rst
> > new file mode 100644
> > index 0000000000..8693891640
> > --- /dev/null
> > +++ b/docs/interop/dbus-vmstate.rst
> > @@ -0,0 +1,74 @@
> > +=============
> > +D-Bus VMState
> > +=============
> > +
> > +Introduction
> > +============
> > +
> > +The QEMU dbus-vmstate object's aim is to migrate helpers' data running
> > +on a QEMU D-Bus bus. (refer to the :doc:`dbus` document for
> > +recommendation on D-Bus usage)
> > +
> > +Upon migration, QEMU will go through the queue of
> > +``org.qemu.VMState1`` D-Bus 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). (D-Bus imposes a time limit on
>
> A few seconds is too long IMHO. I think the expectation ought to
> be a small fraction of a second.
>
> Anything longer than that suggests there is some extra synchronization
> work needing beyond serailizing state, which might suggest the need
> for a separate DBus call. eg a way to tell the backend to "quiesce"
> itself perhaps
>
> For now we can keep it simple and just say that this method should
> not do anything except seriailize state in a fraction of a second.
ok
>
> > +reply anyway, and migration would fail if data isn't given quickly
> > +enough.)
> > +
> > +dbus-vmstate object can be configured with the expected list of
> > +helpers by setting its ``id-list`` property, with a comma-separated
> > +``Id`` list.
> > +
> > +Interface
> > +=========
> > +
> > +On object path ``/org/qemu/VMState1``, the following
> > +``org.qemu.VMState1`` interface should be implemented:
> > +
> > +.. code:: xml
> > +
> > + <interface name="org.qemu.VMState1">
> > + <property name="Id" type="s" access="read"/>
> > + <method name="Load">
> > + <arg type="ay" name="data" direction="in"/>
> > + </method>
> > + <method name="Save">
> > + <arg type="ay" name="data" direction="out"/>
> > + </method>
> > + </interface>
> > +
> > +"Id" property
> > +-------------
> > +
> > +A string that identifies the helper uniquely. (maximum 256 bytes
> > +including terminating NUL byte)
> > +
> > +.. note::
> > +
> > + The helper ID namespace is a separate namespace. In particular, it is not
> > + related to QEMU "id" used in -object/-device objects.
>
> Are there any expectations here on a scheme ? I feel leaving it
> unspecified is probably a mistake. Should it follow a DBUs like
> reverse.domain.name.style ?
In general, it should follow the associated qemu object/device id.
>
> > +
> > +Load(in u8[] bytes) method
> > +--------------------------
> > +
> > +The method called on destination with the state to restore.
> > +
> > +The helper may be initially started in a waiting state (with
> > +an --incoming argument for example), and it may resume on success.
> > +
> > +An error may be returned to the caller.
> > +
> > +Save(out u8[] bytes) method
> > +---------------------------
> > +
> > +The method called on the source to get the current state to be
> > +migrated. The helper should continue to run normally.
> > +
> > +An error may be returned to the caller.
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 8/8] tests: add dbus-vmstate-test
2019-12-13 18:20 ` Dr. David Alan Gilbert
@ 2019-12-16 9:58 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-16 9:58 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: mprivozn, Marc-André Lureau, pbonzini, qemu-devel, quintela
On Fri, Dec 13, 2019 at 06:20:15PM +0000, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> > +static gboolean
> > +vmstate_save(VMState1 *object, GDBusMethodInvocation *invocation,
> > + gpointer user_data)
> > +{
> > + TestServer *h = user_data;
> > + GVariant *var;
> > +
> > + var = g_variant_new_fixed_array(G_VARIANT_TYPE_BYTE,
> > + h->id->data, h->id->size, sizeof(char));
> > + g_dbus_method_invocation_return_value(invocation,
> > + g_variant_new("(@ay)", var));
> > + h->save_called = true;
> > +
> > + return TRUE;
> > +}
> > +
> > +static gboolean
> > +wait_for_migration_complete(gpointer user_data)
>
> It's a shame we don't have a way to share this with migration-test.c;
> we occasionally add more debug/cases in there.
Easy enough to create a tests/migration-helpers.{c,h} file to share
code between tests.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions
2019-12-12 12:06 ` Daniel P. Berrangé
@ 2019-12-19 12:23 ` Marc-André Lureau
2019-12-19 12:30 ` Daniel P. Berrangé
0 siblings, 1 reply; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-19 12:23 UTC (permalink / raw)
To: Daniel P. Berrangé, Philippe Mathieu-Daudé
Cc: Michal Privoznik, Juan Quintela, QEMU, Dr. David Alan Gilbert,
Paolo Bonzini
Hi
On Thu, Dec 12, 2019 at 4:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Dec 11, 2019 at 05:45:05PM +0400, Marc-André Lureau wrote:
> > To get dbus-vmstate test covered.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > tests/docker/dockerfiles/centos7.docker | 1 +
> > tests/docker/dockerfiles/debian10.docker | 1 +
> > tests/docker/dockerfiles/fedora.docker | 1 +
> > tests/docker/dockerfiles/ubuntu.docker | 1 +
> > 4 files changed, 4 insertions(+)
>
> For docker
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> Does it need adding to travis, gitlab, shippable, etc CI configs
> too ?
Those are all good, only shippable has an issue with w64 build:
backends/dbus-vmstate.c:313:22: error: format '%u' expects argument of
type 'unsigned int', but argument 3 has type 'gsize {aka long long
unsigned int}' [-Werror=format=]
error_report("%s: Too large vmstate data to save: %" G_GSIZE_FORMAT,
This seems to be a MXE build issue, since build correctly includes
"/usr/lib/mxe/usr/x86_64-w64-mingw32.shared/include/glib-2.0", and
fedora mingw works. Philippe, any idea?
And I didn't manage to run a Cirrus CI build, help welcome.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions
2019-12-19 12:23 ` Marc-André Lureau
@ 2019-12-19 12:30 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2019-12-19 12:30 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Juan Quintela, Michal Privoznik, QEMU, Dr. David Alan Gilbert,
Paolo Bonzini, Philippe Mathieu-Daudé
On Thu, Dec 19, 2019 at 04:23:48PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Thu, Dec 12, 2019 at 4:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Wed, Dec 11, 2019 at 05:45:05PM +0400, Marc-André Lureau wrote:
> > > To get dbus-vmstate test covered.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > tests/docker/dockerfiles/centos7.docker | 1 +
> > > tests/docker/dockerfiles/debian10.docker | 1 +
> > > tests/docker/dockerfiles/fedora.docker | 1 +
> > > tests/docker/dockerfiles/ubuntu.docker | 1 +
> > > 4 files changed, 4 insertions(+)
> >
> > For docker
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> >
> > Does it need adding to travis, gitlab, shippable, etc CI configs
> > too ?
>
> Those are all good, only shippable has an issue with w64 build:
>
> backends/dbus-vmstate.c:313:22: error: format '%u' expects argument of
> type 'unsigned int', but argument 3 has type 'gsize {aka long long
> unsigned int}' [-Werror=format=]
> error_report("%s: Too large vmstate data to save: %" G_GSIZE_FORMAT,
>
> This seems to be a MXE build issue, since build correctly includes
> "/usr/lib/mxe/usr/x86_64-w64-mingw32.shared/include/glib-2.0", and
> fedora mingw works. Philippe, any idea?
There was a bug with one release of glib where they changed the
printf function annotations, but messed up so G_*_FORMAT was
changed when built with meson, but not changed when built with
autotools.
So if MXE uses that broken glib version & builds mingw with
autotools instead of meson it will be impacted:
https://gitlab.gnome.org/GNOME/glib/issues/1497
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 6/8] configure: add GDBUS_CODEGEN
2019-12-12 12:05 ` Daniel P. Berrangé
@ 2019-12-19 12:54 ` Marc-André Lureau
0 siblings, 0 replies; 22+ messages in thread
From: Marc-André Lureau @ 2019-12-19 12:54 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Michal Privoznik, Juan Quintela, QEMU, Dr. David Alan Gilbert,
Paolo Bonzini
Hi
On Thu, Dec 12, 2019 at 4:05 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Dec 11, 2019 at 05:45:04PM +0400, Marc-André Lureau wrote:
> > gdbus-codegen generated code requires gio-unix on Unix, so add it to
> > GIO libs/cflags.
>
> What is the situation on Windows, is it still supported ?
Yes, it should build fine. Only fd-passing related code is missing.
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > configure | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/configure b/configure
> > index 6099be1d84..68a7705df7 100755
> > --- a/configure
> > +++ b/configure
> > @@ -3720,10 +3720,16 @@ if $pkg_config --atleast-version=$glib_req_ver gio-2.0; then
> > gio=yes
> > gio_cflags=$($pkg_config --cflags gio-2.0)
> > gio_libs=$($pkg_config --libs gio-2.0)
> > + gdbus_codegen=$($pkg_config --variable=gdbus_codegen gio-2.0)
> > else
> > gio=no
> > fi
> >
> > +if $pkg_config --atleast-version=$glib_req_ver gio-unix-2.0; then
> > + gio_cflags="$gio_cflags $($pkg_config --cflags gio-unix-2.0)"
> > + gio_libs="$gio_libs $($pkg_config --libs gio-unix-2.0)"
> > +fi
> > +
> > # Sanity check that the current size_t matches the
> > # size that glib thinks it should be. This catches
> > # problems on multi-arch where people try to build
> > @@ -6949,6 +6955,7 @@ if test "$gio" = "yes" ; then
> > echo "CONFIG_GIO=y" >> $config_host_mak
> > echo "GIO_CFLAGS=$gio_cflags" >> $config_host_mak
> > echo "GIO_LIBS=$gio_libs" >> $config_host_mak
> > + echo "GDBUS_CODEGEN=$gdbus_codegen" >> $config_host_mak
> > fi
> > echo "CONFIG_TLS_PRIORITY=\"$tls_priority\"" >> $config_host_mak
> > if test "$gnutls" = "yes" ; then
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-12-19 13:20 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 13:44 [PATCH v6 0/8] Add dbus-vmstate Marc-André Lureau
2019-12-11 13:44 ` [PATCH v6 1/8] vmstate: add qom interface to get id Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 2/8] vmstate: replace DeviceState with VMStateIf Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 3/8] docs: start a document to describe D-Bus usage Marc-André Lureau
2019-12-12 11:36 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 4/8] util: add dbus helper unit Marc-André Lureau
2019-12-12 11:38 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 5/8] Add dbus-vmstate object Marc-André Lureau
2019-12-12 12:03 ` Daniel P. Berrangé
2019-12-16 7:44 ` Marc-André Lureau
2019-12-13 16:32 ` Dr. David Alan Gilbert
2019-12-11 13:45 ` [PATCH v6 6/8] configure: add GDBUS_CODEGEN Marc-André Lureau
2019-12-12 12:05 ` Daniel P. Berrangé
2019-12-19 12:54 ` Marc-André Lureau
2019-12-11 13:45 ` [PATCH v6 7/8] dockerfiles: add dbus-daemon to some of latest distributions Marc-André Lureau
2019-12-12 12:06 ` Daniel P. Berrangé
2019-12-19 12:23 ` Marc-André Lureau
2019-12-19 12:30 ` Daniel P. Berrangé
2019-12-11 13:45 ` [PATCH v6 8/8] tests: add dbus-vmstate-test Marc-André Lureau
2019-12-12 12:11 ` Daniel P. Berrangé
2019-12-13 18:20 ` Dr. David Alan Gilbert
2019-12-16 9:58 ` Daniel P. Berrangé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.