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

[Marc Hartmayer]

On Thu, Apr 26, 2018 at 05:08 PM +0200, John Ferlan <jferlan at redhat.com> 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 at linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
>> ---
>>  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