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

[Peter Krempa]

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 at redhat.com>
> ---
>  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()