Fix misc locking bugs identified by lock checker

diff --git a/ChangeLog b/ChangeLog
index 9caa9fec2b368b686f11dced65a69fdbb73e5cd8..6545a016175d51e5f2c45c40a7406b941e9ec33f 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,23 @@
+Tue May 19 12:04:22 BST 2009 Daniel P. Berrange <berrange@redhat.com>
+
+       Fix misc locking bugs identified by lock checker
+       * src/test.c: Add missing driver lock calls in testOpen()
+       * src/uml_driver.c: Remove bogus driver unlock call in
+       umlDomainStart. Ensure driver lock is held for the duration
+       of umlDomainSetAutostart.
+       * src/network_driver.c: Ensure driver lock is held for the
+       duration of networkStart, networkDestroy and networkSetAutostart
+       * src/storage_driver.c: Ensure driver lock is held for the
+       duration of storagePoolRefresh, and storagePoolSetAutostart.
+       Ensure driver is locked before re-obtaining pool lock in
+       storageVolumeCreateXML.
+       * src/qemu_driver.c: Ensure lock is held when removing domain
+       event callbacks in qemudClose(). Drop driver lock before calling
+       qemudAutostartConfigs, since that will obtain a lock when calling
+       virConnectClose. Hold lock across duration of suspend, resume,
+       start, get security label, device attach and device detach
+       operations.
+
 Tue May 19 11:10:22 BST 2009 Daniel P. Berrange <berrange@redhat.com>
 
        Add an optional OCaml+CIL mutex lock checker

