[libvirt] [PATCH 0/4] Couple of qemu fixups
The first issue (patch 2/4) is an issue I've found by code investigation. The second (patch 3/4) fixes a build problem and the last patch disables detection of a feature that is not merged in QEMU just yet. Michal Prívozník (4): virdomainobjlist: Documnet virDomainObjListForEach() lib: Grab write lock when modifying list of domains qemu_slirp: Drop unused variable in qemuSlirpStart() qemu_capabilities: Temporarily disable dbus-vmstate capability src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/conf/virdomainobjlist.c | 23 ++++++++++++++++++++++- src/conf/virdomainobjlist.h | 1 + src/libxl/libxl_driver.c | 11 +++++++---- src/lxc/lxc_process.c | 4 ++-- src/qemu/qemu_capabilities.c | 1 - src/qemu/qemu_process.c | 3 ++- src/qemu/qemu_slirp.c | 1 - src/vmware/vmware_driver.c | 2 +- 10 files changed, 37 insertions(+), 13 deletions(-) -- 2.21.0
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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
On Fri, Sep 06, 2019 at 04:25:16PM +0200, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
s/Documnet/Document/ in the commit summary
src/conf/virdomainobjlist.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@redhat.com> --- 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
On Fri, Sep 06, 2019 at 04:25:17PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The @cmdstr variable is not used really. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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
On Fri, Sep 06, 2019 at 04:25:18PM +0200, Michal Privoznik wrote:
The @cmdstr variable is not used really.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_slirp.c | 1 - 1 file changed, 1 deletion(-)
This is a trivial build breaker fix and can be pushed right away regardless of the rest of the series. Jano
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@redhat.com> --- 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
[cc-ing Marc-André] On Fri, Sep 06, 2019 at 04:25:19PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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[] = {
If 1) dbus-vmstate is required for migration of a domain using a slirp-helper and 2) we enable this automatically for existing XML configs with <interface type='user'/> if a) QEMU has QEMU_CAPS_NET_SOCKET_DGRAM b) the slirp-helper process is installed on the system then we've effectively made their machines unmigratable So if we want to enable it automatically, we should add: c) QEMU has QEMU_CAPS_DBUS_VMSTATE regardless of whether dbus-vmstate gets merged into QEMU until libvirt's next release Jano
On 9/6/19 5:55 PM, Ján Tomko wrote:
[cc-ing Marc-André]
On Fri, Sep 06, 2019 at 04:25:19PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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[] = {
If 1) dbus-vmstate is required for migration of a domain using a slirp-helper and 2) we enable this automatically for existing XML configs with <interface type='user'/> if a) QEMU has QEMU_CAPS_NET_SOCKET_DGRAM b) the slirp-helper process is installed on the system then we've effectively made their machines unmigratable
So if we want to enable it automatically, we should add: c) QEMU has QEMU_CAPS_DBUS_VMSTATE regardless of whether dbus-vmstate gets merged into QEMU until libvirt's next release
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
On Mon, Sep 09, 2019 at 11:23:59AM +0200, Michal Privoznik wrote:
On 9/6/19 5:55 PM, Ján Tomko wrote:
[cc-ing Marc-André]
On Fri, Sep 06, 2019 at 04:25:19PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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[] = {
If 1) dbus-vmstate is required for migration of a domain using a slirp-helper and 2) we enable this automatically for existing XML configs with <interface type='user'/> if a) QEMU has QEMU_CAPS_NET_SOCKET_DGRAM b) the slirp-helper process is installed on the system then we've effectively made their machines unmigratable
So if we want to enable it automatically, we should add: c) QEMU has QEMU_CAPS_DBUS_VMSTATE regardless of whether dbus-vmstate gets merged into QEMU until libvirt's next release
So are you saying that we should just temporarily revert QEMU_CAPS_NET_SOCKET_DGRAM detection instead and leave QEMU_CAPS_DBUS_VMSTATE be?
Give that there was a QEMU release which has one but not the other, I'd say permanently revert QEMU_CAPS_NET_SOCKET_DGRAM - even if dbus-vmstate gets merged later, there has been a QEMU release for which we assume one capability and not the other - so libvirt would be starting unmigratable machines. As for QEMU_CAPS_DBUS_VMSTATE, we can either revert it temporarily now or before the libvirt freeze if dbus-vmstate does not get merged by then. Jano
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi On Mon, Sep 9, 2019 at 2:49 PM Ján Tomko <jtomko@redhat.com> wrote:
On Mon, Sep 09, 2019 at 11:23:59AM +0200, Michal Privoznik wrote:
On 9/6/19 5:55 PM, Ján Tomko wrote:
[cc-ing Marc-André]
On Fri, Sep 06, 2019 at 04:25:19PM +0200, Michal Privoznik wrote:
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@redhat.com> --- 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[] = {
If 1) dbus-vmstate is required for migration of a domain using a slirp-helper and 2) we enable this automatically for existing XML configs with <interface type='user'/> if a) QEMU has QEMU_CAPS_NET_SOCKET_DGRAM b) the slirp-helper process is installed on the system then we've effectively made their machines unmigratable
So if we want to enable it automatically, we should add: c) QEMU has QEMU_CAPS_DBUS_VMSTATE regardless of whether dbus-vmstate gets merged into QEMU until libvirt's next release
So are you saying that we should just temporarily revert QEMU_CAPS_NET_SOCKET_DGRAM detection instead and leave QEMU_CAPS_DBUS_VMSTATE be?
Give that there was a QEMU release which has one but not the other, I'd say permanently revert QEMU_CAPS_NET_SOCKET_DGRAM - even if dbus-vmstate gets merged later, there has been a QEMU release for which we assume one capability and not the other - so libvirt would be starting unmigratable machines.
As for QEMU_CAPS_DBUS_VMSTATE, we can either revert it temporarily now or before the libvirt freeze if dbus-vmstate does not get merged by then.
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.
Jano
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Ján Tomko -
Marc-André Lureau -
Michal Privoznik