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

diff --git a/src/lxc_driver.c b/src/lxc_driver.c
index bd0cf0ee1cd9404221b7f843df5c9cc5bd493f59..0ec1e929ac5e84825b6d2e70b25f3e45e077fc30 100644 (file)
--- 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 135397ca058d51231101bec4befdd155fd86e7e3..9587a2dc7241717a9c5810f41f989abfe193e875 100644 (file)
--- 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 fad393916b26c6a94b786000e9a5586ac4f14694..3683ddff7c795f4d6561264c5452ecc03cc83cdc 100644 (file)
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -3660,7 +3660,7 @@ static int qemudDomainSave(virDomainPtr dom,
                            const char *path)
 {
     struct qemud_driver *driver = dom->conn->privateData;
-    virDomainObjPtr vm;
+    virDomainObjPtr vm = NULL;
     char *command = NULL;
     char *info = NULL;
     int fd = -1;
@@ -3675,6 +3675,7 @@ static int qemudDomainSave(virDomainPtr dom,
     memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
     header.version = QEMUD_SAVE_VERSION;
 
+    qemuDriverLock(driver);
     if (driver->saveImageFormat == NULL)
         header.compressed = QEMUD_SAVE_FORMAT_RAW;
     else {
@@ -3684,11 +3685,10 @@ static int qemudDomainSave(virDomainPtr dom,
             qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
                              "%s", _("Invalid save image format specified "
                                      "in configuration file"));
-          return -1;
+            goto cleanup;
         }
     }
 
-    qemuDriverLock(driver);
     vm = virDomainFindByUUID(&driver->domains, dom->uuid);
 
     if (!vm) {
@@ -4510,7 +4510,9 @@ static char *qemuDomainXMLFromNative(virConnectPtr conn,
         goto cleanup;
     }
 
+    qemuDriverLock(driver);
     def = qemuParseCommandLineString(conn, driver->caps, config);
+    qemuDriverUnlock(driver);
     if (!def)
         goto cleanup;
 
@@ -6266,12 +6268,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)
@@ -6280,6 +6283,9 @@ static char *qemuGetSchedulerType(virDomainPtr dom,
     ret = strdup("posix");
     if (!ret)
         virReportOOMError(dom->conn);
+
+cleanup:
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -6293,15 +6299,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,
@@ -6344,6 +6349,7 @@ cleanup:
     virCgroupFree(&group);
     if (vm)
         virDomainObjUnlock(vm);
+    qemuDriverUnlock(driver);
     return ret;
 }
 
@@ -6358,21 +6364,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,
@@ -6402,6 +6407,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 a14bb88895b8dda01e66c5f173d60250329df885..9ab53e14e2c446147fa60ef7df83f9e8f538eb93 100644 (file)
--- 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;
 }
@@ -1319,27 +1322,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);
@@ -1352,7 +1351,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;
@@ -1361,7 +1360,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'"),
@@ -1427,7 +1426,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
     newvol->building = 1;
     virStoragePoolObjUnlock(pool);
 
-    if (diffpool) {
+    if (origpool) {
         origpool->asyncjobs++;
         virStoragePoolObjUnlock(origpool);
     }
@@ -1436,7 +1435,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
 
     storageDriverLock(driver);
     virStoragePoolObjLock(pool);
-    if (diffpool)
+    if (origpool)
         virStoragePoolObjLock(origpool);
     storageDriverUnlock(driver);
 
@@ -1445,7 +1444,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj,
     newvol = NULL;
     pool->asyncjobs--;
 
-    if (diffpool) {
+    if (origpool) {
         origpool->asyncjobs--;
         virStoragePoolObjUnlock(origpool);
         origpool = NULL;
@@ -1467,7 +1466,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 895cce1084460f13301332c65c1f0df65b47e6af..368a3cbfcc84ac3128dcbb31a09d761c9ab1e33c 100644 (file)
--- 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 215a2e58e086cca4790bd197d0d126553fc91488..0c66fc7cf9b14608f8c5fd82f808a0422c176674 100644 (file)
--- 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";
 ]