Re: [libvirt] [PATCH 2/6] conf: Use virDomainObjListFindBy*Locked for virDomainObjListAdd

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt;