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

[John Ferlan]

On 03/13/2018 12:44 PM, Marc Hartmayer wrote:
> On Tue, Mar 13, 2018 at 01:39 PM +0100, John Ferlan <jferlan at 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 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).
> 
> 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
>