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

John Ferlan jferlan at redhat.com
Thu Apr 26 15:07:49 UTC 2018



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 at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at 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;
>  }
>  
> 




More information about the libvir-list mailing list