[PATCH] qemu: Unlock NW filter lock earlier when not necessary

[Erik Skultety <eskultet@xxxxxxxxxx>]

The lock is dropped always at the end of an API, but for example when
attaching devices, there's no point in having the NW filter locked if the
device being attached isn't a network interface. It's always a nice
practice to drop unnecessary locks as soon as possible.
---
 src/qemu/qemu_driver.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

BZ 1181074 reported the daemon to hang (not completely true btw) after trying
to attach /dev/ttyX to a running domain. When going through the code I realized
we acquire a read lock for NW filter, although the device itself is not a network
interface and then releasing it at the end. First clue was that this lock which
won't be unlocked as the thread is blocked in reading header of /dev/ttyX causes
the freeze. As it turned out, the problem resides in calling virsh list afterwards
(Peter already does have some patches prepared), but I think the NW filter might
be released earlier in this case with no harm done.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 74dcb0a..04887e0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8764,6 +8764,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
     virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
     int ret = -1;
     unsigned int affect, parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+    bool nwfilter_locked = false;
     virQEMUCapsPtr qemuCaps = NULL;
     qemuDomainObjPrivatePtr priv;
     virQEMUDriverConfigPtr cfg = NULL;
@@ -8773,6 +8774,7 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
                   VIR_DOMAIN_AFFECT_CONFIG, -1);
 
     virNWFilterReadLockFilterUpdates();
+    nwfilter_locked = true;
 
     cfg = virQEMUDriverGetConfig(driver);
 
@@ -8819,6 +8821,11 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
     if (dev == NULL)
         goto endjob;
 
+    if (dev->type != VIR_DOMAIN_DEVICE_NET) {
+        virNWFilterUnlockFilterUpdates();
+        nwfilter_locked = false;
+    }
+
     if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
         flags & VIR_DOMAIN_AFFECT_LIVE) {
         /* If we are affecting both CONFIG and LIVE
@@ -8884,11 +8891,12 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
     virDomainDefFree(vmdef);
     if (dev != dev_copy)
         virDomainDeviceDefFree(dev_copy);
+    if (nwfilter_locked)
+        virNWFilterUnlockFilterUpdates();
     virDomainDeviceDefFree(dev);
     virDomainObjEndAPI(&vm);
     virObjectUnref(caps);
     virObjectUnref(cfg);
-    virNWFilterUnlockFilterUpdates();
     return ret;
 }
 
@@ -8908,6 +8916,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
     virDomainDefPtr vmdef = NULL;
     virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
     bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0;
+    bool nwfilter_locked = false;
     int ret = -1;
     unsigned int affect;
     virQEMUCapsPtr qemuCaps = NULL;
@@ -8920,6 +8929,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
                   VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
 
     virNWFilterReadLockFilterUpdates();
+    nwfilter_locked = true;
 
     cfg = virQEMUDriverGetConfig(driver);
 
@@ -8966,6 +8976,11 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
     if (dev == NULL)
         goto endjob;
 
+    if (dev->type != VIR_DOMAIN_DEVICE_NET) {
+        virNWFilterUnlockFilterUpdates();
+        nwfilter_locked = false;
+    }
+
     if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
         flags & VIR_DOMAIN_AFFECT_LIVE) {
         /* If we are affecting both CONFIG and LIVE
@@ -9031,11 +9046,12 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
     virDomainDefFree(vmdef);
     if (dev != dev_copy)
         virDomainDeviceDefFree(dev_copy);
+    if (nwfilter_locked)
+        virNWFilterUnlockFilterUpdates();
     virDomainDeviceDefFree(dev);
     virDomainObjEndAPI(&vm);
     virObjectUnref(caps);
     virObjectUnref(cfg);
-    virNWFilterUnlockFilterUpdates();
     return ret;
 }
 
-- 
1.9.3

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list