[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] PATCH: 0/28: Thread safety for libvirtd daemon and drivers



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(&uml_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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]