[libvirt] [PATCH v2 0/4] vmware - Clean/fix test driver usage of FindBy{UUID|ID}
Patches 12-15 of the formerly larger series: https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html Changes since v1: * In patch 4, call virObjectLock after virDomainObjListRemove in vmwareDomainShutdownFlags and vmwareDomainUndefineFlags because both would return a reffed/locked @vm object and we need to relock before falling in the cleanup call to virDomainObjEndAPI John Ferlan (4): vmware: Properly clean up in vmwareDomainLookupByName vmware: Create accessors to virDomainObjListFindByUUID vmware: Add more descriptive error message on Find failure vmware: Use virDomainObjListFindBy{UUID|ID}Ref src/vmware/vmware_driver.c | 240 ++++++++++++++++----------------------------- 1 file changed, 84 insertions(+), 156 deletions(-) -- 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@redhat.com> --- 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
On Mon, Apr 02, 2018 at 09:16:37AM -0400, John Ferlan wrote:
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@redhat.com> --- src/vmware/vmware_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Rather than repeat code throughout, create and use a couple of accessors in order to lookup by UUID. Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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
On Mon, Apr 02, 2018 at 09:16:38AM -0400, John Ferlan wrote:
Rather than repeat code throughout, create and use a couple of accessors in order to lookup by UUID.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vmware/vmware_driver.c | 180 +++++++++++++++------------------------------ 1 file changed, 61 insertions(+), 119 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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@redhat.com> --- 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
On Mon, Apr 02, 2018 at 09:16:39AM -0400, John Ferlan wrote:
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@redhat.com> --- src/vmware/vmware_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
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@redhat.com> --- 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
On Mon, Apr 02, 2018 at 09:16:40AM -0400, John Ferlan wrote:
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@redhat.com> --- src/vmware/vmware_driver.c | 53 +++++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 34 deletions(-)
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
ping? This is less about vmware specifically and more about domain object manipulation coordination... Tks, John On 04/02/2018 09:16 AM, John Ferlan wrote:
Patches 12-15 of the formerly larger series:
https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
Changes since v1:
* In patch 4, call virObjectLock after virDomainObjListRemove in vmwareDomainShutdownFlags and vmwareDomainUndefineFlags because both would return a reffed/locked @vm object and we need to relock before falling in the cleanup call to virDomainObjEndAPI
John Ferlan (4): vmware: Properly clean up in vmwareDomainLookupByName vmware: Create accessors to virDomainObjListFindByUUID vmware: Add more descriptive error message on Find failure vmware: Use virDomainObjListFindBy{UUID|ID}Ref
src/vmware/vmware_driver.c | 240 ++++++++++++++++----------------------------- 1 file changed, 84 insertions(+), 156 deletions(-)
ping^2 Getting closer to being able to fix Add/Remove, but still need a few more baby steps... Tks, John On 04/11/2018 11:41 AM, John Ferlan wrote:
ping? This is less about vmware specifically and more about domain object manipulation coordination...
Tks,
John
On 04/02/2018 09:16 AM, John Ferlan wrote:
Patches 12-15 of the formerly larger series:
https://www.redhat.com/archives/libvir-list/2018-March/msg00489.html
Changes since v1:
* In patch 4, call virObjectLock after virDomainObjListRemove in vmwareDomainShutdownFlags and vmwareDomainUndefineFlags because both would return a reffed/locked @vm object and we need to relock before falling in the cleanup call to virDomainObjEndAPI
John Ferlan (4): vmware: Properly clean up in vmwareDomainLookupByName vmware: Create accessors to virDomainObjListFindByUUID vmware: Add more descriptive error message on Find failure vmware: Use virDomainObjListFindBy{UUID|ID}Ref
src/vmware/vmware_driver.c | 240 ++++++++++++++++----------------------------- 1 file changed, 84 insertions(+), 156 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Pavel Hrdina