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!
+ 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... 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. 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; }