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).
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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
