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

[Amos Kong]

----- Original Message -----
> 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 <address@hidden>
> 
> 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*.

I have implemented it, thanks

> 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;

Above code is used to keep behavior same as original, return this error(connect 
will be failed)

analysis about process EINTR : 
http://www.madore.org/~david/computers/connect-intr.html
""" If connect() is interrupted by a signal that is caught while blocked 
waiting to establish a connection, connect() shall fail and set connect() to 
[EINTR], but the connection request shall not be aborted, and the connection 
shall be established asynchronously."""

When EINTR is got, what will happen if re connect()?
1. block until connected
2. return -1, set errno to EALREADY

However re-connect is not wrong.

-----

        /* connect to peer */
        do {
            err = 0;
            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
                err = -socket_error();
            }
        } while (err == -EINTR || err == -EWOULDBLOCK);

        if (err >= 0) {
            goto success;
        } else if (!block && err == -EINPROGRESS) {
            goto success;


> > +                }
> 
> Can still get an EINTR even in blocking mode,

EINTR can be got in blocking socket. not sure if it can _Only_ be got in 
blocking socket ?

http://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html
""" If the initiating socket is connection-mode, then connect() shall attempt 
to establish a connection to the address specified by the address argument. If 
the connection cannot be established immediately and O_NONBLOCK is not set for 
the file descriptor for the socket, connect() shall block for up to an 
unspecified timeout interval until the connection is established. If the 
timeout interval expires before the connection is established, connect() shall 
fail and the connection attempt shall be aborted. If connect() is interrupted 
by a signal that is caught while blocked waiting to establish a connection, 
connect() shall fail and set errno to [EINTR], but the connection request shall 
not be aborted, and the connection shall be established asynchronously.

If the connection cannot be established immediately and O_NONBLOCK is set for 
the file descriptor for the socket, connect() shall fail and set errno to 
[EINPROGRESS], but the connection request shall not be aborted, and the 
connection shall be established asynchronously. Subsequent calls to connect() 
for the same socket, before the connection is established, shall fail and set 
errno to [EALREADY]. """


> 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:

Yes, need to reconnect.

> EWOULDBLOCK == EAGAIN:

right. macro EWOULDBLOCK is another name for EAGAIN
http://www.gnu.org/software/libc/manual/html_node/Error-Codes.html


>    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.