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

[John Ferlan]

On 04/26/2018 12:40 PM, Marc Hartmayer wrote:
> 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.
> 

That option could work too... I was thinking more about how to avoid the
call Unlock when we don't have the lock.  Probably doesn't matter how
it's done.


John

>>
>> 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
>