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 <akong@xxxxxxxxxx>
> ---
> 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;
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
