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