On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: Is there now no memory leak (as there is a unref missing) when @vm is set to NULL if virDomainObjIsActive(vm) returns false? Similar cases are bhyveDomainCreateXML, bhyveDomainDefineXMLFlags, and bhyveDomainDestroy. But maybe I'm just missing something :) […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Tue, Mar 13, 2018 at 01:39 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: Oh, sorry for that. :) Thanks for the detailed explanation! Okay. And what about bhyveDomainCreateXML and bhyveDomainDefineXMLFlags if (!vm->persistent)? -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

Commit id '45697fe5' added a check for "Domain-0" to generate an error during libxlDomainMigrateBegin3Params; however, by returning NULL, the @vm was left locked since libxlDomObjFromDomain returns a locked @vm. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libxl/libxl_driver.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index c3616a86d..b5101626e 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -5895,9 +5895,10 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain, return NULL; if (STREQ_NULLABLE(vm->def->name, "Domain-0")) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Domain-0 cannot be migrated")); - return NULL; + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Domain-0 cannot be migrated")); + virObjectUnlock(vm); + return NULL; } if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) { -- 2.13.6

On 03/09/2018 03:33 PM, Jim Fehlig wrote: No longer needed since this fix was folded into commit 64370c4b. Regards, Jim

On 03/09/2018 09:47 AM, John Ferlan wrote: For these changes Reviewed-by: Jim Fehlig <jfehlig(a)suse.com&gt; However the migration APIs need some work. Not shown here, but the subsequent code calls libxlDomainMigrationBegin which unrefs and unlocks on exit. libxlDomainMigrationBegin is also called via libxlDomainMigratePerform3Params when doing p2p migration, in which case both functions unref/unlock :-(. IMO libxlDomainMigrationBegin should leave unref/unlock to its callers, where the ref/locking is done. The same is true for libxlDomainMigrationConfirm. I can send a patch to fix these problems, along with a few others I noticed when reviewing the migration code. Regards, Jim

On 03/11/2018 08:13 PM, Jim Fehlig wrote: I've pushed this part of the patch. Your fixes have encouraged me to audit all the begin/end API code in the libxl driver and I'd like to base any fixes on top of items we've already hashed out. Regards, Jim

On 03/09/2018 09:48 AM, John Ferlan wrote: Reviewed-by: Jim Fehlig <jfehlig(a)suse.com&gt; Regards, Jim

Some of the indents were only 2 spaces, make consistent w/ 4 spaces Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/openvz/openvz_driver.c | 64 +++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index ebdc3890e..a211c370e 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -222,10 +222,10 @@ static int openvzSetInitialConfig(virDomainDefPtr vmdef) ret = 0; cleanup: - VIR_FREE(confdir); - virCommandFree(cmd); + VIR_FREE(confdir); + virCommandFree(cmd); - return ret; + return ret; } @@ -267,9 +267,9 @@ openvzSetDiskQuota(virDomainDefPtr vmdef, ret = 0; cleanup: - virCommandFree(cmd); + virCommandFree(cmd); - return ret; + return ret; } @@ -633,40 +633,40 @@ static int openvzDomainSuspend(virDomainPtr dom) static int openvzDomainResume(virDomainPtr dom) { - struct openvz_driver *driver = dom->conn->privateData; - virDomainObjPtr vm; - const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--resume", NULL}; - int ret = -1; + struct openvz_driver *driver = dom->conn->privateData; + virDomainObjPtr vm; + const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--resume", NULL}; + int ret = -1; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); + openvzDriverLock(driver); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); + openvzDriverUnlock(driver); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!vm) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", + _("no domain with matching uuid")); + goto cleanup; + } - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("Domain is not running")); - goto cleanup; - } + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Domain is not running")); + goto cleanup; + } - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { - openvzSetProgramSentinal(prog, vm->def->name); - if (virRun(prog, NULL) < 0) - goto cleanup; - virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNPAUSED); - } + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { + openvzSetProgramSentinal(prog, vm->def->name); + if (virRun(prog, NULL) < 0) + goto cleanup; + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNPAUSED); + } - ret = 0; + ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); - return ret; + if (vm) + virObjectUnlock(vm); + return ret; } static int -- 2.13.6

