[PATCH 01/43] admin: convert virMutex to GMutex

[Pavel Mores]

On Wed, Apr 15, 2020 at 03:58:55PM +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 15, 2020 at 04:11:30PM +0200, Pavel Mores wrote:
> > 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 <r4f4rfs at 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.
> 
> Any OS that is crazy enough to report EPERM for initializing mutex is not
> an OS I wish to support :-)

That seems fair enough. :-)

	pvl