Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/conf/virdomainobjlist.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d640da6205..11fd68745b 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -815,6 +815,19 @@ virDomainObjListHelper(void *payload, } +/** + * virDomainObjListForEach: + * @doms: Pointer to the domain object list + * @callback: callback to run over each domain on the list + * @opaque: opaque data to pass to @callback + * + * For every domain on the list (@doms) run @callback on it. If + * @callback fails (i.e. returns a negative value), the iteration + * carries still on until all domains are visited. + * + * Returns: 0 on success, + * -1 otherwise. + */ int virDomainObjListForEach(virDomainObjListPtr doms, virDomainObjListIterator callback, -- 2.21.0

In some places where virDomainObjListForEach() is called the passed callback calls virDomainObjListRemoveLocked(). Well, this is unsafe, because the former only grabs a read lock but the latter modifies the list. I've identified the following unsafe calls: - qemuProcessReconnectAll() - libxlReconnectDomains() The rest seem to be safe. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/virdomainobjlist.c | 12 ++++++++++-- src/conf/virdomainobjlist.h | 1 + src/libxl/libxl_driver.c | 11 +++++++---- src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_process.c | 3 ++- src/vmware/vmware_driver.c | 2 +- 8 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index e2c1b00080..c52def7607 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -114,7 +114,7 @@ bhyveAutostartDomains(bhyveConnPtr driver) struct bhyveAutostartData data = { driver, conn }; - virDomainObjListForEach(driver->domains, bhyveAutostartDomain, &data); + virDomainObjListForEach(driver->domains, false, bhyveAutostartDomain, &data); virObjectUnref(conn); } diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 4dab6e5e54..5fea2eb51c 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -466,7 +466,7 @@ virBhyveProcessReconnectAll(bhyveConnPtr driver) data.driver = driver; data.kd = kd; - virDomainObjListForEach(driver->domains, virBhyveProcessReconnect, &data); + virDomainObjListForEach(driver->domains, false, virBhyveProcessReconnect, &data); kvm_close(kd); } diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 11fd68745b..2eff6768c2 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -818,25 +818,33 @@ virDomainObjListHelper(void *payload, /** * virDomainObjListForEach: * @doms: Pointer to the domain object list + * @modify: Whether to lock @doms for modify operation * @callback: callback to run over each domain on the list * @opaque: opaque data to pass to @callback * * For every domain on the list (@doms) run @callback on it. If * @callback fails (i.e. returns a negative value), the iteration - * carries still on until all domains are visited. + * carries still on until all domains are visited. Moreover, if + * @callback wants to modify the list of domains (@doms) then + * @modify must be set to true. * * Returns: 0 on success, * -1 otherwise. */ int virDomainObjListForEach(virDomainObjListPtr doms, + bool modify, virDomainObjListIterator callback, void *opaque) { struct virDomainListIterData data = { callback, opaque, 0, }; - virObjectRWLockRead(doms); + + if (modify) + virObjectRWLockWrite(doms); + else + virObjectRWLockRead(doms); virHashForEach(doms->objs, virDomainObjListHelper, &data); virObjectRWUnlock(doms); return data.ret; diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index 7d71bc54d0..da5ec8a57c 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -91,6 +91,7 @@ typedef int (*virDomainObjListIterator)(virDomainObjPtr dom, void *opaque); int virDomainObjListForEach(virDomainObjListPtr doms, + bool modify, virDomainObjListIterator callback, void *opaque); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d0396e4781..5e14aa4fbe 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -497,7 +497,7 @@ libxlReconnectDomain(virDomainObjPtr vm, static void libxlReconnectDomains(libxlDriverPrivatePtr driver) { - virDomainObjListForEach(driver->domains, libxlReconnectDomain, driver); + virDomainObjListForEach(driver->domains, true, libxlReconnectDomain, driver); } static int @@ -800,10 +800,12 @@ libxlStateInitialize(bool privileged, NULL, NULL) < 0) goto error; - virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain, + virDomainObjListForEach(libxl_driver->domains, false, + libxlAutostartDomain, libxl_driver); - virDomainObjListForEach(libxl_driver->domains, libxlDomainManagedSaveLoad, + virDomainObjListForEach(libxl_driver->domains, false, + libxlDomainManagedSaveLoad, libxl_driver); return VIR_DRV_STATE_INIT_COMPLETE; @@ -832,7 +834,8 @@ libxlStateReload(void) libxl_driver->xmlopt, NULL, libxl_driver); - virDomainObjListForEach(libxl_driver->domains, libxlAutostartDomain, + virDomainObjListForEach(libxl_driver->domains, false, + libxlAutostartDomain, libxl_driver); virObjectUnref(cfg); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index cd65e7a0c0..cbdc7b1268 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1639,7 +1639,7 @@ virLXCProcessAutostartAll(virLXCDriverPtr driver) struct virLXCProcessAutostartData data = { driver, conn }; - virDomainObjListForEach(driver->domains, + virDomainObjListForEach(driver->domains, false, virLXCProcessAutostartDomain, &data); @@ -1760,6 +1760,6 @@ virLXCProcessReconnectDomain(virDomainObjPtr vm, int virLXCProcessReconnectAll(virLXCDriverPtr driver, virDomainObjListPtr doms) { - virDomainObjListForEach(doms, virLXCProcessReconnectDomain, driver); + virDomainObjListForEach(doms, false, virLXCProcessReconnectDomain, driver); return 0; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1960f53466..6a261786f9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8369,7 +8369,8 @@ void qemuProcessReconnectAll(virQEMUDriverPtr driver) { struct qemuProcessReconnectData data = {.driver = driver}; - virDomainObjListForEach(driver->domains, qemuProcessReconnectHelper, &data); + virDomainObjListForEach(driver->domains, true, + qemuProcessReconnectHelper, &data); } diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 1bc8a06c39..a87af85916 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -990,7 +990,7 @@ static int vmwareDomainObjListUpdateDomain(virDomainObjPtr dom, void *data) static void vmwareDomainObjListUpdateAll(virDomainObjListPtr doms, struct vmware_driver *driver) { - virDomainObjListForEach(doms, vmwareDomainObjListUpdateDomain, driver); + virDomainObjListForEach(doms, false, vmwareDomainObjListUpdateDomain, driver); } static int -- 2.21.0

The @cmdstr variable is not used really. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_slirp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 24df1e1a20..716b73759d 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -298,7 +298,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, { VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); VIR_AUTOPTR(virCommand) cmd = NULL; - VIR_AUTOFREE(char *) cmdstr = NULL; VIR_AUTOFREE(char *) pidfile = NULL; VIR_AUTOFREE(char *) dbus_path = NULL; VIR_AUTOFREE(char *) dbus_addr = NULL; -- 2.21.0

The qemu side is not merged in yet, so there is a chance that the interface will change. Don't detect the capability just yet then. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 136999ad0d..7be026ae12 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1129,7 +1129,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-serial-pci-non-transitional", QEMU_CAPS_VIRTIO_PCI_TRANSITIONAL }, { "max-x86_64-cpu", QEMU_CAPS_X86_MAX_CPU }, { "bochs-display", QEMU_CAPS_DEVICE_BOCHS_DISPLAY }, - { "dbus-vmstate", QEMU_CAPS_DBUS_VMSTATE }, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { -- 2.21.0

On 9/6/19 5:55 PM, Ján Tomko wrote: So are you saying that we should just temporarily revert QEMU_CAPS_NET_SOCKET_DGRAM detection instead and leave QEMU_CAPS_DBUS_VMSTATE be? Michal

Hi On Mon, Sep 9, 2019 at 2:49 PM Ján Tomko <jtomko(a)redhat.com&gt; wrote: I agree that as long as qemu doesn't have dbus-vmstate, it is better to revert dbus-vmstate support, and disable net-socket-dgram.