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 8b487c4a7c..e1deb35201 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -928,8 +928,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 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 e1deb35201..6c81e5feaa 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; @@ -939,16 +918,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; @@ -962,16 +936,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; @@ -987,20 +956,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; @@ -1120,15 +1081,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; @@ -1158,15 +1112,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; @@ -1213,16 +1160,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

If vmwareDomainLookupByID or vmwareDomainLookupByName 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/vmware/vmware_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 6c81e5feaa..d17fdfe3be 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -841,7 +841,8 @@ vmwareDomainLookupByID(virConnectPtr conn, int id) vmwareDriverUnlock(driver); if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching id '%d'"), id); goto cleanup; } @@ -900,7 +901,8 @@ vmwareDomainLookupByName(virConnectPtr conn, const char *name) vmwareDriverUnlock(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

For vmwareDomObjFromDomainLocked and vmwareDomainLookupByID 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. For vmwareDomainUndefineFlags and vmwareDomainShutdownFlags since virDomainObjListRemove will return an unlocked object, we need to relock before making the EndAPI call. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/vmware/vmware_driver.c | 53 +++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index d17fdfe3be..643c85d4f8 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -67,7 +67,7 @@ vmwareDomObjFromDomainLocked(struct vmware_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, @@ -510,13 +510,12 @@ vmwareDomainShutdownFlags(virDomainPtr dom, if (!vm->persistent) { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmwareDriverUnlock(driver); return ret; } @@ -577,8 +576,7 @@ vmwareDomainSuspend(virDomainPtr dom) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -619,8 +617,7 @@ vmwareDomainResume(virDomainPtr dom) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -660,8 +657,7 @@ vmwareDomainReboot(virDomainPtr dom, unsigned int flags) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -772,8 +768,7 @@ vmwareDomainCreateWithFlags(virDomainPtr dom, ret = vmwareStartVM(driver, vm); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmwareDriverUnlock(driver); return ret; } @@ -811,14 +806,13 @@ vmwareDomainUndefineFlags(virDomainPtr dom, vm->persistent = 0; } else { virDomainObjListRemove(driver->domains, vm); - vm = NULL; + virObjectLock(vm); } ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); vmwareDriverUnlock(driver); return ret; } @@ -837,7 +831,7 @@ vmwareDomainLookupByID(virConnectPtr conn, int id) virDomainPtr dom = NULL; vmwareDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByIDRef(driver->domains, id); vmwareDriverUnlock(driver); if (!vm) { @@ -849,8 +843,7 @@ vmwareDomainLookupByID(virConnectPtr conn, int id) dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -866,8 +859,7 @@ vmwareDomainGetOSType(virDomainPtr dom) ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(vm->def->os.type))); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -884,8 +876,7 @@ vmwareDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -925,8 +916,7 @@ vmwareDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -943,8 +933,7 @@ vmwareDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -964,8 +953,7 @@ vmwareDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) ret = virDomainDefFormat(vm->def, driver->caps, virDomainDefFormatConvertXMLFlags(flags)); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1097,8 +1085,7 @@ vmwareDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1124,8 +1111,7 @@ vmwareDomainGetState(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1167,8 +1153,7 @@ vmwareDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = 0; - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } -- 2.13.6