On Wed, Dec 03, 2008 at 06:52:32PM +0000, Daniel P. Berrange wrote:
On Wed, Dec 03, 2008 at 04:18:21PM +0100, Daniel Veillard wrote:
On Mon, Dec 01, 2008 at 12:26:48AM +0000, Daniel P. Berrange wrote:
This is a diffstat summary for the combined series of 28 patches
Okay, my take at this point is that those should be commited with the few fix found my manual examination, maybe extend the documentation a bit, and start testing it as much as prossible. Some locking debug facility might be a good addition,
I wrote some an OCaml program using CIL to check driver method exit paths and validate that all objects were left in an unlocked state. This found some real bugs !
So here's the incremental fixes for those
I was having so much fun I wrote more checks for incorrect locking order and data access while unlocked, and improved existing detection. A bunch more bugs found and fixed ! lxc_driver.c | 23 +++++++++++++++++++---- network_driver.c | 3 --- qemu_driver.c | 28 +++++++++++++++++----------- storage_driver.c | 10 ++++++++-- test.c | 3 +-- uml_driver.c | 18 ++++++++++++++---- 6 files changed, 59 insertions(+), 26 deletions(-) Daniel diff --git a/src/lxc_driver.c b/src/lxc_driver.c --- a/src/lxc_driver.c +++ b/src/lxc_driver.c @@ -682,24 +682,32 @@ static void lxcMonitorEvent(int watch, virDomainObjPtr vm = NULL; unsigned int i; + lxcDriverLock(driver); for (i = 0 ; i < driver->domains.count ; i++) { + virDomainObjLock(driver->domains.objs[i]); if (driver->domains.objs[i]->monitorWatch == watch) { vm = driver->domains.objs[i]; break; } + virDomainObjUnlock(driver->domains.objs[i]); } if (!vm) { virEventRemoveHandle(watch); - return; + goto cleanup; } if (vm->monitor != fd) { virEventRemoveHandle(watch); - return; + goto cleanup; } if (lxcVmTerminate(NULL, driver, vm, SIGINT) < 0) virEventRemoveHandle(watch); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + lxcDriverUnlock(driver); } @@ -1153,20 +1161,26 @@ static int lxcStartup(void) char *config = NULL; virDomainDefPtr tmp; int rc; - if ((vm->monitor = lxcMonitorClient(NULL, lxc_driver, vm)) < 0) + virDomainObjLock(vm); + if ((vm->monitor = lxcMonitorClient(NULL, lxc_driver, vm)) < 0) { + virDomainObjUnlock(vm); continue; + } /* Read pid from controller */ if ((rc = virFileReadPid(lxc_driver->stateDir, vm->def->name, &vm->pid)) != 0) { close(vm->monitor); vm->monitor = -1; + virDomainObjUnlock(vm); continue; } if ((config = virDomainConfigFile(NULL, lxc_driver->stateDir, - vm->def->name)) == NULL) + vm->def->name)) == NULL) { + virDomainObjUnlock(vm); continue; + } /* Try and load the live config */ tmp = virDomainDefParseFile(NULL, lxc_driver->caps, config); @@ -1184,6 +1198,7 @@ static int lxcStartup(void) close(vm->monitor); vm->monitor = -1; } + virDomainObjUnlock(vm); } lxcDriverUnlock(lxc_driver); diff --git a/src/network_driver.c b/src/network_driver.c --- a/src/network_driver.c +++ b/src/network_driver.c @@ -1005,7 +1005,6 @@ static virNetworkPtr networkCreate(virCo def = NULL; if (networkStartNetworkDaemon(conn, driver, network) < 0) { - virNetworkObjUnlock(network); virNetworkRemoveInactive(&driver->networks, network); network = NULL; @@ -1043,7 +1042,6 @@ static virNetworkPtr networkDefine(virCo driver->networkConfigDir, driver->networkAutostartDir, network) < 0) { - virNetworkObjUnlock(network); virNetworkRemoveInactive(&driver->networks, network); network = NULL; @@ -1083,7 +1081,6 @@ static int networkUndefine(virNetworkPtr if (virNetworkDeleteConfig(net->conn, network) < 0) goto cleanup; - virNetworkObjUnlock(network); virNetworkRemoveInactive(&driver->networks, network); network = NULL; diff --git a/src/qemu_driver.c b/src/qemu_driver.c --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -243,8 +243,7 @@ qemudStartup(void) { goto out_of_memory; if (qemudLoadDriverConfig(qemu_driver, driverConf) < 0) { - qemudShutdown(); - return -1; + goto error; } if (virDomainLoadAllConfigs(NULL, @@ -327,9 +326,13 @@ qemudActive(void) { return 0; qemuDriverLock(qemu_driver); - for (i = 0 ; i < qemu_driver->domains.count ; i++) - if (virDomainIsActive(qemu_driver->domains.objs[i])) + for (i = 0 ; i < qemu_driver->domains.count ; i++) { + virDomainObjPtr vm = qemu_driver->domains.objs[i]; + virDomainObjLock(vm); + if (virDomainIsActive(vm)) active = 1; + virDomainObjUnlock(vm); + } qemuDriverUnlock(qemu_driver); return active; @@ -1088,12 +1091,15 @@ qemudDispatchVMEvent(int watch, int fd, qemuDriverLock(driver); for (i = 0 ; i < driver->domains.count ; i++) { - if (virDomainIsActive(driver->domains.objs[i]) && - (driver->domains.objs[i]->stdout_watch == watch || - driver->domains.objs[i]->stderr_watch == watch)) { - vm = driver->domains.objs[i]; - break; - } + virDomainObjPtr tmpvm = driver->domains.objs[i]; + virDomainObjLock(vm); + if (virDomainIsActive(tmpvm) && + (tmpvm->stdout_watch == watch || + tmpvm->stderr_watch == watch)) { + vm = tmpvm; + break; + } + virDomainObjUnlock(vm); } if (!vm) @@ -2128,7 +2134,7 @@ static int qemudDomainSave(virDomainPtr if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED, "%s", _("failed to write save header")); - return -1; + goto cleanup; } if (safewrite(fd, xml, header.xml_len) != header.xml_len) { diff --git a/src/storage_driver.c b/src/storage_driver.c --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -62,13 +62,14 @@ storageDriverAutostart(virStorageDriverS for (i = 0 ; i < driver->pools.count ; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; - + virStoragePoolObjLock(pool); if (pool->autostart && !virStoragePoolObjIsActive(pool)) { virStorageBackendPtr backend; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { storageLog("Missing backend %d", pool->def->type); + virStoragePoolObjUnlock(pool); continue; } @@ -77,6 +78,7 @@ storageDriverAutostart(virStorageDriverS virErrorPtr err = virGetLastError(); storageLog("Failed to autostart storage pool '%s': %s", pool->def->name, err ? err->message : NULL); + virStoragePoolObjUnlock(pool); continue; } @@ -86,10 +88,12 @@ storageDriverAutostart(virStorageDriverS backend->stopPool(NULL, pool); storageLog("Failed to autostart storage pool '%s': %s", pool->def->name, err ? err->message : NULL); + virStoragePoolObjUnlock(pool); continue; } pool->active = 1; } + virStoragePoolObjUnlock(pool); } } @@ -237,11 +241,12 @@ storageDriverShutdown(void) { /* shutdown active pools */ for (i = 0 ; i < driverState->pools.count ; i++) { virStoragePoolObjPtr pool = driverState->pools.objs[i]; - + virStoragePoolObjLock(pool); if (virStoragePoolObjIsActive(pool)) { virStorageBackendPtr backend; if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { storageLog("Missing backend"); + virStoragePoolObjUnlock(pool); continue; } @@ -253,6 +258,7 @@ storageDriverShutdown(void) { } virStoragePoolObjClearVols(pool); } + virStoragePoolObjUnlock(pool); } /* free inactive pools */ diff --git a/src/test.c b/src/test.c --- a/src/test.c +++ b/src/test.c @@ -1592,7 +1592,6 @@ static int testDomainUndefine(virDomainP } privdom->state = VIR_DOMAIN_SHUTOFF; - virDomainObjUnlock(privdom); virDomainRemoveInactive(&privconn->domains, privdom); privdom = NULL; @@ -2393,7 +2392,7 @@ testStoragePoolCreate(virConnectPtr conn } if (!(pool = virStoragePoolObjAssignDef(conn, &privconn->pools, def))) { - return NULL; + goto cleanup; } def = NULL; diff --git a/src/uml_driver.c b/src/uml_driver.c --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -215,28 +215,29 @@ umlInotifyEvent(int watch, struct uml_driver *driver = data; virDomainObjPtr dom; + umlDriverLock(driver); if (watch != driver->inotifyWatch) - return; + goto cleanup; reread: got = read(fd, buf, sizeof(buf)); if (got == -1) { if (errno == EINTR) goto reread; - return; + goto cleanup; } tmp = buf; while (got) { if (got < sizeof(struct inotify_event)) - return; /* bad */ + goto cleanup; /* bad */ e = (struct inotify_event *)tmp; tmp += sizeof(struct inotify_event); got -= sizeof(struct inotify_event); if (got < e->len) - return; + goto cleanup; tmp += e->len; got -= e->len; @@ -251,6 +252,7 @@ reread: if (e->mask & IN_DELETE) { if (!virDomainIsActive(dom)) { + virDomainObjUnlock(dom); continue; } @@ -263,10 +265,12 @@ reread: dom->state = VIR_DOMAIN_SHUTOFF; } else if (e->mask & (IN_CREATE | IN_MODIFY)) { if (virDomainIsActive(dom)) { + virDomainObjUnlock(dom); continue; } if (umlReadPidFile(NULL, driver, dom) < 0) { + virDomainObjUnlock(dom); continue; } @@ -279,7 +283,11 @@ reread: if (umlIdentifyChrPTY(NULL, driver, dom) < 0) umlShutdownVMDaemon(NULL, driver, dom); } + virDomainObjUnlock(dom); } + +cleanup: + umlDriverUnlock(driver); } /** @@ -465,8 +473,10 @@ umlShutdown(void) { /* shutdown active VMs */ for (i = 0 ; i < uml_driver->domains.count ; i++) { virDomainObjPtr dom = uml_driver->domains.objs[i]; + virDomainObjLock(dom); if (virDomainIsActive(dom)) umlShutdownVMDaemon(NULL, uml_driver, dom); + virDomainObjUnlock(dom); } virDomainObjListFree(¨_driver->domains); -- |: 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 :|