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

[Christophe Fergeau]

On Thu, Mar 08, 2018 at 05:50:29AM -0500, Frediano Ziglio wrote:
> > 
> > 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.

Good idea, I sent a patch doing that, though on second though, I would
have kept that error label doing the cleanup in reds_init_net, and added
a call to reds_cleanup_net(reds) in the err: label in do_spice_init().
Reason for that is that when do_foo() fails, I prefer if it cleans up
whatever it was trying to do. With v3, the caller has to call a method
to do the cleanup when this fails. Not a big deal, I don't think I'm
going to send a v4 just for that if people are fine with v3.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180308/438f8401/attachment.sig>