Re: [Qemu-devel] [PATCH v4 1/2] qemu-socket: change inet_connect() to to support nonblock socket

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Mon, Mar 19, 2012 at 10:11:54PM +0800, Amos Kong wrote:
> Change inet_connect(const char *str, int socktype) to
> inet_connect(const char *str, bool block, int *sock_err),
> socktype is unused, block is used to assign if set socket
> to block/nonblock, sock_err is used to restore socket error.
> 
> Connect's successful for nonblock socket when following errors are returned:
>   -EINPROGRESS or -EWOULDBLOCK
>   -WSAEALREADY or -WSAEINVAL (win32)
> 
> Also change the wrap function inet_connect_opts(QemuOpts *opts)
> to inet_connect_opts(QemuOpts *opts, int *sock_err).
> 
> Add a bool entry(block) for dummy_opts to tag block type.
> Change nbd, vnc to use new interface.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

Looks good, and non-blocking support for qemu-socket interfaces is pretty
useful in and of itself. One general suggestion I would make is to use Error to
pass back errors to the callers rather than an int*.

Some additional comments below.

> ---
>  nbd.c          |    2 +-
>  qemu-char.c    |    2 +-
>  qemu-sockets.c |   73 ++++++++++++++++++++++++++++++++++++++++++++------------
>  qemu_socket.h  |    4 ++-
>  ui/vnc.c       |    2 +-
>  5 files changed, 62 insertions(+), 21 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 567e94e..ad4de06 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -146,7 +146,7 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
> 
>  int tcp_socket_outgoing_spec(const char *address_and_port)
>  {
> -    return inet_connect(address_and_port, SOCK_STREAM);
> +    return inet_connect(address_and_port, true, NULL);
>  }
> 
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-char.c b/qemu-char.c
> index bb9e3f5..d3543ea 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2443,7 +2443,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>          if (is_listen) {
>              fd = inet_listen_opts(opts, 0);
>          } else {
> -            fd = inet_connect_opts(opts);
> +            fd = inet_connect_opts(opts, NULL);
>          }
>      }
>      if (fd < 0) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 6bcb8e3..8ed45f8 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -51,6 +51,9 @@ static QemuOptsList dummy_opts = {
>          },{
>              .name = "ipv6",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "block",
> +            .type = QEMU_OPT_BOOL,
>          },
>          { /* end if list */ }
>      },
> @@ -194,14 +197,15 @@ listen:
>      return slisten;
>  }
> 
> -int inet_connect_opts(QemuOpts *opts)
> +int inet_connect_opts(QemuOpts *opts, int *sock_err)
>  {
>      struct addrinfo ai,*res,*e;
>      const char *addr;
>      const char *port;
>      char uaddr[INET6_ADDRSTRLEN+1];
>      char uport[33];
> -    int sock,rc;
> +    int sock, rc, err;
> +    bool block;
> 
>      memset(&ai,0, sizeof(ai));
>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> @@ -210,9 +214,11 @@ int inet_connect_opts(QemuOpts *opts)
> 
>      addr = qemu_opt_get(opts, "host");
>      port = qemu_opt_get(opts, "port");
> +    block = qemu_opt_get_bool(opts, "block", 0);
>      if (addr == NULL || port == NULL) {
>          fprintf(stderr, "inet_connect: host and/or port not specified\n");
> -        return -1;
> +        err = -EINVAL;
> +        goto err;
>      }
> 
>      if (qemu_opt_get_bool(opts, "ipv4", 0))
> @@ -224,7 +230,8 @@ int inet_connect_opts(QemuOpts *opts)
>      if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
>          fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>                  gai_strerror(rc));
> -	return -1;
> +        err = -EINVAL;
> +        goto err;
>      }
> 
>      for (e = res; e != NULL; e = e->ai_next) {
> @@ -241,21 +248,52 @@ int inet_connect_opts(QemuOpts *opts)
>              continue;
>          }
>          setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -
> +        if (!block) {
> +            socket_set_nonblock(sock);
> +        }
>          /* 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;
> +        do {
> +            err = 0;
> +            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
> +                err = -socket_error();
> +                if (block) {
> +                    break;
> +                }

Can still get an EINTR even in blocking mode, is breaking in that case
intentional?

> +            }
> +        } while (err == -EINTR);
> +
> +        if (err >= 0) {
> +            goto success;
> +        } else if (!block && (err == -EINPROGRESS || err == -EWOULDBLOCK)) {
> +            goto success;

EWOULDBLOCK should actually be an error:

EWOULDBLOCK == EAGAIN:
   No  more  free  local  ports  or insufficient entries in the routing
   cache.      For     AF_INET     see     the      description      of
   /proc/sys/net/ipv4/ip_local_port_range  ip(7) for information on how
   to increase the number of local ports.

There's nothing useful they can do with the socket fd you pass back except try
to call connect() on it again.

> +#ifdef _WIN32
> +        } else if (!block && (sock_err == -WSAEALREADY ||
> +                              sock_err == -WSAEINVAL)) {
> +            goto success;
> +#endif
>          }
> -        freeaddrinfo(res);
> -        return sock;
> +
> +        if (NULL == e->ai_next) {
> +            fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __func__,
> +                    inet_strfamily(e->ai_family),
> +                    e->ai_canonname, uaddr, uport, strerror(errno));
> +        }
> +        closesocket(sock);
>      }
>      freeaddrinfo(res);
> +
> +err:
> +    if (sock_err) {
> +        *sock_err = err;
> +    }
>      return -1;
> +
> +success:
> +    freeaddrinfo(res);
> +    if (sock_err) {
> +        *sock_err = err;
> +    }
> +    return sock;
>  }
> 
>  int inet_dgram_opts(QemuOpts *opts)
> @@ -449,14 +487,17 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
> 
> -int inet_connect(const char *str, int socktype)
> +int inet_connect(const char *str, bool block, int *sock_err)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> 
>      opts = qemu_opts_create(&dummy_opts, NULL, 0);
>      if (inet_parse(opts, str) == 0)
> -        sock = inet_connect_opts(opts);
> +        if (block) {
> +            qemu_opt_set(opts, "block", "on");
> +        }
> +        sock = inet_connect_opts(opts, sock_err);
>      qemu_opts_del(opts);
>      return sock;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index fe4cf6c..4810506 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -44,8 +44,8 @@ int send_all(int fd, const void *buf, int len1);
>  int inet_listen_opts(QemuOpts *opts, int port_offset);
>  int inet_listen(const char *str, char *ostr, int olen,
>                  int socktype, int port_offset);
> -int inet_connect_opts(QemuOpts *opts);
> -int inet_connect(const char *str, int socktype);
> +int inet_connect_opts(QemuOpts *opts, int *sock_err);
> +int inet_connect(const char *str, bool block, int *sock_err);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index deb9ecd..3ae7704 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3068,7 +3068,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>          if (strncmp(display, "unix:", 5) == 0)
>              vs->lsock = unix_connect(display+5);
>          else
> -            vs->lsock = inet_connect(display, SOCK_STREAM);
> +            vs->lsock = inet_connect(display, true, NULL);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
>