From 874617d278b58554feee567b2e79d208aaef214c Mon Sep 17 00:00:00 2001

Message-Id: <874617d278b58554feee567b2e79d208aaef214c@dist-git>

From: Michal Privoznik <mprivozn@redhat.com>

Date: Tue, 6 Aug 2019 13:29:22 +0200

Subject: [PATCH] virDomainObjListAddLocked: Produce better error message than

 'Duplicate key'

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

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

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

(cherry picked from commit a5c71129bf2c12a827f1bc00149acd1c572ffe9c)

Signed-off-by: Ján Tomko <jtomko@redhat.com>

RHEL-8.1.0: https://bugzilla.redhat.com/show_bug.cgi?id=1737790

Message-Id: <c63ca487bf56af5240e273b5a112a753c2bc85a3.1565090956.git.jtomko@redhat.com>

Reviewed-by: Michal Privoznik <mprivozn@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 e7c3e326ca..3b388e2ba2 100644

--- a/src/conf/virdomainobjlist.c

+++ b/src/conf/virdomainobjlist.c

@@ -142,14 +142,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;

 }

@@ -173,6 +168,12 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,

     obj = virDomainObjListFindByUUIDLocked(doms, uuid);

     virObjectRWUnlock(doms);

+    if (obj && obj->removing) {

+        virObjectUnlock(obj);

+        virObjectUnref(obj);

+        obj = NULL;

+    }

+

     return obj;

 }

@@ -184,14 +185,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;

 }

@@ -215,6 +211,12 @@ virDomainObjListFindByName(virDomainObjListPtr doms,

     obj = virDomainObjListFindByNameLocked(doms, name);

     virObjectRWUnlock(doms);

+    if (obj && obj->removing) {

+        virObjectUnlock(obj);

+        virObjectUnref(obj);

+        obj = NULL;

+    }

+

     return obj;

 }

@@ -286,8 +288,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)) {

+            /* 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"),

-- 

2.22.1