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@redhat.com> --- 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@redhat.com>
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