[Qemu-devel] [PATCH v4 0/2] support to migrate with IPv6 address

[Qemu-devel] [PATCH v4 0/2] support to migrate with IPv6 address

[Amos Kong]

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

---

Amos Kong (2):
      qemu-socket: change inet_connect() to to support nonblock socket
      use inet_listen()/inet_connect() to support ipv6 migration


 migration-tcp.c |   75 +++++++++++++++----------------------------------------
 nbd.c           |    2 +
 qemu-char.c     |    2 +
 qemu-sockets.c  |   73 ++++++++++++++++++++++++++++++++++++++++++------------
 qemu_socket.h   |    4 +--
 ui/vnc.c        |    2 +
 6 files changed, 82 insertions(+), 76 deletions(-)

-- 
Amos Kong

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

[Amos Kong]

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(-)

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;

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

[Orit Wasserman]

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

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

[Michael Roth]

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

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

[Qemu-devel] [PATCH v5 0/2] support to migrate with IPv6 address

[Amos Kong]

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(-)

-- 
Amos Kong

[Qemu-devel] [PATCH v5 1/4] sockets: introduce set_socket_error()

[Amos Kong]

Introduce set_socket_error() to set the errno, use
WSASetLastError() for win32.
Sometimes, clean work would rewrite errno in error path,
we can use this function to restore real errno.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu_socket.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/qemu_socket.h b/qemu_socket.h
index fe4cf6c..a4c5170 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -8,6 +8,7 @@
 #include <ws2tcpip.h>
 
 #define socket_error() WSAGetLastError()
+#define set_socket_error(e) WSASetLastError(e)
 #undef EINTR
 #define EWOULDBLOCK WSAEWOULDBLOCK
 #define EINTR       WSAEINTR
@@ -26,6 +27,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include <sys/un.h>
 
 #define socket_error() errno
+#define set_socket_error(e) errno = e
 #define closesocket(s) close(s)
 
 #endif /* !_WIN32 */

Re: [Qemu-devel] [PATCH v5 1/4] sockets: introduce set_socket_error()

[Orit Wasserman]

On 03/22/2012 05:52 AM, Amos Kong wrote:
> Introduce set_socket_error() to set the errno, use
> WSASetLastError() for win32.
> Sometimes, clean work would rewrite errno in error path,
> we can use this function to restore real errno.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu_socket.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu_socket.h b/qemu_socket.h
> index fe4cf6c..a4c5170 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -8,6 +8,7 @@
>  #include <ws2tcpip.h>
>  
>  #define socket_error() WSAGetLastError()
> +#define set_socket_error(e) WSASetLastError(e)
>  #undef EINTR
>  #define EWOULDBLOCK WSAEWOULDBLOCK
>  #define EINTR       WSAEINTR
> @@ -26,6 +27,7 @@ int inet_aton(const char *cp, struct in_addr *ia);
>  #include <sys/un.h>
>  
>  #define socket_error() errno
> +#define set_socket_error(e) errno = e

how about creating a function set_errno(int e) and use it in the macro.

Orit
>  #define closesocket(s) close(s)
>  
>  #endif /* !_WIN32 */
> 
> 

[Qemu-devel] [PATCH v6 1/4] sockets: introduce set_socket_error()

[Amos Kong]

Introduce set_socket_error() to set the errno, use
WSASetLastError() for win32.
Sometimes, clean work would rewrite errno in error path,
we can use this function to restore real errno.

Changes from V5:
- create a function to set errno
    
Signed-off-by: Amos Kong <akong@redhat.com>

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..f822187 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -56,6 +56,11 @@ static QemuOptsList dummy_opts = {
     },
 };
 
