Fix misc locking bugs identified by lock checker

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

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;
 }
 

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) {
+    if (event)
-        qemuDriverLock(driver);
         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) {
+    if (event)
-        qemuDriverLock(driver);
         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) {
+    if (event)
-        qemuDriverLock(driver);
         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;
 }
 

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--;
 

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);

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;
 }