On Fri, Apr 27, 2018 at 02:19 AM +0200, John Ferlan <jferlan@xxxxxxxxxx> wrote:
> On 04/26/2018 12:27 PM, Marc Hartmayer wrote:
>> On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