+void set_errno(int e)
+{
+       errno = e;
+}
+
 static int inet_getport(struct addrinfo *e)
 {
     struct sockaddr_in *i4;
diff --git a/qemu_socket.h b/qemu_socket.h
index fe4cf6c..62c1921 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -8,6 +8,7 @@
 #include <ws2tcpip.h>
 
 #define socket_error() WSAGetLastError()
+#define set_socket_error(e) WSASetLastError(e)
 #undef EINTR
 #define EWOULDBLOCK WSAEWOULDBLOCK
 #define EINTR       WSAEINTR
@@ -25,7 +26,10 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include <netdb.h>
 #include <sys/un.h>
 
+void set_errno(int e);
+
 #define socket_error() errno
+#define set_socket_error(e) set_errno(e)
 #define closesocket(s) close(s)
 
 #endif /* !_WIN32 */

[Qemu-devel] [PATCH v5 2/4] qemu-socket: change inet_connect() to to support nonblock socket

[Amos Kong]

Change inet_connect(const char *str, int socktype) to
inet_connect(const char *str, bool block),
socktype is unused, block is used to assign if set socket
to block/nonblock.

Retry to connect when -EINTR/-EWOULDBLOCK is got.
Connect's successful for nonblock socket when following
errors are got:
  -EINPROGRESS
  -WSAEALREADY/-WSAEINVAL (win32)

Add a bool entry(block) for dummy_opts to tag block type.

Use set_socket_error() to set real errno, set errno to
EINVAL for parse error.

Change nbd, vnc to use new interface.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 nbd.c          |    2 +-
 qemu-sockets.c |   66 +++++++++++++++++++++++++++++++++++++++++++-------------
 qemu_socket.h  |    2 +-
 ui/vnc.c       |    2 +-
 4 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/nbd.c b/nbd.c
index 567e94e..3618344 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);
 }
 
 int tcp_socket_incoming(const char *address, uint16_t port)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6bcb8e3..908479e 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 */ }
     },
@@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
     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,44 @@ 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();
+            }
+        } while (err == -EINTR || err == -EWOULDBLOCK);
+
+        if (err >= 0) {
+            goto success;
+        } else if (!block && err == -EINPROGRESS) {
+            goto success;
+#ifdef _WIN32
+        } else if (!block && (err == -WSAEALREADY || 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:
+    set_socket_error(-err);
     return -1;
+
+success:
+    freeaddrinfo(res);
+    set_socket_error(-err);
+    return sock;
 }
 
 int inet_dgram_opts(QemuOpts *opts)
@@ -449,14 +479,20 @@ 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)
 {
     QemuOpts *opts;
     int sock = -1;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0);
-    if (inet_parse(opts, str) == 0)
+    if (inet_parse(opts, str) == 0) {
+        if (block) {
+            qemu_opt_set(opts, "block", "on");
+        }
         sock = inet_connect_opts(opts);
+    } else {
+        set_socket_error(EINVAL);
+    }
     qemu_opts_del(opts);
     return sock;
 }
diff --git a/qemu_socket.h b/qemu_socket.h
index a4c5170..f86cd3f 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -47,7 +47,7 @@ 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(const char *str, bool block);
 int inet_dgram_opts(QemuOpts *opts);
 const char *inet_strfamily(int family);
 
diff --git a/ui/vnc.c b/ui/vnc.c
index deb9ecd..4a96153 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);
         if (-1 == vs->lsock) {
             g_free(vs->display);
             vs->display = NULL;

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

[Orit Wasserman]

On 03/22/2012 05:52 AM, Amos Kong wrote:
> Change inet_connect(const char *str, int socktype) to
> inet_connect(const char *str, bool block),
> socktype is unused, block is used to assign if set socket
> to block/nonblock.
> 
> Retry to connect when -EINTR/-EWOULDBLOCK is got.
> Connect's successful for nonblock socket when following
> errors are got:
>   -EINPROGRESS
>   -WSAEALREADY/-WSAEINVAL (win32)
> 
> Add a bool entry(block) for dummy_opts to tag block type.
> 
> Use set_socket_error() to set real errno, set errno to
> EINVAL for parse error.
> 
> Change nbd, vnc to use new interface.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  nbd.c          |    2 +-
>  qemu-sockets.c |   66 +++++++++++++++++++++++++++++++++++++++++++-------------
>  qemu_socket.h  |    2 +-
>  ui/vnc.c       |    2 +-
>  4 files changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 567e94e..3618344 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);
>  }
>  
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 6bcb8e3..908479e 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 */ }
>      },
> @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
>      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;

I would call set_socket_error with -EINVAL and than return -1;

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

I would call set_socket_error with EINVAL and than return -1;

