[libvirt] [PATCH 00/20] Rework/rename virDomainObjListFindBy{UUID|ID}Ref
First, don't be scared off by 20 patches... They're not that difficult. Second, don't be scared off by the all the various code touched. Third, look at all the lines of code being removed! That's unlike my normal patch series! This is really the end game of what I started last year trying to make object access code to be "more common". The last and worse is/was the domain object code which really has a mish-mash of usage models and is (to say the least) confusing depending on the algorithm chosen (ByRef or not). This series focuses on the various virDomainObjListFindBy* APIs to do common things *and* allows the callers to use the common virDomainObjEndAPI when done with the object rather than needing to know whether they should virObjectUnlock and/or virObjectUnref. There were one consumer (libxl) that leaked domain objs because there was no Unref after a Ref on many API's. Additionally, there was a possibility of a deadlock because of a missed Unlock. This reduces a lot of code and reduces the number of virObjectUnlock consumers. Now *obviously* I don't have all these domain types available, but since the goal was to remove the FindBy*Ref APIs, I had to touch much more than I really want to. Of course only a couple are really active anyway and I'm hoping they'll take a peek at their modules (e.g. libxl, vz, and bhyve in particular). Once/if this can get accomplished, fixing the Add and Remove API's is the next task. Those are (to say the least) *really* confusing because the caller should never have to call virObjectRef afterwards, but some do because virDomainObjListAddLocked doesn't make enough references on the obj (places the object in 2 lists, but only accounts for 1). Lots of consumers that have to roll their own "adjustments" because of that. John Ferlan (20): bhyve: Use virDomainObjListFindBy{UUID|ID}Ref libxl: Fix possible deadlock in libxlDomainMigrateBegin3Params libxl: Properly cleanup after libxlDomObjFromDomain libxl: Use virDomainObjListFindBy{UUID|ID}Ref openvz: Cleanup indention openvz: Create accessors to virDomainObjListFindByUUID openvz: Add more descriptive error message on Find failure openvz: Use virDomainObjListFindBy{UUID|ID}Ref uml: Create accessors to virDomainObjListFindByUUID uml: Add more specific error message on failed FindBy call uml: Use virDomainObjListFindBy{UUID|ID}Ref vmware: Create accessors to virDomainObjListFindByUUID vmware: Add more descriptive error message on Find failure vmware: Properly clean up in vmwareDomainLookupByName vmware: Use virDomainObjListFindBy{UUID|ID}Ref vz: Unify vzDomObjFromDomain{Ref} vz: Use virDomainObjListFindBy{UUID|ID}Ref test: Use virDomainObjListFindByUUIDRef conf: Rework/rename virDomainObjListFindByUUIDRef conf: Rework/rename virDomainObjListFindByIDRef src/bhyve/bhyve_driver.c | 52 ++---- src/conf/virdomainobjlist.c | 66 ++------ src/conf/virdomainobjlist.h | 4 - src/libvirt_private.syms | 2 - src/libxl/libxl_domain.c | 2 +- src/libxl/libxl_driver.c | 99 ++++------- src/libxl/libxl_migration.c | 3 +- src/lxc/lxc_driver.c | 7 +- src/openvz/openvz_driver.c | 392 ++++++++++++++----------------------------- src/qemu/qemu_driver.c | 9 +- src/test/test_driver.c | 8 +- src/uml/uml_driver.c | 327 +++++++++++------------------------- src/util/virclosecallbacks.c | 4 +- src/vmware/vmware_driver.c | 234 +++++++++----------------- src/vz/vz_driver.c | 117 +++++++------ src/vz/vz_sdk.c | 13 +- src/vz/vz_utils.c | 34 +--- src/vz/vz_utils.h | 1 - 18 files changed, 458 insertions(+), 916 deletions(-) -- 2.13.6
For bhyveDomObjFromDomain, bhyveDomainLookupByUUID, and bhyveDomainLookupByID 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@redhat.com> --- src/bhyve/bhyve_driver.c | 58 ++++++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 849d3abcd..79963570c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -168,7 +168,7 @@ bhyveDomObjFromDomain(virDomainPtr domain) bhyveConnPtr privconn = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUID(privconn->domains, domain->uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -312,8 +312,7 @@ bhyveDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -338,8 +337,7 @@ bhyveDomainGetState(virDomainPtr domain, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -359,8 +357,7 @@ bhyveDomainGetAutostart(virDomainPtr domain, int *autostart) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -423,8 +420,7 @@ bhyveDomainSetAutostart(virDomainPtr domain, int autostart) cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -443,8 +439,7 @@ bhyveDomainIsActive(virDomainPtr domain) ret = virDomainObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -463,8 +458,7 @@ bhyveDomainIsPersistent(virDomainPtr domain) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -484,8 +478,7 @@ bhyveDomainGetOSType(virDomainPtr dom) goto cleanup; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -512,8 +505,7 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) virObjectUnref(caps); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -630,8 +622,7 @@ bhyveDomainUndefine(virDomainPtr domain) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -803,7 +794,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByUUID(privconn->domains, uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -819,8 +810,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -857,7 +847,7 @@ bhyveDomainLookupByID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByID(privconn->domains, id); + vm = virDomainObjListFindByIDRef(privconn->domains, id); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -871,8 +861,7 @@ bhyveDomainLookupByID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -913,8 +902,7 @@ bhyveDomainCreateWithFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_STARTED_BOOTED); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -1028,8 +1016,7 @@ bhyveDomainDestroy(virDomainPtr dom) } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -1056,8 +1043,7 @@ bhyveDomainShutdown(virDomainPtr dom) ret = virBhyveProcessShutdown(vm); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1100,8 +1086,7 @@ bhyveDomainOpenConsole(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1143,7 +1128,7 @@ bhyveDomainSetMetadata(virDomainPtr dom, cleanup: virObjectUnref(caps); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1165,7 +1150,7 @@ bhyveDomainGetMetadata(virDomainPtr dom, ret = virDomainObjGetMetadata(vm, type, uri, flags); cleanup: - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1548,8 +1533,7 @@ bhyveDomainHasManagedSaveImage(virDomainPtr domain, unsigned int flags) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } -- 2.13.6
On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
For bhyveDomObjFromDomain, bhyveDomainLookupByUUID, and bhyveDomainLookupByID 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@redhat.com> ---
src/bhyve/bhyve_driver.c | 58 ++++++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 37 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 849d3abcd..79963570c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -168,7 +168,7 @@ bhyveDomObjFromDomain(virDomainPtr domain) bhyveConnPtr privconn = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN];
- vm = virDomainObjListFindByUUID(privconn->domains, domain->uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -312,8 +312,7 @@ bhyveDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -338,8 +337,7 @@ bhyveDomainGetState(virDomainPtr domain, ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -359,8 +357,7 @@ bhyveDomainGetAutostart(virDomainPtr domain, int *autostart) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -423,8 +420,7 @@ bhyveDomainSetAutostart(virDomainPtr domain, int autostart) cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -443,8 +439,7 @@ bhyveDomainIsActive(virDomainPtr domain) ret = virDomainObjIsActive(obj);
cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; }
@@ -463,8 +458,7 @@ bhyveDomainIsPersistent(virDomainPtr domain) ret = obj->persistent;
cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; }
@@ -484,8 +478,7 @@ bhyveDomainGetOSType(virDomainPtr dom) goto cleanup;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -512,8 +505,7 @@ bhyveDomainGetXMLDesc(virDomainPtr domain, unsigned int flags)
virObjectUnref(caps); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -630,8 +622,7 @@ bhyveDomainUndefine(virDomainPtr domain) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -803,7 +794,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL;
- vm = virDomainObjListFindByUUID(privconn->domains, uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, uuid);
if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -819,8 +810,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; }
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 03/13/2018 03:44 AM, Marc Hartmayer wrote:
On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
For bhyveDomObjFromDomain, bhyveDomainLookupByUUID, and bhyveDomainLookupByID 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@redhat.com> ---
src/bhyve/bhyve_driver.c | 58 ++++++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 37 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 849d3abcd..79963570c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c
[...]
@@ -630,8 +622,7 @@ bhyveDomainUndefine(virDomainPtr domain) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -803,7 +794,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL;
- vm = virDomainObjListFindByUUID(privconn->domains, uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, uuid);
if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -819,8 +810,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; }
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.
Hmm... The only place where vm = NULL && !virDomainObjIsActive is during bhyveDomainUndefine... And yes, that one needs an Unref mainly because of how virDomainObjListRemove operates. (NB: You added a comment under bhyveDomainLookupByUUID which confused me at first). Long story short, for now any virDomainObjListRemove called in the error path of a virDomainObjListAdd would need to set vm = NULL, but other consumers would need to call virObjectUnref (e.g. bhyveDomainUndefine and bhyveDomainDestroy) since ListRemove returns w/ 1 ref and unlocked. Of course I have the "proper handling" of ListRemove in later patches in part of the series that hasn't been posted, so in my mind it's already there ;-)
But maybe I'm just missing something :)
Well sort of ;-) The other half of the series is virDomainObjListAdd will return the correct number of refs. Currently it returns an object with 2 refs and locked - it should be returning with 3 refs (1 for object alloc, 1 for insertion into doms->objs, and 1 for insertion into doms->objsName). Thus after getting a vm from *Add, one should also use the virDomainObjEndAPI to reduce the refcnt by 1. When virDomainObjListRemove is called it will reduce refcnt by 1 for each call to virHashRemoveEntry (e.g. by 2) and unlock before returning with 1 ref (which is essentially the proper way). Note that "some" drivers will return from Add *and* call virObjectRef which does effectively what "should" be done in Add which is where I'm trying to get. John I have added in my local branch a virObjectUnref(vm) for Undefine and Destroy after ListRemove is called and before vm = NULL;
[…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@redhat.com> wrote:
On 03/13/2018 03:44 AM, Marc Hartmayer wrote:
On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
For bhyveDomObjFromDomain, bhyveDomainLookupByUUID, and bhyveDomainLookupByID 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@redhat.com> ---
src/bhyve/bhyve_driver.c | 58 ++++++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 37 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 849d3abcd..79963570c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c
[...]
@@ -630,8 +622,7 @@ bhyveDomainUndefine(virDomainPtr domain) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -803,7 +794,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL;
- vm = virDomainObjListFindByUUID(privconn->domains, uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, uuid);
if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -819,8 +810,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; }
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.
Hmm... The only place where vm = NULL && !virDomainObjIsActive is during bhyveDomainUndefine... And yes, that one needs an Unref mainly because of how virDomainObjListRemove operates. (NB: You added a comment under bhyveDomainLookupByUUID which confused me at first).
Oh, sorry for that.
Long story short, for now any virDomainObjListRemove called in the error path of a virDomainObjListAdd would need to set vm = NULL, but other consumers would need to call virObjectUnref (e.g. bhyveDomainUndefine and bhyveDomainDestroy) since ListRemove returns w/ 1 ref and unlocked.
Of course I have the "proper handling" of ListRemove in later patches in part of the series that hasn't been posted, so in my mind it's already there ;-)
:)
But maybe I'm just missing something :)
Well sort of ;-) The other half of the series is virDomainObjListAdd will return the correct number of refs. Currently it returns an object with 2 refs and locked - it should be returning with 3 refs (1 for object alloc, 1 for insertion into doms->objs, and 1 for insertion into doms->objsName). Thus after getting a vm from *Add, one should also use the virDomainObjEndAPI to reduce the refcnt by 1.
When virDomainObjListRemove is called it will reduce refcnt by 1 for each call to virHashRemoveEntry (e.g. by 2) and unlock before returning with 1 ref (which is essentially the proper way).
Note that "some" drivers will return from Add *and* call virObjectRef which does effectively what "should" be done in Add which is where I'm trying to get.
Thanks for the detailed explanation!
John
I have added in my local branch a virObjectUnref(vm) for Undefine and Destroy after ListRemove is called and before vm = NULL;
Okay. And what about bhyveDomainCreateXML and bhyveDomainDefineXMLFlags if (!vm->persistent)?
[…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
-- 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 03/13/2018 12:44 PM, Marc Hartmayer wrote:
On Tue, Mar 13, 2018 at 01:39 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
On 03/13/2018 03:44 AM, Marc Hartmayer wrote:
On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
For bhyveDomObjFromDomain, bhyveDomainLookupByUUID, and bhyveDomainLookupByID 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@redhat.com> ---
src/bhyve/bhyve_driver.c | 58 ++++++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 37 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 849d3abcd..79963570c 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c
[...]
@@ -630,8 +622,7 @@ bhyveDomainUndefine(virDomainPtr domain) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) virObjectEventStateQueue(privconn->domainEventState, event); return ret; @@ -803,7 +794,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL;
- vm = virDomainObjListFindByUUID(privconn->domains, uuid); + vm = virDomainObjListFindByUUIDRef(privconn->domains, uuid);
if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -819,8 +810,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; }
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.
Hmm... The only place where vm = NULL && !virDomainObjIsActive is during bhyveDomainUndefine... And yes, that one needs an Unref mainly because of how virDomainObjListRemove operates. (NB: You added a comment under bhyveDomainLookupByUUID which confused me at first).
Oh, sorry for that.
Long story short, for now any virDomainObjListRemove called in the error path of a virDomainObjListAdd would need to set vm = NULL, but other consumers would need to call virObjectUnref (e.g. bhyveDomainUndefine and bhyveDomainDestroy) since ListRemove returns w/ 1 ref and unlocked.
Of course I have the "proper handling" of ListRemove in later patches in part of the series that hasn't been posted, so in my mind it's already there ;-)
:)
But maybe I'm just missing something :)
Well sort of ;-) The other half of the series is virDomainObjListAdd will return the correct number of refs. Currently it returns an object with 2 refs and locked - it should be returning with 3 refs (1 for object alloc, 1 for insertion into doms->objs, and 1 for insertion into doms->objsName). Thus after getting a vm from *Add, one should also use the virDomainObjEndAPI to reduce the refcnt by 1.
When virDomainObjListRemove is called it will reduce refcnt by 1 for each call to virHashRemoveEntry (e.g. by 2) and unlock before returning with 1 ref (which is essentially the proper way).
Note that "some" drivers will return from Add *and* call virObjectRef which does effectively what "should" be done in Add which is where I'm trying to get.
Thanks for the detailed explanation!
John
I have added in my local branch a virObjectUnref(vm) for Undefine and Destroy after ListRemove is called and before vm = NULL;
Okay. And what about bhyveDomainCreateXML and bhyveDomainDefineXMLFlags if (!vm->persistent)?
For CreateXML and DefineXML, virDomainObjListAdd returns @vm w/ 2 refs and virDomainObjListRemove will end up calling virObjectUnref twice as part of the virHashRemoveEntry call because virDomainObjListNew set up virObjectFreeHashData as the API to be called when an object is removed from the hash table. So for that case, no virObjectUnref is necessary because by the time we return @vm could already be free'd, but should have refcnt=0. John
[…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
-- 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@redhat.com> --- 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 09:47 AM, John Ferlan wrote:
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@redhat.com> --- 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) {
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
On 03/09/2018 03:33 PM, Jim Fehlig wrote:
On 03/09/2018 09:47 AM, John Ferlan wrote:
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@redhat.com> --- 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) {
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
No longer needed since this fix was folded into commit 64370c4b. Regards, Jim
Commit id '9ac945078' altered libxlDomObjFromDomain to return a locked *and* ref counted object for some specific purposes; however, it neglected to alter all the consumers of the helper to use virDomainObjEndAPI thus leaving many objects with extra ref counts. The two consumers for libxlDomainMigrationConfirm would also originally use the libxlDomObjFromDomain API (either from libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_driver.c | 86 ++++++++++++++++----------------------------- src/libxl/libxl_migration.c | 3 +- 2 files changed, 31 insertions(+), 58 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b5101626e..9aa4a293c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom) goto cleanup; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return type; } @@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom) ret = virDomainDefGetMemoryTotal(vm->def); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = vm->hasManagedSave; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) ret = virDomainDefGetVcpus(def); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, libxl_get_max_cpus(cfg->ctx), NULL); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, ret = maxinfo; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefFormatConvertXMLFlags(flags)); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom, cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) ignore_value(VIR_STRDUP(ret, name)); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom, } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom, VIR_FREE(nodeset); virBitmapFree(nodes); libxl_bitmap_dispose(&nodemap); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom) ret = vm->updated; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom, start_cpu, ncpus); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5897,19 +5872,19 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain, if (STREQ_NULLABLE(vm->def->name, "Domain-0")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain-0 cannot be migrated")); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; } if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; } if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; } @@ -6085,8 +6060,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -6174,7 +6148,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain, return -1; if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return -1; } diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index ccf2daed1..21476f7ac 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1401,8 +1401,7 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver, cleanup: if (event) libxlDomainEventQueue(driver, event); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } -- 2.13.6
On 03/09/2018 09:47 AM, John Ferlan wrote:
Commit id '9ac945078' altered libxlDomObjFromDomain to return a locked *and* ref counted object for some specific purposes; however, it neglected to alter all the consumers of the helper to use virDomainObjEndAPI thus leaving many objects with extra ref counts.
The two consumers for libxlDomainMigrationConfirm would also originally use the libxlDomObjFromDomain API (either from libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_driver.c | 86 ++++++++++++++++----------------------------- src/libxl/libxl_migration.c | 3 +- 2 files changed, 31 insertions(+), 58 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b5101626e..9aa4a293c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) }
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) }
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom) goto cleanup;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return type; }
@@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom) ret = virDomainDefGetMemoryTotal(vm->def);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom, ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = vm->hasManagedSave;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags)
cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) ret = virDomainDefGetVcpus(def);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, libxl_get_max_cpus(cfg->ctx), NULL);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, ret = maxinfo;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefFormatConvertXMLFlags(flags));
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom,
cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart) ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) ignore_value(VIR_STRDUP(ret, name));
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom, ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom, }
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom, VIR_FREE(nodeset); virBitmapFree(nodes); libxl_bitmap_dispose(&nodemap); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj);
cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; }
@@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom) ret = obj->persistent;
cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; }
@@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom) ret = vm->updated;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom, start_cpu, ncpus);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom, ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom, ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
For these changes Reviewed-by: Jim Fehlig <jfehlig@suse.com> However the migration APIs need some work.
@@ -5897,19 +5872,19 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain, if (STREQ_NULLABLE(vm->def->name, "Domain-0")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain-0 cannot be migrated")); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; }
if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; }
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; }
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
@@ -6085,8 +6060,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom, ret = 0;
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
@@ -6174,7 +6148,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain, return -1;
if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return -1; }
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index ccf2daed1..21476f7ac 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1401,8 +1401,7 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver, cleanup: if (event) libxlDomainEventQueue(driver, event); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; }
On 03/11/2018 10:13 PM, Jim Fehlig wrote:
On 03/09/2018 09:47 AM, John Ferlan wrote:
Commit id '9ac945078' altered libxlDomObjFromDomain to return a locked *and* ref counted object for some specific purposes; however, it neglected to alter all the consumers of the helper to use virDomainObjEndAPI thus leaving many objects with extra ref counts.
The two consumers for libxlDomainMigrationConfirm would also originally use the libxlDomObjFromDomain API (either from libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_driver.c | 86 ++++++++++++++++----------------------------- src/libxl/libxl_migration.c | 3 +- 2 files changed, 31 insertions(+), 58 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b5101626e..9aa4a293c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c
[...]
For these changes
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Tks -
However the migration APIs need some work.
@@ -5897,19 +5872,19 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain, if (STREQ_NULLABLE(vm->def->name, "Domain-0")) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain-0 cannot be migrated")); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; } if (virDomainMigrateBegin3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; } if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return NULL; }
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
Hmm... right, I noticed that the migration API's were treating @vm a bit oddly. I looked backwards through the call stack figuring I'd got things right, but certainly having libxlDomainMigrationBegin do that EndAPI is not good! I figure getting all these patches reviewed could take some time, so if you want to clean up the libxlDomainMigration paths as you note that'd be fine. If it becomes problematic trying to work thru things without pushing what's R-B'd so far in libxl, I can do so. I can also merge in what you can get done into my changes. Whatever works best. Tks - John
@@ -6085,8 +6060,7 @@ libxlDomainMigratePerform3Params(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -6174,7 +6148,7 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain, return -1; if (virDomainMigrateConfirm3ParamsEnsureACL(domain->conn, vm->def) < 0) { - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return -1; } diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index ccf2daed1..21476f7ac 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -1401,8 +1401,7 @@ libxlDomainMigrationConfirm(libxlDriverPrivatePtr driver, cleanup: if (event) libxlDomainEventQueue(driver, event); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; }
On 03/11/2018 08:13 PM, Jim Fehlig wrote:
On 03/09/2018 09:47 AM, John Ferlan wrote:
Commit id '9ac945078' altered libxlDomObjFromDomain to return a locked *and* ref counted object for some specific purposes; however, it neglected to alter all the consumers of the helper to use virDomainObjEndAPI thus leaving many objects with extra ref counts.
The two consumers for libxlDomainMigrationConfirm would also originally use the libxlDomObjFromDomain API (either from libxlDomainMigrateConfirm3Params or libxlDoMigrateP2P via libxlDomainMigrationPerformP2P and libxlDomainMigratePerform3Params.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_driver.c | 86 ++++++++++++++++----------------------------- src/libxl/libxl_migration.c | 3 +- 2 files changed, 31 insertions(+), 58 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b5101626e..9aa4a293c 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1322,8 +1322,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1373,8 +1372,7 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1459,8 +1457,7 @@ libxlDomainGetOSType(virDomainPtr dom) goto cleanup; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return type; } @@ -1479,8 +1476,7 @@ libxlDomainGetMaxMemory(virDomainPtr dom) ret = virDomainDefGetMemoryTotal(vm->def); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1658,8 +1654,7 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -1685,8 +1680,7 @@ libxlDomainGetState(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2110,8 +2104,7 @@ libxlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = vm->hasManagedSave; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2140,8 +2133,7 @@ libxlDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2352,8 +2344,7 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) ret = virDomainDefGetVcpus(def); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2484,8 +2475,7 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps, libxl_get_max_cpus(cfg->ctx), NULL); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2548,8 +2538,7 @@ libxlDomainGetVcpus(virDomainPtr dom, virVcpuInfoPtr info, int maxinfo, ret = maxinfo; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2580,8 +2569,7 @@ libxlDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) virDomainDefFormatConvertXMLFlags(flags)); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -2901,8 +2889,7 @@ libxlDomainUndefineFlags(virDomainPtr dom, cleanup: VIR_FREE(name); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) libxlDomainEventQueue(driver, event); virObjectUnref(cfg); @@ -4261,8 +4248,7 @@ libxlDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4394,8 +4380,7 @@ libxlDomainGetAutostart(virDomainPtr dom, int *autostart) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -4521,8 +4506,7 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) ignore_value(VIR_STRDUP(ret, name)); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4587,8 +4571,7 @@ libxlDomainGetSchedulerParametersFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4750,8 +4733,7 @@ libxlDomainOpenConsole(virDomainPtr dom, } cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -4886,8 +4868,7 @@ libxlDomainGetNumaParameters(virDomainPtr dom, VIR_FREE(nodeset); virBitmapFree(nodes); libxl_bitmap_dispose(&nodemap); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); virObjectUnref(cfg); return ret; } @@ -4908,8 +4889,7 @@ libxlDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -4928,8 +4908,7 @@ libxlDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -4948,8 +4927,7 @@ libxlDomainIsUpdated(virDomainPtr dom) ret = vm->updated; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5107,8 +5085,7 @@ libxlDomainGetCPUStats(virDomainPtr dom, start_cpu, ncpus); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5211,8 +5188,7 @@ libxlDomainGetJobInfo(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -5263,8 +5239,7 @@ libxlDomainGetJobStats(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; }
For these changes
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
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
For libxlDomainLookupByID and libxlDomainLookupByUUID 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@redhat.com> --- src/libxl/libxl_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 9aa4a293c..e78fe2d4b 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 = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByIDRef(driver->domains, id); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; @@ -1102,8 +1102,7 @@ libxlDomainLookupByID(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; } @@ -1114,7 +1113,7 @@ libxlDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByUUID(driver->domains, uuid); + vm = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; @@ -1126,8 +1125,7 @@ libxlDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) 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 03/09/2018 09:48 AM, John Ferlan wrote:
For libxlDomainLookupByID and libxlDomainLookupByUUID 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@redhat.com> --- src/libxl/libxl_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 9aa4a293c..e78fe2d4b 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 = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByIDRef(driver->domains, id); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; @@ -1102,8 +1102,7 @@ libxlDomainLookupByID(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; }
@@ -1114,7 +1113,7 @@ libxlDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainObjPtr vm; virDomainPtr dom = NULL;
- vm = virDomainObjListFindByUUID(driver->domains, uuid); + vm = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; @@ -1126,8 +1125,7 @@ libxlDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; }
On 03/11/2018 08:14 PM, Jim Fehlig wrote:
On 03/09/2018 09:48 AM, John Ferlan wrote:
For libxlDomainLookupByID and libxlDomainLookupByUUID 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@redhat.com> --- src/libxl/libxl_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
I pushed this one as well. Regards, Jim
Some of the indents were only 2 spaces, make consistent w/ 4 spaces Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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 Fri, Mar 09, 2018 at 05:48 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
Some of the indents were only 2 spaces, make consistent w/ 4 spaces
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Small nits: You’re missing some indentation cleanups for openvzDomainSetNetwork, openvzDomainDefineXMLFlags, and openvzDomainUpdateDeviceFlags. -- 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 03/13/2018 03:58 AM, Marc Hartmayer wrote:
On Fri, Mar 09, 2018 at 05:48 PM +0100, John Ferlan <jferlan@redhat.com> wrote:
Some of the indents were only 2 spaces, make consistent w/ 4 spaces
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Small nits:
You’re missing some indentation cleanups for openvzDomainSetNetwork, openvzDomainDefineXMLFlags, and openvzDomainUpdateDeviceFlags.
No FindByUUID calls there, so I wasn't looking that close ;-)... Fixed up ones in those functions in my branch. Tks - John
-- 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
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/openvz/openvz_driver.c | 266 +++++++++++++-------------------------------- 1 file changed, 76 insertions(+), 190 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index a211c370e..167ba2f7a 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -87,6 +87,39 @@ static void openvzDriverUnlock(struct openvz_driver *driver) struct openvz_driver ovz_driver; + +static virDomainObjPtr +openvzDomObjFromDomainLocked(struct openvz_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 +openvzDomObjFromDomain(struct openvz_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + + openvzDriverLock(driver); + vm = openvzDomObjFromDomainLocked(driver, uuid); + openvzDriverUnlock(driver); + return vm; +} + + static int openvzDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -281,15 +314,8 @@ openvzDomainGetHostname(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virCheckFlags(0, NULL); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL; hostname = openvzVEGetStringParam(dom, "hostname"); if (hostname == NULL) @@ -359,18 +385,11 @@ static char *openvzDomainGetOSType(virDomainPtr dom) virDomainObjPtr vm; char *ret = NULL; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL; ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(vm->def->os.type))); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -384,18 +403,11 @@ static virDomainPtr openvzDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, uuid))) + return NULL; dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - cleanup: if (vm) virObjectUnlock(vm); return dom; @@ -432,15 +444,8 @@ static int openvzDomainGetInfo(virDomainPtr dom, int state; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzGetVEStatus(vm, &state, NULL) == -1) goto cleanup; @@ -480,19 +485,11 @@ openvzDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; ret = openvzGetVEStatus(vm, state, reason); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -505,16 +502,11 @@ static int openvzDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = virDomainObjIsActive(obj); - cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -527,16 +519,11 @@ static int openvzDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = obj->persistent; - cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -554,20 +541,12 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { /* Flags checked by virDomainDefFormat */ - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL; ret = virDomainDefFormat(vm->def, driver->caps, virDomainDefFormatConvertXMLFlags(flags)); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -600,15 +579,8 @@ static int openvzDomainSuspend(virDomainPtr dom) const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--suspend", NULL}; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -638,15 +610,8 @@ static int openvzDomainResume(virDomainPtr dom) const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--resume", NULL}; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -681,15 +646,8 @@ openvzDomainShutdownFlags(virDomainPtr dom, virCheckFlags(0, -1); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -744,15 +702,8 @@ static int openvzDomainReboot(virDomainPtr dom, virCheckFlags(0, -1); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -1210,12 +1161,8 @@ openvzDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1); openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -1255,15 +1202,8 @@ openvzDomainSetAutostart(virDomainPtr dom, int autostart) "--save", NULL }; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; openvzSetProgramSentinal(prog, vm->def->name); if (virRun(prog, NULL) < 0) @@ -1284,15 +1224,8 @@ openvzDomainGetAutostart(virDomainPtr dom, int *autostart) char *value = NULL; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzReadVPSConfigParam(strtoI(vm->def->name), "ONBOOT", &value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1383,15 +1316,8 @@ static int openvzDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, return -1; } - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (nvcpus <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1987,17 +1913,8 @@ openvzDomainInterfaceStats(virDomainPtr dom, virDomainNetDefPtr net = NULL; int ret = -1; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2082,13 +1999,8 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1); openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virStrToLong_i(vm->def->name, NULL, 10, &veid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2230,15 +2142,8 @@ openvzDomainMigrateBegin3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, OPENVZ_MIGRATION_PARAMETERS) < 0) return NULL; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) + return NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2394,15 +2299,8 @@ openvzDomainMigratePerform3Params(virDomainPtr domain, &uri_str) < 0) goto cleanup; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) goto cleanup; - } /* parse dst host:port from uri */ uri = virURIParse(uri_str); @@ -2504,15 +2402,8 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, OPENVZ_MIGRATION_PARAMETERS) < 0) goto cleanup; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) goto cleanup; - } if (cancelled) { if (openvzGetVEStatus(vm, &status, NULL) == -1) @@ -2552,16 +2443,11 @@ openvzDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) virCheckFlags(0, -1); - openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = 0; - cleanup: if (obj) virObjectUnlock(obj); return ret; -- 2.13.6
On 03/09/2018 09:48 AM, 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/openvz/openvz_driver.c | 266 +++++++++++++-------------------------------- 1 file changed, 76 insertions(+), 190 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index a211c370e..167ba2f7a 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -87,6 +87,39 @@ static void openvzDriverUnlock(struct openvz_driver *driver)
struct openvz_driver ovz_driver;
+ +static virDomainObjPtr +openvzDomObjFromDomainLocked(struct openvz_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 +openvzDomObjFromDomain(struct openvz_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + + openvzDriverLock(driver); + vm = openvzDomObjFromDomainLocked(driver, uuid); + openvzDriverUnlock(driver); + return vm; +} + + static int openvzDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -281,15 +314,8 @@ openvzDomainGetHostname(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm;
virCheckFlags(0, NULL); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL;
It looks like the cleanup label can be removed from this function too.
hostname = openvzVEGetStringParam(dom, "hostname"); if (hostname == NULL) @@ -359,18 +385,11 @@ static char *openvzDomainGetOSType(virDomainPtr dom) virDomainObjPtr vm; char *ret = NULL;
- openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL;
ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(vm->def->os.type)));
- cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -384,18 +403,11 @@ static virDomainPtr openvzDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL;
- openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, uuid))) + return NULL;
dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id);
- cleanup: if (vm) virObjectUnlock(vm); return dom; @@ -432,15 +444,8 @@ static int openvzDomainGetInfo(virDomainPtr dom, int state; int ret = -1;
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (openvzGetVEStatus(vm, &state, NULL) == -1) goto cleanup; @@ -480,19 +485,11 @@ openvzDomainGetState(virDomainPtr dom,
virCheckFlags(0, -1);
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
ret = openvzGetVEStatus(vm, state, reason);
- cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -505,16 +502,11 @@ static int openvzDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1;
- openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = virDomainObjIsActive(obj);
- cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -527,16 +519,11 @@ static int openvzDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1;
- openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = obj->persistent;
- cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -554,20 +541,12 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) {
/* Flags checked by virDomainDefFormat */
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL;
ret = virDomainDefFormat(vm->def, driver->caps, virDomainDefFormatConvertXMLFlags(flags));
- cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -600,15 +579,8 @@ static int openvzDomainSuspend(virDomainPtr dom) const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--suspend", NULL}; int ret = -1;
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -638,15 +610,8 @@ static int openvzDomainResume(virDomainPtr dom) const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--resume", NULL}; int ret = -1;
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -681,15 +646,8 @@ openvzDomainShutdownFlags(virDomainPtr dom,
virCheckFlags(0, -1);
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -744,15 +702,8 @@ static int openvzDomainReboot(virDomainPtr dom,
virCheckFlags(0, -1);
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -1210,12 +1161,8 @@ openvzDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1);
openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - }
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?
if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -1255,15 +1202,8 @@ openvzDomainSetAutostart(virDomainPtr dom, int autostart) "--save", NULL }; int ret = -1;
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
openvzSetProgramSentinal(prog, vm->def->name); if (virRun(prog, NULL) < 0) @@ -1284,15 +1224,8 @@ openvzDomainGetAutostart(virDomainPtr dom, int *autostart) char *value = NULL; int ret = -1;
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (openvzReadVPSConfigParam(strtoI(vm->def->name), "ONBOOT", &value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1383,15 +1316,8 @@ static int openvzDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, return -1; }
- 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (nvcpus <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1987,17 +1913,8 @@ openvzDomainInterfaceStats(virDomainPtr dom, virDomainNetDefPtr net = NULL; int ret = -1;
- openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) + return -1;
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2082,13 +1999,8 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1);
openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - }
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@suse.com> Regards, Jim
if (virStrToLong_i(vm->def->name, NULL, 10, &veid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2230,15 +2142,8 @@ openvzDomainMigrateBegin3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, OPENVZ_MIGRATION_PARAMETERS) < 0) return NULL;
- openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) + return NULL;
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2394,15 +2299,8 @@ openvzDomainMigratePerform3Params(virDomainPtr domain, &uri_str) < 0) goto cleanup;
- openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) goto cleanup; - }
/* parse dst host:port from uri */ uri = virURIParse(uri_str); @@ -2504,15 +2402,8 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, OPENVZ_MIGRATION_PARAMETERS) < 0) goto cleanup;
- openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) goto cleanup; - }
if (cancelled) { if (openvzGetVEStatus(vm, &status, NULL) == -1) @@ -2552,16 +2443,11 @@ openvzDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags)
virCheckFlags(0, -1);
- openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = 0;
- cleanup: if (obj) virObjectUnlock(obj); return ret;
On 03/29/2018 01:36 PM, Jim Fehlig wrote:
On 03/09/2018 09:48 AM, 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/openvz/openvz_driver.c | 266 +++++++++++++-------------------------------- 1 file changed, 76 insertions(+), 190 deletions(-)
diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index a211c370e..167ba2f7a 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -87,6 +87,39 @@ static void openvzDriverUnlock(struct openvz_driver *driver) struct openvz_driver ovz_driver; + +static virDomainObjPtr +openvzDomObjFromDomainLocked(struct openvz_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 +openvzDomObjFromDomain(struct openvz_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + + openvzDriverLock(driver); + vm = openvzDomObjFromDomainLocked(driver, uuid); + openvzDriverUnlock(driver); + return vm; +} + + static int openvzDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -281,15 +314,8 @@ openvzDomainGetHostname(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virCheckFlags(0, NULL); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL;
It looks like the cleanup label can be removed from this function too.
True, but it's not "as clean" as others since error does the goto clean thing. The others where the cleanup: label disappears is because there was only one goto cleanup... and the "if (vm)" would be useless too, but that's handled in a couple of patches anyway. In any case, I'll add an adjustment with my eventual followup series that alters virDomainObjAdd processing.
hostname = openvzVEGetStringParam(dom, "hostname"); if (hostname == NULL) @@ -359,18 +385,11 @@ static char *openvzDomainGetOSType(virDomainPtr dom) virDomainObjPtr vm; char *ret = NULL; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL; ignore_value(VIR_STRDUP(ret, virDomainOSTypeToString(vm->def->os.type))); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -384,18 +403,11 @@ static virDomainPtr openvzDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, uuid))) + return NULL; dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); - cleanup: if (vm) virObjectUnlock(vm); return dom; @@ -432,15 +444,8 @@ static int openvzDomainGetInfo(virDomainPtr dom, int state; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzGetVEStatus(vm, &state, NULL) == -1) goto cleanup; @@ -480,19 +485,11 @@ openvzDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; ret = openvzGetVEStatus(vm, state, reason); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -505,16 +502,11 @@ static int openvzDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = virDomainObjIsActive(obj); - cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -527,16 +519,11 @@ static int openvzDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = obj->persistent; - cleanup: if (obj) virObjectUnlock(obj); return ret; @@ -554,20 +541,12 @@ static char *openvzDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { /* Flags checked by virDomainDefFormat */ - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return NULL; ret = virDomainDefFormat(vm->def, driver->caps, virDomainDefFormatConvertXMLFlags(flags)); - cleanup: if (vm) virObjectUnlock(vm); return ret; @@ -600,15 +579,8 @@ static int openvzDomainSuspend(virDomainPtr dom) const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--suspend", NULL}; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -638,15 +610,8 @@ static int openvzDomainResume(virDomainPtr dom) const char *prog[] = {VZCTL, "--quiet", "chkpnt", PROGRAM_SENTINEL, "--resume", NULL}; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -681,15 +646,8 @@ openvzDomainShutdownFlags(virDomainPtr dom, virCheckFlags(0, -1); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -744,15 +702,8 @@ static int openvzDomainReboot(virDomainPtr dom, virCheckFlags(0, -1); - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -1210,12 +1161,8 @@ openvzDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1); openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - }
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?
My assumption, old driver, no one updated it once the domainobjlist code became self locking. I will add it to a future adjustment when I have to mess with the Add code Tks - John
if (openvzGetVEStatus(vm, &status, NULL) == -1) goto cleanup; @@ -1255,15 +1202,8 @@ openvzDomainSetAutostart(virDomainPtr dom, int autostart) "--save", NULL }; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; openvzSetProgramSentinal(prog, vm->def->name); if (virRun(prog, NULL) < 0) @@ -1284,15 +1224,8 @@ openvzDomainGetAutostart(virDomainPtr dom, int *autostart) char *value = NULL; int ret = -1; - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (openvzReadVPSConfigParam(strtoI(vm->def->name), "ONBOOT", &value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1383,15 +1316,8 @@ static int openvzDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, return -1; } - 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 = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (nvcpus <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -1987,17 +1913,8 @@ openvzDomainInterfaceStats(virDomainPtr dom, virDomainNetDefPtr net = NULL; int ret = -1; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2082,13 +1999,8 @@ openvzDomainUpdateDeviceFlags(virDomainPtr dom, const char *xml, VIR_DOMAIN_DEVICE_MODIFY_CONFIG, -1); openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - }
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@suse.com>
Regards, Jim
if (virStrToLong_i(vm->def->name, NULL, 10, &veid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -2230,15 +2142,8 @@ openvzDomainMigrateBegin3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, OPENVZ_MIGRATION_PARAMETERS) < 0) return NULL; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) + return NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -2394,15 +2299,8 @@ openvzDomainMigratePerform3Params(virDomainPtr domain, &uri_str) < 0) goto cleanup; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) goto cleanup; - } /* parse dst host:port from uri */ uri = virURIParse(uri_str); @@ -2504,15 +2402,8 @@ openvzDomainMigrateConfirm3Params(virDomainPtr domain, if (virTypedParamsValidate(params, nparams, OPENVZ_MIGRATION_PARAMETERS) < 0) goto cleanup; - openvzDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); - openvzDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = openvzDomObjFromDomain(driver, domain->uuid))) goto cleanup; - } if (cancelled) { if (openvzGetVEStatus(vm, &status, NULL) == -1) @@ -2552,16 +2443,11 @@ openvzDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) virCheckFlags(0, -1); - openvzDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - openvzDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = openvzDomObjFromDomain(driver, dom->uuid))) + return -1; + ret = 0; - cleanup: if (obj) virObjectUnlock(obj); return ret;
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@redhat.com> --- 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
On 03/09/2018 09:48 AM, John Ferlan wrote:
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@redhat.com> --- src/openvz/openvz_driver.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
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; }
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@redhat.com> --- 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/09/2018 09:48 AM, John Ferlan wrote:
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@redhat.com> --- src/openvz/openvz_driver.c | 76 ++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 50 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
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; }
On 03/29/2018 01:52 PM, Jim Fehlig wrote:
On 03/09/2018 09:48 AM, John Ferlan wrote:
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@redhat.com> --- src/openvz/openvz_driver.c | 76 ++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 50 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Regards, Jim
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
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; }
Rather than repeat code throughout, create and use a couple of accessors in order to lookup by UUID. This will also generate a common error message including the failed uuidstr for lookup rather than just returning nothing in some instances. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/uml/uml_driver.c | 244 +++++++++++++++------------------------------------ 1 file changed, 70 insertions(+), 174 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index ab7fa7f27..ea8fca38b 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -163,6 +163,39 @@ umlVMDriverUnlock(void) umlDriverUnlock(uml_driver); } + +static virDomainObjPtr +umlDomObjFromDomainLocked(struct uml_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 +umlDomObjFromDomain(struct uml_driver *driver, + const unsigned char *uuid) +{ + virDomainObjPtr vm; + + umlDriverLock(driver); + vm = umlDomObjFromDomainLocked(driver, uuid); + umlDriverUnlock(driver); + return vm; +} + + static virNWFilterCallbackDriver umlCallbackDriver = { .name = "UML", .vmFilterRebuild = umlVMFilterRebuild, @@ -1368,20 +1401,14 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, } static virDomainPtr umlDomainLookupByUUID(virConnectPtr conn, - const unsigned char *uuid) + const unsigned char *uuid) { struct uml_driver *driver = (struct uml_driver *)conn->privateData; virDomainObjPtr vm; virDomainPtr dom = NULL; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, uuid))) + return NULL; if (virDomainLookupByUUIDEnsureACL(conn, vm->def) < 0) goto cleanup; @@ -1427,13 +1454,8 @@ static int umlDomainIsActive(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - umlDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainIsActiveEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -1453,13 +1475,8 @@ static int umlDomainIsPersistent(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - umlDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainIsPersistentEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -1478,13 +1495,8 @@ static int umlDomainIsUpdated(virDomainPtr dom) virDomainObjPtr obj; int ret = -1; - umlDriverLock(driver); - obj = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!obj) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(obj = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainIsUpdatedEnsureACL(dom->conn, obj->def) < 0) goto cleanup; @@ -1637,14 +1649,8 @@ static int umlDomainShutdownFlags(virDomainPtr dom, virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching id %d"), dom->id); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -1683,12 +1689,8 @@ umlDomainDestroyFlags(virDomainPtr dom, virCheckFlags(0, -1); umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching id %d"), dom->id); - goto cleanup; - } + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) + return -1; if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1726,14 +1728,8 @@ static char *umlDomainGetOSType(virDomainPtr dom) { virDomainObjPtr vm; char *type = NULL; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return NULL; if (virDomainGetOSTypeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1755,18 +1751,8 @@ umlDomainGetMaxMemory(virDomainPtr dom) virDomainObjPtr vm; unsigned long long ret = 0; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainGetMaxMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1785,18 +1771,8 @@ static int umlDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) virDomainObjPtr vm; int ret = -1; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainSetMaxMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1822,18 +1798,8 @@ static int umlDomainSetMemory(virDomainPtr dom, unsigned long newmem) virDomainObjPtr vm; int ret = -1; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(dom->uuid, uuidstr); - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainSetMemoryEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1866,15 +1832,8 @@ static int umlDomainGetInfo(virDomainPtr dom, virDomainObjPtr vm; int ret = -1; - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1915,15 +1874,8 @@ umlDomainGetState(virDomainPtr dom, virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainGetStateEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -1948,14 +1900,8 @@ static char *umlDomainGetXMLDesc(virDomainPtr dom, /* Flags checked by virDomainDefFormat */ umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainGetXMLDescEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; @@ -2015,13 +1961,8 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) virNWFilterReadLockFilterUpdates(); umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainCreateWithFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2115,12 +2056,8 @@ static int umlDomainUndefineFlags(virDomainPtr dom, virCheckFlags(0, -1); umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainUndefineFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2213,14 +2150,8 @@ static int umlDomainAttachDevice(virDomainPtr dom, const char *xml) umlDriverLock(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 = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainAttachDeviceEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2333,14 +2264,8 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml) int ret = -1; umlDriverLock(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 = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainDetachDeviceEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2403,13 +2328,8 @@ static int umlDomainGetAutostart(virDomainPtr dom, int ret = -1; umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainGetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2433,13 +2353,8 @@ static int umlDomainSetAutostart(virDomainPtr dom, int ret = -1; umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainSetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2509,15 +2424,8 @@ umlDomainBlockPeek(virDomainPtr dom, virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, "%s", - _("no domain with matching uuid")); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainBlockPeekEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2573,7 +2481,6 @@ umlDomainOpenConsole(virDomainPtr dom, { struct uml_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; int ret = -1; virDomainChrDefPtr chr = NULL; size_t i; @@ -2581,13 +2488,8 @@ umlDomainOpenConsole(virDomainPtr dom, virCheckFlags(0, -1); umlDriverLock(driver); - virUUIDFormat(dom->uuid, uuidstr); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, - _("no domain with matching uuid '%s'"), uuidstr); + if (!(vm = umlDomObjFromDomainLocked(driver, dom->uuid))) goto cleanup; - } if (virDomainOpenConsoleEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -2924,14 +2826,8 @@ umlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) virCheckFlags(0, -1); - umlDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); - umlDriverUnlock(driver); - - if (!vm) { - virReportError(VIR_ERR_NO_DOMAIN, NULL); - goto cleanup; - } + if (!(vm = umlDomObjFromDomain(driver, dom->uuid))) + return -1; if (virDomainHasManagedSaveImageEnsureACL(dom->conn, vm->def) < 0) goto cleanup; -- 2.13.6
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@redhat.com> --- 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
For umlDomObjFromDomainLocked and umlDomainLookupByID 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@redhat.com> --- src/uml/uml_driver.c | 81 ++++++++++++++++++---------------------------------- 1 file changed, 28 insertions(+), 53 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index d10a9ba62..41c607e66 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -171,7 +171,7 @@ umlDomObjFromDomainLocked(struct uml_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, @@ -767,8 +767,7 @@ static int umlProcessAutoDestroyDom(void *payload, return 0; } - if (!(dom = virDomainObjListFindByUUID(data->driver->domains, - uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(data->driver->domains, uuid))) { VIR_DEBUG("No domain object to kill"); return 0; } @@ -780,11 +779,10 @@ static int umlProcessAutoDestroyDom(void *payload, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - if (dom && !dom->persistent) + if (!dom->persistent) virDomainObjListRemove(data->driver->domains, dom); - if (dom) - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); if (event) umlDomainEventQueue(data->driver, event); virHashRemoveEntry(data->driver->autodestroy, uuidstr); @@ -1381,7 +1379,7 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, virDomainPtr dom = NULL; umlDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); + vm = virDomainObjListFindByIDRef(driver->domains, id); umlDriverUnlock(driver); if (!vm) { @@ -1396,8 +1394,7 @@ static virDomainPtr umlDomainLookupByID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -1417,8 +1414,7 @@ static virDomainPtr umlDomainLookupByUUID(virConnectPtr conn, dom = virGetDomain(conn, vm->def->name, vm->def->uuid, vm->def->id); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return dom; } @@ -1465,8 +1461,7 @@ static int umlDomainIsActive(virDomainPtr dom) ret = virDomainObjIsActive(obj); cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1486,8 +1481,7 @@ static int umlDomainIsPersistent(virDomainPtr dom) ret = obj->persistent; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1506,8 +1500,7 @@ static int umlDomainIsUpdated(virDomainPtr dom) ret = obj->updated; cleanup: - if (obj) - virObjectUnlock(obj); + virDomainObjEndAPI(&obj); return ret; } @@ -1668,8 +1661,7 @@ static int umlDomainShutdownFlags(virDomainPtr dom, cleanup: VIR_FREE(info); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1710,8 +1702,7 @@ umlDomainDestroyFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); @@ -1740,8 +1731,7 @@ static char *umlDomainGetOSType(virDomainPtr dom) { goto cleanup; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return type; } @@ -1762,8 +1752,7 @@ umlDomainGetMaxMemory(virDomainPtr dom) ret = virDomainDefGetMemoryTotal(vm->def); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1789,8 +1778,7 @@ static int umlDomainSetMaxMemory(virDomainPtr dom, unsigned long newmax) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1822,8 +1810,7 @@ static int umlDomainSetMemory(virDomainPtr dom, unsigned long newmem) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1858,8 +1845,7 @@ static int umlDomainGetInfo(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1886,8 +1872,7 @@ umlDomainGetState(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1913,8 +1898,7 @@ static char *umlDomainGetXMLDesc(virDomainPtr dom, virDomainDefFormatConvertXMLFlags(flags)); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -1978,8 +1962,7 @@ static int umlDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) VIR_DOMAIN_EVENT_STARTED_BOOTED); cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); if (event) umlDomainEventQueue(driver, event); umlDriverUnlock(driver); @@ -2083,8 +2066,7 @@ static int umlDomainUndefineFlags(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2190,8 +2172,7 @@ static int umlDomainAttachDevice(virDomainPtr dom, const char *xml) cleanup: virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2298,8 +2279,7 @@ static int umlDomainDetachDevice(virDomainPtr dom, const char *xml) cleanup: virDomainDeviceDefFree(dev); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2340,8 +2320,7 @@ static int umlDomainGetAutostart(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2405,8 +2384,7 @@ static int umlDomainSetAutostart(virDomainPtr dom, cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2469,8 +2447,7 @@ umlDomainBlockPeek(virDomainPtr dom, cleanup: VIR_FORCE_CLOSE(fd); - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } @@ -2537,8 +2514,7 @@ umlDomainOpenConsole(virDomainPtr dom, ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); umlDriverUnlock(driver); return ret; } @@ -2837,8 +2813,7 @@ umlDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) ret = 0; cleanup: - if (vm) - virObjectUnlock(vm); + virDomainObjEndAPI(&vm); return ret; } -- 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@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 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
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 121751e27..9cd0bc438 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
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 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
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. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vmware/vmware_driver.c | 49 ++++++++++++++++------------------------------ 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index d17fdfe3b..37a7b1932 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, @@ -515,8 +515,7 @@ vmwareDomainShutdownFlags(virDomainPtr dom, 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; } @@ -817,8 +812,7 @@ vmwareDomainUndefineFlags(virDomainPtr dom, 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
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@redhat.com> --- 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
For vzDomainLookupByID and vzDomainLookupByUUID 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. Also adjust the prlsdkHandle{VmState|VmRemoved|Perf}Event APIs in the same manner. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vz/vz_driver.c | 8 ++++---- src/vz/vz_sdk.c | 15 ++++++++------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 1118ef92f..68ae2f8e0 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 = virDomainObjListFindByID(privconn->driver->domains, id); + dom = virDomainObjListFindByIDRef(privconn->driver->domains, id); if (dom == NULL) { virReportError(VIR_ERR_NO_DOMAIN, NULL); @@ -591,7 +591,7 @@ vzDomainLookupByID(virConnectPtr conn, int id) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } @@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr ret = NULL; virDomainObjPtr dom; - dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid); if (dom == NULL) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -618,7 +618,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) ret = virGetDomain(conn, dom->def->name, dom->def->uuid, dom->def->id); cleanup: - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); return ret; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index a5b9f2da6..b8f13f88a 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, virDomainEventType lvEventType = 0; int lvEventTypeDetails = 0; - dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); if (dom == NULL) return; @@ -2166,7 +2166,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, cleanup: PrlHandle_Free(eventParam); - virObjectUnlock(dom); + virObjectEndAPI(&dom); return; } @@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL; - dom = virDomainObjListFindByUUID(driver->domains, uuid); + dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); /* domain was removed from the list from the libvirt * API function in current connection */ if (dom == NULL) @@ -2235,6 +2235,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, VIR_DOMAIN_EVENT_UNDEFINED_REMOVED); virDomainObjListRemove(driver->domains, dom); + virDomainObjEndAPI(&dom); return; } @@ -2246,7 +2247,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, virDomainObjPtr dom = NULL; vzDomObjPtr privdom = NULL; - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { PrlHandle_Free(event); return; } @@ -2255,7 +2256,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, PrlHandle_Free(privdom->stats); privdom->stats = event; - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); } static void @@ -2269,7 +2270,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, PRL_HANDLE param = PRL_INVALID_HANDLE; PRL_RESULT pret; - if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) + if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) return; pret = PrlEvent_GetParam(event, 0, ¶m); @@ -2283,7 +2284,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, cleanup: PrlHandle_Free(param); - virObjectUnlock(dom); + virDomainObjEndAPI(&dom); } static PRL_RESULT -- 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@redhat.com> --- 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
Now that every caller is using virDomainObjListFindByUUIDRef, let's just remove it and keep the name as virDomainObjListFindByUUID. That allows reworking the virdomainobjlist API's a bit to be more like virDomainObjListFindByName. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/bhyve/bhyve_driver.c | 4 ++-- src/conf/virdomainobjlist.c | 35 +++++++---------------------------- src/conf/virdomainobjlist.h | 2 -- src/libvirt_private.syms | 1 - src/libxl/libxl_driver.c | 4 ++-- src/lxc/lxc_driver.c | 4 ++-- src/openvz/openvz_driver.c | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/test/test_driver.c | 4 ++-- src/uml/uml_driver.c | 4 ++-- src/util/virclosecallbacks.c | 4 ++-- src/vmware/vmware_driver.c | 2 +- src/vz/vz_driver.c | 4 ++-- src/vz/vz_sdk.c | 14 +++++++------- src/vz/vz_utils.c | 2 +- 15 files changed, 33 insertions(+), 57 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 79963570c..4f95f6c15 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -168,7 +168,7 @@ bhyveDomObjFromDomain(virDomainPtr domain) bhyveConnPtr privconn = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUIDRef(privconn->domains, domain->uuid); + vm = virDomainObjListFindByUUID(privconn->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -794,7 +794,7 @@ bhyveDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByUUIDRef(privconn->domains, uuid); + vm = virDomainObjListFindByUUID(privconn->domains, uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index 87a742b1e..3290dfa29 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -152,10 +152,10 @@ virDomainObjListFindByIDRef(virDomainObjListPtr doms, return virDomainObjListFindByIDInternal(doms, id, true); } -static virDomainObjPtr -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, - const unsigned char *uuid, - bool ref) + +virDomainObjPtr +virDomainObjListFindByUUID(virDomainObjListPtr doms, + const unsigned char *uuid) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainObjPtr obj; @@ -164,41 +164,20 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms, virUUIDFormat(uuid, uuidstr); obj = virHashLookup(doms->objs, uuidstr); - 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 -virDomainObjListFindByUUID(virDomainObjListPtr doms, - const unsigned char *uuid) -{ - return virDomainObjListFindByUUIDInternal(doms, uuid, false); -} - - -virDomainObjPtr -virDomainObjListFindByUUIDRef(virDomainObjListPtr doms, - const unsigned char *uuid) -{ - return virDomainObjListFindByUUIDInternal(doms, uuid, true); -} - - virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, const char *name) { diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h index bb186bde3..1b77a95ba 100644 --- a/src/conf/virdomainobjlist.h +++ b/src/conf/virdomainobjlist.h @@ -38,8 +38,6 @@ virDomainObjPtr virDomainObjListFindByIDRef(virDomainObjListPtr doms, int id); virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms, const unsigned char *uuid); -virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms, - const unsigned char *uuid); virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms, const char *name); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3766e20d3..6f0cd9680 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -951,7 +951,6 @@ virDomainObjListFindByID; virDomainObjListFindByIDRef; virDomainObjListFindByName; virDomainObjListFindByUUID; -virDomainObjListFindByUUIDRef; virDomainObjListForEach; virDomainObjListGetActiveIDs; virDomainObjListGetInactiveNames; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index e78fe2d4b..1379e9b83 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -313,7 +313,7 @@ libxlDomObjFromDomain(virDomainPtr dom) libxlDriverPrivatePtr driver = dom->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUIDRef(driver->domains, dom->uuid); + vm = virDomainObjListFindByUUID(driver->domains, dom->uuid); if (!vm) { virUUIDFormat(dom->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -1113,7 +1113,7 @@ libxlDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByUUIDRef(driver->domains, uuid); + vm = virDomainObjListFindByUUID(driver->domains, uuid); 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 fa6fc4643..ed3f0fbc9 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -138,7 +138,7 @@ lxcDomObjFromDomain(virDomainPtr domain) virLXCDriverPtr driver = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); + vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -281,7 +281,7 @@ static virDomainPtr lxcDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByUUIDRef(driver->domains, uuid); + vm = virDomainObjListFindByUUID(driver->domains, uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index b0b72b171..3d4e66168 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 = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { + if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) { virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e13544f83..4972b1fef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -198,7 +198,7 @@ qemuDomObjFromDomain(virDomainPtr domain) virQEMUDriverPtr driver = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); + vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -1556,7 +1556,7 @@ static virDomainPtr qemuDomainLookupByUUID(virConnectPtr conn, virDomainObjPtr vm; virDomainPtr dom = NULL; - vm = virDomainObjListFindByUUIDRef(driver->domains, uuid); + vm = virDomainObjListFindByUUID(driver->domains, uuid); if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ce8c1001d..bef15c826 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -578,7 +578,7 @@ testDomObjFromDomain(virDomainPtr domain) testDriverPtr driver = domain->conn->privateData; char uuidstr[VIR_UUID_STRING_BUFLEN]; - vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); + vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -1725,7 +1725,7 @@ static virDomainPtr testDomainLookupByUUID(virConnectPtr conn, virDomainPtr ret = NULL; virDomainObjPtr dom; - if (!(dom = virDomainObjListFindByUUIDRef(privconn->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUID(privconn->domains, uuid))) { virReportError(VIR_ERR_NO_DOMAIN, NULL); goto cleanup; } diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 41c607e66..91797c0eb 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -171,7 +171,7 @@ umlDomObjFromDomainLocked(struct uml_driver *driver, virDomainObjPtr vm; char uuidstr[VIR_UUID_STRING_BUFLEN]; - if (!(vm = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { + if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) { virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, @@ -767,7 +767,7 @@ static int umlProcessAutoDestroyDom(void *payload, return 0; } - if (!(dom = virDomainObjListFindByUUIDRef(data->driver->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUID(data->driver->domains, uuid))) { VIR_DEBUG("No domain object to kill"); return 0; } diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 4db50e8b6..544938e90 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -347,8 +347,8 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, virDomainObjPtr vm; /* Grab a ref and lock to the vm */ - if (!(vm = virDomainObjListFindByUUIDRef(domains, - list->entries[i].uuid))) { + if (!(vm = virDomainObjListFindByUUID(domains, + list->entries[i].uuid))) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(list->entries[i].uuid, uuidstr); VIR_DEBUG("No domain object with UUID %s", uuidstr); diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 37a7b1932..f427361d1 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 = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { + if (!(vm = virDomainObjListFindByUUID(driver->domains, uuid))) { virUUIDFormat(uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 68ae2f8e0..caa0170da 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -602,7 +602,7 @@ vzDomainLookupByUUID(virConnectPtr conn, const unsigned char *uuid) virDomainPtr ret = NULL; virDomainObjPtr dom; - dom = virDomainObjListFindByUUIDRef(privconn->driver->domains, uuid); + dom = virDomainObjListFindByUUID(privconn->driver->domains, uuid); if (dom == NULL) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -837,7 +837,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags) if (virDomainDefineXMLFlagsEnsureACL(conn, def) < 0) goto cleanup; - dom = virDomainObjListFindByUUIDRef(driver->domains, def->uuid); + dom = virDomainObjListFindByUUID(driver->domains, def->uuid); if (dom == NULL) { virResetLastError(); if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b8f13f88a..0d14a1678 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1958,7 +1958,7 @@ prlsdkLoadDomain(vzDriverPtr driver, goto error; virObjectLock(driver); - if (!(olddom = virDomainObjListFindByUUIDRef(driver->domains, def->uuid))) + if (!(olddom = virDomainObjListFindByUUID(driver->domains, def->uuid))) dom = virDomainObjListAdd(driver->domains, def, driver->xmlopt, 0, NULL); virObjectUnlock(driver); @@ -2144,7 +2144,7 @@ prlsdkHandleVmStateEvent(vzDriverPtr driver, virDomainEventType lvEventType = 0; int lvEventTypeDetails = 0; - dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); + dom = virDomainObjListFindByUUID(driver->domains, uuid); if (dom == NULL) return; @@ -2177,7 +2177,7 @@ prlsdkHandleVmConfigEvent(vzDriverPtr driver, virDomainObjPtr dom = NULL; bool job = false; - dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); + dom = virDomainObjListFindByUUID(driver->domains, uuid); if (dom == NULL) return; @@ -2207,7 +2207,7 @@ prlsdkHandleVmAddedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL; - if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid)) && + if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid)) && !(dom = prlsdkAddDomainByUUID(driver, uuid))) goto cleanup; @@ -2225,7 +2225,7 @@ prlsdkHandleVmRemovedEvent(vzDriverPtr driver, { virDomainObjPtr dom = NULL; - dom = virDomainObjListFindByUUIDRef(driver->domains, uuid); + dom = virDomainObjListFindByUUID(driver->domains, uuid); /* domain was removed from the list from the libvirt * API function in current connection */ if (dom == NULL) @@ -2247,7 +2247,7 @@ prlsdkHandlePerfEvent(vzDriverPtr driver, virDomainObjPtr dom = NULL; vzDomObjPtr privdom = NULL; - if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) { + if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) { PrlHandle_Free(event); return; } @@ -2270,7 +2270,7 @@ prlsdkHandleMigrationProgress(vzDriverPtr driver, PRL_HANDLE param = PRL_INVALID_HANDLE; PRL_RESULT pret; - if (!(dom = virDomainObjListFindByUUIDRef(driver->domains, uuid))) + if (!(dom = virDomainObjListFindByUUID(driver->domains, uuid))) return; pret = PrlEvent_GetParam(event, 0, ¶m); diff --git a/src/vz/vz_utils.c b/src/vz/vz_utils.c index 8f4e3e347..13f5deeaa 100644 --- a/src/vz/vz_utils.c +++ b/src/vz/vz_utils.c @@ -76,7 +76,7 @@ vzDomObjFromDomain(virDomainPtr domain) char uuidstr[VIR_UUID_STRING_BUFLEN]; vzDriverPtr driver = privconn->driver; - vm = virDomainObjListFindByUUIDRef(driver->domains, domain->uuid); + vm = virDomainObjListFindByUUID(driver->domains, domain->uuid); if (!vm) { virUUIDFormat(domain->uuid, uuidstr); virReportError(VIR_ERR_NO_DOMAIN, -- 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@redhat.com> --- 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
participants (3)
-
Jim Fehlig -
John Ferlan -
Marc Hartmayer