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

[John Ferlan]

On 03/13/2018 03:44 AM, Marc Hartmayer wrote:
> On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan at 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 at 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
>