Re: [libvirt] [PATCH 01/20] bhyve: Use virDomainObjListFindBy{UUID|ID}Ref

On 03/13/2018 03:44 AM, Marc Hartmayer wrote: > On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; 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(a)redhat.com&gt; >> --- > > >> 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 >