On 03/13/2018 03:58 AM, Marc Hartmayer wrote: No FindByUUID calls there, so I wasn't looking that close ;-)... Fixed up ones in those functions in my branch. Tks - John

On 03/09/2018 09:48 AM, John Ferlan wrote: It looks like the cleanup label can be removed from this function too. Not related to your goal, but I wonder why the pattern here is different? Why does the driver need to be locked during the entire undefine operation? Same here. It's odd that the driver needs to be locked for the whole operation. But it's orthogonal to your work. Reviewed-by: Jim Fehlig <jfehlig(a)suse.com&gt; Regards, Jim

If openvzDomainLookupByID or openvzDomainLookupByName fails to find a vm, let's be a bit more descriptive by providing the failing id or name in the error message. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/openvz/openvz_driver.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 167ba2f7a..b31bf0714 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -351,7 +351,8 @@ static virDomainPtr openvzDomainLookupByID(virConnectPtr conn, openvzDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching id '%d'"), id); goto cleanup; } @@ -425,7 +426,8 @@ static virDomainPtr openvzDomainLookupByName(virConnectPtr conn, openvzDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), name); goto cleanup; } @@ -1113,8 +1115,8 @@ openvzDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) openvzDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching id")); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), dom->name); goto cleanup; } -- 2.13.6

For openvzDomObjFromDomainLocked and openvzDomainLookupByID let's return a locked and referenced @vm object so that callers can then use the common and more consistent virDomainObjEndAPI in order to handle cleanup rather than needing to know that the returned object is locked and calling virObjectUnlock. The LookupByName already returns the ref counted and locked object, so this will make things more consistent. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/openvz/openvz_driver.c | 76 ++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 50 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b31bf0714..b0b72b171 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -95,7 +95,7 @@ openvzDomObjFromDomainLocked(struct openvz_driver *driver, virDomainObjPtr vm; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) { + if (!(vm = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -329,8 +329,7 @@ openvzDomainGetHostname(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return hostname; error: @@ -347,7 +346,7 @@ static virDomainPtr openvzDomainLookupByID(virConnectPtr conn, virDomainPtr dom = NULL; openvzDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByIDRef(driver->domains, id); openvzDriverUnlock(driver); if (!vm) { @@ -359,8 +358,7 @@ static virDomainPtr openvzDomainLookupByID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -391,8 +389,7 @@ static char *openvzDomainGetOSType(virDomainPtr dom) ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(vm->def->os.type))); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -409,8 +406,7 @@ static virDomainPtr openvzDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -469,8 +465,7 @@ static int openvzDomainGetInfo(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -492,8 +487,7 @@ openvzDomainGetState(virDomainPtr dom, ret = openvzGetVEStatus(vm, state, reason); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -509,8 +503,7 @@ static int openvzDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -526,8 +519,7 @@ static int openvzDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -549,8 +541,7 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { ret = virDomainDefFormat(vm->def, driver->caps, virDomainDefFormatConvertXMLFlags(flags)); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -600,8 +591,7 @@ static int openvzDomainSuspend(virDomainPtr dom) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -631,8 +621,7 @@ static int openvzDomainResume(virDomainPtr dom) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -670,8 +659,7 @@ openvzDomainShutdownFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -724,8 +712,7 @@ static int openvzDomainReboot(virDomainPtr dom, virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_BOOTED); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1183,8 +1170,7 @@ openvzDomainUndefineFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); openvzDriverUnlock(driver); return ret; } @@ -1213,8 +1199,7 @@ openvzDomainSetAutostart(virDomainPtr dom, int autostart) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1243,8 +1228,7 @@ openvzDomainGetAutostart(virDomainPtr dom, int *autostart) cleanup: VIR_FREE(value); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1336,8 +1320,7 @@ static int openvzDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1934,8 +1917,7 @@ openvzDomainInterfaceStats(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2029,8 +2011,7 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, cleanup: openvzDriverUnlock(driver); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2166,8 +2147,7 @@ openvzDomainMigrateBegin3Params(virDomainPtr domain, VIR_DOMAIN_DEF_FORMAT_SECURE); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return xml; } @@ -2269,8 +2249,7 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn, done: VIR_FREE(my_hostname); virURIFree(uri); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2323,8 +2302,7 @@ openvzDomainMigratePerform3Params(virDomainPtr domain, cleanup: virCommandFree(cmd); virURIFree(uri); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2431,8 +2409,7 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2450,8 +2427,7 @@ openvzDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = 0; - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } -- 2.13.6

