On Tue, Mar 13, 2018 at 01:39 PM +0100, John Ferlan <jferlan@xxxxxxxxxx> wrote:
> On 03/13/2018 03:44 AM, Marc Hartmayer wrote:
>> On Fri, Mar 09, 2018 at 05:47 PM +0100, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>
>>
>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
