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

[Daniel P. Berrangé]

On Thu, Aug 05, 2021 at 05:44:15PM +0200, Peter Krempa wrote:
> On Thu, Aug 05, 2021 at 16:33:57 +0100, Daniel P. Berrangé wrote:
> > On Thu, Aug 05, 2021 at 05:29:24PM +0200, Peter Krempa wrote:
> > > On Thu, Aug 05, 2021 at 16:11:15 +0100, Daniel P. Berrangé wrote:
> > > > On Thu, Aug 05, 2021 at 02:58:03PM +0100, Daniel P. Berrangé wrote:
> > > > > On Thu, Aug 05, 2021 at 03:33:57PM +0200, Tim Wiederhake wrote:
> > > > > > 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:
> > > 
> > > [...]
> > > 
> > > > The pthread_mutex_destroy call is the only one that I see
> > > > returning errors with PTHREAD_MUTEX_NORMAL. So I don't think
> > > > there's benefit to adding the code to lock/unlock paths.
> > > 
> > > And as noted I'd be very careful with that one too. I've got at least
> > > one counterexample of a code path which effectively does the same on a
> > > very common code path.
> > > 
> > > Namely 'virDomainObjParseXML' uses:
> > > 
> > > g_autoptr(virDomainObj) obj = NULL;
> > > 
> > > and then returns directly on common failures such as XML parsing errors,
> > > post-parse callback errors or even errors from the validation callbacks.
> > > 
> > > Spamming logs in such a common code path is definitely not acceptable.
> > 
> > Destroying a locked mutex though is a clear bug that should be fixed
> > though, not a false positive. If it is a commonly triggered bug that
> > is even worse and more reason to fix it.
> 
> I definitely agree, but we need a better approach to do this, because it
> will keep happening since the g_auto* infrastructure is really
> convenient. And this is something that can happen also without it.
> 
> Unfortunately I don't think that just replacing
> 
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainObj, virObjectUnref);
> 
> by an equivalent of virObjectUnrefAndUnlock is sufficient.
> 
> IMO we should do some form of lock state tracking for
> 'virObjectLockable' so that we can keep using g_auto for it without
> either accidentaly unlocking or unrefing a locked object.

Lock state tracking doesn't sound very appealing to me.

QEMU makes use of g_auto for unlocking mutexes within scopes

    WITH_QEMU_LOCK_GUARD(&mutex) {
        ...
        if (error) {
            return; <-- mutex is automatically unlocked
        }
 
        if (early_exit) {
            break;  <-- leave this scope early, unlocking
        }
        ...
    }


and another pattern

    ... <-- mutex not locked
    QEMU_LOCK_GUARD(&mutex); <-- mutex locked from here onwards
    ...
    if (error) {
        return; <-- mutex is automatically unlocked
    }

This would work for libvirt too, at least for cases where the
lock+unlock are in the same scope (which should be the majority).

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