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&gt; --- src/internal.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/internal.h b/src/internal.h index b6e4332542..4cfb022b41 100644 --- a/src/internal.h +++ b/src/internal.h @@ -100,6 +100,9 @@ #define STREQ_NULLABLE(a, b) (g_strcmp0(a, b) == 0) #define STRNEQ_NULLABLE(a, b) (g_strcmp0(a, b) != 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_auto(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 = virLockGuardLock(m); /* `m` is locked, and released automatically on scope exit */ ... while (expression) { VIR_LOCK_GUARD lock2 = virLockGuardLock(...); /* similar */ } } Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/util/virthread.c | 15 +++++++++++++++ src/util/virthread.h | 10 ++++++++++ 3 files changed, 27 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bc6fa191bf..5da9874e17 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3388,6 +3388,8 @@ virCondInit; virCondSignal; virCondWait; virCondWaitUntil; +virLockGuardLock; +virLockGuardUnlock; virMutexDestroy; virMutexInit; virMutexInitRecursive; diff --git a/src/util/virthread.c b/src/util/virthread.c index e89c1a09fb..5422bb74fd 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -96,6 +96,21 @@ void virMutexUnlock(virMutex *m) pthread_mutex_unlock(&m->lock); } +virLockGuard virLockGuardLock(virMutex *m) +{ + virLockGuard l = { m }; + virMutexLock(m); + return l; +} + +void virLockGuardUnlock(virLockGuard *l) +{ + if (!l || !l->mutex) + 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..6cdaf2820e 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,11 @@ void virMutexLock(virMutex *m); void virMutexUnlock(virMutex *m); +virLockGuard virLockGuardLock(virMutex *m); +void virLockGuardUnlock(virLockGuard *l); +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virLockGuard, virLockGuardUnlock); +#define VIR_LOCK_GUARD g_auto(virLockGuard) G_GNUC_UNUSED + 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). See comment for typical usage. Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; --- src/util/virthread.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/util/virthread.h b/src/util/virthread.h index 6cdaf2820e..23abe0b6c9 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -209,3 +209,24 @@ int virThreadLocalSet(virThreadLocal *l, void*) G_GNUC_WARN_UNUSED_RESULT; return 0; \ } \ struct classname ## EatSemicolon + +#define VIR_WITH_MUTEX_LOCK_GUARD_(m, name) \ + for (g_auto(virLockGuard) name = virLockGuardLock(m); name.mutex; \ + name.mutex = (virLockGuardUnlock(&name), NULL)) +/** + * 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) \ + VIR_WITH_MUTEX_LOCK_GUARD_(m, CONCAT(var, __COUNTER__)) -- 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(a)redhat.com&gt; --- 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 5da9874e17..9bc3d9530b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2956,6 +2956,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLockGuard; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 588b41802c..74cc84205e 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 virLockGuardLock(&obj->lock); +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockable diff --git a/src/util/virobject.h b/src/util/virobject.h index 1e34c77744..dc1ce66a4f 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). See comment for typical usage. Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; --- src/util/virobject.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/util/virobject.h b/src/util/virobject.h index dc1ce66a4f..a1e16aee77 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -148,3 +148,25 @@ virObjectListFree(void *list); void virObjectListFreeCount(void *list, size_t count); + +#define VIR_WITH_OBJECT_LOCK_GUARD_(o, name) \ + for (g_auto(virLockGuard) name = virObjectLockGuard(o); name.mutex; \ + name.mutex = (virLockGuardUnlock(&name), NULL)) + +/** + * 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) \ + VIR_WITH_OBJECT_LOCK_GUARD_(o, CONCAT(var, __COUNTER__)) -- 2.31.1

Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; --- 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 b5477b03d5..c9b2134e3b 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 = virLockGuardLock(&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(a)redhat.com&gt; --- 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 c9b2134e3b..8610f0ac5c 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); - g_clear_pointer(&devs->hash, g_hash_table_unref); - virMutexUnlock(&devs->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&devs->lock) { + virHashForEachSafe(devs->hash, virChrdevFreeClearCallbacks, NULL); + g_clear_pointer(&devs->hash, g_hash_table_unref); + } virMutexDestroy(&devs->lock); g_free(devs); -- 2.31.1

Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; --- 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 f291f12e50..47ee98e650 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(a)redhat.com&gt; --- 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 7bc39120ee..42053de9c3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4045,9 +4045,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

On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote: My preference is the keep the scope of the guarded section explicit using {} even with a 1 line body. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Tue, Feb 01, 2022 at 05:23:33PM +0100, Tim Wiederhake wrote: OK, I don't really have a horse in this race. But in that case it would be nice to document it in our guidelines, maybe together with few words around the new macros. In a later patch for example.

On Wed, Feb 02, 2022 at 08:17:53AM +0100, Michal Prívozník wrote: It depends how much we like consistency, right? I used to prefer curly brackets around any sub-statement, even one-liners, but then I started contributing to libvirt and had to adapt. If we want to be 100% consistent then it would make sense to just have them all the time even though I do not prefer that style any more, but I would adapt. Everyone would, I think. And that leads me to another idea which I mentioned in the past already. I don't want to repeat myself too much, but "since you bought this up" and this is very related, here goes nothing. All this talk about "which subjective way of writing the same code should we choose" seems a bit of a moo point since even though we always mention consistency we end up choosing what we like better. But since no decision will cater to everyone and everyone will, at some point, have to adapt, maybe there is a better solution to the problem. Maybe the solution is even more consistency so that there are fewer exceptions. So what if we find another advantage/reason for deciding on a way to format the code? One possible advantage would be that if we choose to comply with a format that can be easily enforced (something that indent / clang-format / whatever can do a good job of) we could erase lot of docs, lot of syntax checking code, reduce the knowledge required for newcomers to send proper code, make it easier to automatically fix code to comply to the rules, and so on. The only disadvantage would be that most of us would have to adapt and my point is that we have to do that anyway (new syntax, new "libvirt language", working on other projects) and more consistency just makes it even easier IMHO, even if it takes a bit longer. Anyway, just something I felt like mentioning Martin