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 :| -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list