From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:56646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RummA-0007Vn-CD for qemu-devel@nongnu.org; Tue, 07 Feb 2012 10:16:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rumm5-0006cB-BE for qemu-devel@nongnu.org; Tue, 07 Feb 2012 10:16:10 -0500 Received: from e9.ny.us.ibm.com ([32.97.182.139]:38665) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rumm4-0006bn-LG for qemu-devel@nongnu.org; Tue, 07 Feb 2012 10:16:04 -0500 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 7 Feb 2012 10:16:03 -0500 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 21C6A6E8204 for ; Tue, 7 Feb 2012 10:13:23 -0500 (EST) Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q17FDI29252034 for ; Tue, 7 Feb 2012 10:13:20 -0500 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q17FDEc4001286 for ; Tue, 7 Feb 2012 10:13:14 -0500 Message-ID: <4F313F86.7080103@us.ibm.com> Date: Tue, 07 Feb 2012 09:13:10 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1328623766-12287-1-git-send-email-armbru@redhat.com> <1328623766-12287-10-git-send-email-armbru@redhat.com> In-Reply-To: <1328623766-12287-10-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org On 02/07/2012 08:09 AM, Markus Armbruster wrote: > Convert to error_report(). This adds error locations to the messages, > which is particularly important when the location is buried in a > configuration file. Moreover, we'll need this when we create a > monitor command to add character devices, so its errors actually > appear in the monitor, not stderr. > > Also clean up the messages, and get rid of some that look like errors, > but aren't. > > Improves user-hostile messages like this one for "-chardev > socket,id=foo,host=blackfin,port=1,server" > > inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied > chardev: opening backend "socket" failed > > to > > qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server: Can't bind port blackfin:1: Permission denied > chardev: opening backend "socket" failed > > and this one for "-chardev udp,id=foo,localport=1,port=1" > > inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK > inet_dgram_opts failed > chardev: opening backend "udp" failed > > to > > qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind port :1: Permission denied > chardev: opening backend "udp" failed > > You got to love the "OK" part. > > The uninformative extra "opening backend failed" message will be > cleaned up shortly. > > Signed-off-by: Markus Armbruster > --- > qemu-char.c | 1 - > qemu-sockets.c | 154 +++++++++++++++++++++++++------------------------------ > 2 files changed, 70 insertions(+), 85 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index bb9e3f5..d591f70 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) > > fd = inet_dgram_opts(opts); > if (fd< 0) { > - fprintf(stderr, "inet_dgram_opts failed\n"); > goto return_err; > } Let's add an Error ** argument here. It's easy to bridge errors to error_report (qerror_report_err) so it's conducive to incremental refactoring. Plus it starts getting us away from terminal errors and into proper erroro propagation. > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 6bcb8e3..67e0559 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -20,7 +20,8 @@ > #include > > #include "qemu_socket.h" > -#include "qemu-common.h" /* for qemu_isdigit */ > +#include "qemu-common.h" > +#include "qemu-error.h" > > #ifndef AI_ADDRCONFIG > # define AI_ADDRCONFIG 0 > @@ -107,7 +108,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > char port[33]; > char uaddr[INET6_ADDRSTRLEN+1]; > char uport[33]; > - int slisten, rc, to, port_min, port_max, p; > + int slisten, rc, to, port_min, port_max, p, sav_errno; > > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; > @@ -116,7 +117,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > > if ((qemu_opt_get(opts, "host") == NULL) || > (qemu_opt_get(opts, "port") == NULL)) { > - fprintf(stderr, "%s: host and/or port not specified\n", __FUNCTION__); > + error_report("inet socket character device requires parameters host and port"); > return -1; > } > pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port")); > @@ -133,8 +134,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > snprintf(port, sizeof(port), "%d", atoi(port) + port_offset); > rc = getaddrinfo(strlen(addr) ? addr : NULL, port,&ai,&res); > if (rc != 0) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + error_report("Can't resolve %s:%s: %s", > + addr, port, gai_strerror(rc)); > return -1; > } > > @@ -145,9 +146,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > NI_NUMERICHOST | NI_NUMERICSERV); > slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > if (slisten< 0) { > - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), strerror(errno)); > - continue; > + continue; /* try next address */ > } > > setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > @@ -166,25 +165,33 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > goto listen; > } > - if (p == port_max) { > - fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), uaddr, inet_getport(e), > - strerror(errno)); > - } > } > + > + sav_errno = errno; > closesocket(slisten); > + errno = sav_errno; > + /* try next address */ > + } > + > + /* no address worked, errno is from last failed socket() or bind() */ > + if (to) { > + error_report("Can't bind any port %s:%s..%d: %s", > + addr, port, to, strerror(errno)); > + } else { > + error_report("Can't bind port %s:%s: %s", > + addr, port, strerror(errno)); > } > - fprintf(stderr, "%s: FAILED\n", __FUNCTION__); > freeaddrinfo(res); > return -1; > > listen: > if (listen(slisten,1) != 0) { > - perror("listen"); > + error_report("Can't listen on %s:%d: %s", addr, p, strerror(errno)); > closesocket(slisten); > freeaddrinfo(res); > return -1; > } > + > snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset); > qemu_opt_set(opts, "host", uaddr); > qemu_opt_set(opts, "port", uport); > @@ -199,8 +206,6 @@ int inet_connect_opts(QemuOpts *opts) > struct addrinfo ai,*res,*e; > const char *addr; > const char *port; > - char uaddr[INET6_ADDRSTRLEN+1]; > - char uport[33]; > int sock,rc; > > memset(&ai,0, sizeof(ai)); > @@ -211,7 +216,7 @@ int inet_connect_opts(QemuOpts *opts) > addr = qemu_opt_get(opts, "host"); > port = qemu_opt_get(opts, "port"); > if (addr == NULL || port == NULL) { > - fprintf(stderr, "inet_connect: host and/or port not specified\n"); > + error_report("inet socket character device requires parameters host and port"); > return -1; > } > > @@ -222,38 +227,29 @@ int inet_connect_opts(QemuOpts *opts) > > /* lookup */ > if (0 != (rc = getaddrinfo(addr, port,&ai,&res))) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + error_report("Can't resolve %s:%s: %s", > + addr, port, gai_strerror(rc)); > return -1; > } > > for (e = res; e != NULL; e = e->ai_next) { > - if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > - uaddr,INET6_ADDRSTRLEN,uport,32, > - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { > - fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__); > - continue; > - } > sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > if (sock< 0) { > - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), strerror(errno)); > - continue; > + continue; /* try next address */ > } > setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > > /* connect to peer */ > if (connect(sock,e->ai_addr,e->ai_addrlen)< 0) { > - if (NULL == e->ai_next) > - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), > - e->ai_canonname, uaddr, uport, strerror(errno)); > closesocket(sock); > - continue; > + continue; /* try next address */ > } > freeaddrinfo(res); > return sock; > } > + > + /* no address worked, errno is from last failed socket() or connect() */ > + error_report("Can't connect to %s:%s: %s", addr, port, strerror(errno)); > freeaddrinfo(res); > return -1; > } > @@ -261,12 +257,9 @@ int inet_connect_opts(QemuOpts *opts) > int inet_dgram_opts(QemuOpts *opts) > { > struct addrinfo ai, *peer = NULL, *local = NULL; > - const char *addr; > - const char *port; > - char uaddr[INET6_ADDRSTRLEN+1]; > - char uport[33]; > + const char *addr, *port; > + const char *localaddr, *localport; > int sock = -1, rc; > - > /* lookup peer addr */ > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; > @@ -279,7 +272,7 @@ int inet_dgram_opts(QemuOpts *opts) > addr = "localhost"; > } > if (port == NULL || strlen(port) == 0) { > - fprintf(stderr, "inet_dgram: port not specified\n"); > + error_report("udp character device requires parameter port"); > return -1; > } > > @@ -289,8 +282,8 @@ int inet_dgram_opts(QemuOpts *opts) > ai.ai_family = PF_INET6; > > if (0 != (rc = getaddrinfo(addr, port,&ai,&peer))) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + error_report("Can't resolve %s:%s: %s", > + addr, port, gai_strerror(rc)); > return -1; > } > > @@ -300,53 +293,40 @@ int inet_dgram_opts(QemuOpts *opts) > ai.ai_family = peer->ai_family; > ai.ai_socktype = SOCK_DGRAM; > > - addr = qemu_opt_get(opts, "localaddr"); > - port = qemu_opt_get(opts, "localport"); > - if (addr == NULL || strlen(addr) == 0) { > - addr = NULL; > + localaddr = qemu_opt_get(opts, "localaddr"); > + localport = qemu_opt_get(opts, "localport"); > + if (localaddr == NULL || strlen(localaddr) == 0) { > + localaddr = NULL; > } > - if (!port || strlen(port) == 0) > - port = "0"; > + if (!localport || strlen(localport) == 0) > + localport = "0"; > > - if (0 != (rc = getaddrinfo(addr, port,&ai,&local))) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + if (0 != (rc = getaddrinfo(localaddr, localport,&ai,&local))) { > + error_report("Can't resolve %s:%s: %s", > + localaddr ? localaddr : NULL, localport, > + gai_strerror(rc)); > return -1; > } > > /* create socket */ > sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol); > if (sock< 0) { > - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > - inet_strfamily(peer->ai_family), strerror(errno)); > - goto err; > + goto cant_bind; > } > setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > > /* bind socket */ > - if (getnameinfo((struct sockaddr*)local->ai_addr,local->ai_addrlen, > - uaddr,INET6_ADDRSTRLEN,uport,32, > - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { > - fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__); > - goto err; > - } > if (bind(sock, local->ai_addr, local->ai_addrlen)< 0) { > - fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__, > - inet_strfamily(local->ai_family), uaddr, inet_getport(local)); > + cant_bind: > + error_report("Can't bind port %s:%s: %s", > + localaddr ? localaddr : "", localport, strerror(errno)); > goto err; > } > > /* connect to peer */ > - if (getnameinfo((struct sockaddr*)peer->ai_addr, peer->ai_addrlen, > - uaddr, INET6_ADDRSTRLEN, uport, 32, > - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { > - fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__); > - goto err; > - } > if (connect(sock,peer->ai_addr,peer->ai_addrlen)< 0) { > - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > - inet_strfamily(peer->ai_family), > - peer->ai_canonname, uaddr, uport, strerror(errno)); > + error_report("Can't connect to %s:%s: %s", > + addr, port, strerror(errno)); > goto err; > } > > @@ -471,8 +451,7 @@ int unix_listen_opts(QemuOpts *opts) > > sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); > if (sock< 0) { > - perror("socket(unix)"); > - return -1; > + goto err; > } > > memset(&un, 0, sizeof(un)); > @@ -496,18 +475,20 @@ int unix_listen_opts(QemuOpts *opts) > > unlink(un.sun_path); > if (bind(sock, (struct sockaddr*)&un, sizeof(un))< 0) { > - fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno)); > goto err; > } > if (listen(sock, 1)< 0) { > - fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno)); > goto err; > } > > return sock; > > err: > - closesocket(sock); > + error_report("Can't create socket %s: %s", > + un.sun_path, strerror(errno)); > + if (sock>= 0) { > + closesocket(sock); > + } > return -1; > } > > @@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts) > int sock; > > if (NULL == path) { > - fprintf(stderr, "unix connect: no path specified\n"); > + error_report("unix socket character device requires parameter path"); > return -1; > } > > sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); > if (sock< 0) { > - perror("socket(unix)"); > - return -1; > + goto err; > } > > memset(&un, 0, sizeof(un)); > un.sun_family = AF_UNIX; > snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > if (connect(sock, (struct sockaddr*)&un, sizeof(un))< 0) { > - fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno)); > - close(sock); > - return -1; > + goto err; > } > > return sock; > + > +err: > + error_report("Can't connect to socket %s: %s", > + un.sun_path, strerror(errno)); > + if (sock>= 0) { > + close(sock); > + } > + return -1; > } Basically, same thing here and the remaining functions. Let's not introduce additional uses of error_report(). That said, I imagine you don't want to introduce a bunch of error types for these different things and that's probably not productive anyway. So let's compromise and introduce a generic QERR_INTERNAL_ERROR that takes a single human readable string as an argument. We can have a wrapper for it that also records location information in the error object. Regards, Anthony Liguori