[libvirt] [PATCH 1/4] secret: Return with locked obj from virSecretObjListRemove

[Erik Skultety]

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