Re: [libvirt] [PATCH 1/4] secret: Return with locked obj from virSecretObjListRemove
On Wed, Mar 21, 2018 at 11:53:32AM -0400, John Ferlan wrote:
Rather than unlock the object that was expected to be locked on
input, let the caller perform the unlock or more succinctly a
virSecretObjEndAPI on the object which will perform the Unref
and Unlock and clear the @obj.
I understand the incentive for this change and I agree with it, but I do have a
question below.
Also clean up the virSecretObjListRemove function comments.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/virsecretobj.c | 15 ++++++++-------
src/secret/secret_driver.c | 4 ----
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
index 47e0b28968..49c341484b 100644
--- a/src/conf/virsecretobj.c
+++ b/src/conf/virsecretobj.c
@@ -284,11 +284,14 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets,
/*
* virSecretObjListRemove:
* @secrets: list of secret objects
- * @secret: a secret object
+ * @obj: a secret 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.
+ * Remove @obj from the secret obj list hash table. The caller must hold
+ * the lock on @obj to ensure no one else is either waiting for @obj or
+ * still using it.
+ *
+ * Upon return the @obj remains locked with at least 1 reference and
+ * the caller is expected to use virSecretObjEndAPI on it.
*/
void
virSecretObjListRemove(virSecretObjListPtr secrets,
@@ -308,7 +311,6 @@ virSecretObjListRemove(virSecretObjListPtr secrets,
virObjectRWLockWrite(secrets);
virObjectLock(obj);
virHashRemoveEntry(secrets->objs, uuidstr);
- virObjectUnlock(obj);
virObjectUnref(obj);
virObjectRWUnlock(secrets);
So, why do we require the caller to hold a lock on the object when the very
first thing we do is that we ref the @obj, UNLOCK it to avoid a deadlock with
someone else already holding a lock on @secrets and waiting on @obj and only
when we have the lock on @secrets, we re-lock @obj. Why isn't it sufficient to
just ref @obj since that's an atomic op and then only lock it once we acquire a
lock on @secrets too. In this case, you don't have to accept and return a locked
object, you only lock when absolutely necessary, what am I missing? In my
opinion this lock-unlock-lock (with occasional ref) fire dance is quite
cumbersome, but I guess I might be missing some important aspect here which I
don't see at the first glance.
}
@@ -927,8 +929,7 @@ virSecretLoad(virSecretObjListPtr secrets,
if (virSecretLoadValue(obj) < 0) {
virSecretObjListRemove(secrets, obj);
- virObjectUnref(obj);
- obj = NULL;
+ virSecretObjEndAPI(&obj); /* clears obj */
This can be further simplified by moving the ObjEndAPI to the cleanup section
and removing the braces on this 'if' block.
Erik