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

On Thu, Apr 26, 2018 at 05:08 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; wrote: > 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. Yep. I saw it… another patch was in planning :) > > 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 This may lead to locking issues and it would increase the critical sections for “no reason”. What’s about: Add another goto-label? Not beautiful but it would work.

> > 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; >> > -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294