2:24 p.m.
V1: https://listman.redhat.com/archives/libvir-list/2021-August/msg00823.html
V2: https://listman.redhat.com/archives/libvir-list/2021-September/msg00249.html
Changes since V2:
* Dropped VIR_XPATH_NODE_AUTORESTORE simplification. Moved to a
separate series.
* Dropped the attempt to work around g_auto* / clang unused variable
warnings (See https://bugs.llvm.org/show_bug.cgi?id=3888,
https://bugs.llvm.org/show_bug.cgi?id=43482, and
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/2272).
Instead, introduce VIR_LOCK_GUARD macro that adds G_GNUC_UNUSED.
Regards,
Tim
Tim Wiederhake (9):
internal: Add CONCAT macro
virthread: Introduce virLockGuard
virthread: Introduce VIR_WITH_MUTEX_LOCK_GUARD
virobject: Introduce virObjectLockGuard
virobject: Introduce VIR_WITH_OBJECT_LOCK_GUARD
virChrdevFDStreamCloseCb: Use virLockGuardNew
virChrdevFree: Use VIR_WITH_MUTEX_LOCK
bhyveAutostartDomain: Use virObjectLockGuard
lxcDomainDetachDeviceHostdevUSBLive: Use VIR_WITH_OBJECT_LOCK_GUARD
src/bhyve/bhyve_driver.c | 4 ++--
src/conf/virchrdev.c | 12 +++++-------
src/internal.h | 3 +++
src/libvirt_private.syms | 4 ++++
src/lxc/lxc_driver.c | 6 +++---
src/util/virobject.c | 16 ++++++++++++++++
src/util/virobject.h | 24 ++++++++++++++++++++++++
src/util/virthread.c | 26 ++++++++++++++++++++++++++
src/util/virthread.h | 31 +++++++++++++++++++++++++++++++
9 files changed, 114 insertions(+), 12 deletions(-)
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 1/9] internal: Add CONCAT macro
Using the two-step idiom to force resolution of other macros, e.g.:
#define bar BAR
CONCAT_(foo, bar) // foobar
CONCAT(foo, bar) // fooBAR
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/internal.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/internal.h b/src/internal.h
index e1250a59fe..48188e6fa3 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -93,6 +93,9 @@
#define NUL_TERMINATE(buf) do { (buf)[sizeof(buf)-1] = '\0'; } while (0)
+#define CONCAT_(a, b) a ## b
+#define CONCAT(a, b) CONCAT_(a, b)
+
#ifdef WIN32
# ifndef O_CLOEXEC
# define O_CLOEXEC _O_NOINHERIT
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 2/9] virthread: Introduce virLockGuard
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;
+}
+
+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
+
int virRWLockInit(virRWLock *m) G_GNUC_WARN_UNUSED_RESULT;
void virRWLockDestroy(virRWLock *m);
--
2.31.1
12:08 a.m.
New subject: [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
2:24 p.m.
New subject: [libvirt PATCH v3 3/9] virthread: Introduce VIR_WITH_MUTEX_LOCK_GUARD
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h).
Uses "__LINE__" instead of "__COUNTER__", as the latter is a GNU
extension.
See comment for typical usage.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/util/virthread.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/util/virthread.h b/src/util/virthread.h
index d896a6ad3a..78b3684193 100644
--- a/src/util/virthread.h
+++ b/src/util/virthread.h
@@ -210,3 +210,23 @@ int virThreadLocalSet(virThreadLocal *l, void*)
G_GNUC_WARN_UNUSED_RESULT;
return 0; \
} \
struct classname ## EatSemicolon
+
+/**
+ * VIR_WITH_MUTEX_LOCK_GUARD:
+ *
+ * This macro defines a lock scope such that entering the scope takes the lock
+ * and leaving the scope releases the lock. Return statements are allowed
+ * within the scope and release the lock. Break and continue statements leave
+ * the scope early and release the lock.
+ *
+ * virMutex *mutex = ...;
+ *
+ * VIR_WITH_MUTEX_LOCK_GUARD(mutex) {
+ * // `mutex` is locked, and released automatically on scope exit
+ * ...
+ * }
+ */
+#define VIR_WITH_MUTEX_LOCK_GUARD(m) \
+ for (g_autoptr(virLockGuard) CONCAT(var, __LINE__) = virLockGuardNew(m); \
+ CONCAT(var, __LINE__); \
+ CONCAT(var, __LINE__) = (virLockGuardFree(CONCAT(var, __LINE__)), NULL))
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 4/9] virobject: Introduce virObjectLockGuard
Typical usage:
void foobar(virObjectLockable *obj)
{
VIR_LOCK_GUARD lock = virObjectLockGuard(obj);
/* `obj` is locked, and released automatically on scope exit */
...
}
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virobject.c | 16 ++++++++++++++++
src/util/virobject.h | 4 ++++
3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f41674781d..733e40384e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2947,6 +2947,7 @@ virObjectListFree;
virObjectListFreeCount;
virObjectLock;
virObjectLockableNew;
+virObjectLockGuard;
virObjectNew;
virObjectRef;
virObjectRWLockableNew;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 3412985b79..4812b79d56 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -426,6 +426,22 @@ virObjectGetRWLockableObj(void *anyobj)
}
+/**
+ * virObjectLockGuard:
+ * @anyobj: any instance of virObjectLockable
+ *
+ * Acquire a lock on @anyobj that will be managed by the virLockGuard object
+ * returned to the caller.
+ */
+virLockGuard *
+virObjectLockGuard(void *anyobj)
+{
+ virObjectLockable *obj = virObjectGetLockableObj(anyobj);
+
+ return virLockGuardNew(&obj->lock);
+}
+
+
/**
* virObjectLock:
* @anyobj: any instance of virObjectLockable
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 1e34c77744..0c9cbea726 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -118,6 +118,10 @@ void *
virObjectRWLockableNew(virClass *klass)
ATTRIBUTE_NONNULL(1);
+virLockGuard *
+virObjectLockGuard(void *lockableobj)
+ ATTRIBUTE_NONNULL(1);
+
void
virObjectLock(void *lockableobj)
ATTRIBUTE_NONNULL(1);
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 5/9] virobject: Introduce VIR_WITH_OBJECT_LOCK_GUARD
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h).
Uses "__LINE__" instead of "__COUNTER__", as the latter is a GNU
extension.
See comment for typical usage.
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/util/virobject.h | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 0c9cbea726..a95985b69a 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -148,3 +148,23 @@ virObjectListFree(void *list);
void
virObjectListFreeCount(void *list,
size_t count);
+
+/**
+ * VIR_WITH_OBJECT_LOCK_GUARD:
+ *
+ * This macro defines a lock scope such that entering the scope takes the lock
+ * and leaving the scope releases the lock. Return statements are allowed
+ * within the scope and release the lock. Break and continue statements leave
+ * the scope early and release the lock.
+ *
+ * virObjectLockable *lockable = ...;
+ *
+ * VIR_WITH_OBJECT_LOCK_GUARD(lockable) {
+ * // `lockable` is locked, and released automatically on scope exit
+ * ...
+ * }
+ */
+#define VIR_WITH_OBJECT_LOCK_GUARD(o) \
+ for (g_autoptr(virLockGuard) CONCAT(var, __LINE__) = virObjectLockGuard(o); \
+ CONCAT(var, __LINE__); \
+ CONCAT(var, __LINE__) = (virLockGuardFree(CONCAT(var, __LINE__)), NULL))
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 6/9] virChrdevFDStreamCloseCb: Use virLockGuardNew
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/virchrdev.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
index 5d6de68427..0184a1ae10 100644
--- a/src/conf/virchrdev.c
+++ b/src/conf/virchrdev.c
@@ -237,12 +237,10 @@ static void virChrdevFDStreamCloseCb(virStreamPtr st G_GNUC_UNUSED,
void *opaque)
{
virChrdevStreamInfo *priv = opaque;
- virMutexLock(&priv->devs->lock);
+ VIR_LOCK_GUARD lock = virLockGuardNew(&priv->devs->lock);
/* remove entry from hash */
virHashRemoveEntry(priv->devs->hash, priv->path);
-
- virMutexUnlock(&priv->devs->lock);
}
/**
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 7/9] virChrdevFree: Use VIR_WITH_MUTEX_LOCK
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/conf/virchrdev.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c
index 0184a1ae10..1cf727d46e 100644
--- a/src/conf/virchrdev.c
+++ b/src/conf/virchrdev.c
@@ -291,10 +291,10 @@ void virChrdevFree(virChrdevs *devs)
if (!devs)
return;
- virMutexLock(&devs->lock);
- virHashForEachSafe(devs->hash, virChrdevFreeClearCallbacks, NULL);
- virHashFree(devs->hash);
- virMutexUnlock(&devs->lock);
+ VIR_WITH_MUTEX_LOCK_GUARD(&devs->lock) {
+ virHashForEachSafe(devs->hash, virChrdevFreeClearCallbacks, NULL);
+ virHashFree(devs->hash);
+ }
virMutexDestroy(&devs->lock);
g_free(devs);
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 8/9] bhyveAutostartDomain: Use virObjectLockGuard
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/bhyve/bhyve_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 516490f6cd..c41de30be9 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -87,7 +87,8 @@ bhyveAutostartDomain(virDomainObj *vm, void *opaque)
{
const struct bhyveAutostartData *data = opaque;
int ret = 0;
- virObjectLock(vm);
+ VIR_LOCK_GUARD lock = virObjectLockGuard(vm);
+
if (vm->autostart && !virDomainObjIsActive(vm)) {
virResetLastError();
ret = virBhyveProcessStart(data->conn, vm,
@@ -98,7 +99,6 @@ bhyveAutostartDomain(virDomainObj *vm, void *opaque)
vm->def->name, virGetLastErrorMessage());
}
}
- virObjectUnlock(vm);
return ret;
}
--
2.31.1
2:24 p.m.
New subject: [libvirt PATCH v3 9/9] lxcDomainDetachDeviceHostdevUSBLive: Use VIR_WITH_OBJECT_LOCK_GUARD
Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
---
src/lxc/lxc_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index e2720a6f89..528af5d164 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4079,9 +4079,9 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriver *driver,
VIR_WARN("cannot deny device %s for domain %s: %s",
dst, vm->def->name, virGetLastErrorMessage());
- virObjectLock(hostdev_mgr->activeUSBHostdevs);
- virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs, usb);
- virObjectUnlock(hostdev_mgr->activeUSBHostdevs);
+ VIR_WITH_OBJECT_LOCK_GUARD(hostdev_mgr->activeUSBHostdevs) {
+ virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs, usb);
+ }
virDomainHostdevRemove(vm->def, idx);
virDomainHostdevDefFree(def);
--
2.31.1
1219
days inactive
1224
days old
10 comments
2 participants
participants (2)
-
Jonathon Jongsma -
Tim Wiederhake