On Thu, Aug 05, 2021 at 03:43:11PM +0200, Peter Krempa wrote: > On Thu, Aug 05, 2021 at 15:08:50 +0200, Tim Wiederhake wrote: > > `pthread_mutex_destroy`, `pthread_mutex_lock` and `pthread_mutex_unlock` > > return an error code that is currently ignored. > > > > Add debug information if one of these operations failed, e.g. when there > > is an attempt to destroy a still locked mutex or unlock and already > > unlocked mutex. > > > > Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx> > > --- > > src/util/virthread.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/src/util/virthread.c b/src/util/virthread.c > > index e89c1a09fb..f64dbee9e9 100644 > > --- a/src/util/virthread.c > > +++ b/src/util/virthread.c > > @@ -35,7 +35,10 @@ > > > > #include "viralloc.h" > > #include "virthreadjob.h" > > +#include "virlog.h" > > > > +#define VIR_FROM_THIS VIR_FROM_THREAD > > +VIR_LOG_INIT("util.thread"); > > > > int virOnce(virOnceControl *once, virOnceFunc init) > > { > > @@ -83,17 +86,23 @@ int virMutexInitRecursive(virMutex *m) > > > > void virMutexDestroy(virMutex *m) > > { > > - pthread_mutex_destroy(&m->lock); > > + if (pthread_mutex_destroy(&m->lock)) { > > + VIR_WARN("Failed to destroy mutex=%p", m); > > + } > > } > > > > void virMutexLock(virMutex *m) > > { > > - pthread_mutex_lock(&m->lock); > > + if (pthread_mutex_lock(&m->lock)) { > > + VIR_WARN("Failed to lock mutex=%p", m); > > + } > > } > > > > void virMutexUnlock(virMutex *m) > > { > > - pthread_mutex_unlock(&m->lock); > > + if (pthread_mutex_unlock(&m->lock)) { > > + VIR_WARN("Failed to unlock mutex=%p", m); > > + } > > } > > I'm not very persuaded that this is a good idea: > > 1) The warning itself is not useful > > The pointer itself is valid only when the program is running, having > this in the logs has almost no value. > > 2) It spams logs > > As noted above, by getting just the log you don't get any useful > information and users tend to report that log is being spammed in > certain cases, so while this would note that there is something going > on a lot of additional work is required to figure out what, thus the > value of such entry is rather low. > > 3) It can create an endless loop > > VIR_WARN -> VIR_WARN_INT -> virLogMessage -> virLogVMessage -> > virLogLock -> virMutexLock() I think we could use perror() to make it more useful and avoid the loop void virMutexDestroy(virMutex *m) { if ((errno = pthread_mutex_destroy(&m->lock)) != 0) { perror("pthread_mutex_destroy"); } } still not going to be enough to diagnose the problem fro the logs though, as we have no use context. This change would give a bit more of a fighting chance at diagnosis: in virthread.h #define virMutexDestroy(m) \ virMutexDestroyImpl(m, __FILE__, __func__, __LINE__) void virMutexDestroyImpl(virMutex *mm, const char *file, const char *func, int line); and in virthread.c void virMutexDestroyImpl(virMutex *m, const char *file, const char *func, int line) { if ((errno = pthread_mutex_destroy(&m->lock)) != 0) { g_autofree char *msg = g_strdup_printf("pthread_mutex_destroy (caller %s in %s:%d)", func, file, line); perror(msg); } } That results in: $ ./build/tests/qemusecuritytest TEST: qemusecuritytest pthread_mutex_destroy (caller virObjectLockableDispose in ../src/util/virobject.c:349): Device or resource busy !pthread_mutex_destroy (caller virObjectLockableDispose in ../src/util/virobject.c:349): Device or resource busy !pthread_mutex_destroy (caller virObjectLockableDispose in ../src/util/virobject.c:349): Device or resource busy !pthread_mutex_destroy (caller virObjectLockableDispose in ../src/util/virobject.c:349): Device or resource busy !pthread_mutex_destroy (caller virObjectLockableDispose in ../src/util/virobject.c:349): Device or resource busy !pthread_mutex_destroy (caller virObjectLockableDispose in ../src/util/virobject.c:349): Device or resource busy 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 :|