From de658ab4e406304ed2f475d8937afdc2eaab21ad Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Tue, 19 May 2009 11:06:25 +0000 Subject: [PATCH] Fix misc locking bugs identified by lock checker --- ChangeLog | 20 ++++++++++++++++++++ src/network_driver.c | 6 +++--- src/qemu_driver.c | 42 +++++++++++++++++------------------------- src/storage_driver.c | 7 +++++-- src/test.c | 3 +++ src/uml_driver.c | 4 ++-- 6 files changed, 50 insertions(+), 32 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9caa9fec2b..6545a01617 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,23 @@ +Tue May 19 12:04:22 BST 2009 Daniel P. Berrange + + 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 Add an optional OCaml+CIL mutex lock checker diff --git a/src/network_driver.c b/src/network_driver.c index a163b15aa1..112346ffef 100644 --- 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 67b5e1c963..4c5e8e5558 100644 --- 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 b72f0a0c3a..c2de6fa8b3 100644 --- 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 f5ec6ec637..d0e7ae83da 100644 --- 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 0e2c4dde43..704ca4308d 100644 --- 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; }