[libvirt PATCH v3 0/9] Automatic mutex management
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
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@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
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@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
On Thu, Sep 30, 2021 at 6:29 AM Tim Wiederhake <twiederh@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@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
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@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
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@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
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@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
Signed-off-by: Tim Wiederhake <twiederh@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
Signed-off-by: Tim Wiederhake <twiederh@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
Signed-off-by: Tim Wiederhake <twiederh@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
Signed-off-by: Tim Wiederhake <twiederh@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
participants (2)
-
Jonathon Jongsma -
Tim Wiederhake