[libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent

[Marc Hartmayer]

On Fri, Apr 27, 2018 at 02:19 AM +0200, John Ferlan <jferlan at redhat.com> wrote:
> On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
>> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>>> This allows us to get rid of another usage of the global variable
>>>> remoteProgram.
>>>>
>>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
>>>> ---
>>>>  src/remote/remote_daemon_dispatch.c | 18 ++++++++++++++----
>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
>>>> index b4c0e01b0832..cf2cd0add7d6 100644
>>>> --- a/src/remote/remote_daemon_dispatch.c
>>>> +++ b/src/remote/remote_daemon_dispatch.c
>>>> @@ -1661,12 +1661,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
>>>>  static
>>>>  void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque)
>>>>  {
>>>> -    virNetServerClientPtr client = opaque;
>>>> +    daemonClientEventCallbackPtr callback = opaque;
>>>>
>>>>      VIR_DEBUG("Relaying connection closed event, reason %d", reason);
>>>>
>>>>      remote_connect_event_connection_closed_msg msg = { reason };
>>>> -    remoteDispatchObjectEventSend(client, remoteProgram,
>>>> +    remoteDispatchObjectEventSend(callback->client, callback->program,
>>>>                                    REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
>>>>                                    (xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
>>>>                                    &msg);
>>>> @@ -3836,6 +3836,7 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>>                                             virNetMessageErrorPtr rerr)
>>>>  {
>>>>      int rv = -1;
>>>> +    daemonClientEventCallbackPtr callback = NULL;
>>>>      struct daemonClientPrivate *priv =
>>>>          virNetServerClientGetPrivateData(client);
>>>>
>>>> @@ -3846,9 +3847,16 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS
>>>>          goto cleanup;
>>>>      }
>>>>
>>>> +    if (VIR_ALLOC(callback) < 0)
>>>> +        goto cleanup;
>>>> +    callback->client = virObjectRef(client);
>>>
>>> Oh and this would seem to fix a problem with the callback possibly using
>>> @client after it had been freed!
>>
>> The problem is more of a theoretical nature as Nikolay had explained:
>>
>> “Refcounting was here originally but then removed in [1] as it conflicts with
>> virConnectRegisterCloseCallback returns 0 in case connectRegisterCloseCallback
>> is not implemented. This is safe though (at least nobody touch this place :).
>>
>> [1] ce35122cfe: "daemon: fixup refcounting in close callback handling"”
>>
>>>
>>>> +    callback->program = virObjectRef(remoteProgram);
>>>> +    /* eventID, callbackID, and legacy are not used */
>>>> +    callback->eventID = -1;
>>>> +    callback->callbackID = -1;
>>>>      if (virConnectRegisterCloseCallback(priv->conn,
>>>>                                          remoteRelayConnectionClosedEvent,
>>>> -                                        client, NULL) < 0)
>>>> +                                        callback, remoteEventCallbackFree) < 0)
>>>>          goto cleanup;
>>>>
>>>
>>> @callback would be leaked in the normal path...
>>
>> By normal path, you mean without the first patch?
>>
>
> I was following how the other register functions proceeded and they
> saved the callback in a list to be freed at unregister.  So if Register
> succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called
> and we leak callback.  At least that's how I read it

First - sorry for the late response!

The unregister/free’ing is either done within the
'remoteClientFreePrivateCallbacks' call or by an explicit call of
'remoteDispatchConnectUnregisterCloseCallback'. Do we agree on this? If
yes: the function 'remoteClientFreePrivateCallbacks' calls the
virConnectUnregisterCloseCallback -> remoteEventCallbackFree => no
memory leak.

There will be only a memory leak if the virConnectRegisterCloseCallback
call succeeds but the used driver had no support for registering a close
callback (conn->driver->connectRegisterCloseCallback was NULL) AND the
first patch of this series were not applied.

Did I miss something?

>

[…snip…]

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