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

[libvirt] [PATCH] Fix misc thread locking bugs / bogus warnings



Fix all thread locking bugs reported by object-locking test
case.

NB, some of the driver locking is getting too coarse. Driver
mutexes really need to be turned into RW locks instead to
significantly increase concurrency.

* src/lxc_driver.c: Fix useof driver when unlocked in the methods
  lxcDomainGetInfo, lxcSetSchedulerParameters, and
  lxcGetSchedulerParameters
* src/opennebula/one_driver.c: Fix missing unlock in oneDomainUndefine.
  Fix use of driver when unlocked in oneDomainGetInfo,
  oneGetOSType, oneDomainShutdown
* src/qemu_driver.c: Fix use of driver when unlocked in
  qemudDomainSavem, qemuGetSchedulerType, qemuSetSchedulerParameters
  and qemuGetSchedulerParameters
* src/storage_driver.c: Re-work storagePoolCreate to avoid bogus
  lock checking warning. Re-work storageVolumeCreateXMLFrom to
  remove a potential NULL de-reference & avoid bogus lock check
  warnings
* src/test.c: Remove testDomainAssignDef since it break lock chekc
  warnings.
* tests/object-locking.ml: Add oneDriverLock, oneDriverUnlock
  and one_driver_t methods/types to allow lock checking on the
   OpenNebula drivers
---
 src/lxc_driver.c            |    6 ++--
 src/opennebula/one_driver.c |   35 +++++++++++++++---------
 src/qemu_driver.c           |   60 +++++++++++++++++++++++-------------------
 src/storage_driver.c        |   35 ++++++++++++-------------
 src/test.c                  |   55 ++++++++++++++++++++-------------------
 tests/object-locking.ml     |    7 +++-
 6 files changed, 108 insertions(+), 90 deletions(-)

