Re: [Qemu-devel] [PATCH v5 3/4] sockets: pass back errors in inet_listen()

[Michael Roth <mdroth@linux.vnet.ibm.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 <akong@redhat.com>
> ---
>  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;
>