Re: [libvirt PATCH v3 2/9] virthread: Introduce virLockGuard
On Thu, Sep 30, 2021 at 6:29 AM Tim Wiederhake <twiederh(a)redhat.com> 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(a)redhat.com>
---
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