On Thu, Aug 05, 2021 at 03:08:50PM +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. We ignore the errors because in practice we found impls don't return any error code when using PTHREAD_MUTEX_NORMAL - it would require use of PTHREAD_MUTEX_ERRORCHECK. Did you actually see errors returned for real ?

> Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; > --- >  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); > +    } >  } >   >   > -- > 2.31.1 > Regards, Daniel

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(a)redhat.com&gt; > --- > 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()