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

On 04/26/2018 12:27 PM, Marc Hartmayer wrote: > On Thu, Apr 26, 2018 at 05:07 PM +0200, John Ferlan <jferlan(a)redhat.com&gt; 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(a)linux.vnet.ibm.com>>> Reviewed-by: Boris Fiuczynski <fiuczy(a)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