On Wed, Apr 15, 2020 at 01:55:34PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 14, 2020 at 05:24:50PM +0200, Pavel Mores wrote:
> > On Fri, Apr 10, 2020 at 03:54:28PM +0200, Rafael Fonseca wrote:
> > > Signed-off-by: Rafael Fonseca <r4f4...@gmail.com>
> > > ---
> > > src/admin/admin_server_dispatch.c | 13 ++++---------
> > > 1 file changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/admin/admin_server_dispatch.c
> > > b/src/admin/admin_server_dispatch.c
> > > index b3da577995..2515528779 100644
> > > --- a/src/admin/admin_server_dispatch.c
> > > +++ b/src/admin/admin_server_dispatch.c
> > > @@ -45,7 +45,7 @@ typedef daemonAdmClientPrivate
> > > *daemonAdmClientPrivatePtr;
> > > /* Separate private data for admin connection */
> > > struct daemonAdmClientPrivate {
> > > /* Just a placeholder, not that there is anything to be locked */
> > > - virMutex lock;
> > > + GMutex lock;
> > >
> > > virNetDaemonPtr dmn;
> > > };
> > > @@ -55,7 +55,7 @@ remoteAdmClientFree(void *data)
> > > {
> > > struct daemonAdmClientPrivate *priv = data;
> > >
> > > - virMutexDestroy(&priv->lock);
> > > + g_mutex_clear(&priv->lock);
> > > virObjectUnref(priv->dmn);
> > > VIR_FREE(priv);
> > > }
> > > @@ -91,11 +91,7 @@ remoteAdmClientNew(virNetServerClientPtr client
> > > G_GNUC_UNUSED,
> > > if (VIR_ALLOC(priv) < 0)
> > > return NULL;
> > >
> > > - if (virMutexInit(&priv->lock) < 0) {
> > > - VIR_FREE(priv);
> > > - virReportSystemError(errno, "%s", _("unable to init mutex"));
> > > - return NULL;
> > > - }
> > > + g_mutex_init(&priv->lock);
> > >
> > > /*
> > > * We don't necessarily need to ref this object right now as there
> > > @@ -167,9 +163,9 @@ adminDispatchConnectOpen(virNetServerPtr server
> > > G_GNUC_UNUSED,
> > > struct daemonAdmClientPrivate *priv =
> > > virNetServerClientGetPrivateData(client);
> > > int ret = -1;
> > > + g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&priv->lock);
> > >
> > > VIR_DEBUG("priv=%p dmn=%p", priv, priv->dmn);
> > > - virMutexLock(&priv->lock);
> > >
> > > flags = args->flags;
> > > virCheckFlagsGoto(0, cleanup);
> > > @@ -178,7 +174,6 @@ adminDispatchConnectOpen(virNetServerPtr server
> > > G_GNUC_UNUSED,
> > > cleanup:
> > > if (ret < 0)
> > > virNetMessageSaveError(rerr);
> > > - virMutexUnlock(&priv->lock);
> > > return ret;
> > > }
> > >
> > > --
> > > 2.25.2
> >
> > I was wondering why virMutexInit() returns a value whereas g_mutex_init()
> > does
> > not, to make sure there weren't any additional adjustments necessary, but
> > it's
> > probably OK.
> >
> > I mean, it does feel slightly dubious as virMutexInit() fails if
> > pthread_mutex_init() fails which can happen under a bunch of seemingly
> > fairly
> > realistic scenarios. I assume g_mutex_init() ultimately calls
> > pthread_mutex_init() as well so these scenarios should still apply.
> >
> > However, this seems ultimately a problem of glib API designers to decide how
> > realistic the scenarios are (at least some of them seem to be related to
> > memory
> > allocation which glib solves by aborting) and whether to report them to
> > their
> > users, and they made the decision that they made, hopefully for good
> > reasons.
>
> Honestly, none of the reasons mutex init can fail are especially
> interesting to callers. There's essentially nothing useful callers
> can do when a mutex init fails, as its symptomatic of much bigger
> problems. Thus I think abort'ing is a reasonable approach. Likewise
> for lock/unlock.
Yes, agreed. At the risk of going off-topic, what makes me wonder is that
phtread_mutex_init() POSIX manual page lists also EPERM among the reasons why
the call might fail. I've never heard of that, let alone encountered it, and
it might not actually be implemented by OS's in practice. However if EPERM can
happen, then I guess that could be worth reporting to the user.
Well it's not like we need to solve this problem on this list anyway...
pvl
