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

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.