Re: [Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

2018-03-07 Thread Christophe Fergeau 
On Wed, Mar 07, 2018 at 12:01:48PM +0200, Uri Lublin wrote:
> On 03/06/2018 03:53 PM, Christophe Fergeau 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 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.
> > ---
> >   server/reds.c | 5 +
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/server/reds.c b/server/reds.c
> > index a31ed4e96..7867e8573 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -2595,6 +2595,8 @@ static int reds_init_net(RedsState *reds)
> >SPICE_WATCH_EVENT_READ,
> >reds_accept, reds);
> >   if (reds->listen_watch == NULL) {
> > +close(reds->listen_socket);
> > +reds->listen_socket = -1;
> >   spice_warning("set fd handle failed");
> >   return -1;
> >   }
> > @@ -2610,6 +2612,8 @@ static int reds_init_net(RedsState *reds)
> >   
> > SPICE_WATCH_EVENT_READ,
> >   
> > reds_accept_ssl_connection, reds);
> >   if (reds->secure_listen_watch == NULL) {
> > +close(reds->secure_listen_socket);
> > +reds->secure_listen_socket = -1;
> 
> Looks good.
> 
> Note that it still does not clean up everything.
> For example it may be that listen_socket stays open.

Indeed, good point, I'll clean up more things ;)

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

2018-03-07 Thread Uri Lublin 

On 03/06/2018 03:53 PM, Christophe Fergeau 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 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.
---
  server/reds.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/server/reds.c b/server/reds.c
index a31ed4e96..7867e8573 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2595,6 +2595,8 @@ static int reds_init_net(RedsState *reds)
   SPICE_WATCH_EVENT_READ,
   reds_accept, reds);
  if (reds->listen_watch == NULL) {
+close(reds->listen_socket);
+reds->listen_socket = -1;
  spice_warning("set fd handle failed");
  return -1;
  }
@@ -2610,6 +2612,8 @@ static int reds_init_net(RedsState *reds)
  
SPICE_WATCH_EVENT_READ,
  
reds_accept_ssl_connection, reds);
  if (reds->secure_listen_watch == NULL) {
+close(reds->secure_listen_socket);
+reds->secure_listen_socket = -1;


Looks good.

Note that it still does not clean up everything.
For example it may be that listen_socket stays open.

Uri.


  spice_warning("set fd handle failed");
  return -1;
  }
@@ -2621,6 +2625,7 @@ static int reds_init_net(RedsState *reds)
   SPICE_WATCH_EVENT_READ,
   reds_accept, reds);
  if (reds->listen_watch == NULL) {
+reds->listen_socket = -1;
  spice_warning("set fd handle failed");
  return -1;
  }



___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

2018-03-06 Thread Christophe Fergeau 
On Tue, Mar 06, 2018 at 11:38:15AM -0500, Frediano Ziglio wrote:
> Looks fine.
> Actually when this function returns -1 spice_server_init returns -1 and Qemu
> calls exit so there's no much difference at runtime.
> But I suppose this is necessary for your tests.

I did not try the tests without this patch, it's something I noticed
before starting writing them. Maybe the tests are fine without this.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

2018-03-06 Thread 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 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.
> ---
>  server/reds.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index a31ed4e96..7867e8573 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2595,6 +2595,8 @@ static int reds_init_net(RedsState *reds)
>   SPICE_WATCH_EVENT_READ,
>   reds_accept, reds);
>  if (reds->listen_watch == NULL) {
> +close(reds->listen_socket);
> +reds->listen_socket = -1;
>  spice_warning("set fd handle failed");
>  return -1;
>  }
> @@ -2610,6 +2612,8 @@ static int reds_init_net(RedsState *reds)
>  
> SPICE_WATCH_EVENT_READ,
>  
> reds_accept_ssl_connection,
>  reds);
>  if (reds->secure_listen_watch == NULL) {
> +close(reds->secure_listen_socket);
> +reds->secure_listen_socket = -1;
>  spice_warning("set fd handle failed");
>  return -1;
>  }
> @@ -2621,6 +2625,7 @@ static int reds_init_net(RedsState *reds)
>   SPICE_WATCH_EVENT_READ,
>   reds_accept, reds);
>  if (reds->listen_watch == NULL) {
> +reds->listen_socket = -1;
>  spice_warning("set fd handle failed");
>  return -1;
>  }

Looks fine.
Actually when this function returns -1 spice_server_init returns -1 and Qemu
calls exit so there's no much difference at runtime.
But I suppose this is necessary for your tests.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel