Re: [Qemu-devel] [PATCH for-2.7 v4 19/36] char: free the tcp connection data when closing

[Paolo Bonzini <pbonzini@redhat.com>]

On 05/08/2016 10:24, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Make sure the connection data got freed when closing the chardev, to
> avoid leaks. Introduce tcp_chr_free_connection() to clean all connection
> related data, and move some tcp_chr_close() clean-ups there.
> 
> (while at it, set write_msgfds_num to 0 when clearing array in
> tcp_set_msgfds())
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-char.c | 50 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index a50b8fb..f20d385 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2763,6 +2763,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num)
>      /* clear old pending fd array */
>      g_free(s->write_msgfds);
>      s->write_msgfds = NULL;
> +    s->write_msgfds_num = 0;
>  
>      if (!s->connected ||
>          !qio_channel_has_feature(s->ioc,
> @@ -2843,19 +2844,24 @@ static GSource *tcp_chr_add_watch(CharDriverState *chr, GIOCondition cond)
>      return qio_channel_create_watch(s->ioc, cond);
>  }
>  
> -static void tcp_chr_disconnect(CharDriverState *chr)
> +static void tcp_chr_free_connection(CharDriverState *chr)
>  {
>      TCPCharDriver *s = chr->opaque;
> +    int i;
>  
>      if (!s->connected) {
>          return;
>      }
>  
> -    s->connected = 0;
> -    if (s->listen_ioc) {
> -        s->listen_tag = qio_channel_add_watch(
> -            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
> +    if (s->read_msgfds_num) {
> +        for (i = 0; i < s->read_msgfds_num; i++) {
> +            close(s->read_msgfds[i]);
> +        }
> +        g_free(s->read_msgfds);
> +        s->read_msgfds = NULL;
> +        s->read_msgfds_num = 0;
>      }
> +
>      tcp_set_msgfds(chr, NULL, 0);
>      remove_fd_in_watch(chr);
>      object_unref(OBJECT(s->sioc));
> @@ -2863,6 +2869,24 @@ static void tcp_chr_disconnect(CharDriverState *chr)
>      object_unref(OBJECT(s->ioc));
>      s->ioc = NULL;
>      g_free(chr->filename);
> +    chr->filename = NULL;
> +    s->connected = 0;
> +}
> +
> +static void tcp_chr_disconnect(CharDriverState *chr)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +
> +    if (!s->connected) {
> +        return;
> +    }
> +
> +    tcp_chr_free_connection(chr);
> +
> +    if (s->listen_ioc) {
> +        s->listen_tag = qio_channel_add_watch(
> +            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
> +    }
>      chr->filename = SocketAddress_to_str("disconnected:", s->addr,
>                                           s->is_listen, s->is_telnet);
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> @@ -3177,17 +3201,14 @@ int qemu_chr_wait_connected(CharDriverState *chr, Error **errp)
>  static void tcp_chr_close(CharDriverState *chr)
>  {
>      TCPCharDriver *s = chr->opaque;
> -    int i;
> +
> +    tcp_chr_free_connection(chr);
>  
>      if (s->reconnect_timer) {
>          g_source_remove(s->reconnect_timer);
>          s->reconnect_timer = 0;
>      }
>      qapi_free_SocketAddress(s->addr);
> -    remove_fd_in_watch(chr);
> -    if (s->ioc) {
> -        object_unref(OBJECT(s->ioc));
> -    }
>      if (s->listen_tag) {
>          g_source_remove(s->listen_tag);
>          s->listen_tag = 0;
> @@ -3195,18 +3216,9 @@ static void tcp_chr_close(CharDriverState *chr)
>      if (s->listen_ioc) {
>          object_unref(OBJECT(s->listen_ioc));
>      }
> -    if (s->read_msgfds_num) {
> -        for (i = 0; i < s->read_msgfds_num; i++) {
> -            close(s->read_msgfds[i]);
> -        }
> -        g_free(s->read_msgfds);
> -    }
>      if (s->tls_creds) {
>          object_unref(OBJECT(s->tls_creds));
>      }
> -    if (s->write_msgfds_num) {
> -        g_free(s->write_msgfds);
> -    }
>      g_free(s);
>      qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>