>      }
>  
>      for (e = res; e != NULL; e = e->ai_next) {
> @@ -241,21 +248,44 @@ 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();
> +            }
> +        } while (err == -EINTR || err == -EWOULDBLOCK);
> +

this is only relevant for non blocking socket
it should look something like this :

while ( (rc = connect(sock, e->ai_addr, e->ai_addrlen)) < 0 && !block &&
	(socket_error() == EINTR || socket_error() == -EWOULDBLOCK))
	
> +        if (err >= 0) {
> +            goto success;
> +        } else if (!block && err == -EINPROGRESS) {
> +            goto success;
> +#ifdef _WIN32
> +        } else if (!block && (err == -WSAEALREADY || err == -WSAEINVAL)) {
> +            goto success;
> +#endif

I prefer to check for error the code should look:

	if (rc < 0 &&  (block ||
#ifdef _WIN32
	   (socket_error() != WSAEALREADY && socket_error() != -WSAEINVAL)) {
#else
	(socket_error() != EINPROGRESS)) {
#endif
	   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);
	   continue;
	} 
...

Orit
>          }
> -        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:
> +    set_socket_error(-err);
>      return -1;
> +
> +success:
> +    freeaddrinfo(res);
> +    set_socket_error(-err);
> +    return sock;
>  }
>  
>  int inet_dgram_opts(QemuOpts *opts)
> @@ -449,14 +479,20 @@ 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)
>  {
>      QemuOpts *opts;
>      int sock = -1;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0);
> -    if (inet_parse(opts, str) == 0)
> +    if (inet_parse(opts, str) == 0) {
> +        if (block) {
> +            qemu_opt_set(opts, "block", "on");
> +        }
>          sock = inet_connect_opts(opts);
> +    } else {
> +        set_socket_error(EINVAL);
> +    }
>      qemu_opts_del(opts);
>      return sock;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index a4c5170..f86cd3f 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -47,7 +47,7 @@ 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(const char *str, bool block);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index deb9ecd..4a96153 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);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
> 
> 

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

[Amos Kong]

----- Original Message -----
> On 03/22/2012 05:52 AM, Amos Kong wrote:
> > Change inet_connect(const char *str, int socktype) to
> > inet_connect(const char *str, bool block),
> > socktype is unused, block is used to assign if set socket
> > to block/nonblock.
> > 
> > Retry to connect when -EINTR/-EWOULDBLOCK is got.
> > Connect's successful for nonblock socket when following
> > errors are got:
> >   -EINPROGRESS
> >   -WSAEALREADY/-WSAEINVAL (win32)
> > 
> > Add a bool entry(block) for dummy_opts to tag block type.
> > 
> > Use set_socket_error() to set real errno, set errno to
> > EINVAL for parse error.
> > 
> > Change nbd, vnc to use new interface.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> >  nbd.c          |    2 +-
> >  qemu-sockets.c |   66
> >  +++++++++++++++++++++++++++++++++++++++++++-------------
> >  qemu_socket.h  |    2 +-
> >  ui/vnc.c       |    2 +-
> >  4 files changed, 54 insertions(+), 18 deletions(-)
> > 
> > diff --git a/nbd.c b/nbd.c
> > index 567e94e..3618344 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);
> >  }
> >  
> >  int tcp_socket_incoming(const char *address, uint16_t port)
> > diff --git a/qemu-sockets.c b/qemu-sockets.c
> > index 6bcb8e3..908479e 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 */ }
> >      },
> > @@ -201,7 +204,8 @@ int inet_connect_opts(QemuOpts *opts)
> >      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;
> 
> I would call set_socket_error with -EINVAL and than return -1;

Hi Orit,

This is what I did.
restore "-EINVAL" to 'err', set_socket_error(-err), then return -1


> >      }
> >  
> >      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;
> 
> I would call set_socket_error with EINVAL and than return -1;
> 
> >      }
> >  
> >      for (e = res; e != NULL; e = e->ai_next) {
> > @@ -241,21 +248,44 @@ 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();
> > +            }
> > +        } while (err == -EINTR || err == -EWOULDBLOCK);
> > +
> 
> this is only relevant for non blocking socket

For blocking socket, 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]
So just re-connect when EINTR/EWOULDBLOCK are set.

http://marc.info/?l=kvm&m=133238606300535&w=2


> it should look something like this :
> 
> while ( (rc = connect(sock, e->ai_addr, e->ai_addrlen)) < 0 && !block
> &&
> 	(socket_error() == EINTR || socket_error() == -EWOULDBLOCK))


  while ( (rc = connect(sock, e->ai_addr, e->ai_addrlen)) < 0  &&
  	(socket_error() == EINTR || socket_error() == EWOULDBLOCK))
        ;
 	
> > +        if (err >= 0) {
> > +            goto success;
> > +        } else if (!block && err == -EINPROGRESS) {
> > +            goto success;
> > +#ifdef _WIN32
> > +        } else if (!block && (err == -WSAEALREADY || err ==
> > -WSAEINVAL)) {
> > +            goto success;
> > +#endif
> 
> I prefer to check for error the code should look:
>
> 	if (rc < 0 &&  (block ||
> #ifdef _WIN32
> 	   (socket_error() != WSAEALREADY && socket_error() != -WSAEINVAL))
> 	   {
> #else
> 	(socket_error() != EINPROGRESS)) {
> #endif
> 	   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);
> 	   continue;
> 	}
> ...
> 
> Orit
> >          }
> > -        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:
> > +    set_socket_error(-err);
> >      return -1;


> > +
> > +success:
> > +    freeaddrinfo(res);
> > +    set_socket_error(-err);
> > +    return sock;
> >  }
> >  
> >  int inet_dgram_opts(QemuOpts *opts)
> > @@ -449,14 +479,20 @@ 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)
> >  {
> >      QemuOpts *opts;
> >      int sock = -1;
> >  
> >      opts = qemu_opts_create(&dummy_opts, NULL, 0);
> > -    if (inet_parse(opts, str) == 0)
> > +    if (inet_parse(opts, str) == 0) {
> > +        if (block) {
> > +            qemu_opt_set(opts, "block", "on");
> > +        }
> >          sock = inet_connect_opts(opts);
> > +    } else {
> > +        set_socket_error(EINVAL);
> > +    }
> >      qemu_opts_del(opts);
> >      return sock;
> >  }
> > diff --git a/qemu_socket.h b/qemu_socket.h
> > index a4c5170..f86cd3f 100644
> > --- a/qemu_socket.h
> > +++ b/qemu_socket.h
> > @@ -47,7 +47,7 @@ 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(const char *str, bool block);
> >  int inet_dgram_opts(QemuOpts *opts);
> >  const char *inet_strfamily(int family);
> >  
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index deb9ecd..4a96153 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);
> >          if (-1 == vs->lsock) {
> >              g_free(vs->display);
> >              vs->display = NULL;
> > 
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Amos Kong]

----- Original Message -----
> ----- Original Message -----
> > On 03/22/2012 05:52 AM, Amos Kong wrote:
> > > Change inet_connect(const char *str, int socktype) to
> > > inet_connect(const char *str, bool block),
> > > socktype is unused, block is used to assign if set socket
> > > to block/nonblock.



> > > Retry to connect when -EINTR/-EWOULDBLOCK is got.
> > > Connect's successful for nonblock socket when following
> > > errors are got:
> > >   -EINPROGRESS
> > >   -WSAEALREADY/-WSAEINVAL (win32)


      Retry to connect when -EINTR(win32 & posix)/-EWOULDBLOCK(only win32) is got.
      Connect's successful for nonblock socket when following
      errors are got:
          -EINPROGRESS
          -WSAEALREADY(win32)


Discussed with <mdroth> in IRC.
For win32, need to re-connect when EWOULDBLOCK is got.
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx)

For posix, it's safe to treat it as fail, otherwise, 
it might hang qemu on the order of seconds. (man 2 connect : EAGAIN)

For non-blocking socket, EINPROGRESS maybe got in (win32 & posix),
but WSAEALREADY can only be got after re-connecting 
(EWOULDBLOCK is got in first connecting)

Please correct me if something is wrong, thanks.

...

