On 05/03/2018 12:02 PM, Erik Skultety wrote:
> On Tue, Apr 24, 2018 at 08:28:05AM -0400, John Ferlan wrote:
>> Use the FindBy{UUID|Name}Locked helpers which will return a locked
>> and ref counted object rather than the direct virHashLookup and
>> virObjectLock of the returned object. We'll need to temporarily
>> virObjectUnref when we assign a new domain @def, but that will
>> change shortly when virDomainObjListAddObjLocked returns the
>> correct reference counted object.
>>
>> Use the virDomainObjEndAPI in the error path to Unref/Unlock for
>> the corresponding Unref/Unlock of either the FindBy* return or
>> the virDomainObjNew since both return a reffed/locked object.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/conf/virdomainobjlist.c | 22 +++++++++-------------
>> 1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>> index 9aa2abd8c3..6752f6c572 100644
>> --- a/src/conf/virdomainobjlist.c
>> +++ b/src/conf/virdomainobjlist.c
>> @@ -280,11 +280,8 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>> if (oldDef)
>> *oldDef = NULL;
>>
>> - virUUIDFormat(def->uuid, uuidstr);
>> -
>> /* See if a VM with matching UUID already exists */
>> - if ((vm = virHashLookup(doms->objs, uuidstr))) {
>> - virObjectLock(vm);
>> + if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
>> /* UUID matches, but if names don't match, refuse it */
>> if (STRNEQ(vm->def->name, def->name)) {
>> virUUIDFormat(vm->def->uuid, uuidstr);
>> @@ -314,10 +311,12 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>> def,
>> !!(flags & VIR_DOMAIN_OBJ_LIST_ADD_LIVE),
>> oldDef);
>> + /* XXX: Temporary until this API is fixed to return a locked and
>> + * refcnt'd object */
>> + virObjectUnref(vm);
>
> I didn't bother trying, but would the tests pass even without ^this temporary
> unref? Because if they do, we should drop it, since you do that anyway within
> the same series, so who cares...
>
> With or without the adjustment
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
>
Yes, the tests would pass.
It's needed temporarily because virDomainObjListFindByUUIDLocked returns
a reffed/locked @vm; whereas, virHashLookup just returns @vm which was
locked before return.
The caller expects to receive @vm without that extra ref. All things
being equal removing this would temporarily leak @vm. I was trying to be
"correct" through each stage of the changes.
And yes by patch 6 (or 4 if things are rearrange) it's a moot point
because the Add API will return the correct number of ref's on @vm.
Tks -
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
