[libvirt PATCH v5 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 V3: https://listman.redhat.com/archives/libvir-list/2021-September/msg00964.html V4: https://listman.redhat.com/archives/libvir-list/2022-January/msg01262.html Changes since V4: * Replaced usage of "__LINE__" with "__COUNTER__" 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 virLockGuardLock 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 | 3 +++ src/lxc/lxc_driver.c | 6 +++--- src/util/virobject.c | 16 ++++++++++++++++ src/util/virobject.h | 26 ++++++++++++++++++++++++++ src/util/virthread.c | 15 +++++++++++++++ src/util/virthread.h | 31 +++++++++++++++++++++++++++++++ 9 files changed, 104 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 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
On Tue, Feb 01, 2022 at 02:20:09PM +0100, Tim Wiederhake wrote:
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(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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@redhat.com> --- 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
On Tue, Feb 01, 2022 at 02:20:10PM +0100, Tim Wiederhake wrote:
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@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virthread.c | 15 +++++++++++++++ src/util/virthread.h | 10 ++++++++++ 3 files changed, 27 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h). See comment for typical usage. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- 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
On Tue, Feb 01, 2022 at 02:20:11PM +0100, Tim Wiederhake wrote:
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h).
See comment for typical usage.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virthread.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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 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
On Tue, Feb 01, 2022 at 02:20:12PM +0100, Tim Wiederhake wrote:
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(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h). See comment for typical usage. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- 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
On Tue, Feb 01, 2022 at 02:20:13PM +0100, Tim Wiederhake wrote:
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h).
See comment for typical usage.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virobject.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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 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
On Tue, Feb 01, 2022 at 02:20:14PM +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virchrdev.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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 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
On Tue, Feb 01, 2022 at 02:20:15PM +0100, Tim Wiederhake wrote:
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 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); + }
Your code change is perfectly correct given the starting state, so Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> but I seriously question the sanity of the original code. If virMutexLock here passes without waiting that's good because it means nothing else is using the virChrdev pointer. if virMutexLock hits contention and has to wait though, this feels very bad because it implies there is a 2nd thread here that holds a copy of the virChrdevs pointer and is actively still using it concurrently with us calling virChrdevFree. This feels very risky as a design pattern. Normally if we expect multiple threads to be accessing the same struct, we would want to have it ref counted, and both threads hold their own reference, such that we don't get into running any destructor until we know only one thread is left using the object. Thus calling MutexLock would be unnecessary. Maybe we lucky in our usage for virChrdev but it feels like it deserves future attention. 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 :|
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 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
On Tue, Feb 01, 2022 at 02:20:16PM +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/bhyve/bhyve_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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 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 02:20:17PM +0100, Tim Wiederhake wrote:
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 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); + }
Even nicer you can omit the curly brackets and make this a +2/-3 ;) Otherwise I guess we'd need some kind of stylistic guide to keep us consistent in this regard right from the start.
virDomainHostdevRemove(vm->def, idx); virDomainHostdevDefFree(def); -- 2.31.1
On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
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 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); + }
Even nicer you can omit the curly brackets and make this a +2/-3 ;) Otherwise I guess we'd need some kind of stylistic guide to keep us consistent in this regard right from the start.
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, 2022-02-01 at 16:07 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
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 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); + }
Even nicer you can omit the curly brackets and make this a +2/-3 ;) Otherwise I guess we'd need some kind of stylistic guide to keep us consistent in this regard right from the start.
My preference is the keep the scope of the guarded section explicit using {} even with a 1 line body.
Same for me, fwiw. Cheers, Tim
On Tue, Feb 01, 2022 at 05:23:33PM +0100, Tim Wiederhake wrote:
On Tue, 2022-02-01 at 16:07 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
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 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); + }
Even nicer you can omit the curly brackets and make this a +2/-3 ;) Otherwise I guess we'd need some kind of stylistic guide to keep us consistent in this regard right from the start.
My preference is the keep the scope of the guarded section explicit using {} even with a 1 line body.
Same for me, fwiw.
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.
Cheers, Tim
On 2/2/22 01:06, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 05:23:33PM +0100, Tim Wiederhake wrote:
On Tue, 2022-02-01 at 16:07 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
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 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); + }
Even nicer you can omit the curly brackets and make this a +2/-3 ;) Otherwise I guess we'd need some kind of stylistic guide to keep us consistent in this regard right from the start.
My preference is the keep the scope of the guarded section explicit using {} even with a 1 line body.
Same for me, fwiw.
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.
Since you brought this up, I vaguely remember us discussing this earlier, though it was for one line body of if()-s. I too am in favor of having curly braces in that case, except maybe when the body is a goto or return statement. IOW: if (cond) return -1; // good if (cond) { func(); // good } if (cond) { goto cleanup; // not a big fan } OTOH, consistency matters more so I can live with the last example too. Michal
On Wed, Feb 02, 2022 at 08:17:53AM +0100, Michal Prívozník wrote:
On 2/2/22 01:06, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 05:23:33PM +0100, Tim Wiederhake wrote:
On Tue, 2022-02-01 at 16:07 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 01, 2022 at 03:25:55PM +0100, Martin Kletzander wrote:
On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
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 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); + }
Even nicer you can omit the curly brackets and make this a +2/-3 ;) Otherwise I guess we'd need some kind of stylistic guide to keep us consistent in this regard right from the start.
My preference is the keep the scope of the guarded section explicit using {} even with a 1 line body.
Same for me, fwiw.
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.
Since you brought this up, I vaguely remember us discussing this earlier, though it was for one line body of if()-s. I too am in favor of having curly braces in that case, except maybe when the body is a goto or return statement. IOW:
if (cond) return -1; // good
if (cond) { func(); // good }
if (cond) { goto cleanup; // not a big fan }
OTOH, consistency matters more so I can live with the last example too.
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
On Tue, Feb 01, 2022 at 02:20:17PM +0100, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/lxc/lxc_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 02:20:08PM +0100, Tim Wiederhake wrote:
V1: https://listman.redhat.com/archives/libvir-list/2021-August/msg00823.html V2: https://listman.redhat.com/archives/libvir-list/2021-September/msg00249.html V3: https://listman.redhat.com/archives/libvir-list/2021-September/msg00964.html V4: https://listman.redhat.com/archives/libvir-list/2022-January/msg01262.html
Changes since V4:
* Replaced usage of "__LINE__" with "__COUNTER__"
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 virLockGuardLock virChrdevFree: Use VIR_WITH_MUTEX_LOCK bhyveAutostartDomain: Use virObjectLockGuard lxcDomainDetachDeviceHostdevUSBLive: Use VIR_WITH_OBJECT_LOCK_GUARD
Time to learn yet another new language, libvirt22 =D Anyway, I like it, but I did not go through the previous versions. With that in mind (not sure if there are some unresolved conflicts) Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
src/bhyve/bhyve_driver.c | 4 ++-- src/conf/virchrdev.c | 12 +++++------- src/internal.h | 3 +++ src/libvirt_private.syms | 3 +++ src/lxc/lxc_driver.c | 6 +++--- src/util/virobject.c | 16 ++++++++++++++++ src/util/virobject.h | 26 ++++++++++++++++++++++++++ src/util/virthread.c | 15 +++++++++++++++ src/util/virthread.h | 31 +++++++++++++++++++++++++++++++ 9 files changed, 104 insertions(+), 12 deletions(-)
-- 2.31.1
participants (4)
-
Daniel P. Berrangé -
Martin Kletzander -
Michal Prívozník -
Tim Wiederhake