diff --git a/src/network_driver.c b/src/network_driver.c
index a163b15aa1d35ca5cd889e85d2f4631f67f9a94d..112346ffef9b66c3bbe230a29081ee0436127fcb 100644 (file)
--- a/src/network_driver.c
+++ b/src/network_driver.c
@@ -1217,7 +1217,6 @@ static int networkStart(virNetworkPtr net) {
 
     networkDriverLock(driver);
     network = virNetworkFindByUUID(&driver->networks, net->uuid);
-    networkDriverUnlock(driver);
 
     if (!network) {
         networkReportError(net->conn, NULL, net, VIR_ERR_INVALID_NETWORK,
@@ -1230,6 +1229,7 @@ static int networkStart(virNetworkPtr net) {
 cleanup:
     if (network)
         virNetworkObjUnlock(network);
+    networkDriverUnlock(driver);
     return ret;
 }
 
@@ -1240,7 +1240,6 @@ static int networkDestroy(virNetworkPtr net) {
 
     networkDriverLock(driver);
     network = virNetworkFindByUUID(&driver->networks, net->uuid);
-    networkDriverUnlock(driver);
 
     if (!network) {
         networkReportError(net->conn, NULL, net, VIR_ERR_INVALID_NETWORK,
@@ -1258,6 +1257,7 @@ static int networkDestroy(virNetworkPtr net) {
 cleanup:
     if (network)
         virNetworkObjUnlock(network);
+    networkDriverUnlock(driver);
     return ret;
 }
 
@@ -1349,7 +1349,6 @@ static int networkSetAutostart(virNetworkPtr net,
 
     networkDriverLock(driver);
     network = virNetworkFindByUUID(&driver->networks, net->uuid);
-    networkDriverUnlock(driver);
 
     if (!network) {
         networkReportError(net->conn, NULL, net, VIR_ERR_INVALID_NETWORK,
@@ -1399,6 +1398,7 @@ cleanup:
     VIR_FREE(autostartLink);
     if (network)
         virNetworkObjUnlock(network);
+    networkDriverUnlock(driver);
     return ret;
 }
 

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index 67b5e1c963759c3eaa785e849635d0b44191e534..4c5e8e5558ee574d0344264895adca07858a83e5 100644 (file)
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -215,6 +215,7 @@ qemudAutostartConfigs(struct qemud_driver *driver) {
                                         "qemu:///system");
     /* Ignoring NULL conn which is mostly harmless here */
 
+    qemuDriverLock(driver);
     for (i = 0 ; i < driver->domains.count ; i++) {
         virDomainObjPtr vm = driver->domains.objs[i];
         virDomainObjLock(vm);
@@ -237,6 +238,7 @@ qemudAutostartConfigs(struct qemud_driver *driver) {
         }
         virDomainObjUnlock(vm);
     }
+    qemuDriverUnlock(driver);
 
     if (conn)
         virConnectClose(conn);
@@ -519,9 +521,10 @@ qemudStartup(void) {
                                 NULL, NULL) < 0)
         goto error;
     qemudReconnectVMs(qemu_driver);
+    qemuDriverUnlock(qemu_driver);
+
     qemudAutostartConfigs(qemu_driver);
 
-    qemuDriverUnlock(qemu_driver);
 
     return 0;
 
@@ -567,9 +570,9 @@ qemudReload(void) {
                             qemu_driver->configDir,
                             qemu_driver->autostartDir,
                             qemudNotifyLoadDomain, qemu_driver);
+    qemuDriverUnlock(qemu_driver);
 
     qemudAutostartConfigs(qemu_driver);
-    qemuDriverUnlock(qemu_driver);
 
     return 0;
 }
@@ -1812,7 +1815,9 @@ static int qemudClose(virConnectPtr conn) {
     struct qemud_driver *driver = conn->privateData;
 
     /* Get rid of callbacks registered for this conn */
+    qemuDriverLock(driver);
     virDomainEventCallbackListRemoveConn(conn, driver->domainEventCallbacks);
+    qemuDriverUnlock(driver);
 
     conn->privateData = NULL;
 
@@ -2229,7 +2234,6 @@ static int qemudDomainSuspend(virDomainPtr dom) {
 
     qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2264,11 +2268,9 @@ cleanup:
     if (vm)
         virDomainObjUnlock(vm);
 
-    if (event) {
-        qemuDriverLock(driver);
+    if (event)
         qemuDomainEventQueue(driver, event);
-        qemuDriverUnlock(driver);
-    }
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -2282,7 +2284,6 @@ static int qemudDomainResume(virDomainPtr dom) {
 
     qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -2316,11 +2317,9 @@ static int qemudDomainResume(virDomainPtr dom) {
 cleanup:
     if (vm)
         virDomainObjUnlock(vm);
-    if (event) {
-        qemuDriverLock(driver);
+    if (event)
         qemuDomainEventQueue(driver, event);
-        qemuDriverUnlock(driver);
-    }
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -3153,7 +3152,6 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr sec
 
     qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -3199,6 +3197,7 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr sec
 cleanup:
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -3473,7 +3472,6 @@ static int qemudDomainStart(virDomainPtr dom) {
 
     qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -3492,11 +3490,9 @@ static int qemudDomainStart(virDomainPtr dom) {
 cleanup:
     if (vm)
         virDomainObjUnlock(vm);
-    if (event) {
-        qemuDriverLock(driver);
+    if (event)
         qemuDomainEventQueue(driver, event);
-        qemuDriverUnlock(driver);
-    }
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -3984,7 +3980,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
-        qemuDriverUnlock(driver);
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN,
                          _("no domain with matching uuid '%s'"), uuidstr);
@@ -3992,7 +3987,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
     }
 
     if (!virDomainIsActive(vm)) {
-        qemuDriverUnlock(driver);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID,
                          "%s", _("cannot attach device on inactive domain"));
         goto cleanup;
@@ -4000,7 +3994,6 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
 
     dev = virDomainDeviceDefParse(dom->conn, driver->caps, vm->def, xml,
                                   VIR_DOMAIN_XML_INACTIVE);
-    qemuDriverUnlock(driver);
     if (dev == NULL)
         goto cleanup;
 
@@ -4053,6 +4046,7 @@ cleanup:
         virDomainDeviceDefFree(dev);
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -4136,7 +4130,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
-        qemuDriverUnlock(driver);
         virUUIDFormat(dom->uuid, uuidstr);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_DOMAIN,
                          _("no domain with matching uuid '%s'"), uuidstr);
@@ -4144,7 +4137,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
     }
 
     if (!virDomainIsActive(vm)) {
-        qemuDriverUnlock(driver);
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_INVALID,
                          "%s", _("cannot detach device on inactive domain"));
         goto cleanup;
@@ -4152,7 +4144,6 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
 
     dev = virDomainDeviceDefParse(dom->conn, driver->caps, vm->def, xml,
                                   VIR_DOMAIN_XML_INACTIVE);
-    qemuDriverUnlock(driver);
     if (dev == NULL)
         goto cleanup;
 
@@ -4176,6 +4167,7 @@ cleanup:
     virDomainDeviceDefFree(dev);
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -4215,7 +4207,6 @@ static int qemudDomainSetAutostart(virDomainPtr dom,
 
     qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (!vm) {
         char uuidstr[VIR_UUID_STRING_BUFLEN];
@@ -4273,6 +4264,7 @@ cleanup:
     VIR_FREE(autostartLink);
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 

diff --git a/src/storage_driver.c b/src/storage_driver.c
index b72f0a0c3aa35b123ece559e60fb74dfc4f6c1ec..c2de6fa8b317dd24c3bb1b9c1524d533cabd8e62 100644 (file)
--- a/src/storage_driver.c
+++ b/src/storage_driver.c
@@ -792,7 +792,6 @@ storagePoolRefresh(virStoragePoolPtr obj,
 
     storageDriverLock(driver);
     pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
-    storageDriverUnlock(driver);
 
     if (!pool) {
         virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
@@ -834,6 +833,7 @@ storagePoolRefresh(virStoragePoolPtr obj,
 cleanup:
     if (pool)
         virStoragePoolObjUnlock(pool);
+    storageDriverUnlock(driver);
     return ret;
 }
 
@@ -939,7 +939,6 @@ storagePoolSetAutostart(virStoragePoolPtr obj,
 
     storageDriverLock(driver);
     pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
-    storageDriverUnlock(driver);
 
     if (!pool) {
         virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
@@ -988,6 +987,7 @@ storagePoolSetAutostart(virStoragePoolPtr obj,
 cleanup:
     if (pool)
         virStoragePoolObjUnlock(pool);
+    storageDriverUnlock(driver);
     return ret;
 }
 
@@ -1274,7 +1274,10 @@ storageVolumeCreateXML(virStoragePoolPtr obj,
 
         buildret = backend->buildVol(obj->conn, buildvoldef);
 
+        storageDriverLock(driver);
         virStoragePoolObjLock(pool);
+        storageDriverUnlock(driver);
+
         voldef->building = 0;
         pool->asyncjobs--;
 

diff --git a/src/test.c b/src/test.c
index f5ec6ec63714101eb609eea67c6ad6ae3912b979..d0e7ae83daa1ccec83188849d9e6fa6c0f23073f 100644 (file)
--- a/src/test.c
+++ b/src/test.c
@@ -659,10 +659,12 @@ static virDrvOpenStatus testOpen(virConnectPtr conn,
 
     if (ret == VIR_DRV_OPEN_SUCCESS) {
         testConnPtr privconn = conn->privateData;
+        testDriverLock(privconn);
         /* Init callback list */
         if (VIR_ALLOC(privconn->domainEventCallbacks) < 0 ||
             !(privconn->domainEventQueue = virDomainEventQueueNew())) {
             virReportOOMError(NULL);
+            testDriverUnlock(privconn);
             testClose(conn);
             return VIR_DRV_OPEN_ERROR;
         }
@@ -671,6 +673,7 @@ static virDrvOpenStatus testOpen(virConnectPtr conn,
              virEventAddTimeout(-1, testDomainEventFlush, privconn, NULL)) < 0)
             DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. "
                    "continuing without events.");
+        testDriverUnlock(privconn);
     }
 
     return (ret);

diff --git a/src/uml_driver.c b/src/uml_driver.c
index 0e2c4dde430a64ff336769cf37508f90f6f816cb..704ca4308da8b9fa1be2e00119948e9cbb83ce58 100644 (file)
--- a/src/uml_driver.c
+++ b/src/uml_driver.c
@@ -1553,7 +1553,6 @@ static int umlDomainStart(virDomainPtr dom) {
 
     umlDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    umlDriverUnlock(driver);
 
     if (!vm) {
         umlReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -1672,6 +1671,7 @@ static int umlDomainGetAutostart(virDomainPtr dom,
 cleanup:
     if (vm)
         virDomainObjUnlock(vm);
+    umlDriverUnlock(driver);
     return ret;
 }
 
@@ -1684,7 +1684,6 @@ static int umlDomainSetAutostart(virDomainPtr dom,
 
     umlDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    umlDriverUnlock(driver);
 
     if (!vm) {
         umlReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
@@ -1740,6 +1739,7 @@ cleanup:
     VIR_FREE(autostartLink);
     if (vm)
         virDomainObjUnlock(vm);
+    umlDriverUnlock(driver);
     return ret;
 }