On Thu, May 14, 2009 at 10:19:46PM +0200, Daniel Veillard wrote:
On Thu, May 14, 2009 at 05:53:41PM +0100, Daniel P. Berrange wrote:
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; }
A lot of the changes which are not in that list look like the above, instead of unlocking immediately after finding the network structure, the unlocking is postponed until the end of the operation. What's the reasonning ? Is that a safety against possible changes in the functions i.e. just a preventive change or is there something we expected to be safe which turns out not ?
It is
@@ -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
they all share the pattern of changes above
The QEMU ones are all similar - the virDomainSaveStatus method is taking a 'driver' parameter, meaning we need to keep the lock on 'driver' held for longer. I don't really like this, and think it would be better to remove the driver parameter from this method. We're holding coarse locks for far too long already.
but a hint about the systematic changes about extended locking would be good :-)
Here is an example of what the test reports as error, without the fixes applied. It should help understand the changes I made ================================================================ Function: umlDomainStart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 1564 "uml_driver.c" ret = umlStartVMDaemon(dom->conn, driver___0, vm); ================================================================ ================================================================ Function: umlDomainGetAutostart ---------------------------------------------------------------- - Total exit points with locked vars: 1 - At exit on #line 1675 return (ret); ^^^^^^^^^ variables still locked are | struct uml_driver * driver___0 - Total blocks with lock ordering mistakes: 0 ================================================================ ================================================================ Function: umlDomainSetAutostart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 4 - Driver used while unlocked on #line 1704 configFile = virDomainConfigFile(dom->conn, (char const *)driver___0->configDir, (char const *)(vm->def)->name); - Driver used while unlocked on #line 1706 autostartLink = virDomainConfigFile(dom->conn, (char const *)driver___0->autostartDir, (char const *)(vm->def)->name); - Driver used while unlocked on #line 1712 err = virFileMakePath((char const *)driver___0->autostartDir); - Driver used while unlocked on #line 1713 virReportSystemErrorFull(dom->conn, 21, err, "uml_driver.c", "umlDomainSetAutostart", 1715U, (char const *)tmp___1, driver___0->autostartDir); ================================================================ ================================================================ Function: testOpen ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 3 - Driver used while unlocked on #line 663 "test.c" tmp___20 = virAlloc((void *)(& privconn->domainEventCallbacks), sizeof(*(privconn->domainEventCallbacks))); - Driver used while unlocked on #line 663 privconn->domainEventQueue = virDomainEventQueueNew(); - Driver used while unlocked on #line 670 privconn->domainEventTimer = virEventAddTimeout(-1, & testDomainEventFlush, (void *)privconn, (void (*)(void *opaque ))((void *)0)); ================================================================ ================================================================ Function: qemudClose ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 1815 "qemu_driver.c" virDomainEventCallbackListRemoveConn(conn, driver___0->domainEventCallbacks); ================================================================ ================================================================ Function: qemudDomainSuspend ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 2259 tmp___4 = qemudSaveDomainStatus(dom->conn, driver___0, vm); ================================================================ ================================================================ Function: qemudDomainResume ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 2312 tmp___4 = qemudSaveDomainStatus(dom->conn, driver___0, vm); ================================================================ ================================================================ Function: qemudDomainGetSecurityLabel ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 3189 tmp___2 = (*((driver___0->securityDriver)->domainGetSecurityLabel))(dom->conn, vm, seclabel); ================================================================ ================================================================ Function: qemudDomainStart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 3630 ret = qemudStartVMDaemon(dom->conn, driver___0, vm, (char const *)((void *)0), -1); ================================================================ ================================================================ Function: qemudDomainAttachDevice ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 4192 tmp___8 = qemudSaveDomainStatus(dom->conn, driver___0, vm); ================================================================ ================================================================ Function: qemudDomainDetachDevice ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 4316 tmp___3 = qemudSaveDomainStatus(dom->conn, driver___0, vm); ================================================================ ================================================================ Function: qemudDomainSetAutostart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 4 - Driver used while unlocked on #line 4381 configFile = virDomainConfigFile(dom->conn, (char const *)driver___0->configDir, (char const *)(vm->def)->name); - Driver used while unlocked on #line 4383 autostartLink = virDomainConfigFile(dom->conn, (char const *)driver___0->autostartDir, (char const *)(vm->def)->name); - Driver used while unlocked on #line 4389 err = virFileMakePath((char const *)driver___0->autostartDir); - Driver used while unlocked on #line 4390 virReportSystemErrorFull(dom->conn, 10, err, "qemu_driver.c", "qemudDomainSetAutostart", 4392U, (char const *)tmp___1, driver___0->autostartDir); ================================================================ ================================================================ Function: qemudDomainMigratePrepare2 ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Object fetched while locked objects exist #line 4990 vm = virDomainAssignDef(dconn, & driver___0->domains, def); ================================================================ ================================================================ Function: storagePoolRefresh ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 827 "storage_driver.c" virStoragePoolObjRemove(& driver___0->pools, pool); ================================================================ ================================================================ Function: storagePoolSetAutostart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 2 - Driver used while unlocked on #line 962 err = virFileMakePath((char const *)driver___0->autostartDir); - Driver used while unlocked on #line 963 virReportSystemErrorFull(obj->conn, 18, err, "storage_driver.c", "storagePoolSetAutostart", 965U, (char const *)tmp___1, driver___0->autostartDir); ================================================================ ================================================================ Function: storageVolumeCreateXML ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Object locked while driver is unlocked on #line 1277 virStoragePoolObjLock(pool); ================================================================ ================================================================ Function: networkStart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 1228 "network_driver.c" ret = networkStartNetworkDaemon(net->conn, driver___0, network); ================================================================ ================================================================ Function: networkDestroy ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 1 - Driver used while unlocked on #line 1251 ret = networkShutdownNetworkDaemon(net->conn, driver___0, network); ================================================================ ================================================================ Function: networkSetAutostart ---------------------------------------------------------------- - Total exit points with locked vars: 0 - Total blocks with lock ordering mistakes: 4 - Driver used while unlocked on #line 1363 configFile = virNetworkConfigFile(net->conn, (char const *)driver___0->networkConfigDir, (char const *)(network->def)->name); - Driver used while unlocked on #line 1365 autostartLink = virNetworkConfigFile(net->conn, (char const *)driver___0->networkAutostartDir, (char const *)(network->def)->name); - Driver used while unlocked on #line 1371 err = virFileMakePath((char const *)driver___0->networkAutostartDir); - Driver used while unlocked on #line 1372 virReportSystemErrorFull(net->conn, 19, *tmp___1, "network_driver.c", "networkSetAutostart", 1374U, (char const *)tmp___0, driver___0->networkAutostartDir); ================================================================ Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|