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

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!

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