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