> > >      for (e = res; e != NULL; e = e->ai_next) {
> > > @@ -241,21 +248,44 @@ 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();
> > > +            }
> > > +        } while (err == -EINTR || err == -EWOULDBLOCK);
> > > +
> > 
> > this is only relevant for non blocking socket
> 
> For blocking socket, 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]
> So just re-connect when EINTR/EWOULDBLOCK are set.
> 
> http://marc.info/?l=kvm&m=133238606300535&w=2
> 
> 
> > it should look something like this :
> > 
> > while ( (rc = connect(sock, e->ai_addr, e->ai_addrlen)) < 0 &&
> > !block
> > &&
> > 	(socket_error() == EINTR || socket_error() == -EWOULDBLOCK))
> 
> 
>   while ( (rc = connect(sock, e->ai_addr, e->ai_addrlen)) < 0  &&
>   	(socket_error() == EINTR || socket_error() == EWOULDBLOCK))
>         ;


        do {
                err = 0;
                if (rc = connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
                        err = -socket_error();
                }
#ifndef _WIN32
        } while (err == -EINTR || err == -EWOULDBLOCK);
#else
        } while (err == -EINTR);
#endif

        if (rc >= 0) {
                goto success;
        } else if (!block && err == -EINPROGRESS) {
                goto success;
#ifndef _WIN32
        } else if (!block && err == -WSAEALREADY) {
                goto success;
#endif
        }


> > > +        if (err >= 0) {
> > > +            goto success;
> > > +        } else if (!block && err == -EINPROGRESS) {
> > > +            goto success;
> > > +#ifdef _WIN32
> > > +        } else if (!block && (err == -WSAEALREADY || err ==
> > > -WSAEINVAL)) {
> > > +            goto success;
> > > +#endif
> > 
> > I prefer to check for error the code should look:
> >
> > 	if (rc < 0 &&  (block ||
> > #ifdef _WIN32
> > 	   (socket_error() != WSAEALREADY && socket_error() !=
> > 	   -WSAEINVAL))
> > 	   {
> > #else
> > 	(socket_error() != EINPROGRESS)) {
> > #endif
> > 	   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);
> > 	   continue;
> > 	}
> > ...
> > 
> > Orit
> > >          }
> > > -        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:
> > > +    set_socket_error(-err);
> > >      return -1;
> 
> 
> > > +
> > > +success:
> > > +    freeaddrinfo(res);
> > > +    set_socket_error(-err);
> > > +    return sock;
> > >  }

...

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

[Amos Kong]

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

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

[Michael Roth]

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

[Qemu-devel] [PATCH v5 4/4] use inet_listen()/inet_connect() to support ipv6 migration

[Amos Kong]

Use help functions in qemu-socket.c for tcp migration,
which already support ipv6 addresses.

For IPv6 brackets must be mandatory if you require a port.
Referencing to RFC5952, the recommended format is:

     [2312::8274]:5200

test status: Successed
listen side: qemu-kvm .... -incoming tcp:[2312::8274]:5200
client side: qemu-kvm ...
             (qemu) migrate -d tcp:[2312::8274]:5200

Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   74 +++++++++++++++----------------------------------------
 1 files changed, 20 insertions(+), 54 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 35a5781..251d955 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -81,43 +81,32 @@ static void tcp_wait_for_connect(void *opaque)
 
 int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
 {
-    struct sockaddr_in addr;
-    int ret;
-
-    ret = parse_host_port(&addr, host_port);
-    if (ret < 0) {
-        return ret;
-    }
+    int err;
 
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s->fd == -1) {
-        DPRINTF("Unable to open socket");
-        return -socket_error();
-    }
-
-    socket_set_nonblock(s->fd);
+    s->fd = inet_connect(host_port, false);
+    err = -socket_error();
 
-    do {
-        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1) {
-            ret = -socket_error();
-        }
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
-            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
-            return 0;
+    if (err == -EINPROGRESS) {
+        DPRINTF("connect in progress");
+        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
+#ifdef _WIN32
+    } else if (err == -WSAEALREADY || err == -WSAEINVAL) {
+        DPRINTF("connect in progress");
+        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
+#endif
+    } else if (err < 0) {
+        DPRINTF("connect failed: %s\n", strerror(-err));
+        if (s->fd != -1) {
+            migrate_fd_error(s);
         }
-    } while (ret == -EINTR);
-
-    if (ret < 0) {
-        DPRINTF("connect failed\n");
-        migrate_fd_error(s);
-        return ret;
+        return err;
+    } else {
+        migrate_fd_connect(s);
     }
-    migrate_fd_connect(s);
     return 0;
 }
 
@@ -157,38 +146,15 @@ out2:
 
 int tcp_start_incoming_migration(const char *host_port)
 {
-    struct sockaddr_in addr;
-    int val;
     int s;
 
-    DPRINTF("Attempting to start an incoming migration\n");
-
-    if (parse_host_port(&addr, host_port) < 0) {
-        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
-        return -EINVAL;
-    }
-
-    s = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s == -1) {
+    s = inet_listen(host_port, NULL, 256, SOCK_STREAM, 0);
+    if (s < 0) {
         return -socket_error();
     }
 
-    val = 1;
-    setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
-
-    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
-        goto err;
-    }
-    if (listen(s, 1) == -1) {
-        goto err;
-    }
-
     qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
                          (void *)(intptr_t)s);
 
     return 0;
-
-err:
-    close(s);
-    return -socket_error();
 }

[Qemu-devel] [PATCH v4 2/2] use inet_listen()/inet_connect() to support ipv6 migration

[Amos Kong]

Use help functions in qemu-socket.c for tcp migration,
which already support ipv6 addresses.

For IPv6 brackets must be mandatory if you require a port.
Referencing to RFC5952, the recommended format is:

     [2312::8274]:5200

test status: Successed
listen side: qemu-kvm .... -incoming tcp:[2312::8274]:5200
client side: qemu-kvm ...
             (qemu) migrate -d tcp:[2312::8274]:5200

Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   75 +++++++++++++++----------------------------------------
 1 files changed, 20 insertions(+), 55 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 35a5781..6c66c7a 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -81,43 +81,31 @@ static void tcp_wait_for_connect(void *opaque)
 
 int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
 {
-    struct sockaddr_in addr;
-    int ret;
-
-    ret = parse_host_port(&addr, host_port);
-    if (ret < 0) {
-        return ret;
-    }
+    int sock_err;
 
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s->fd == -1) {
-        DPRINTF("Unable to open socket");
-        return -socket_error();
-    }
-
-    socket_set_nonblock(s->fd);
+    s->fd = inet_connect(host_port, false, &sock_err);
 
-    do {
-        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1) {
-            ret = -socket_error();
-        }
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
-            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
-            return 0;
+    if (sock_err == -EINPROGRESS || sock_err == -EWOULDBLOCK) {
+        DPRINTF("connect in progress");
+        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
+#ifdef _WIN32
+    } else if (sock_err == -WSAEALREADY || sock_err == -WSAEINVAL) {
+        DPRINTF("connect in progress");
+        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
+#endif
+    } else if (sock_err < 0) {
+        DPRINTF("connect failed: %s\n", strerror(-sock_err));
+        if (s->fd != -1) {
+            migrate_fd_error(s);
         }
-    } while (ret == -EINTR);
-
-    if (ret < 0) {
-        DPRINTF("connect failed\n");
-        migrate_fd_error(s);
-        return ret;
+        return sock_err;
+    } else {
+        migrate_fd_connect(s);
     }
-    migrate_fd_connect(s);
     return 0;
 }
 
@@ -157,38 +145,15 @@ out2:
 
 int tcp_start_incoming_migration(const char *host_port)
 {
-    struct sockaddr_in addr;
-    int val;
     int s;
 
-    DPRINTF("Attempting to start an incoming migration\n");
-
-    if (parse_host_port(&addr, host_port) < 0) {
-        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
-        return -EINVAL;
-    }
-
-    s = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s == -1) {
-        return -socket_error();
-    }
-
-    val = 1;
-    setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
-
-    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
-        goto err;
-    }
-    if (listen(s, 1) == -1) {
-        goto err;
+    s = inet_listen(host_port, NULL, 256, SOCK_STREAM, 0);
+    if (s < 0) {
+        return s;
     }
 
     qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
                          (void *)(intptr_t)s);
 
     return 0;
-
-err:
-    close(s);
-    return -socket_error();
 }