On Fri, Apr 27, 2018 at 02:19 AM +0200, John Ferlan <jferlan@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@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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@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