[PATCH v2 2/9] secret: Alter virSecretObjListRemove processing

[John Ferlan <jferlan@xxxxxxxxxx>]

Current processing requires a "fire dance" unlocking the @obj,
adding an @obj ref, locking the @secrets, and relocking @obj
in order to ensure proper lock ordering.

This can be avoided by changing virSecretObjListRemove to take
a @uuidstr instead of @obj. Then, we can lock the @secrets list,
look up the @obj by @uuidstr (like we do when adding), and remove
the @obj from the list. This removes the last reference to the
object effectively reaping it.

NB: Since prior to calling we remove the reference to the object
we cannot pass anything contained within the object (such as the
obj->def) because it's possible that the object could be reaped
by two competing remove threads.

Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
 src/conf/virsecretobj.c    | 38 +++++++++++++++++---------------------
 src/conf/virsecretobj.h    |  2 +-
 src/secret/secret_driver.c | 15 +++++++++------
 3 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 4aaf47b5d..09f6ead64 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -284,32 +284,25 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets,
 /*
  * virSecretObjListRemove:
  * @secrets: list of secret objects
- * @secret: a secret object
+ * @uuidstr: secret uuid to find
+ *
+ * Find the object by the @uuidstr in the list, remove the object from
+ * the list hash table, and free the object.
  *
- * Remove the object from the hash table.  The caller must hold the lock
- * on the driver owning @secrets and must have also locked @secret to
- * ensure no one else is either waiting for @secret or still using it.
+ * Upon entry it's expected that prior to entry any locks on
+ * the object related to @uuidstr will have been removed.
  */
 void
 virSecretObjListRemove(virSecretObjListPtr secrets,
-                       virSecretObjPtr obj)
+                       const char *uuidstr)
 {
-    char uuidstr[VIR_UUID_STRING_BUFLEN];
-    virSecretDefPtr def;
-
-    if (!obj)
-        return;
-    def = obj->def;
-
-    virUUIDFormat(def->uuid, uuidstr);
-    virObjectRef(obj);
-    virObjectUnlock(obj);
+    virSecretObjPtr obj;
 
     virObjectRWLockWrite(secrets);
-    virObjectLock(obj);
-    virHashRemoveEntry(secrets->objs, uuidstr);
-    virObjectUnlock(obj);
-    virObjectUnref(obj);
+    if ((obj = virSecretObjListFindByUUIDLocked(secrets, uuidstr))) {
+        virHashRemoveEntry(secrets->objs, uuidstr);
+        virSecretObjEndAPI(&obj);
+    }
     virObjectRWUnlock(secrets);
 }
 
@@ -927,8 +920,11 @@ virSecretLoad(virSecretObjListPtr secrets,
     def = NULL;
 
     if (virSecretLoadValue(obj) < 0) {
-        virSecretObjListRemove(secrets, obj);
-        virObjectLock(obj);
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+        virUUIDFormat(obj->def->uuid, uuidstr);
+        virSecretObjEndAPI(&obj);
+        virSecretObjListRemove(secrets, uuidstr);
         goto cleanup;
     }
 
diff --git a/src/conf/virsecretobj.h b/src/conf/virsecretobj.h
index d412ee6a7..68bafc718 100644
--- a/src/conf/virsecretobj.h
+++ b/src/conf/virsecretobj.h
@@ -49,7 +49,7 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets,
 
 void
 virSecretObjListRemove(virSecretObjListPtr secrets,
-                       virSecretObjPtr obj);
+                       const char *uuidstr);
 
 virSecretObjPtr
 virSecretObjListAdd(virSecretObjListPtr secrets,
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 23a3c9bda..75bc2b0cf 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -270,9 +270,11 @@ secretDefineXML(virConnectPtr conn,
         virSecretObjSetDef(obj, backup);
         VIR_STEAL_PTR(def, objDef);
     } else {
-        virSecretObjListRemove(driver->secrets, obj);
-        virObjectUnref(obj);
-        obj = NULL;
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+        virUUIDFormat(objDef->uuid, uuidstr);
+        virSecretObjEndAPI(&obj);
+        virSecretObjListRemove(driver->secrets, uuidstr);
     }
 
  cleanup:
@@ -392,6 +394,7 @@ secretUndefine(virSecretPtr secret)
     int ret = -1;
     virSecretObjPtr obj;
     virSecretDefPtr def;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
     virObjectEventPtr event = NULL;
 
     if (!(obj = secretObjFromSecret(secret)))
@@ -412,9 +415,9 @@ secretUndefine(virSecretPtr secret)
 
     virSecretObjDeleteData(obj);
 
-    virSecretObjListRemove(driver->secrets, obj);
-    virObjectUnref(obj);
-    obj = NULL;
+    virUUIDFormat(def->uuid, uuidstr);
+    virSecretObjEndAPI(&obj);
+    virSecretObjListRemove(driver->secrets, uuidstr);
 
     ret = 0;
 
-- 
2.13.6

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list