[Qemu-devel] [PATCH]: VNC: set listener socket to non-blocking mode

[Qemu-devel] [PATCH]: VNC: set listener socket to non-blocking mode

[Gianni Tedesco]

This prevents qemu from hanging waiting for a client to connect. I have
reproduced this when doing a loadvm but it may be a more general problem
in that poll/accept may race if a client aborts the connection with a
RST before the accept has completed. In either case the fix seems
harmless.

Signed-off-by: Gianni Tedesco <gianni.tedesco@citrix.com>

diff --git a/ui/vnc.c b/ui/vnc.c
index 864342e..172b988 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2736,5 +2736,6 @@ int vnc_display_open(DisplayState *ds, const char *display)
             vs->display = dpy;
         }
     }
+    socket_set_nonblock(vs->lsock);
     return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
 }

Re: [Qemu-devel] [PATCH]: VNC: set listener socket to non-blocking mode

[Anthony Liguori]

On 10/21/2010 11:18 AM, Gianni Tedesco wrote:
> This prevents qemu from hanging waiting for a client to connect. I have
> reproduced this when doing a loadvm but it may be a more general problem
> in that poll/accept may race if a client aborts the connection with a
> RST before the accept has completed. In either case the fix seems
> harmless.
>
> Signed-off-by: Gianni Tedesco<gianni.tedesco@citrix.com>
>    

I'd feel a little better with explicit error handling in the accept() 
path and a comment explaining why this was necessary.

But otherwise, nice catch!

Regards,

Anthony Liguori

> diff --git a/ui/vnc.c b/ui/vnc.c
> index 864342e..172b988 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2736,5 +2736,6 @@ int vnc_display_open(DisplayState *ds, const char *display)
>               vs->display = dpy;
>           }
>       }
> +    socket_set_nonblock(vs->lsock);
>       return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
>   }
>
>
>
>    

Re: [Qemu-devel] [PATCH]: VNC: set listener socket to non-blocking mode

[Gianni Tedesco]

On Thu, 2010-10-21 at 17:44 +0100, Anthony Liguori wrote:
> On 10/21/2010 11:18 AM, Gianni Tedesco wrote:
> > This prevents qemu from hanging waiting for a client to connect. I have
> > reproduced this when doing a loadvm but it may be a more general problem
> > in that poll/accept may race if a client aborts the connection with a
> > RST before the accept has completed. In either case the fix seems
> > harmless.
> >
> > Signed-off-by: Gianni Tedesco<gianni.tedesco@citrix.com>
> >    
> 
> I'd feel a little better with explicit error handling in the accept() 
> path and a comment explaining why this was necessary.
> 
> But otherwise, nice catch!

Thanks!

Not sure what you mean about the explicit error handling in the accept()
path though? Accept can fail for a few reasons, the most worrying is
EMFILE which easily leads to livelock. Poll reports listener as ready to
accept but accept fails because we maxed out the fd table so go back to
poll and the listener is still ready to accept etc... It's not always
clear how to handle specific accept() errors.

I think only calling vnc_connect() when we have an fd is the best we can
hope for right now.

Unless I misunderstood?

-----8<-----8<-----8<-----

diff --git a/ui/vnc.c b/ui/vnc.c
index 864342e..65dc55c 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2736,5 +2736,9 @@ int vnc_display_open(DisplayState *ds, const char *display)
             vs->display = dpy;
         }
     }
+    /* necessary to prevent accept() hanging indefinitely if a clients
+     * connection-reset wins the race between poll() and accept()
+     */
+    socket_set_nonblock(vs->lsock);
     return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
 }