From b04629b62934caa8786e73c3db985672422fc662 Mon Sep 17 00:00:00 2001 From: John Ferlan Date: Mon, 23 Apr 2018 10:40:48 -0400 Subject: [PATCH] conf: Clean up object referencing for Add and Remove When adding a new object to the domain object list, there should have been 2 virObjectRef calls made one for each list into which the object was placed to match the 2 virObjectUnref calls that would occur during Remove as part of virHashRemoveEntry when virObjectFreeHashData is called when the element is removed from the hash table as set up in virDomainObjListNew. Some drivers (libxl, lxc, qemu, and vz) handled this inconsistency by calling virObjectRef upon successful return from virDomainObjListAdd in order to use virDomainObjEndAPI when done with the returned @vm. While others (bhyve, openvz, test, and vmware) handled this via only calling virObjectUnlock upon successful return from virDomainObjListAdd. This patch will "unify" the approach to use virDomainObjEndAPI for any @vm successfully returned from virDomainObjListAdd. Because list removal is so tightly coupled with list addition, this patch fixes the list removal algorithm to return the object as entered - "locked and reffed". This way, the callers can then decide how to uniformly handle add/remove success and failure. This removes the onus on the caller to "specially handle" the @vm during removal processing. The Add/Remove logic allows for some logic simplification such as in libxl where we can Remove the @vm directly rather than needing to set a @remove_dom boolean and removing after the libxlDomainObjEndJob completes as the @vm is locked/reffed. Signed-off-by: John Ferlan Reviewed-by: Erik Skultety --- src/bhyve/bhyve_driver.c | 21 ++++-------- src/conf/virdomainobjlist.c | 33 +++++++++---------- src/libxl/libxl_driver.c | 64 ++++++++----------------------------- src/libxl/libxl_migration.c | 31 ++++-------------- src/lxc/lxc_driver.c | 21 +++--------- src/lxc/lxc_process.c | 17 +++++----- src/openvz/openvz_conf.c | 2 +- src/openvz/openvz_driver.c | 20 ++++-------- src/qemu/qemu_domain.c | 17 +--------- src/qemu/qemu_driver.c | 6 +--- src/qemu/qemu_migration.c | 3 +- src/test/test_driver.c | 56 ++++++++++---------------------- src/uml/uml_driver.c | 37 ++++++--------------- src/vmware/vmware_conf.c | 3 +- src/vmware/vmware_driver.c | 17 +++------- src/vz/vz_driver.c | 1 - src/vz/vz_sdk.c | 3 -- 17 files changed, 99 insertions(+), 253 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 768578a43f..6001f0806c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -550,7 +550,6 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag if (virDomainSaveConfig(BHYVE_CONFIG_DIR, caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(privconn->domains, vm); - vm = NULL; goto cleanup; } @@ -566,8 +565,7 @@ bhyveDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag virObjectUnref(caps); virDomainDefFree(def); virDomainDefFree(oldDef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); @@ -609,12 +607,10 @@ bhyveDomainUndefine(virDomainPtr domain) VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(privconn->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -958,10 +954,8 @@ bhyveDomainCreateXML(virConnectPtr conn, VIR_DOMAIN_RUNNING_BOOTED, start_flags) < 0) { /* If domain is not persistent, remove its data */ - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(privconn->domains, vm); - vm = NULL; - } goto cleanup; } @@ -974,8 +968,7 @@ bhyveDomainCreateXML(virConnectPtr conn, cleanup: virObjectUnref(caps); virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); @@ -1007,10 +1000,8 @@ bhyveDomainDestroy(virDomainPtr dom) VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(privconn->domains, vm); - virObjectLock(vm); - } cleanup: virDomainObjEndAPI(&vm); diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index d1eb87e5cf..72064d7c66 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -226,9 +226,13 @@ virDomainObjListFindByName(virDomainObjListPtr doms, * Upon entry @vm should have at least 1 ref and be locked. * * Add the @vm into the @doms->objs and @doms->objsName hash - * tables. + * tables. Once successfully added into a table, increase the + * reference count since upon removal in virHashRemoveEntry + * the virObjectUnref will be called since the hash tables were + * configured to call virObjectFreeHashData when the object is + * removed from the hash table. * - * Returns 0 on success with 2 references and locked + * Returns 0 on success with 3 references and locked * -1 on failure with 1 reference and locked */ static int @@ -240,15 +244,12 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms, virUUIDFormat(vm->def->uuid, uuidstr); if (virHashAddEntry(doms->objs, uuidstr, vm) < 0) return -1; + virObjectRef(vm); if (virHashAddEntry(doms->objsName, vm->def->name, vm) < 0) { - virObjectRef(vm); virHashRemoveEntry(doms->objs, uuidstr); return -1; } - - /* Since domain is in two hash tables, increment the - * reference counter */ virObjectRef(vm); return 0; @@ -266,6 +267,9 @@ virDomainObjListAddObjLocked(virDomainObjListPtr doms, * the @def being added is assumed to represent a * live config, not a future inactive config * + * The returned @vm from this function will be locked and ref + * counted. The caller is expected to use virDomainObjEndAPI + * when it completes usage. */ static virDomainObjPtr virDomainObjListAddLocked(virDomainObjListPtr doms, @@ -311,9 +315,6 @@ virDomainObjListAddLocked(virDomainObjListPtr doms, def, !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE), oldDef); - /* XXX: Temporary until this API is fixed to return a locked and - * refcnt'd object */ - virObjectUnref(vm); } else { /* UUID does not match, but if a name matches, refuse it */ if ((vm = virDomainObjListFindByNameLocked(doms, def->name))) { @@ -371,7 +372,6 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms, virHashRemoveEntry(doms->objs, uuidstr); virHashRemoveEntry(doms->objsName, dom->def->name); - virObjectUnlock(dom); } @@ -386,8 +386,7 @@ virDomainObjListRemoveLocked(virDomainObjListPtr doms, * no one else is either waiting for 'dom' or still using it. * * When this function returns, @dom will be removed from the hash - * tables, unlocked, and returned with the refcnt that was present - * upon entry. + * tables and returned with lock and refcnt that was present upon entry. */ void virDomainObjListRemove(virDomainObjListPtr doms, @@ -453,9 +452,11 @@ virDomainObjListRename(virDomainObjListPtr doms, if (virHashAddEntry(doms->objsName, new_name, dom) < 0) goto cleanup; - /* Okay, this is crazy. virHashAddEntry() does not increment - * the refcounter of @dom, but virHashRemoveEntry() does - * decrement it. We need to work around it. */ + /* Increment the refcnt for @new_name. We're about to remove + * the @old_name which will cause the refcnt to be decremented + * via the virObjectUnref call made during the virObjectFreeHashData + * as a result of removing something from the object list hash + * table as set up during virDomainObjListNew. */ virObjectRef(dom); rc = callback(dom, new_name, flags, opaque); @@ -624,7 +625,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, if (dom) { if (!liveStatus) dom->persistent = 1; - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); } else { VIR_ERROR(_("Failed to load config for domain '%s'"), entry->d_name); } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 55a93a489b..34adef8d48 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -450,13 +450,8 @@ libxlReconnectDomain(virDomainObjPtr vm, error: libxlDomainCleanup(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemoveLocked(driver->domains, vm); - - /* virDomainObjListRemoveLocked leaves the object unlocked, - * lock it again to factorize more code. */ - virObjectLock(vm); - } goto cleanup; } @@ -605,7 +600,6 @@ libxlAddDom0(libxlDriverPrivatePtr driver) 0, &oldDef))) goto cleanup; - def = NULL; vm->persistent = 1; @@ -626,8 +620,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) libxl_dominfo_dispose(&d_info); virDomainDefFree(def); virDomainDefFree(oldDef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1043,23 +1036,18 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } if (libxlDomainStartNew(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto endjob; } @@ -1396,10 +1384,8 @@ libxlDomainDestroyFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); libxlDomainCleanup(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -1759,7 +1745,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; - bool remove_dom = false; #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME virReportUnsupportedError(); @@ -1791,7 +1776,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, goto endjob; if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); ret = 0; @@ -1799,10 +1784,6 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); return ret; } @@ -1850,24 +1831,19 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } ret = libxlDomainStartRestore(driver, vm, (flags & VIR_DOMAIN_SAVE_PAUSED) != 0, fd, hdr.version); - if (ret < 0 && !vm->persistent) { + if (ret < 0 && !vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } libxlDomainObjEndJob(driver, vm); @@ -1893,7 +1869,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); virDomainObjPtr vm; virObjectEventPtr event = NULL; - bool remove_dom = false; bool paused = false; int ret = -1; @@ -1951,7 +1926,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); } ret = 0; @@ -1972,10 +1947,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); @@ -1990,7 +1961,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm = NULL; char *name = NULL; int ret = -1; - bool remove_dom = false; virCheckFlags(0, -1); @@ -2023,7 +1993,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) goto endjob; if (!vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); ret = 0; @@ -2031,10 +2001,6 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); VIR_FREE(name); return ret; @@ -2765,16 +2731,14 @@ libxlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flag 0, &oldDef))) goto cleanup; - - virObjectRef(vm); def = NULL; + vm->persistent = 1; if (virDomainSaveConfig(cfg->configDir, cfg->caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); goto cleanup; } @@ -2851,12 +2815,10 @@ libxlDomainUndefineFlags(virDomainPtr dom, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index d37a4a687a..206b878292 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -265,7 +265,6 @@ libxlDoMigrateDstReceive(void *opaque) int recvfd = args->recvfd; size_t i; int ret; - bool remove_dom = 0; virObjectRef(vm); virObjectLock(vm); @@ -280,7 +279,7 @@ libxlDoMigrateDstReceive(void *opaque) args->migcookie->xenMigStreamVer); if (ret < 0 && !vm->persistent) - remove_dom = true; + virDomainObjListRemove(driver->domains, vm); /* Remove all listen socks from event handler, and close them. */ for (i = 0; i < nsocks; i++) { @@ -296,10 +295,6 @@ libxlDoMigrateDstReceive(void *opaque) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom) { - virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } virDomainObjEndAPI(&vm); } @@ -581,9 +576,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - - virObjectRef(vm); *def = NULL; + priv = vm->privateData; if (taint_hook) { @@ -633,10 +627,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn, VIR_FORCE_CLOSE(dataFD[0]); virObjectUnref(args); /* Remove virDomainObj from domain list */ - if (vm) { + if (vm) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } done: virDomainObjEndAPI(&vm); @@ -680,9 +672,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto error; - - virObjectRef(vm); *def = NULL; + priv = vm->privateData; if (taint_hook) { @@ -807,10 +798,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn, priv->migrationPort = 0; } /* Remove virDomainObj from domain list */ - if (vm) { + if (vm) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } done: VIR_FREE(xmlout); @@ -1336,11 +1325,8 @@ libxlDomainMigrationDstFinish(virConnectPtr dconn, VIR_DOMAIN_SHUTOFF_FAILED); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - /* Caller passed a locked vm and expects the same on return */ - virObjectLock(vm); - } } if (event) @@ -1392,11 +1378,8 @@ libxlDomainMigrationSrcConfirm(libxlDriverPrivatePtr driver, if (flags & VIR_MIGRATE_UNDEFINE_SOURCE) virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); - if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) { + if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) virDomainObjListRemove(driver->domains, vm); - /* Caller passed a locked vm and expects the same on return */ - virObjectLock(vm); - } ret = 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index dd4e28fc7f..a451a0e552 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -478,14 +478,12 @@ lxcDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) 0, &oldDef))) goto cleanup; - virObjectRef(vm); def = NULL; vm->persistent = 1; if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); goto cleanup; } @@ -546,12 +544,10 @@ static int lxcDomainUndefineFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_UNDEFINED, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -1233,14 +1229,11 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (virLXCDomainObjBeginJob(driver, vm, LXC_JOB_MODIFY) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } @@ -1250,10 +1243,8 @@ lxcDomainCreateXMLWithFiles(virConnectPtr conn, VIR_DOMAIN_RUNNING_BOOTED) < 0) { virDomainAuditStart(vm, "booted", false); virLXCDomainObjEndJob(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } goto cleanup; } @@ -1523,10 +1514,8 @@ lxcDomainDestroyFlags(virDomainPtr dom, endjob: virLXCDomainObjEndJob(driver, vm); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } cleanup: virDomainObjEndAPI(&vm); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0c5b24affa..cc6ed12526 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -671,10 +671,8 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon, } else { VIR_DEBUG("Stop event has already been sent"); } - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } } else { int ret = virLXCProcessReboot(driver, vm); virDomainAuditStop(vm, "reboot"); @@ -685,15 +683,15 @@ static void virLXCProcessMonitorEOFNotify(virLXCMonitorPtr mon, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, priv->stopReason); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } } } - if (vm) - virObjectUnlock(vm); + /* NB: virLXCProcessConnectMonitor will perform the virObjectRef(vm) + * before adding monitorCallbacks. Since we are now done with the @vm + * we can Unref/Unlock */ + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(driver->domainEventState, event); } @@ -803,7 +801,8 @@ static virLXCMonitorPtr virLXCProcessConnectMonitor(virLXCDriverPtr driver, goto cleanup; /* Hold an extra reference because we can't allow 'vm' to be - * deleted while the monitor is active */ + * deleted while the monitor is active. This will be unreffed + * during EOFNotify processing. */ virObjectRef(vm); monitor = virLXCMonitorNew(vm, cfg->stateDir, &monitorCallbacks); diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 72ef63a355..5ed2b423cb 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -611,7 +611,7 @@ int openvzLoadDomains(struct openvz_driver *driver) /* XXX OpenVZ doesn't appear to have concept of a transient domain */ dom->persistent = 1; - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); dom = NULL; def = NULL; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index c10d6df663..14295dfda0 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -982,8 +982,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla cleanup: virDomainDefFree(vmdef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); openvzDriverUnlock(driver); return dom; } @@ -1071,8 +1070,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml, cleanup: virDomainDefFree(vmdef); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); openvzDriverUnlock(driver); return dom; } @@ -1151,12 +1149,10 @@ openvzDomainUndefineFlags(virDomainPtr dom, if (virRun(prog, NULL) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; @@ -2232,16 +2228,13 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn, error: virDomainDefFree(def); - if (vm) { + if (vm) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } done: VIR_FREE(my_hostname); virURIFree(uri); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2396,7 +2389,6 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, VIR_DEBUG("Domain '%s' successfully migrated", vm->def->name); virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); ret = 0; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0d752e1464..727ea33f14 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7167,24 +7167,9 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, VIR_FREE(snapDir); } - virObjectRef(vm); - virDomainObjListRemove(driver->domains, vm); - /* - * virDomainObjListRemove() leaves the domain unlocked so it can - * be unref'd for other drivers that depend on that, but we still - * need to reset a job and we have a reference from the API that - * called this function. So we need to lock it back. This is - * just a workaround for the qemu driver. - * - * XXX: Ideally, the global handling of domain objects and object - * lists would be refactored so we don't need hacks like - * this, but since that requires refactor of all drivers, - * it's a work for another day. - */ - virObjectLock(vm); + virObjectUnref(cfg); - virObjectUnref(vm); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 82e1566c79..83fc191085 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1765,7 +1765,6 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (qemuProcessBeginJob(driver, vm, VIR_DOMAIN_JOB_OPERATION_START, @@ -6737,7 +6736,6 @@ qemuDomainRestoreFlags(virConnectPtr conn, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (flags & VIR_DOMAIN_SAVE_RUNNING) @@ -7411,9 +7409,8 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, driver->xmlopt, 0, &oldDef))) goto cleanup; - - virObjectRef(vm); def = NULL; + if (qemuDomainHasBlockjob(vm, true)) { virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s", _("domain has active block job")); @@ -16420,7 +16417,6 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn, NULL))) goto cleanup; - virObjectRef(vm); def = NULL; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 2bdd58bb50..7602a304c5 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2280,9 +2280,8 @@ qemuMigrationDstPrepareAny(virQEMUDriverPtr driver, VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE, NULL))) goto cleanup; - - virObjectRef(vm); *def = NULL; + priv = vm->privateData; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 9089dc9a25..467587b19f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -893,7 +893,7 @@ testParseDomains(testDriverPtr privconn, int num, ret = -1; size_t i; xmlNodePtr *nodes = NULL; - virDomainObjPtr obj; + virDomainObjPtr obj = NULL; num = virXPathNodeSet("/node/domain", ctxt, &nodes); if (num < 0) @@ -921,10 +921,8 @@ testParseDomains(testDriverPtr privconn, goto error; } - if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) { - virObjectUnlock(obj); + if (testParseDomainSnapshots(privconn, obj, file, ctxt) < 0) goto error; - } nsdata = def->namespaceData; obj->persistent = !nsdata->transient; @@ -932,20 +930,19 @@ testParseDomains(testDriverPtr privconn, if (nsdata->runstate != VIR_DOMAIN_SHUTOFF) { if (testDomainStartState(privconn, obj, - VIR_DOMAIN_RUNNING_BOOTED) < 0) { - virObjectUnlock(obj); + VIR_DOMAIN_RUNNING_BOOTED) < 0) goto error; - } } else { testDomainShutdownState(NULL, obj, 0); } virDomainObjSetState(obj, nsdata->runstate, 0); - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); } ret = 0; error: + virDomainObjEndAPI(&obj); VIR_FREE(nodes); return ret; } @@ -1678,10 +1675,8 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, def = NULL; if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_BOOTED) < 0) { - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(privconn->domains, dom); - dom = NULL; - } goto cleanup; } @@ -1692,8 +1687,7 @@ testDomainCreateXML(virConnectPtr conn, const char *xml, ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); testObjectEventQueue(privconn, event); virDomainDefFree(def); testDriverUnlock(privconn); @@ -1787,10 +1781,8 @@ static int testDomainDestroyFlags(virDomainPtr domain, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; cleanup: @@ -1888,10 +1880,8 @@ static int testDomainShutdownFlags(virDomainPtr domain, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; cleanup: @@ -1957,10 +1947,8 @@ static int testDomainReboot(virDomainPtr domain, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } } ret = 0; @@ -2095,10 +2083,8 @@ testDomainSaveFlags(virDomainPtr domain, const char *path, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SAVED); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; cleanup: @@ -2200,10 +2186,8 @@ testDomainRestoreFlags(virConnectPtr conn, def = NULL; if (testDomainStartState(privconn, dom, VIR_DOMAIN_RUNNING_RESTORED) < 0) { - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(privconn->domains, dom); - dom = NULL; - } goto cleanup; } @@ -2216,8 +2200,7 @@ testDomainRestoreFlags(virConnectPtr conn, virDomainDefFree(def); VIR_FREE(xml); VIR_FORCE_CLOSE(fd); - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); testObjectEventQueue(privconn, event); return ret; } @@ -2280,10 +2263,8 @@ static int testDomainCoreDumpWithFormat(virDomainPtr domain, event = virDomainEventLifecycleNewFromObj(privdom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); - if (!privdom->persistent) { + if (!privdom->persistent) virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } } ret = 0; @@ -2800,8 +2781,7 @@ static virDomainPtr testDomainDefineXMLFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); virDomainDefFree(oldDef); - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); testObjectEventQueue(privconn, event); return ret; } @@ -3068,12 +3048,10 @@ static int testDomainUndefineFlags(virDomainPtr domain, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); privdom->hasManagedSave = false; - if (virDomainObjIsActive(privdom)) { + if (virDomainObjIsActive(privdom)) privdom->persistent = 0; - } else { + else virDomainObjListRemove(privconn->domains, privdom); - virObjectLock(privdom); - } ret = 0; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index b0774b288e..53ec64e10f 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -379,10 +379,8 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN); - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); - } } else if (e.mask & (IN_CREATE | IN_MODIFY)) { VIR_DEBUG("Got inotify domain startup '%s'", name); if (virDomainObjIsActive(dom)) { @@ -412,10 +410,8 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); - } } else if (umlIdentifyChrPTY(driver, dom) < 0) { VIR_WARN("Could not identify character devices for new domain"); umlShutdownVMDaemon(driver, dom, @@ -424,10 +420,8 @@ umlInotifyEvent(int watch, event = virDomainEventLifecycleNewFromObj(dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); - } } } virDomainObjEndAPI(&dom); @@ -785,10 +779,8 @@ static int umlProcessAutoDestroyDom(void *payload, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!dom->persistent) { + if (!dom->persistent) virDomainObjListRemove(data->driver->domains, dom); - virObjectLock(dom); - } virDomainObjEndAPI(&dom); if (event) @@ -1609,10 +1601,8 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, if (umlStartVMDaemon(conn, driver, vm, (flags & VIR_DOMAIN_START_AUTODESTROY)) < 0) { virDomainAuditStart(vm, "booted", false); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } goto cleanup; } virDomainAuditStart(vm, "booted", true); @@ -1624,8 +1614,7 @@ static virDomainPtr umlDomainCreateXML(virConnectPtr conn, const char *xml, cleanup: virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); @@ -1694,10 +1683,8 @@ umlDomainDestroyFlags(virDomainPtr dom, event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; cleanup: @@ -2009,7 +1996,6 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virDomainSaveConfig(driver->configDir, driver->caps, vm->newDef ? vm->newDef : vm->def) < 0) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; goto cleanup; } @@ -2017,8 +2003,7 @@ umlDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) cleanup: virDomainDefFree(def); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return dom; } @@ -2054,12 +2039,10 @@ static int umlDomainUndefineFlags(virDomainPtr dom, if (virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; diff --git a/src/vmware/vmware_conf.c b/src/vmware/vmware_conf.c index b9f18e6ac4..e1d6956df0 100644 --- a/src/vmware/vmware_conf.c +++ b/src/vmware/vmware_conf.c @@ -186,10 +186,9 @@ vmwareLoadDomains(struct vmware_driver *driver) VIR_DOMAIN_RUNNING_UNKNOWN); vm->persistent = 1; - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmdef = NULL; - vm = NULL; } ret = 0; diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 21c10b6605..f94b3252fe 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -502,10 +502,8 @@ vmwareDomainShutdownFlags(virDomainPtr dom, if (vmwareStopVM(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN) < 0) goto cleanup; - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; cleanup: @@ -717,10 +715,8 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, vmdef = NULL; if (vmwareStartVM(driver, vm) < 0) { - if (!vm->persistent) { + if (!vm->persistent) virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } goto cleanup; } @@ -730,8 +726,7 @@ vmwareDomainCreateXML(virConnectPtr conn, const char *xml, virDomainDefFree(vmdef); VIR_FREE(vmx); VIR_FREE(vmxPath); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmwareDriverUnlock(driver); return dom; } @@ -796,12 +791,10 @@ vmwareDomainUndefineFlags(virDomainPtr dom, if (vmwareUpdateVMStatus(driver, vm) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { + if (virDomainObjIsActive(vm)) vm->persistent = 0; - } else { + else virDomainObjListRemove(driver->domains, vm); - virObjectLock(vm); - } ret = 0; diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index d3fcae491a..48a9a866d9 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3161,7 +3161,6 @@ vzDomainMigratePerformStep(virDomainObjPtr dom, goto cleanup; virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); ret = 0; diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index bb8f15e4fe..cb82b9d582 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1969,7 +1969,6 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; } - virObjectRef(dom); pdom = dom->privateData; pdom->sdkdom = sdkdom; PrlHandle_AddRef(sdkdom); @@ -2235,7 +2234,6 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); virDomainObjEndAPI(&dom); return; } @@ -4334,7 +4332,6 @@ prlsdkUnregisterDomain(vzDriverPtr driver, virDomainObjPtr dom, unsigned int fla VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); - virObjectLock(dom); ret = 0; cleanup: