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