[libvirt] [PATCH 2/3] virDomainObjListAddLocked: Produce better error message than 'Duplicate key'

[Cole Robinson]

On 3/19/19 9:49 AM, Michal Privoznik wrote:
> If there are two concurrent threads, one of which is removing a
> domain from the list and the other is trying to add it back they
> may serialize in the following order:
> 
> 1) vm->removing is set and @vm is unlocked.
> 2) The tread that's trying to add the domain onto the list locks
>    the list and tries to find, if the domain already exists.
> 3) Our lookup functions say it doesn't, so the thread proceeds to
>    virHashAddEntry() which fails with 'Duplicate key' error.
> 
> This is obviously not helpful error message at all.
> 
> The problem lies in our lookup functions
> (virDomainObjListFindByUUIDLocked() and
> virDomainObjListFindByNameLocked()) which return NULL even if the
> object is still on the list. They do this so that the object is
> not mistakenly looked up by some driver. The fix consists of
> moving 'removing' check one level up and thus allowing
> virDomainObjListAddLocked() to produce meaningful error message.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/conf/virdomainobjlist.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 3631cf16d0..23734ad815 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -141,14 +141,9 @@ virDomainObjListFindByUUIDLocked(virDomainObjListPtr doms,
>  
>      virUUIDFormat(uuid, uuidstr);
>      obj = virHashLookup(doms->objs, uuidstr);
> -    virObjectRef(obj);
>      if (obj) {
> +        virObjectRef(obj);
>          virObjectLock(obj);
> -        if (obj->removing) {
> -            virObjectUnlock(obj);
> -            virObjectUnref(obj);
> -            obj = NULL;
> -        }
>      }
>      return obj;
>  }
> @@ -172,6 +167,12 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,
>      obj = virDomainObjListFindByUUIDLocked(doms, uuid);
>      virObjectRWUnlock(doms);
>  
> +    if (obj && obj->removing) {
> +        virObjectUnlock(obj);
> +        virObjectUnref(obj);
> +        obj = NULL;
> +    }
> +
>      return obj;
>  }
>  
> @@ -183,14 +184,9 @@ virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
>      virDomainObjPtr obj;
>  
>      obj = virHashLookup(doms->objsName, name);
> -    virObjectRef(obj);
>      if (obj) {
> +        virObjectRef(obj);
>          virObjectLock(obj);
> -        if (obj->removing) {
> -            virObjectUnlock(obj);
> -            virObjectUnref(obj);
> -            obj = NULL;
> -        }
>      }
>      return obj;
>  }
> @@ -214,6 +210,12 @@ virDomainObjListFindByName(virDomainObjListPtr doms,
>      obj = virDomainObjListFindByNameLocked(doms, name);
>      virObjectRWUnlock(doms);
>  
> +    if (obj && obj->removing) {
> +        virObjectUnlock(obj);
> +        virObjectUnref(obj);
> +        obj = NULL;
> +    }
> +
>      return obj;
>  }
>  
> @@ -285,8 +287,13 @@ virDomainObjListAddLocked(virDomainObjListPtr doms,
>  
>      /* See if a VM with matching UUID already exists */
>      if ((vm = virDomainObjListFindByUUIDLocked(doms, def->uuid))) {
> -        /* UUID matches, but if names don't match, refuse it */
> -        if (STRNEQ(vm->def->name, def->name)) {
> +        if (vm->removing) {
> +            virReportError(VIR_ERR_OPERATION_FAILED,
> +                           _("domain '%s' is already being removed"),
> +                           vm->def->name);
> +            goto error;
> +        } else  if (STRNEQ(vm->def->name, def->name)) {

Extra space here

> +            /* UUID matches, but if names don't match, refuse it */
>              virUUIDFormat(vm->def->uuid, uuidstr);
>              virReportError(VIR_ERR_OPERATION_FAILED,
>                             _("domain '%s' is already defined with uuid %s"),
> 

Makes sense to me. virDomainObjListFindByID more or less already does
the same thing. I thought about the locking a lot and couldn't come up
with a reason why this would be problematic

Reviewed-by: Cole Robinson <crobinso at redhat.com>

- Cole