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@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