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

[Erik Skultety]

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