On 03/29/2018 01:52 PM, Jim Fehlig wrote: Thanks - although based on some of the libxl adjustments and comments in/for the bhyve patch #1 - the following 3 functions need adjustment as well since there's a reffed @vm object: openvzDomainUndefineFlags - Since we have a reffed/locked object calling into virDomainObjListRemove, we need to Lock it upon return and allow the subsequent virDomainObjEndAPI handle the removal. openvzDomainMigratePrepare3Params - Original change too aggressive here as virDomainObjListAdd returns 2 refs and locked and we need to leave it that way. openvzDomainMigrateConfirm3Params - Same as openvzDomainUndefineFlags Changes I'd squash in are: diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 649922e59..ec9541840 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1165,7 +1165,7 @@ openvzDomainUndefineFlags(virDomainPtr dom, vm->persistent = 0; } else { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } ret = 0; @@ -2263,7 +2263,8 @@ openvzDomainMigratePrepare3Params(virConnectPtr dconn, done: VIR_FREE(my_hostname); virURIFree(uri); - virDomainObjEndAPI(&vm); + if (vm) + virObjectUnlock(vm); return ret; } @@ -2418,7 +2419,7 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, VIR_DEBUG("Domain '%s' successfully migrated", vm->def->name); virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); ret = 0; John

Rather than an empty failed to find, let's provide a bit more knowledge about what we failed to find by using the name string or the id value. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/uml/uml_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ea8fca38b..d10a9ba62 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1385,7 +1385,8 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, umlDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching id '%d'"), id); goto cleanup; } @@ -1433,7 +1434,8 @@ static virDomainPtr umlDomainLookupByName(virConnectPtr conn, umlDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching name '%s'"), name); goto cleanup; } -- 2.13.6

Rather than repeat code throughout, create and use a couple of accessors in order to lookup by UUID. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/vmware/vmware_driver.c | 180 +++++++++++++++------------------------------ 1 file changed, 61 insertions(+), 119 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 8b487c4a7..121751e27 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -59,6 +59,39 @@ vmwareDriverUnlock(struct vmware_driver *driver) virMutexUnlock(&driver->lock); } + +static virDomainObjPtr +vmwareDomObjFromDomainLocked(struct vmware_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) { + virUUIDFormat(uuid, uuidstr); + + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + return NULL; + } + + return vm; +} + + +static virDomainObjPtr +vmwareDomObjFromDomain(struct vmware_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + + vmwareDriverLock(driver); + vm = vmwareDomObjFromDomainLocked(driver, uuid); + vmwareDriverUnlock(driver); + return vm; +} + + static void * vmwareDataAllocFunc(void *opaque ATTRIBUTE_UNUSED) { @@ -460,13 +493,8 @@ vmwareDomainShutdownFlags(virDomainPtr dom, vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (vmwareUpdateVMStatus(driver, vm) < 0) goto cleanup; @@ -531,15 +559,8 @@ vmwareDomainSuspend(virDomainPtr dom) return ret; } - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; vmwareSetSentinal(cmd, vmwareDriverTypeToString(driver->type)); vmwareSetSentinal(cmd, ((vmwareDomainPtr) vm->privateData)->vmxPath); @@ -580,15 +601,8 @@ vmwareDomainResume(virDomainPtr dom) return ret; } - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; vmwareSetSentinal(cmd, vmwareDriverTypeToString(driver->type)); vmwareSetSentinal(cmd, ((vmwareDomainPtr) vm->privateData)->vmxPath); @@ -624,15 +638,8 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags) virCheckFlags(0, -1); - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; vmxPath = ((vmwareDomainPtr) vm->privateData)->vmxPath; vmwareSetSentinal(cmd, vmwareDriverTypeToString(driver->type)); @@ -750,14 +757,8 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, virCheckFlags(0, -1); vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("No domain with matching uuid '%s'"), uuidstr); + if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (vmwareUpdateVMStatus(driver, vm) < 0) goto cleanup; @@ -794,16 +795,8 @@ vmwareDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1); vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = vmwareDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -867,18 +860,11 @@ vmwareDomainGetOSType(virDomainPtr dom) virDomainObjPtr vm; char *ret = NULL; - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return NULL; ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(vm->def->os.type))); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -892,18 +878,11 @@ vmwareDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainObjPtr vm; virDomainPtr dom = NULL; - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, uuid))) + return NULL; dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - cleanup: if (vm) virObjectUnlock(vm); return dom; @@ -940,16 +919,11 @@ vmwareDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - vmwareDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = virDomainObjIsActive(obj); - cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -963,16 +937,11 @@ vmwareDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - vmwareDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = obj->persistent; - cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -988,20 +957,12 @@ vmwareDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) /* Flags checked by virDomainDefFormat */ - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return NULL; ret = virDomainDefFormat(vm->def, driver->caps, virDomainDefFormatConvertXMLFlags(flags)); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -1121,15 +1082,8 @@ vmwareDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) virDomainObjPtr vm; int ret = -1; - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; if (vmwareUpdateVMStatus(driver, vm) < 0) goto cleanup; @@ -1159,15 +1113,8 @@ vmwareDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - vmwareDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; if (vmwareUpdateVMStatus(driver, vm) < 0) goto cleanup; @@ -1214,16 +1161,11 @@ vmwareDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) virCheckFlags(0, -1); - vmwareDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - vmwareDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = vmwareDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = 0; - cleanup: if (obj) virObjectUnlock(obj); return ret; -- 2.13.6

The virDomainObjListFindByName returns a locked and reffed domain object, all we did was unlock it, leaving an extra ref. Use the virDomainObjEndAPI to cleanup instead. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/vmware/vmware_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 9cd0bc438..d17fdfe3b 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -909,8 +909,7 @@ vmwareDomainLookupByName(virConnectPtr conn, const char *name) dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } -- 2.13.6

