[Spice-devel] [spice-server v2] reds: Close sockets when failing to watch them

[Frediano Ziglio]

> 
> Currently if we fail to set up the watch waiting for accept() to be
> called on the socket, we still keep the network socket(s) open even if we
> are not going to be able to use it. This commit makes sure it's closed a
> set to -1 when such a failure occurs rather than having a half
> initialized spice-server instance.
> ---
>  server/reds.c | 47 +++++++++++++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index a31ed4e96..829158ca3 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2584,19 +2584,36 @@ void reds_set_client_mm_time_latency(RedsState *reds,
> RedClient *client, uint32_
>      }
>  }
>  
> +static void reds_cleanup_net(SpiceServer *reds)
> +{
> +    if (reds->listen_socket != -1) {
> +       reds_core_watch_remove(reds, reds->listen_watch);
> +       if (reds->config->spice_listen_socket_fd != reds->listen_socket) {
> +          close(reds->listen_socket);
> +       }
> +       reds->listen_watch = NULL;
> +       reds->listen_socket = -1;
> +    }
> +    if (reds->secure_listen_socket != -1) {
> +       reds_core_watch_remove(reds, reds->secure_listen_watch);
> +       close(reds->secure_listen_socket);
> +       reds->secure_listen_watch = NULL;
> +       reds->secure_listen_socket = -1;
> +    }
> +}
> +
>  static int reds_init_net(RedsState *reds)
>  {
>      if (reds->config->spice_port != -1 || reds->config->spice_family ==
>      AF_UNIX) {
>          reds->listen_socket = reds_init_socket(reds->config->spice_addr,
>          reds->config->spice_port, reds->config->spice_family);
>          if (-1 == reds->listen_socket) {
> -            return -1;
> +            goto error;
>          }
>          reds->listen_watch = reds_core_watch_add(reds, reds->listen_socket,
>                                                   SPICE_WATCH_EVENT_READ,
>                                                   reds_accept, reds);
>          if (reds->listen_watch == NULL) {
> -            spice_warning("set fd handle failed");
> -            return -1;
> +            goto error;
>          }
>      }
>  
> @@ -2604,14 +2621,13 @@ static int reds_init_net(RedsState *reds)
>          reds->secure_listen_socket =
>          reds_init_socket(reds->config->spice_addr,
>          reds->config->spice_secure_port,
>                                                        reds->config->spice_family);
>          if (-1 == reds->secure_listen_socket) {
> -            return -1;
> +            goto error;
>          }
>          reds->secure_listen_watch = reds_core_watch_add(reds,
>          reds->secure_listen_socket,
>                                                          SPICE_WATCH_EVENT_READ,
>                                                          reds_accept_ssl_connection,
>                                                          reds);
>          if (reds->secure_listen_watch == NULL) {
> -            spice_warning("set fd handle failed");
> -            return -1;
> +            goto error;
>          }
>      }
>  
> @@ -2621,11 +2637,15 @@ static int reds_init_net(RedsState *reds)
>                                                   SPICE_WATCH_EVENT_READ,
>                                                   reds_accept, reds);
>          if (reds->listen_watch == NULL) {
> -            spice_warning("set fd handle failed");
> -            return -1;
> +            reds->listen_socket = -1;

Is this necessary taking into account the check in reds_cleanup_net

> +            goto error;
>          }
>      }
>      return 0;
> +
> +error:
> +    reds_cleanup_net(reds);

Maybe this call can be in the err label inside do_spice_init?
This would reduce reds_init_net changes and improve do_spice_init too.

> +    return -1;
>  }
>  
>  static int load_dh_params(SSL_CTX *ctx, char *file)
> @@ -3632,16 +3652,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>      if (reds->main_dispatcher) {
>          g_object_unref(reds->main_dispatcher);
>      }
> -    if (reds->listen_socket != -1) {
> -       reds_core_watch_remove(reds, reds->listen_watch);
> -       if (reds->config->spice_listen_socket_fd != reds->listen_socket) {
> -          close(reds->listen_socket);
> -       }
> -    }
> -    if (reds->secure_listen_socket != -1) {
> -       reds_core_watch_remove(reds, reds->secure_listen_watch);
> -       close(reds->secure_listen_socket);
> -    }
> +    reds_cleanup_net(reds);
>      g_clear_object(&reds->agent_dev);
>      spice_buffer_free(&reds->client_monitors_config);
>      red_record_unref(reds->record);

Frediano