From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51551) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCfbb-0006FR-9b for qemu-devel@nongnu.org; Tue, 27 Mar 2012 19:15:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SCfbY-0000mK-HQ for qemu-devel@nongnu.org; Tue, 27 Mar 2012 19:15:10 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:58611) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SCfbY-0000lL-7X for qemu-devel@nongnu.org; Tue, 27 Mar 2012 19:15:08 -0400 Received: by obbwd20 with SMTP id wd20so541756obb.4 for ; Tue, 27 Mar 2012 16:15:05 -0700 (PDT) Sender: fluxion Date: Tue, 27 Mar 2012 18:15:00 -0500 From: Michael Roth Message-ID: <20120327231500.GA31300@illuin> References: <20120322035052.2431.4994.stgit@dhcp-8-167.nay.redhat.com> <20120322035254.2431.12215.stgit@dhcp-8-167.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120322035254.2431.12215.stgit@dhcp-8-167.nay.redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 3/4] sockets: pass back errors in inet_listen() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: aliguori@us.ibm.com, kvm@vger.kernel.org, quintela@redhat.com, jasowang@redhat.com, qemu-devel@nongnu.org, owasserm@redhat.com, laine@redhat.com On Thu, Mar 22, 2012 at 11:52:54AM +0800, Amos Kong wrote: > Use set_socket_error() to restore real erron, > set errno to EINVAL for parse error. > > Signed-off-by: Amos Kong > --- > qemu-sockets.c | 21 ++++++++++++++++----- > 1 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 908479e..f1c6524 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -110,7 +110,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, err; > > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; > @@ -120,7 +120,8 @@ 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__); > - return -1; > + err = -EINVAL; > + goto err; > } > pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port")); > addr = qemu_opt_get(opts, "host"); > @@ -138,7 +139,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > if (rc != 0) { > fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > gai_strerror(rc)); > - return -1; > + err = -EINVAL; > + goto err; > } > > /* create socket + bind */ > @@ -150,6 +152,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > if (slisten < 0) { > fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > inet_strfamily(e->ai_family), strerror(errno)); > + err = -socket_error(); > continue; > } > > @@ -169,6 +172,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > goto listen; > } > + err = -socket_error(); > if (p == port_max) { > fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, > inet_strfamily(e->ai_family), uaddr, inet_getport(e), > @@ -179,14 +183,15 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > } > fprintf(stderr, "%s: FAILED\n", __FUNCTION__); > freeaddrinfo(res); > - return -1; > + goto err; > > listen: > if (listen(slisten,1) != 0) { > + err = -socket_error(); > perror("listen"); > closesocket(slisten); > freeaddrinfo(res); > - return -1; > + goto err; > } > snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset); > qemu_opt_set(opts, "host", uaddr); > @@ -195,6 +200,10 @@ listen: > qemu_opt_set(opts, "ipv4", (e->ai_family != PF_INET6) ? "on" : "off"); > freeaddrinfo(res); > return slisten; > + > +err: > + set_socket_error(-err); > + return -1; Sorry I didn't see this sooner. On the last series where I suggested you switch to using Error, I was referring to the error propagation stuff in Error.c, namely error_set(Error *err) I don't it's clear to users when socket_error() should be used as opposed to errno (or nothing at all), especially in a utility function like this that makes a lot of system calls. errno actually gets used explicitly in a few spots in qemu-sockets.c, for w32, ironically, so it's missed completely by socket_error()/WSAGetLastError() checks. Unfortunately, creating distinct error classes for every permutation of bind/listen/connect/socket and EINPROGRESS/EWOULDBLOCK/EINTR/EXXX is too much. But really the whole reason we now need error propagation is basically just to let the new non-blocking users know when a connect() "failure" was simply EINPROGRESS, right? For other errors the user probably just wants to know what function failed (and maybe a strerror(errno) but that's not typically how we use Error/QError currently). Currently, other than for ENOTSUP which we use errno for, everything in qemu-sockets.c just prints errors to stderr and returns -1, 0, or a valid fd, and no current users, AFAICT, check errno/socket_error()/etc. So we're free to do it up nice and clean, and here's what I'd suggest: 1) Rename the following functions: inet_listen_opts(...) -> inet_listen_opts_err(..., Error **errp) inet_connect_opts(...) -> inet_connect_opts_err(..., Error **errp) unix_listen_opts(...) -> inet_listen_opts_err(..., Error **errp) unix_connect_opts(...) -> inet_connect_opts_err(..., Error **errp) inet_dgram_opts(...) -> inet_listen_opts_err(..., Error **errp) Make wrappers for the originals that call them with errp == NULL. 2) Add the following error classes to qerror.c/qerror.h: QERR_SOCKET_CONNECT_IN_PROGRESS QERR_SOCKET_CONNECT_FAILED QERR_SOCKET_LISTEN_FAILED QERR_SOCKET_BIND_FAILED QERR_SOCKET_CREATE_FAILED And maybe a handful of others that seem worth reporting. QERR_UNDEFINED_ERROR is probably okay for the more obscure failures. For inet_*_opts_err(), set these errors alongside where we currently print to stderr and return -1. Handling errors for the others are probably outside the scope of your patches and can be done easily enough later, but would be nice to at least pass back QERR_UNDEFINED_ERROR as a place holder. 3) Add your non-blocking support, document QERR_SOCKET_CONNECT_IN_PROGRESS as being significant in that the user should begin select()'ing for completion, checking SO_ERROR, etc. 4) Use it for ipv6 migration. IMO, anyway. I think the series is basically there otherwise, we just have some nicer error-handling infrastructure in place since socket_error() was first added and it makes sense to use if we need to introduce error propagation to qemu-sockets.c > } > > int inet_connect_opts(QemuOpts *opts) > @@ -474,6 +483,8 @@ int inet_listen(const char *str, char *ostr, int olen, > optstr ? optstr : ""); > } > } > + } else { > + set_socket_error(EINVAL); > } > qemu_opts_del(opts); > return sock; >