Rather than have two API's doing different things for different callers, let's make one API that will always return a locked and ref counted object. That way, the callers will always know that they must call virDomainObjEndAPI and not have to decide whether they should call virObjectUnlock instead. This will make things consistent with LookupByName which returns the locked and ref counted object. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/vz/vz_driver.c | 111 ++++++++++++++++++++++++++--------------------------- src/vz/vz_utils.c | 32 +-------------- src/vz/vz_utils.h | 1 - 3 files changed, 56 insertions(+), 88 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 6d02ef274..1118ef92f 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -654,7 +654,7 @@ vzDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) vzDomObjPtr privdom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (virDomainGetInfoEnsureACL(domain->conn, dom->def) < 0) @@ -703,7 +703,7 @@ vzDomainGetOSType(virDomainPtr domain) ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(dom->def->os.type))); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -722,7 +722,7 @@ vzDomainIsPersistent(virDomainPtr domain) ret = 1; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -745,7 +745,7 @@ vzDomainGetState(virDomainPtr domain, ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -771,7 +771,7 @@ vzDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) ret = virDomainDefFormat(def, privconn->driver->caps, flags); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -791,7 +791,7 @@ vzDomainGetAutostart(virDomainPtr domain, int *autostart) ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -985,7 +985,7 @@ vzDomainGetVcpus(virDomainPtr domain, size_t i; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainGetVcpusEnsureACL(domain->conn, dom->def) < 0) @@ -1087,7 +1087,7 @@ vzDomainSuspend(virDomainPtr domain) int ret = -1; bool job = false; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSuspendEnsureACL(domain->conn, dom->def) < 0) @@ -1124,7 +1124,7 @@ vzDomainResume(virDomainPtr domain) int ret = -1; bool job = false; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainResumeEnsureACL(domain->conn, dom->def) < 0) @@ -1163,7 +1163,7 @@ vzDomainCreateWithFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainCreateWithFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1202,7 +1202,7 @@ vzDomainDestroyFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainDestroyFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1247,7 +1247,7 @@ vzDomainShutdownFlags(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainShutdownFlagsEnsureACL(domain->conn, dom->def, flags) < 0) @@ -1291,7 +1291,7 @@ vzDomainReboot(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainRebootEnsureACL(domain->conn, dom->def, flags) < 0) @@ -1334,7 +1334,7 @@ static int vzDomainIsActive(virDomainPtr domain) ret = virDomainObjIsActive(dom); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1357,7 +1357,7 @@ vzDomainUndefineFlags(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainUndefineFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1409,7 +1409,7 @@ vzDomainHasManagedSaveImage(virDomainPtr domain, unsigned int flags) ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1426,7 +1426,7 @@ vzDomainManagedSave(virDomainPtr domain, unsigned int flags) virCheckFlags(VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainManagedSaveEnsureACL(domain->conn, dom->def) < 0) @@ -1469,7 +1469,7 @@ vzDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainManagedSaveRemoveEnsureACL(domain->conn, dom->def) < 0) @@ -1522,7 +1522,7 @@ static int vzDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -1577,7 +1577,7 @@ static int vzDomainDetachDeviceFlags(virDomainPtr domain, const char *xml, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - dom = vzDomObjFromDomainRef(domain); + dom = vzDomObjFromDomain(domain); if (dom == NULL) return -1; @@ -1634,7 +1634,7 @@ vzDomainSetUserPassword(virDomainPtr domain, bool job = false; virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSetUserPasswordEnsureACL(domain->conn, dom->def) < 0) @@ -1670,7 +1670,7 @@ static int vzDomainUpdateDeviceFlags(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainUpdateDeviceFlagsEnsureACL(domain->conn, dom->def, flags) < 0) @@ -1723,7 +1723,7 @@ vzDomainGetMaxMemory(virDomainPtr domain) ret = virDomainDefGetMemoryTotal(dom->def); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1784,7 +1784,7 @@ vzDomainBlockStats(virDomainPtr domain, virDomainObjPtr dom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainBlockStatsEnsureACL(domain->conn, dom->def) < 0) @@ -1851,7 +1851,7 @@ vzDomainBlockStatsFlags(virDomainPtr domain, /* We don't return strings, and thus trivially support this flag. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainBlockStatsFlagsEnsureACL(domain->conn, dom->def) < 0) @@ -1880,7 +1880,7 @@ vzDomainInterfaceStats(virDomainPtr domain, vzDomObjPtr privdom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainInterfaceStatsEnsureACL(domain->conn, dom->def) < 0) @@ -1907,7 +1907,7 @@ vzDomainMemoryStats(virDomainPtr domain, int ret = -1; virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainMemoryStatsEnsureACL(domain->conn, dom->def) < 0) @@ -1946,7 +1946,7 @@ vzDomainGetVcpusFlags(virDomainPtr domain, ret = virDomainDefGetVcpus(dom->def); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -1973,7 +1973,7 @@ static int vzDomainIsUpdated(virDomainPtr domain) ret = 0; cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -2110,7 +2110,7 @@ static int vzDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -2142,7 +2142,7 @@ static int vzDomainSetMemory(virDomainPtr domain, unsigned long memory) int ret = -1; bool job = false; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSetMemoryEnsureACL(domain->conn, dom->def) < 0) @@ -2217,7 +2217,7 @@ vzDomainSnapshotNum(virDomainPtr domain, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSnapshotNumEnsureACL(domain->conn, dom->def) < 0) @@ -2248,7 +2248,7 @@ vzDomainSnapshotListNames(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainSnapshotListNamesEnsureACL(domain->conn, dom->def) < 0) @@ -2278,7 +2278,7 @@ vzDomainListAllSnapshots(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainListAllSnapshotsEnsureACL(domain->conn, dom->def) < 0) @@ -2308,7 +2308,7 @@ vzDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return NULL; if (virDomainSnapshotGetXMLDescEnsureACL(snapshot->domain->conn, dom->def, flags) < 0) @@ -2345,7 +2345,7 @@ vzDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotNumChildrenEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2380,7 +2380,7 @@ vzDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotListChildrenNamesEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2414,7 +2414,7 @@ vzDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotListAllChildrenEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2447,7 +2447,7 @@ vzDomainSnapshotLookupByName(virDomainPtr domain, virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return NULL; if (virDomainSnapshotLookupByNameEnsureACL(domain->conn, dom->def) < 0) @@ -2477,7 +2477,7 @@ vzDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainHasCurrentSnapshotEnsureACL(domain->conn, dom->def) < 0) @@ -2505,7 +2505,7 @@ vzDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return NULL; if (virDomainSnapshotGetParentEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2543,7 +2543,7 @@ vzDomainSnapshotCurrent(virDomainPtr domain, unsigned int flags) virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return NULL; if (virDomainSnapshotCurrentEnsureACL(domain->conn, dom->def) < 0) @@ -2577,7 +2577,7 @@ vzDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotIsCurrentEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2607,7 +2607,7 @@ vzDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotHasMetadataEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2645,7 +2645,7 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, virCheckFlags(0, NULL); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return NULL; if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, dom->def, flags) < 0) @@ -2708,7 +2708,7 @@ vzDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainSnapshotDeleteEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2732,7 +2732,7 @@ vzDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1); - if (!(dom = vzDomObjFromDomainRef(snapshot->domain))) + if (!(dom = vzDomObjFromDomain(snapshot->domain))) return -1; if (virDomainRevertToSnapshotEnsureACL(snapshot->domain->conn, dom->def) < 0) @@ -2978,8 +2978,7 @@ vzDomainMigrateBegin3Params(virDomainPtr domain, cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return xml; } @@ -3309,7 +3308,7 @@ vzDomainMigratePerform3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, VZ_MIGRATION_PARAMETERS) < 0) return -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainMigratePerform3ParamsEnsureACL(domain->conn, dom->def) < 0) @@ -3432,7 +3431,7 @@ vzDomainGetJobInfo(virDomainPtr domain, virDomainJobInfoPtr info) ret = vzDomainGetJobInfoImpl(dom, info); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -3504,7 +3503,7 @@ vzDomainGetJobStats(virDomainPtr domain, ret = vzDomainJobInfoToParams(&info, type, params, nparams); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -3883,7 +3882,7 @@ vzDomainAbortJob(virDomainPtr domain) virDomainObjPtr dom; int ret = -1; - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainAbortJobEnsureACL(domain->conn, dom->def) < 0) @@ -3906,7 +3905,7 @@ vzDomainReset(virDomainPtr domain, unsigned int flags) virCheckFlags(0, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) return -1; if (virDomainResetEnsureACL(domain->conn, dom->def) < 0) @@ -3938,7 +3937,7 @@ static int vzDomainSetVcpusFlags(virDomainPtr domain, unsigned int nvcpus, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (vzCheckConfigUpdateFlags(dom, &flags) < 0) @@ -3981,7 +3980,7 @@ vzDomainBlockResize(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1); - if (!(dom = vzDomObjFromDomainRef(domain))) + if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; if (virDomainBlockResizeEnsureACL(domain->conn, dom->def) < 0) diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 6fb27169a..8f4e3e347 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -63,43 +63,13 @@ static virDomainControllerType vz7ControllerTypes[] = {VIR_DOMAIN_CONTROLLER_TYP * @domain: Domain pointer that has to be looked up * * This function looks up @domain and returns the appropriate virDomainObjPtr - * that has to be unlocked by virObjectUnlock(). - * - * Returns the domain object without incremented reference counter which is locked - * on success, NULL otherwise. - */ -virDomainObjPtr -vzDomObjFromDomain(virDomainPtr domain) -{ - virDomainObjPtr vm; - vzConnPtr privconn = domain->conn->privateData; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - vzDriverPtr driver = privconn->driver; - - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - if (!vm) { - virUUIDFormat(domain->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s' (%s)"), - uuidstr, domain->name); - return NULL; - } - - return vm; -} - -/** - * vzDomObjFromDomainRef: - * @domain: Domain pointer that has to be looked up - * - * This function looks up @domain and returns the appropriate virDomainObjPtr * that has to be released by calling virDomainObjEndAPI(). * * Returns the domain object with incremented reference counter which is locked * on success, NULL otherwise. */ virDomainObjPtr -vzDomObjFromDomainRef(virDomainPtr domain) +vzDomObjFromDomain(virDomainPtr domain) { virDomainObjPtr vm; vzConnPtr privconn = domain->conn->privateData; diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index f9e9dee42..40a1ce23d 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -120,7 +120,6 @@ void* vzDomObjAlloc(void *opaque); void vzDomObjFree(void *p); virDomainObjPtr vzDomObjFromDomain(virDomainPtr domain); -virDomainObjPtr vzDomObjFromDomainRef(virDomainPtr domain); char * vzGetOutput(const char *binary, ...) ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; -- 2.13.6

Rather than using virDomainObjListFindByUUID, use the ref counting API in order to use virDomainObjEndAPI instead of virObjectUnlock (for common look and feel). Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/test/test_driver.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 043caa976..ce8c1001d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1725,7 +1725,7 @@ static virDomainPtr testDomainLookupByUUID(virConnectPtr conn, virDomainPtr ret = NULL; virDomainObjPtr dom; - if (!(dom = virDomainObjListFindByUUID(privconn->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(privconn->domains, uuid))) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; } @@ -1733,8 +1733,7 @@ static virDomainPtr testDomainLookupByUUID(virConnectPtr conn, ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } -- 2.13.6

Rework the code such that virDomainObjListFindByID will always return a locked/ref counted object so that the callers can always do the same cleanup logic to call virDomainObjEndAPI. Makes accessing the objects much more consistent. NB: There were 3 callers (lxcDomainLookupByID, testDomainLookupByID, and qemuDomainLookupByID) that were already using the ByID name, but not the virDomainObjEndAPI - these were changed as well in this update/patch. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/bhyve/bhyve_driver.c | 2 +- src/conf/virdomainobjlist.c | 35 +++++++++-------------------------- src/conf/virdomainobjlist.h | 2 -- src/libvirt_private.syms | 1 - src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 3 +-- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 5 ++--- src/test/test_driver.c | 3 +-- src/uml/uml_driver.c | 2 +- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 2 +- 13 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 4f95f6c15..1fd31912d 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -847,7 +847,7 @@ bhyveDomainLookupByID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByIDRef(privconn->domains, id); + vm = virDomainObjListFindByID(privconn->domains, id); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 3290dfa29..983b6fda7 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -112,44 +112,27 @@ static int virDomainObjListSearchID(const void *payload, return want; } -static virDomainObjPtr -virDomainObjListFindByIDInternal(virDomainObjListPtr doms, - int id, - bool ref) + +virDomainObjPtr +virDomainObjListFindByID(virDomainObjListPtr doms, + int id) { virDomainObjPtr obj; + virObjectRWLockRead(doms); obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id, NULL); - if (ref) { - virObjectRef(obj); - virObjectRWUnlock(doms); - } + virObjectRef(obj); + virObjectRWUnlock(doms); if (obj) { virObjectLock(obj); if (obj->removing) { virObjectUnlock(obj); - if (ref) - virObjectUnref(obj); + virObjectUnref(obj); obj = NULL; } } - if (!ref) - virObjectRWUnlock(doms); - return obj; -} - -virDomainObjPtr -virDomainObjListFindByID(virDomainObjListPtr doms, - int id) -{ - return virDomainObjListFindByIDInternal(doms, id, false); -} -virDomainObjPtr -virDomainObjListFindByIDRef(virDomainObjListPtr doms, - int id) -{ - return virDomainObjListFindByIDInternal(doms, id, true); + return obj; } diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index 1b77a95ba..7e2dece3a 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -34,8 +34,6 @@ virDomainObjListPtr virDomainObjListNew(void); virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms, int id); -virDomainObjPtr virDomainObjListFindByIDRef(virDomainObjListPtr doms, - int id); virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms, const unsigned char *uuid); virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6f0cd9680..5b6a9d899 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -948,7 +948,6 @@ virDomainObjListCollect; virDomainObjListConvert; virDomainObjListExport; virDomainObjListFindByID; -virDomainObjListFindByIDRef; virDomainObjListFindByName; virDomainObjListFindByUUID; virDomainObjListForEach; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index e76740247..6332b9ae2 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -443,7 +443,7 @@ libxlDomainShutdownThread(void *opaque) cfg = libxlDriverConfigGet(driver); - vm = virDomainObjListFindByIDRef(driver->domains, ev->domid); + vm = virDomainObjListFindByID(driver->domains, ev->domid); if (!vm) { VIR_INFO("Received event for unknown domain ID %d", ev->domid); goto cleanup; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 1379e9b83..5091592c6 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1090,7 +1090,7 @@ libxlDomainLookupByID(virConnectPtr conn, int id) virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByIDRef(driver->domains, id); + vm = virDomainObjListFindByID(driver->domains, id); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ed3f0fbc9..afa533c40 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -269,8 +269,7 @@ static virDomainPtr lxcDomainLookupByID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 3d4e66168..5b88118de 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -346,7 +346,7 @@ static virDomainPtr openvzDomainLookupByID(virConnectPtr conn, virDomainPtr dom = NULL; openvzDriverLock(driver); - vm = virDomainObjListFindByIDRef(driver->domains, id); + vm = virDomainObjListFindByID(driver->domains, id); openvzDriverUnlock(driver); if (!vm) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4972b1fef..53e597f23 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1530,7 +1530,7 @@ static virDomainPtr qemuDomainLookupByID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByID(driver->domains, id); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -1544,8 +1544,7 @@ static virDomainPtr qemuDomainLookupByID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index bef15c826..f39e785a1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1713,8 +1713,7 @@ static virDomainPtr testDomainLookupByID(virConnectPtr conn, ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 91797c0eb..bbccbf5e1 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1379,7 +1379,7 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, virDomainPtr dom = NULL; umlDriverLock(driver); - vm = virDomainObjListFindByIDRef(driver->domains, id); + vm = virDomainObjListFindByID(driver->domains, id); umlDriverUnlock(driver); if (!vm) { diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index f427361d1..94c7fbadd 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -831,7 +831,7 @@ vmwareDomainLookupByID(virConnectPtr conn, int id) virDomainPtr dom = NULL; vmwareDriverLock(driver); - vm = virDomainObjListFindByIDRef(driver->domains, id); + vm = virDomainObjListFindByID(driver->domains, id); vmwareDriverUnlock(driver); if (!vm) { diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index caa0170da..850faa3b1 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -578,7 +578,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) virDomainPtr ret = NULL; virDomainObjPtr dom; - dom = virDomainObjListFindByIDRef(privconn->driver->domains, id); + dom = virDomainObjListFindByID(privconn->driver->domains, id); if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); -- 2.13.6