Re: [libvirt] PATCH: Fix misc locking bugs, and add lock checker

This patch fixes a number of locking bugs, some serious, some not. It also adds the CIL lock checker I previously wrote. It is improved since last time, because it explicitly looks for the virDriver static variables and uses that to extract list of functions to be checked. This removes a huge number of false positives. It also now checks for a couple of dead lock scenarios, in addition to previous checks for lock ordering correctness. The serious locking bugs - qemudDomainMigratePrepare2: didn't unlock VM causing deadlock - storagePoolRefresh: unlocked the driver too early - storageVolumeCreateXML: locked the pool without having locked driver - umlDomainGetAutostart: missing unlock call with later deadlock

--- a/src/network_driver.c Thu May 14 17:38:11 2009 +0100 +++ b/src/network_driver.c Thu May 14 17:47:01 2009 +0100 @@ -1217,7 +1217,6 @@ static int networkStart(virNetworkPtr ne 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 ne cleanup: if (network) virNetworkObjUnlock(network); + networkDriverUnlock(driver); return ret; }

@@ -1240,7 +1240,6 @@ static int networkDestroy(virNetworkPtr @@ -1349,7 +1349,6 @@ static int networkSetAutostart(virNetwor @@ -2229,7 +2230,6 @@ static int qemudDomainSuspend(virDomainP @@ -2282,7 +2280,6 @@ static int qemudDomainResume(virDomainPt @@ -3153,7 +3148,6 @@ static int qemudDomainGetSecurityLabel(v @@ -3617,7 +3612,6 @@ static int qemudDomainStart(virDomainPtr @@ -4144,7 +4136,6 @@ static int qemudDomainAttachDevice(virDo @@ -4296,7 +4288,6 @@ static int qemudDomainDetachDevice(virDo @@ -4359,7 +4351,6 @@ static int qemudDomainSetAutostart(virDo @@ -792,7 +792,6 @@ storagePoolRefresh(virStoragePoolPtr obj @@ -939,7 +939,6 @@ storagePoolSetAutostart(virStoragePoolPt @@ -1553,7 +1553,6 @@ static int umlDomainStart(virDomainPtr d @@ -1684,7 +1684,6 @@ static int umlDomainSetAutostart(virDoma

diff -r 71a7d1d0ad9b src/qemu_driver.c --- a/src/qemu_driver.c Thu May 14 17:38:11 2009 +0100 +++ b/src/qemu_driver.c Thu May 14 17:47:01 2009 +0100 @@ -1811,9 +1811,10 @@ static virDrvOpenStatus qemudOpen(virCon static int qemudClose(virConnectPtr conn) { struct qemud_driver *driver = conn->privateData; + qemuDriverLock(driver); /* Get rid of callbacks registered for this conn */ virDomainEventCallbackListRemoveConn(conn, driver->domainEventCallbacks); - + qemuDriverUnlock(driver); conn->privateData = NULL;

return 0; @@ -4985,6 +4977,7 @@ qemudDomainMigratePrepare2 (virConnectPt vm->def->name); goto cleanup; } + virDomainObjUnlock(vm); }

if (!(vm = virDomainAssignDef(dconn, diff -r 71a7d1d0ad9b src/storage_driver.c --- a/src/storage_driver.c Thu May 14 17:38:11 2009 +0100 +++ b/src/storage_driver.c Thu May 14 17:47:01 2009 +0100 @@ -1274,7 +1274,10 @@ storageVolumeCreateXML(virStoragePoolPtr buildret = backend->buildVol(obj->conn, buildvoldef); + storageDriverLock(driver); virStoragePoolObjLock(pool); + storageDriverUnlock(driver); + voldef->building = 0; pool->asyncjobs--;

diff -r 71a7d1d0ad9b src/test.c --- a/src/test.c Thu May 14 17:38:11 2009 +0100 +++ b/src/test.c Thu May 14 17:47:01 2009 +0100 @@ -659,10 +659,12 @@ static virDrvOpenStatus testOpen(virConn 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(virConn virEventAddTimeout(-1, testDomainEventFlush, privconn, NULL)) < 0) DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. " "continuing without events."); + testDriverUnlock(privconn); }

return (ret);