Re: [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)

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"; ]