Re: [libvirt PATCH v3 2/9] virthread: Introduce virLockGuard

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

On Thu, Sep 30, 2021 at 6:29 AM Tim Wiederhake <twiederh@xxxxxxxxxx> wrote:
>
> Locks a virMutex on creation and unlocks it in its destructor.
>
> The VIR_LOCK_GUARD macro is used instead of "g_autoptr(virLockGuard)" to
> work around a clang issue (see https://bugs.llvm.org/show_bug.cgi?id=3888
> and https://bugs.llvm.org/show_bug.cgi?id=43482).
>
> Typical usage:
>
>     void function(virMutex *m)
>     {
>         VIR_LOCK_GUARD lock = virLockGuardNew(m);
>         /* `m` is locked, and released automatically on scope exit */
>
>         ...
>         while (expression) {
>             VIR_LOCK_GUARD lock2 = virLockGuardNew(...);
>             /* similar */
>         }
>     }
>
> Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx>
> ---
>  src/libvirt_private.syms |  3 +++
>  src/util/virthread.c     | 26 ++++++++++++++++++++++++++
>  src/util/virthread.h     | 11 +++++++++++
>  3 files changed, 40 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 6de9d9aef1..f41674781d 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -3373,6 +3373,9 @@ virCondInit;
>  virCondSignal;
>  virCondWait;
>  virCondWaitUntil;
> +virLockGuardFree;
> +virLockGuardNew;
> +virLockGuardUnlock;
>  virMutexDestroy;
>  virMutexInit;
>  virMutexInitRecursive;
> diff --git a/src/util/virthread.c b/src/util/virthread.c
> index e89c1a09fb..a5a948985f 100644
> --- a/src/util/virthread.c
> +++ b/src/util/virthread.c
> @@ -96,6 +96,32 @@ void virMutexUnlock(virMutex *m)
>      pthread_mutex_unlock(&m->lock);
>  }
>
> +virLockGuard *virLockGuardNew(virMutex *m)
> +{
> +    virLockGuard *l = g_new0(virLockGuard, 1);
> +    l->mutex = m;
> +
> +    virMutexLock(l->mutex);
> +    return l;
> +}

I realize that I'm jumping into the discussion in the third iteration
of these patches, so I apologize if this has already been discussed
(though I did look back at the earlier versions but didn't see
anything). Is there a reason that we're dynamically allocating the
lock guard rather than just allocating it as a local variable on the
stack? (i.e. using g_autoptr() vs. g_auto()).

> +
> +void virLockGuardFree(virLockGuard *l)
> +{
> +    if (!l)
> +        return;
> +
> +    virLockGuardUnlock(l);
> +    g_free(l);
> +}
> +
> +void virLockGuardUnlock(virLockGuard *l)
> +{
> +    if (!l)
> +        return;
> +
> +    virMutexUnlock(g_steal_pointer(&l->mutex));
> +}
> +
>
>  int virRWLockInit(virRWLock *m)
>  {
> diff --git a/src/util/virthread.h b/src/util/virthread.h
> index 55c8263ae6..d896a6ad3a 100644
> --- a/src/util/virthread.h
> +++ b/src/util/virthread.h
> @@ -31,6 +31,11 @@ struct virMutex {
>      pthread_mutex_t lock;
>  };
>
> +typedef struct virLockGuard virLockGuard;
> +struct virLockGuard {
> +    virMutex *mutex;
> +};
> +
>  typedef struct virRWLock virRWLock;
>  struct virRWLock {
>      pthread_rwlock_t lock;
> @@ -121,6 +126,12 @@ void virMutexLock(virMutex *m);
>  void virMutexUnlock(virMutex *m);
>
>
> +virLockGuard *virLockGuardNew(virMutex *m);
> +void virLockGuardFree(virLockGuard *l);
> +void virLockGuardUnlock(virLockGuard *l);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virLockGuard, virLockGuardFree);
> +#define VIR_LOCK_GUARD g_autoptr(virLockGuard) G_GNUC_UNUSED

As long as we're essentially required to use a custom macro for the
type declaration, I almost feel like we should just go all the way and
put the variable declaration inside the macro so that the usage looks
more like:

void function(virMutex *m)
{
      VIR_LOCK_GUARD(m); /* variable gets declared and assigned here */
     /* m is locked within function and released on exit
    ...
}

Maybe even use a name like VIR_SCOPED_LOCK?




> +
>  int virRWLockInit(virRWLock *m) G_GNUC_WARN_UNUSED_RESULT;
>  void virRWLockDestroy(virRWLock *m);
>
> --
> 2.31.1
>