From b701e45be5e80b4670dadacac70d9fa51599dec8 Mon Sep 17 00:00:00 2001
From: Michal Privoznik <mprivozn@redhat.com>
Date: Tue, 19 Mar 2019 14:39:20 +0100
Subject: [PATCH] virNWFilterBindingObjListAddLocked: Produce better error
 message than 'Duplicate key'

If there are two concurrent threads, one of which is removing an
nwfilter from the list and the other is trying to add it back they
may serialize in the following order:

1) obj->removing is set and @obj is unlocked.
2) The tread that's trying to add the nwfilter onto the list locks
   the list and tries to find, if the nwfilter 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 function
(virNWFilterBindingObjListFindByPortDevLocked()) 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 API. The fix
consists of moving 'removing' check one level up and thus
allowing virNWFilterBindingObjListAddLocked() to produce
meaningful error message.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cole Robinson <crobinso@redhat.com>
---
 src/conf/virnwfilterbindingobjlist.c | 29 +++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/src/conf/virnwfilterbindingobjlist.c b/src/conf/virnwfilterbindingobjlist.c
index 4ee2c1b194..9fe62ed4ed 100644
--- a/src/conf/virnwfilterbindingobjlist.c
+++ b/src/conf/virnwfilterbindingobjlist.c
@@ -91,14 +91,9 @@ virNWFilterBindingObjListFindByPortDevLocked(virNWFilterBindingObjListPtr bindin
     virNWFilterBindingObjPtr obj;
 
     obj = virHashLookup(bindings->objs, name);
-    virObjectRef(obj);
     if (obj) {
+        virObjectRef(obj);
         virObjectLock(obj);
-        if (virNWFilterBindingObjGetRemoving(obj)) {
-            virObjectUnlock(obj);
-            virObjectUnref(obj);
-            obj = NULL;
-        }
     }
     return obj;
 }
@@ -122,6 +117,12 @@ virNWFilterBindingObjListFindByPortDev(virNWFilterBindingObjListPtr bindings,
     obj = virNWFilterBindingObjListFindByPortDevLocked(bindings, name);
     virObjectRWUnlock(bindings);
 
+    if (obj && virNWFilterBindingObjGetRemoving(obj)) {
+        virObjectUnlock(obj);
+        virObjectUnref(obj);
+        obj = NULL;
+    }
+
     return obj;
 }
 
@@ -170,11 +171,17 @@ virNWFilterBindingObjListAddLocked(virNWFilterBindingObjListPtr bindings,
     bool stealDef = false;
 
     /* See if a binding with matching portdev already exists */
-    if ((binding = virNWFilterBindingObjListFindByPortDevLocked(
-             bindings, def->portdevname))) {
-        virReportError(VIR_ERR_OPERATION_FAILED,
-                       _("binding '%s' already exists"),
-                       def->portdevname);
+    binding = virNWFilterBindingObjListFindByPortDevLocked(bindings, def->portdevname);
+    if (binding) {
+        if (virNWFilterBindingObjGetRemoving(binding)) {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("binding '%s' is already being removed"),
+                           def->portdevname);
+        } else {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("binding '%s' already exists"),
+                           def->portdevname);
+        }
         goto error;
     }