[libvirt] [PATCH] Fix misc thread locking bugs / bogus warnings
Daniel Veillard
veillard at redhat.com
Tue Sep 8 14:33:24 UTC 2009
On Wed, Sep 02, 2009 at 02:11:50PM +0100, Daniel P. Berrange wrote:
> 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)
I'm just surprized by this series of fixes, I though we needed to drop
locks when calling into errors routines like lxcError ?
> 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";
> ]
A lot of those are not so trivial changes, but since checks are
programmatic, that sounds right to fix them even at this stage,
ACK.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list