diff --git a/src/lxc_driver.c b/src/lxc_driver.c
index bd0cf0e..0ec1e92 100644
--- a/src/lxc_driver.c
+++ b/src/lxc_driver.c
@@ -439,7 +439,6 @@ static int lxcDomainGetInfo(virDomainPtr dom,
 
     lxcDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    lxcDriverUnlock(driver);
 
     if (!vm) {
         lxcError(dom->conn, dom, VIR_ERR_INVALID_DOMAIN,
@@ -470,6 +469,7 @@ static int lxcDomainGetInfo(virDomainPtr dom,
     ret = 0;
 
 cleanup:
+    lxcDriverUnlock(driver);
     if (cgroup)
         virCgroupFree(&cgroup);
     if (vm)
@@ -1667,7 +1667,6 @@ static int lxcSetSchedulerParameters(virDomainPtr domain,
 
     lxcDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, domain->uuid);
-    lxcDriverUnlock(driver);
 
     if (vm == NULL) {
         lxcError(NULL, domain, VIR_ERR_INTERNAL_ERROR,
@@ -1698,6 +1697,7 @@ static int lxcSetSchedulerParameters(virDomainPtr domain,
     ret = 0;
 
 cleanup:
+    lxcDriverUnlock(driver);
     virCgroupFree(&group);
     if (vm)
         virDomainObjUnlock(vm);
@@ -1725,7 +1725,6 @@ static int lxcGetSchedulerParameters(virDomainPtr domain,
 
     lxcDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, domain->uuid);
-    lxcDriverUnlock(driver);
 
     if (vm == NULL) {
         lxcError(NULL, domain, VIR_ERR_INTERNAL_ERROR,
@@ -1745,6 +1744,7 @@ static int lxcGetSchedulerParameters(virDomainPtr domain,
     ret = 0;
 
 cleanup:
+    lxcDriverUnlock(driver);
     virCgroupFree(&group);
     if (vm)
         virDomainObjUnlock(vm);
diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c
index 135397c..9587a2d 100644
--- a/src/opennebula/one_driver.c
+++ b/src/opennebula/one_driver.c
@@ -310,6 +310,8 @@ static int oneDomainUndefine(virDomainPtr dom)
     ret=0;
 
 return_point:
+    if (vm)
+        virDomainObjUnlock(vm);
     oneDriverUnlock(driver);
     return ret;
 }
@@ -392,9 +394,9 @@ static int oneDomainGetInfo(virDomainPtr dom,
 
 static char *oneGetOSType(virDomainPtr dom)
 {
-
     one_driver_t *driver = (one_driver_t *)dom->conn->privateData;
     virDomainObjPtr vm = NULL;
+    char *ret = NULL;
 
     oneDriverLock(driver);
     vm =virDomainFindByUUID(&driver->domains, dom->uuid);
@@ -402,11 +404,17 @@ static char *oneGetOSType(virDomainPtr dom)
     if (!vm) {
         oneError(dom->conn,dom, VIR_ERR_INVALID_DOMAIN,
                  "%s", _("no domain with matching uuid"));
-        return NULL;
+        goto cleanup;
     }
 
-    virDomainObjUnlock(vm);
-    return strdup(vm->def->os.type);
+    ret = strdup(vm->def->os.type);
+    if (!ret)
+        virReportOOMError(dom->conn);
+
+cleanup:
+    if (vm)
+        virDomainObjUnlock(vm);
+    return ret;
 }
 
 static int oneDomainStart(virDomainPtr dom)
@@ -499,24 +507,25 @@ static int oneDomainShutdown(virDomainPtr dom)
     int ret=-1;
 
     oneDriverLock(driver);
-    if((vm=virDomainFindByID(&driver->domains, dom->id))) {
-        if(!(c_oneShutdown(vm->pid) ) ) {
-            vm->state=VIR_DOMAIN_SHUTDOWN;
-            ret= 0;
-            goto return_point;
-        }
+    if (!(vm=virDomainFindByID(&driver->domains, dom->id))) {
+        oneError(dom->conn,dom, VIR_ERR_INVALID_DOMAIN,
+                 _("no domain with id %d"), dom->id);
+        goto return_point;
+    }
+
+    if (c_oneShutdown(vm->pid)) {
         oneError(dom->conn, dom, VIR_ERR_OPERATION_INVALID,
                  _("Wrong state to perform action"));
         goto return_point;
     }
-    oneError(dom->conn,dom, VIR_ERR_INVALID_DOMAIN,
-             _("no domain with id %d"), dom->id);
-    goto return_point;
+    vm->state=VIR_DOMAIN_SHUTDOWN;
+    ret= 0;
 
     if (!vm->persistent) {
         virDomainRemoveInactive(&driver->domains, vm);
         vm = NULL;
     }
+
 return_point:
     if(vm)
         virDomainObjUnlock(vm);
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index d45d33a..b4cd63d 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -3575,24 +3575,6 @@ static int qemudDomainSave(virDomainPtr dom,
     memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
     header.version = QEMUD_SAVE_VERSION;
 
-    if (driver->saveImageFormat == NULL)
-        header.compressed = QEMUD_SAVE_FORMAT_RAW;
-    else if (STREQ(driver->saveImageFormat, "raw"))
-        header.compressed = QEMUD_SAVE_FORMAT_RAW;
-    else if (STREQ(driver->saveImageFormat, "gzip"))
-        header.compressed = QEMUD_SAVE_FORMAT_GZIP;
-    else if (STREQ(driver->saveImageFormat, "bzip2"))
-        header.compressed = QEMUD_SAVE_FORMAT_BZIP2;
-    else if (STREQ(driver->saveImageFormat, "lzma"))
-        header.compressed = QEMUD_SAVE_FORMAT_LZMA;
-    else if (STREQ(driver->saveImageFormat, "lzop"))
-        header.compressed = QEMUD_SAVE_FORMAT_LZOP;
-    else {
-        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
-                         "%s", _("Invalid save image format specified in configuration file"));
-        return -1;
-    }
-
     qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
 
@@ -3632,6 +3614,24 @@ static int qemudDomainSave(virDomainPtr dom,
     }
     header.xml_len = strlen(xml) + 1;
 
+    if (driver->saveImageFormat == NULL)
+        header.compressed = QEMUD_SAVE_FORMAT_RAW;
+    else if (STREQ(driver->saveImageFormat, "raw"))
+        header.compressed = QEMUD_SAVE_FORMAT_RAW;
+    else if (STREQ(driver->saveImageFormat, "gzip"))
+        header.compressed = QEMUD_SAVE_FORMAT_GZIP;
+    else if (STREQ(driver->saveImageFormat, "bzip2"))
+        header.compressed = QEMUD_SAVE_FORMAT_BZIP2;
+    else if (STREQ(driver->saveImageFormat, "lzma"))
+        header.compressed = QEMUD_SAVE_FORMAT_LZMA;
+    else if (STREQ(driver->saveImageFormat, "lzop"))
+        header.compressed = QEMUD_SAVE_FORMAT_LZOP;
+    else {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                         "%s", _("Invalid save image format specified in configuration file"));
+        goto cleanup;
+    }
+
     /* Write header to file, followed by XML */
     if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
@@ -4429,7 +4429,9 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn,
         goto cleanup;
     }
 
+    qemuDriverLock(driver);
     def = qemuParseCommandLineString(conn, driver->caps, config);
+    qemuDriverUnlock(driver);
     if (!def)
         goto cleanup;
 
@@ -6183,12 +6185,13 @@ static char *qemuGetSchedulerType(virDomainPtr dom,
                                   int *nparams)
 {
     struct qemud_driver *driver = dom->conn->privateData;
-    char *ret;
+    char *ret = NULL;
 
+    qemuDriverLock(driver);
     if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                          __FUNCTION__);
-        return NULL;
+        goto cleanup;
     }
 
     if (nparams)
@@ -6197,6 +6200,9 @@ static char *qemuGetSchedulerType(virDomainPtr dom,
     ret = strdup("posix");
     if (!ret)
         virReportOOMError(dom->conn);
+
+cleanup:
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -6210,15 +6216,14 @@ static int qemuSetSchedulerParameters(virDomainPtr dom,
     virDomainObjPtr vm = NULL;
     int ret = -1;
 
+    qemuDriverLock(driver);
     if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                          __FUNCTION__);
-        return -1;
+        goto cleanup;
     }
 
-    qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (vm == NULL) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
@@ -6261,6 +6266,7 @@ cleanup:
     virCgroupFree(&group);
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -6275,21 +6281,20 @@ static int qemuGetSchedulerParameters(virDomainPtr dom,
     int ret = -1;
     int rc;
 
+    qemuDriverLock(driver);
     if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
         qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_SUPPORT,
                          __FUNCTION__);
-        return -1;
+        goto cleanup;
     }
 
     if ((*nparams) != 1) {
         qemudReportError(dom->conn, domain, NULL, VIR_ERR_INVALID_ARG,
                          "%s", _("Invalid parameter count"));
-        return -1;
+        goto cleanup;
     }
 
-    qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
-    qemuDriverUnlock(driver);
 
     if (vm == NULL) {
         qemudReportError(dom->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR,
@@ -6319,6 +6324,7 @@ cleanup:
     virCgroupFree(&group);
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 
diff --git a/src/storage_driver.c b/src/storage_driver.c
index e9ecb20..14e3bc2 100644
--- a/src/storage_driver.c
+++ b/src/storage_driver.c
@@ -489,24 +489,27 @@ storagePoolCreate(virConnectPtr conn,
     def = NULL;
 
     if (backend->startPool &&
-        backend->startPool(conn, pool) < 0)
+        backend->startPool(conn, pool) < 0) {
+        virStoragePoolObjRemove(&driver->pools, pool);
+        pool = NULL;
         goto cleanup;
+    }
 
     if (backend->refreshPool(conn, pool) < 0) {
         if (backend->stopPool)
             backend->stopPool(conn, pool);
+        virStoragePoolObjRemove(&driver->pools, pool);
+        pool = NULL;
         goto cleanup;
     }
     pool->active = 1;
 
     ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid);
-    virStoragePoolObjUnlock(pool);
-    pool = NULL;
 
 cleanup:
     virStoragePoolDefFree(def);
     if (pool)
-        virStoragePoolObjRemove(&driver->pools, pool);
+        virStoragePoolObjUnlock(pool);
     storageDriverUnlock(driver);
     return ret;
 }
@@ -1321,27 +1324,23 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
     virStorageBackendPtr backend;
     virStorageVolDefPtr origvol = NULL, newvol = NULL;
     virStorageVolPtr ret = NULL, volobj = NULL;
-    int buildret, diffpool;
-
-    diffpool = !STREQ(obj->name, vobj->pool);
+    int buildret;
 
     storageDriverLock(driver);
     pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid);
-    if (diffpool) {
+    if (pool && STRNEQ(obj->name, vobj->pool)) {
         virStoragePoolObjUnlock(pool);
         origpool = virStoragePoolObjFindByName(&driver->pools, vobj->pool);
         virStoragePoolObjLock(pool);
-    } else
-        origpool = pool;
+    }
     storageDriverUnlock(driver);
-
     if (!pool) {
         virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL,
                               "%s", _("no storage pool with matching uuid"));
         goto cleanup;
     }
 
-    if (diffpool && !origpool) {
+    if (STRNEQ(obj->name, vobj->pool) && !origpool) {
         virStorageReportError(obj->conn, VIR_ERR_NO_STORAGE_POOL,
                              _("no storage pool with matching name '%s'"),
                               vobj->pool);
@@ -1354,7 +1353,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
         goto cleanup;
     }
 
-    if (diffpool && !virStoragePoolObjIsActive(origpool)) {
+    if (origpool && !virStoragePoolObjIsActive(origpool)) {
         virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR,
                               "%s", _("storage pool is not active"));
         goto cleanup;
@@ -1363,7 +1362,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
     if ((backend = virStorageBackendForType(pool->def->type)) == NULL)
         goto cleanup;
 
-    origvol = virStorageVolDefFindByName(origpool, vobj->name);
+    origvol = virStorageVolDefFindByName(origpool ? origpool : pool, vobj->name);
     if (!origvol) {
         virStorageReportError(obj->conn, VIR_ERR_NO_STORAGE_VOL,
                              _("no storage vol with matching name '%s'"),
@@ -1429,7 +1428,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
     newvol->building = 1;
     virStoragePoolObjUnlock(pool);
 
-    if (diffpool) {
+    if (origpool) {
         origpool->asyncjobs++;
         virStoragePoolObjUnlock(origpool);
     }
@@ -1438,7 +1437,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
 
     storageDriverLock(driver);
     virStoragePoolObjLock(pool);
-    if (diffpool)
+    if (origpool)
         virStoragePoolObjLock(origpool);
     storageDriverUnlock(driver);
 
@@ -1447,7 +1446,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
     newvol = NULL;
     pool->asyncjobs--;
 
-    if (diffpool) {
+    if (origpool) {
         origpool->asyncjobs--;
         virStoragePoolObjUnlock(origpool);
         origpool = NULL;
@@ -1469,7 +1468,7 @@ cleanup:
     virStorageVolDefFree(newvol);
     if (pool)
         virStoragePoolObjUnlock(pool);
-    if (diffpool && origpool)
+    if (origpool)
         virStoragePoolObjUnlock(origpool);
     return ret;
 }
diff --git a/src/test.c b/src/test.c
index 7c8f85b..0e4203e 100644
--- a/src/test.c
+++ b/src/test.c
@@ -261,12 +261,10 @@ testDomainGenerateIfname(virConnectPtr conn,
     return NULL;
 }
 
-static virDomainObjPtr
-testDomainAssignDef(virConnectPtr conn,
-                    virDomainObjList *domlist,
-                    virDomainDefPtr domdef)
+static int
+testDomainGenerateIfnames(virConnectPtr conn,
+                          virDomainDefPtr domdef)
 {
-    virDomainObjPtr domobj = NULL;
     int i = 0;
 
     for (i = 0; i < domdef->nnets; i++) {
@@ -276,18 +274,15 @@ testDomainAssignDef(virConnectPtr conn,
 
         ifname = testDomainGenerateIfname(conn, domdef);
         if (!ifname)
-            goto error;
+            return -1;
 
         domdef->nets[i]->ifname = ifname;
     }
 
-    if (!(domobj = virDomainAssignDef(conn, domlist, domdef)))
-        goto error;
-
-error:
-    return domobj;
+    return 0;
 }
 
+
 static int testOpenDefault(virConnectPtr conn) {
     int u;
     struct timeval tv;
@@ -342,10 +337,11 @@ static int testOpenDefault(virConnectPtr conn) {
                                            defaultDomainXML,
                                            VIR_DOMAIN_XML_INACTIVE)))
         goto error;
-    if (!(domobj = testDomainAssignDef(conn, &privconn->domains, domdef))) {
-        virDomainDefFree(domdef);
+    if (testDomainGenerateIfnames(conn, domdef) < 0)
         goto error;
-    }
+    if (!(domobj = virDomainAssignDef(conn, &privconn->domains, domdef)))
+        goto error;
+    domdef = NULL;
     domobj->def->id = privconn->nextDomID++;
     domobj->state = VIR_DOMAIN_RUNNING;
     domobj->persistent = 1;
@@ -399,6 +395,7 @@ error:
     testDriverUnlock(privconn);
     conn->privateData = NULL;
     VIR_FREE(privconn);
+    virDomainDefFree(domdef);
     return VIR_DRV_OPEN_ERROR;
 }
 
@@ -668,7 +665,8 @@ static int testOpenFromFile(virConnectPtr conn,
                 goto error;
         }
 
-        if (!(dom = testDomainAssignDef(conn, &privconn->domains, def))) {
+        if (testDomainGenerateIfnames(conn, def) < 0 ||
+            !(dom = virDomainAssignDef(conn, &privconn->domains, def))) {
             virDomainDefFree(def);
             goto error;
         }
@@ -980,11 +978,11 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
                                        VIR_DOMAIN_XML_INACTIVE)) == NULL)
         goto cleanup;
 
-    if ((dom = testDomainAssignDef(conn, &privconn->domains,
-                                   def)) == NULL) {
-        virDomainDefFree(def);
+    if (testDomainGenerateIfnames(conn, def) < 0)
         goto cleanup;
-    }
+    if (!(dom = virDomainAssignDef(conn, &privconn->domains, def)))
+        goto cleanup;
+    def = NULL;
     dom->state = VIR_DOMAIN_RUNNING;
     dom->def->id = privconn->nextDomID++;
 
@@ -992,15 +990,17 @@ testDomainCreateXML(virConnectPtr conn, const char *xml,
                                      VIR_DOMAIN_EVENT_STARTED,
                                      VIR_DOMAIN_EVENT_STARTED_BOOTED);
 
-    ret = virGetDomain(conn, def->name, def->uuid);
+    ret = virGetDomain(conn, dom->def->name, dom->def->uuid);
     if (ret)
-        ret->id = def->id;
+        ret->id = dom->def->id;
 
 cleanup:
     if (dom)
         virDomainObjUnlock(dom);
     if (event)
         testDomainEventQueue(privconn, event);
+    if (def)
+        virDomainDefFree(def);
     testDriverUnlock(privconn);
     return ret;
 }
@@ -1531,13 +1531,14 @@ static int testDomainRestore(virConnectPtr conn,
     if (!def)
         goto cleanup;
 
-    if ((dom = testDomainAssignDef(conn, &privconn->domains,
-                                   def)) == NULL)
+    if (testDomainGenerateIfnames(conn, def) < 0)
         goto cleanup;
+    if (!(dom = virDomainAssignDef(conn, &privconn->domains, def)))
+        goto cleanup;
+    def = NULL;
 
     dom->state = VIR_DOMAIN_RUNNING;
     dom->def->id = privconn->nextDomID++;
-    def = NULL;
     event = virDomainEventNewFromObj(dom,
                                      VIR_DOMAIN_EVENT_STARTED,
                                      VIR_DOMAIN_EVENT_STARTED_RESTORED);
@@ -1823,10 +1824,10 @@ static virDomainPtr testDomainDefineXML(virConnectPtr conn,
                                        VIR_DOMAIN_XML_INACTIVE)) == NULL)
         goto cleanup;
 
-    if ((dom = testDomainAssignDef(conn, &privconn->domains,
-                                   def)) == NULL) {
+    if (testDomainGenerateIfnames(conn, def) < 0)
+        goto cleanup;
+    if (!(dom = virDomainAssignDef(conn, &privconn->domains, def)))
         goto cleanup;
-    }
     def = NULL;
     dom->persistent = 1;
 
diff --git a/tests/object-locking.ml b/tests/object-locking.ml
index 215a2e5..0c66fc7 100644
--- a/tests/object-locking.ml
+++ b/tests/object-locking.ml
@@ -128,7 +128,8 @@ let driverLockMethods = [
     "umlDriverLock";
     "nodedevDriverLock";
     "networkDriverLock";
-    "storageDriverLock"
+    "storageDriverLock";
+    "oneDriverLock"
 ]
 
 (*
@@ -142,7 +143,8 @@ let driverUnlockMethods = [
     "umlDriverUnlock";
     "nodedevDriverUnlock";
     "networkDriverUnlock";
-    "storageDriverUnlock"
+    "storageDriverUnlock";
+    "oneDriverUnlock"
 ]
 
 (*
@@ -159,6 +161,7 @@ let lockableDrivers = [
       "virStorageDriverStatePtr";
       "network_driver";
       "virDeviceMonitorState";
+      "one_driver_t";
 ]
 
 
-- 
1.6.2.5


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