diff mbox

[v4,1/2] qemu-socket: change inet_connect() to to support nonblock socket

Message ID 20120319141154.1865.16418.stgit@dhcp-8-167.nay.redhat.com
State New
Headers show

Commit Message

Amos Kong March 19, 2012, 2:11 p.m. UTC 
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>
---
 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(-)

Comments

Orit Wasserman March 20, 2012, 10:58 a.m. UTC | #1 
On 03/19/2012 04:11 PM, 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.

It is more common to call the parameter blocking/nonblocking.

You removed socktype (I noticed it was not implemented) , can you keep the same API but fix the implementation ?
I don't like the use of sock_err.
How about adding a function set_socket_error that will set the errno and for win32 you can use SetLastError,
you will be able to read the error using socket_error function.

Cheers,
Orit
> 
> 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>
> ---
>  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;
> +                }
> +            }
> +        } while (err == -EINTR);
> +
> +        if (err >= 0) {
> +            goto success;
> +        } else if (!block && (err == -EINPROGRESS || err == -EWOULDBLOCK)) {
> +            goto success;
> +#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;
> 
>
Michael Roth March 21, 2012, 11:46 p.m. UTC | #2 
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;
>
Amos Kong March 22, 2012, 3:13 a.m. UTC | #3 
----- 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 <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*.

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.
Amos Kong March 22, 2012, 3:52 a.m. UTC | #4 
Those patches make tcp migration use the help functions in
qemu-socket.c for support IPv6 migration.

Changes from v1:
- split different changes to small patches, it will be easier to review
- fixed some problem according to Kevin's comment

Changes from v2:
- fix issue of returning real error 
- set s->fd to -1 when parse fails, won't call migrate_fd_error()

Changes from v3:
- try to use help functions in qemu-socket.c

Changes from v4:
- introduce set_socket_error() to restore real errno
- fix connect error process

---

Amos Kong (4):
      sockets: introduce set_socket_error()
      qemu-socket: change inet_connect() to to support nonblock socket
      sockets: pass back errors in inet_listen()
      use inet_listen()/inet_connect() to support ipv6 migration


 migration-tcp.c |   74 +++++++++++++----------------------------------
 nbd.c           |    2 +
 qemu-sockets.c  |   87 ++++++++++++++++++++++++++++++++++++++++++-------------
 qemu_socket.h   |    4 ++-
 ui/vnc.c        |    2 +
 5 files changed, 92 insertions(+), 77 deletions(-)
diff mbox

Patch

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;
+                }
+            }
+        } while (err == -EINTR);
+
+        if (err >= 0) {
+            goto success;
+        } else if (!block && (err == -EINPROGRESS || err == -EWOULDBLOCK)) {
+            goto success;
+#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;