Re: [libvirt PATCH 6/7] virMutex*: Warn on error

On Thu, 2021-08-05 at 14:24 +0100, Daniel P. Berrangé wrote: > 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 ? Yes. Please note that I apprently copy-pasted the wrong link in the cover letter. The pipeline for this series is correct (https://gitlab.com/twiederh/libvirt/-/pipelines/348505233), the pipeline for only patches 5, 6, 7 that demonstrate the non-zero return values of pthread_mutex_{lock,unlock,destroy} is here: https://gitlab.com/twiederh/libvirt/-/pipelines/348505770

> > 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