From 5c8d3d3bca1ee6e0999ebd7a045d8aef280fbe85 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 2 Sep 2009 14:02:06 +0100 Subject: [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 | 30 ++++++++++++-------- src/storage_driver.c | 35 ++++++++++++----------- src/test.c | 55 +++++++++++++++++++------------------ tests/object-locking.ml | 7 +++-- 6 files changed, 93 insertions(+), 75 deletions(-) diff --git a/src/lxc_driver.c b/src/lxc_driver.c index bd0cf0ee1c..0ec1e929ac 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 135397ca05..9587a2dc72 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 fad393916b..3683ddff7c 100644 --- 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 a14bb88895..9ab53e14e2 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; } @@ -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 895cce1084..368a3cbfcc 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 215a2e58e0..0c66fc7cf9 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"; ]