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

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(a)gmail.com&gt; > > > --- > > > 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.