From patchwork Tue May 4 13:49:50 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Reinhard Max X-Patchwork-Id: 51616 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1F383B7D28 for ; Wed, 5 May 2010 01:09:18 +1000 (EST) Received: from localhost ([127.0.0.1]:59524 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9Jjs-0000sQ-3y for incoming@patchwork.ozlabs.org; Tue, 04 May 2010 11:08:48 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O9IVf-000299-L9 for qemu-devel@nongnu.org; Tue, 04 May 2010 09:50:03 -0400 Received: from [140.186.70.92] (port=53042 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O9IVb-00022t-QO for qemu-devel@nongnu.org; Tue, 04 May 2010 09:50:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O9IVY-00045z-4P for qemu-devel@nongnu.org; Tue, 04 May 2010 09:49:59 -0400 Received: from cantor2.suse.de ([195.135.220.15]:40970 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O9IVX-00044d-FM for qemu-devel@nongnu.org; Tue, 04 May 2010 09:49:56 -0400 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) by mx2.suse.de (Postfix) with ESMTP id 19C3A87104 for ; Tue, 4 May 2010 15:49:53 +0200 (CEST) Date: Tue, 4 May 2010 15:49:50 +0200 (CEST) From: Reinhard Max To: qemu-devel@nongnu.org Message-ID: User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4-2.6 X-Mailman-Approved-At: Tue, 04 May 2010 11:01:21 -0400 Subject: [Qemu-devel] Patch to improve handling of server sockets X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Hi, I am maintaining the tightvnc package for openSUSE and was recently confronted with an alleged vnc problem with QWMU that turned out to be a shortcoming in QEMU's code for handling TCP server sockets, which is used by the vnc and char modules. The problem occurs when the address to listen on is given as a name which resolves to multiple IP addresses the most prominent example being "localhost" resolving to 127.0.0.1 and ::1 . The existing code stopped walking the list of addresses returned by getaddrinfo() as soon as one socket was successfully opened and bound. The result was that a qemu instance started with "-vnc localhost:42" only listened on ::1, wasn't reachable through 127.0.0.1. The fact that the code set the IPV6_V6ONLY socket option didn't help, because that option only works when the socket gets bound to the IPv6 wildcard address (::), but is useless for explicit address bindings. The attached patch against QEMU 0.11.0 extends inet_listen() to create sockets for as many addresses from the address list as possible and adapts its callers and their data structures to deal with a linked list of socket FDs rather than a single file descriptor. So far I've only done some testing with the -vnc option. More testing is needed in the qemu-char area and for the parts of the code that get triggered from QEMU's Monitor. Please review and comment. cu Reinhard P.S. Please keep me in Cc when replying. Index: qemu-char.c =================================================================== --- qemu-char.c.orig +++ qemu-char.c @@ -1831,7 +1831,8 @@ return_err: /* TCP Net console */ typedef struct { - int fd, listen_fd; + int fd; + FdList *listen_fds; int connected; int max_size; int do_telnetopt; @@ -1983,6 +1984,7 @@ static void tcp_chr_read(void *opaque) TCPCharDriver *s = chr->opaque; uint8_t buf[1024]; int len, size; + FdList *fdl; if (!s->connected || s->max_size <= 0) return; @@ -1993,10 +1995,9 @@ static void tcp_chr_read(void *opaque) if (size == 0) { /* connection closed */ s->connected = 0; - if (s->listen_fd >= 0) { - qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr); - } - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + for (fdl = s->listen_fds; fdl != NULL; fdl = fdl->next) + qemu_set_fd_handler(fdl->fd, tcp_chr_accept, NULL, fdl); + qemu_set_fd_handler(s->fd, NULL, NULL, NULL); closesocket(s->fd); s->fd = -1; } else if (size > 0) { @@ -2045,7 +2046,8 @@ static void socket_set_nodelay(int fd) static void tcp_chr_accept(void *opaque) { - CharDriverState *chr = opaque; + FdList *fdl = opaque; + CharDriverState *chr = fdl->opaque; TCPCharDriver *s = chr->opaque; struct sockaddr_in saddr; #ifndef _WIN32 @@ -2066,7 +2068,7 @@ static void tcp_chr_accept(void *opaque) len = sizeof(saddr); addr = (struct sockaddr *)&saddr; } - fd = accept(s->listen_fd, addr, &len); + fd = accept(fdl->fd, addr, &len); if (fd < 0 && errno != EINTR) { return; } else if (fd >= 0) { @@ -2079,20 +2081,24 @@ static void tcp_chr_accept(void *opaque) if (s->do_nodelay) socket_set_nodelay(fd); s->fd = fd; - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL); + for (fdl = s->listen_fds; fdl != NULL; fdl = fdl->next) + qemu_set_fd_handler(fdl->fd, NULL, NULL, NULL); tcp_chr_connect(chr); } static void tcp_chr_close(CharDriverState *chr) { TCPCharDriver *s = chr->opaque; + FdList *fdl, *fdtmp; if (s->fd >= 0) { qemu_set_fd_handler(s->fd, NULL, NULL, NULL); closesocket(s->fd); } - if (s->listen_fd >= 0) { - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL); - closesocket(s->listen_fd); + for (fdl = s->listen_fds; fdl != NULL; fdl = fdtmp) { + fdtmp = fdl->next; + qemu_set_fd_handler(fdl->fd, NULL, NULL, NULL); + closesocket(fdl->fd); + free(fdl); } qemu_free(s); } @@ -2108,6 +2114,7 @@ static CharDriverState *qemu_chr_open_tc int is_waitconnect = 1; int do_nodelay = 0; const char *ptr; + FdList *sockets = NULL, *fdl, *fdtmp; ptr = host_str; while((ptr = strchr(ptr,','))) { @@ -2155,11 +2162,18 @@ static CharDriverState *qemu_chr_open_tc } else { if (is_listen) { fd = inet_listen(host_str, chr->filename + offset, 256 - offset, - SOCK_STREAM, 0); + SOCK_STREAM, 0, &sockets); } else { fd = inet_connect(host_str, SOCK_STREAM); } } + + if (sockets == NULL) { + sockets = malloc(sizeof(*sockets)); + sockets->next = NULL; + sockets->fd = fd; + } + if (fd < 0) goto fail; @@ -2168,7 +2182,7 @@ static CharDriverState *qemu_chr_open_tc s->connected = 0; s->fd = -1; - s->listen_fd = -1; + s->listen_fds = NULL; s->msgfd = -1; s->is_unix = is_unix; s->do_nodelay = do_nodelay && !is_unix; @@ -2179,8 +2193,11 @@ static CharDriverState *qemu_chr_open_tc chr->get_msgfd = tcp_get_msgfd; if (is_listen) { - s->listen_fd = fd; - qemu_set_fd_handler(s->listen_fd, tcp_chr_accept, NULL, chr); + s->listen_fds = sockets; + for (fdl = sockets; fdl != NULL; fdl = fdl->next) { + fdl->opaque = chr; + qemu_set_fd_handler(fdl->fd, tcp_chr_accept, NULL, fdl); + } if (is_telnet) s->do_telnetopt = 1; } else { @@ -2191,16 +2208,21 @@ static CharDriverState *qemu_chr_open_tc } if (is_listen && is_waitconnect) { + FdList *fdl; printf("QEMU waiting for connection on: %s\n", chr->filename ? chr->filename : host_str); tcp_chr_accept(chr); - socket_set_nonblock(s->listen_fd); + for (fdl = s->listen_fds; fdl != NULL; fdl = fdl->next) + socket_set_nonblock(fdl->fd); } return chr; fail: - if (fd >= 0) - closesocket(fd); + for (fdl = sockets; fdl != NULL; fdl = fdtmp) { + fdtmp = fdl->next; + closesocket(fdl->fd); + free(fdl); + } qemu_free(s); qemu_free(chr); return NULL; Index: qemu-sockets.c =================================================================== --- qemu-sockets.c.orig +++ qemu-sockets.c @@ -89,7 +89,7 @@ static void inet_print_addrinfo(const ch } int inet_listen(const char *str, char *ostr, int olen, - int socktype, int port_offset) + int socktype, int port_offset, FdList **sockets) { struct addrinfo ai,*res,*e; char addr[64]; @@ -97,8 +97,11 @@ int inet_listen(const char *str, char *o char uaddr[INET6_ADDRSTRLEN+1]; char uport[33]; const char *opts, *h; - int slisten,rc,pos,to,try_next; + int slisten,rc,pos,to,try_next,portno; + int retval = 0; + FdList *fdcur = NULL, *fdnew; + *sockets = NULL; memset(&ai,0, sizeof(ai)); ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; ai.ai_family = PF_UNSPEC; @@ -174,9 +177,9 @@ int inet_listen(const char *str, char *o setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); #ifdef IPV6_V6ONLY if (e->ai_family == PF_INET6) { - /* listen on both ipv4 and ipv6 */ - setsockopt(slisten,IPPROTO_IPV6,IPV6_V6ONLY,(void*)&off, - sizeof(off)); + /* listen on IPv6 only, IPv4 is handled by a separate socket */ + setsockopt(slisten,IPPROTO_IPV6,IPV6_V6ONLY,(void*)&on, + sizeof(on)); } #endif @@ -184,8 +187,22 @@ int inet_listen(const char *str, char *o if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { if (sockets_debug) fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__, - inet_strfamily(e->ai_family), uaddr, inet_getport(e)); - goto listen; + inet_strfamily(e->ai_family), uaddr, inet_getport(e)); + if (listen(slisten,1) != 0) { + closesocket(slisten); + } else { + fdnew = malloc(sizeof(*fdnew)); + fdnew->fd = slisten; + fdnew->next = NULL; + if (fdcur == NULL) { + *sockets = fdnew; + } else { + fdcur->next = fdnew; + } + fdcur = fdnew; + portno = inet_getport(e); + } + break; } try_next = to && (inet_getport(e) <= to + port_offset); if (!try_next || sockets_debug) @@ -196,32 +213,18 @@ int inet_listen(const char *str, char *o inet_setport(e, inet_getport(e) + 1); continue; } + closesocket(slisten); break; } - closesocket(slisten); } - fprintf(stderr, "%s: FAILED\n", __FUNCTION__); - freeaddrinfo(res); - return -1; - -listen: - if (listen(slisten,1) != 0) { - perror("listen"); - closesocket(slisten); - freeaddrinfo(res); - return -1; - } - if (ostr) { - if (e->ai_family == PF_INET6) { - snprintf(ostr, olen, "[%s]:%d%s", uaddr, - inet_getport(e) - port_offset, opts); - } else { - snprintf(ostr, olen, "%s:%d%s", uaddr, - inet_getport(e) - port_offset, opts); - } + if (fdcur == NULL) { + fprintf(stderr, "%s: FAILED\n", __FUNCTION__); + retval = -1; + } else if (ostr) { + snprintf(ostr, olen, "%s:%d%s", addr, portno - port_offset, opts); } freeaddrinfo(res); - return slisten; + return retval; } int inet_connect(const char *str, int socktype) Index: qemu_socket.h =================================================================== --- qemu_socket.h.orig +++ qemu_socket.h @@ -29,13 +29,20 @@ int inet_aton(const char *cp, struct in_ #endif /* !_WIN32 */ +struct FdList; +typedef struct FdList { + int fd; + void *opaque; + struct FdList *next; +} FdList; + /* misc helpers */ void socket_set_nonblock(int fd); int send_all(int fd, const void *buf, int len1); /* New, ipv6-ready socket helper functions, see qemu-sockets.c */ int inet_listen(const char *str, char *ostr, int olen, - int socktype, int port_offset); + int socktype, int port_offset, FdList **sockets); int inet_connect(const char *str, int socktype); int unix_listen(const char *path, char *ostr, int olen); Index: vnc.c =================================================================== --- vnc.c.orig +++ vnc.c @@ -26,7 +26,6 @@ #include "vnc.h" #include "sysemu.h" -#include "qemu_socket.h" #include "qemu-timer.h" #include "acl.h" @@ -181,15 +180,18 @@ void do_info_vnc(Monitor *mon) if (vnc_display == NULL || vnc_display->display == NULL) { monitor_printf(mon, "Server: disabled\n"); } else { - char *serverAddr = vnc_socket_local_addr(" address: %s:%s\n", - vnc_display->lsock); - - if (!serverAddr) - return; - + FdList *fdl; + char *serverAddr; + monitor_printf(mon, "Server:\n"); - monitor_printf(mon, "%s", serverAddr); - free(serverAddr); + for (fdl = vnc_display->lsocks; fdl != NULL; fdl = fdl->next) { + serverAddr = vnc_socket_local_addr(" address: %s:%s\n", + fdl->fd); + if (!serverAddr) + continue; + monitor_printf(mon, "%s", serverAddr); + free(serverAddr); + } monitor_printf(mon, " auth: %s\n", vnc_auth_name(vnc_display)); if (vnc_display->clients) { @@ -2113,14 +2115,15 @@ static void vnc_connect(VncDisplay *vd, static void vnc_listen_read(void *opaque) { - VncDisplay *vs = opaque; + FdList *fds = opaque; + VncDisplay *vs = fds->opaque; struct sockaddr_in addr; socklen_t addrlen = sizeof(addr); /* Catch-up */ vga_hw_update(); - int csock = accept(vs->lsock, (struct sockaddr *)&addr, &addrlen); + int csock = accept(fds->fd, (struct sockaddr *)&addr, &addrlen); if (csock != -1) { vnc_connect(vs, csock); } @@ -2136,7 +2139,7 @@ void vnc_display_init(DisplayState *ds) dcl->idle = 1; vnc_display = vs; - vs->lsock = -1; + vs->lsocks = NULL; vs->ds = ds; @@ -2159,6 +2162,7 @@ void vnc_display_init(DisplayState *ds) void vnc_display_close(DisplayState *ds) { VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display; + FdList *fds, *fdtmp; if (!vs) return; @@ -2166,11 +2170,13 @@ void vnc_display_close(DisplayState *ds) qemu_free(vs->display); vs->display = NULL; } - if (vs->lsock != -1) { - qemu_set_fd_handler2(vs->lsock, NULL, NULL, NULL, NULL); - close(vs->lsock); - vs->lsock = -1; + for (fds = vs->lsocks; fds != NULL; fds = fdtmp) { + fdtmp = fds->next; + qemu_set_fd_handler2(fds->fd, NULL, NULL, NULL, NULL); + close(fds->fd); + free(fds); } + vs->lsocks = NULL; vs->auth = VNC_AUTH_INVALID; #ifdef CONFIG_VNC_TLS vs->subauth = VNC_AUTH_INVALID; @@ -2207,7 +2213,8 @@ char *vnc_display_local_addr(DisplayStat { VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display; - return vnc_socket_local_addr("%s:%s", vs->lsock); + /* FIXME: should return all addresses */ + return vnc_socket_local_addr("%s:%s", vs->lsocks->fd); } int vnc_display_open(DisplayState *ds, const char *display) @@ -2225,7 +2232,8 @@ int vnc_display_open(DisplayState *ds, c int saslErr; #endif int acl = 0; - + FdList *socks = NULL, *fds; + if (!vnc_display) return -1; vnc_display_close(ds); @@ -2391,39 +2399,52 @@ int vnc_display_open(DisplayState *ds, c #endif if (reverse) { + int sock; /* connect to viewer */ if (strncmp(display, "unix:", 5) == 0) - vs->lsock = unix_connect(display+5); + sock = unix_connect(display+5); else - vs->lsock = inet_connect(display, SOCK_STREAM); - if (-1 == vs->lsock) { + sock = inet_connect(display, SOCK_STREAM); + if (-1 == sock) { free(vs->display); vs->display = NULL; return -1; } else { - int csock = vs->lsock; - vs->lsock = -1; - vnc_connect(vs, csock); + vnc_connect(vs, sock); } return 0; } else { /* listen for connects */ char *dpy; + int fd; + dpy = qemu_malloc(256); if (strncmp(display, "unix:", 5) == 0) { pstrcpy(dpy, 256, "unix:"); - vs->lsock = unix_listen(display+5, dpy+5, 256-5); + fd = unix_listen(display+5, dpy+5, 256-5); } else { - vs->lsock = inet_listen(display, dpy, 256, SOCK_STREAM, 5900); + fd = inet_listen(display, dpy, 256, SOCK_STREAM, 5900, + &socks); } - if (-1 == vs->lsock) { + + if (-1 == fd) { free(dpy); return -1; } else { + if (socks == NULL) { + socks = malloc(sizeof(*socks)); + socks->fd = fd; + socks->next = NULL; + } + vs->lsocks = socks; free(vs->display); vs->display = dpy; } } - return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs); + for (fds = socks; fds != NULL; fds = fds->next) { + fds->opaque = vs; + qemu_set_fd_handler2(fds->fd, NULL, vnc_listen_read, NULL, fds); + } + return 0; } Index: vnc.h =================================================================== --- vnc.h.orig +++ vnc.h @@ -28,6 +28,7 @@ #define __QEMU_VNC_H #include "qemu-common.h" +#include "qemu_socket.h" #include "console.h" #include "monitor.h" #include "audio/audio.h" @@ -87,7 +88,7 @@ typedef struct VncDisplay VncDisplay; struct VncDisplay { - int lsock; + FdList *lsocks; DisplayState *ds; VncState *clients; kbd_layout_t *kbd_layout;