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

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(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. Any OS that is crazy enough to report EPERM for initializing mutex is not an OS I wish to support :-)