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

[Marc Hartmayer]

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)?

>
>>
>> […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