Re: [libvirt] [PATCH libvirt v2 9/9] remote: shrink the critical sections

On 04/12/2018 08:41 AM, Marc Hartmayer wrote: > No lock is required to access 'priv->conn' therefore we can shrink the > critical sections. > > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > src/remote/remote_daemon_dispatch.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > While this follows other usages, that means virMutexUnlock in cleanup: may be called without holding the lock.

Perhaps this and other *Register/*Deregister API's should take the lock around anything that could end up in cleanup - perhaps even the same history as the previous patch

John > diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c > index 041e7c7440bf..d6699418392e 100644 > --- a/src/remote/remote_daemon_dispatch.c > +++ b/src/remote/remote_daemon_dispatch.c > @@ -3841,8 +3841,6 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server, > virNetServerClientGetPrivateData(client); > virNetServerProgramPtr program; > > - virMutexLock(&priv->lock); > - > if (!priv->conn) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); > goto cleanup; > @@ -3853,6 +3851,8 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server, > goto cleanup; > } > > + virMutexLock(&priv->lock); > + > if (VIR_ALLOC(callback) < 0) > goto cleanup; > callback->client = virObjectRef(client); > @@ -3887,13 +3887,13 @@ remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server ATTRIBUTE_UN > struct daemonClientPrivate *priv = > virNetServerClientGetPrivateData(client); > > - virMutexLock(&priv->lock); > - > if (!priv->conn) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); > goto cleanup; > } > > + virMutexLock(&priv->lock); > + > if (virConnectUnregisterCloseCallback(priv->conn, > remoteRelayConnectionClosedEvent) < 0) > goto cleanup; >