[libvirt] [PATCH] Fix misc thread locking bugs / bogus warnings
Daniel P. Berrange
berrange at redhat.com
Wed Sep 2 13:11:50 UTC 2009
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
More information about the libvir-list
mailing list