On Fri, Apr 10, 2020 at 03:54:28PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca <r4f4rfs@xxxxxxxxx>
> ---
> 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.
Reviewed-by: Pavel Mores <pmores@xxxxxxxxxx>
