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

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&gt; >> Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; >> --- >> 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?

> If you consider all the > other callbacks will load @callback onto some list that gets run during > remoteClientFreePrivateCallbacks via DEREG_CB, this code would need to > do something similar. Since there's only 1 it's perhaps easier at > least. > >> priv->closeRegistered = true; > > Perhaps change closeRegistered from bool to daemonClientEventCallbackPtr > and handle it that way similar to how other callback processing is > handled. This would be one way how to deal with it (even without the first patch). But a double free error must be prevented. > > John > >> @@ -3856,8 +3864,10 @@ remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server ATTRIBUTE_UNUS >> >> cleanup: >> virMutexUnlock(&priv->lock); >> - if (rv < 0) >> + if (rv < 0) { >> + remoteEventCallbackFree(callback); >> virNetMessageSaveError(rerr); >> + } >> return rv; >> } >> >> > -- 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