virDomainObjListAddLocked: Produce better error message than 'Duplicate key'

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>

diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index a814fc10a39bd9e8ba90457c8a44f64f8b0dd938..f5a7ecc4a47f8aabee262b540c0b86e4f5854ed9 100644 (file)
--- 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)) {
+            /* 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"),