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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux