[Qemu-devel] [PATCH v2 0/3] nonblocking connect address handling cleanup

[Qemu-devel] [PATCH v2 0/3] nonblocking connect address handling cleanup

[Orit Wasserman]

getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one.  This is common on desktop setups that often have ipv6
configured but not actually working.
A simple way to reproduce the problem is migration:
for the destination use -incoming tcp:0:4444, run migrate -d tcp:localhost:4444
migration will fail on hosts that have both IPv4 and IPV6 address for localhost.

To fix this, refactor address resolution code and make inet_nonblocking_connect
retry connection with a different address.

Orit Wasserman (3):
  Refactor inet_connect_opts function
  Separate inet_connect into inet_connect (blocking) and
    inet_nonblocking_connect
  Fix address handling in inet_nonblocking_connect

 migration-tcp.c |   29 ++-----
 nbd.c           |    2 +-
 qemu-sockets.c  |  254 +++++++++++++++++++++++++++++++++++++++++--------------
 qemu_socket.h   |    9 ++-
 ui/vnc.c        |    2 +-
 5 files changed, 208 insertions(+), 88 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Orit Wasserman]

From: Michael S. Tsirkin <mst@redhat.com>

refactor address resolution code to fix nonblocking connect

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 qemu-sockets.c |  139 +++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 82 insertions(+), 57 deletions(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 361d890..68e4d30 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -209,32 +209,25 @@ listen:
     return slisten;
 }
 
-int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
+static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
 {
-    struct addrinfo ai,*res,*e;
+    struct addrinfo ai, *res;
+    int rc;
     const char *addr;
     const char *port;
-    char uaddr[INET6_ADDRSTRLEN+1];
-    char uport[33];
-    int sock,rc;
-    bool block;
 
     memset(&ai,0, sizeof(ai));
     ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
     ai.ai_family = PF_UNSPEC;
     ai.ai_socktype = SOCK_STREAM;
 
-    if (in_progress) {
-        *in_progress = false;
-    }
-
     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");
+        fprintf(stderr,
+                "inet_parse_connect_opts: host and/or port not specified\n");
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
-        return -1;
+        return NULL;
     }
 
     if (qemu_opt_get_bool(opts, "ipv4", 0))
@@ -247,57 +240,89 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
         fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
                 gai_strerror(rc));
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
-	return -1;
+        return NULL;
     }
+    return res;
+}
 
-    for (e = res; e != NULL; e = e->ai_next) {
-        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
-                            uaddr,INET6_ADDRSTRLEN,uport,32,
-                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
-            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
-            continue;
-        }
-        sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
-        if (sock < 0) {
-            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
-            inet_strfamily(e->ai_family), strerror(errno));
-            continue;
+#ifdef _WIN32
+#define QEMU_SOCKET_RC_INPROGRESS(rc) \
+    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
+#else
+#define QEMU_SOCKET_RC_INPROGRESS(rc) \
+    ((rc) == -EINPROGRESS)
+#endif
+
+static int inet_connect_addr(struct addrinfo *addr, bool block,
+                             bool *in_progress, Error **errp)
+{
+    char uaddr[INET6_ADDRSTRLEN + 1];
+    char uport[33];
+    int sock, rc;
+
+    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
+                    uaddr, INET6_ADDRSTRLEN, uport, 32,
+                    NI_NUMERICHOST | NI_NUMERICSERV)) {
+        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
+        return -1;
+    }
+    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
+    if (sock < 0) {
+        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
+                inet_strfamily(addr->ai_family), strerror(errno));
+        return -1;
+    }
+    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
+    if (!block) {
+        socket_set_nonblock(sock);
+    }
+    /* connect to peer */
+    do {
+        rc = 0;
+        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
+            rc = -socket_error();
         }
-        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
-        if (!block) {
-            socket_set_nonblock(sock);
+    } while (rc == -EINTR);
+
+    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+        if (in_progress) {
+            *in_progress = true;
         }
-        /* connect to peer */
-        do {
-            rc = 0;
-            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
-                rc = -socket_error();
-            }
-        } while (rc == -EINTR);
-
-  #ifdef _WIN32
-        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
-                       || rc == -WSAEALREADY)) {
-  #else
-        if (!block && (rc == -EINPROGRESS)) {
-  #endif
-            if (in_progress) {
-                *in_progress = true;
-            }
-        } else if (rc < 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;
+    } else if (rc < 0) {
+        closesocket(sock);
+        return -1;
+    }
+    return sock;
+}
+
+int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
+{
+    struct addrinfo *res, *e;
+    int sock = -1;
+    bool block = qemu_opt_get_bool(opts, "block", 0);
+
+    res = inet_parse_connect_opts(opts, errp);
+    if (!res) {
+        return -1;
+    }
+
+    if (in_progress) {
+        *in_progress = false;
+    }
+
+    for (e = res; e != NULL; e = e->ai_next) {
+        sock = inet_connect_addr(e, block, in_progress, errp);
+        if (in_progress && *in_progress) {
+            return sock;
+        } else if (sock >= 0) {
+            break;
         }
-        freeaddrinfo(res);
-        return sock;
     }
-    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
+    if (sock < 0) {
+        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
+    }
     freeaddrinfo(res);
-    return -1;
+    return sock;
 }
 
 int inet_dgram_opts(QemuOpts *opts)
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> From: Michael S. Tsirkin <mst@redhat.com>
>
> refactor address resolution code to fix nonblocking connect
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  qemu-sockets.c |  139 +++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 82 insertions(+), 57 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 361d890..68e4d30 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -209,32 +209,25 @@ listen:
>      return slisten;
>  }
>  
> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>  {
> -    struct addrinfo ai,*res,*e;
> +    struct addrinfo ai, *res;
> +    int rc;
>      const char *addr;
>      const char *port;
> -    char uaddr[INET6_ADDRSTRLEN+1];
> -    char uport[33];
> -    int sock,rc;
> -    bool block;
>  
>      memset(&ai,0, sizeof(ai));
>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>      ai.ai_family = PF_UNSPEC;
>      ai.ai_socktype = SOCK_STREAM;
>  
> -    if (in_progress) {
> -        *in_progress = false;
> -    }
> -
>      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");
> +        fprintf(stderr,
> +                "inet_parse_connect_opts: host and/or port not specified\n");
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> -        return -1;
> +        return NULL;
>      }
>  
>      if (qemu_opt_get_bool(opts, "ipv4", 0))
> @@ -247,57 +240,89 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>          fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>                  gai_strerror(rc));
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> -	return -1;
> +        return NULL;
>      }
> +    return res;
> +}
>  
> -    for (e = res; e != NULL; e = e->ai_next) {
> -        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> -                            uaddr,INET6_ADDRSTRLEN,uport,32,
> -                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
> -            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
> -            continue;
> -        }
> -        sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> -        if (sock < 0) {
> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
> -            inet_strfamily(e->ai_family), strerror(errno));
> -            continue;
> +#ifdef _WIN32
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
> +#else
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS)
> +#endif
> +
> +static int inet_connect_addr(struct addrinfo *addr, bool block,
> +                             bool *in_progress, Error **errp)
> +{
> +    char uaddr[INET6_ADDRSTRLEN + 1];
> +    char uport[33];
> +    int sock, rc;
> +
> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
> +        return -1;
> +    }

uaddr[] and uport[] are write-only.  Let's drop getnameinfo().

> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
> +    if (sock < 0) {
> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> +                inet_strfamily(addr->ai_family), strerror(errno));
> +        return -1;
> +    }
> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> +    if (!block) {
> +        socket_set_nonblock(sock);
> +    }
> +    /* connect to peer */
> +    do {
> +        rc = 0;
> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
> +            rc = -socket_error();
>          }
> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -        if (!block) {
> -            socket_set_nonblock(sock);
> +    } while (rc == -EINTR);
> +
> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        if (in_progress) {
> +            *in_progress = true;
>          }
> -        /* connect to peer */
> -        do {
> -            rc = 0;
> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
> -                rc = -socket_error();
> -            }
> -        } while (rc == -EINTR);
> -
> -  #ifdef _WIN32
> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
> -                       || rc == -WSAEALREADY)) {
> -  #else
> -        if (!block && (rc == -EINPROGRESS)) {
> -  #endif
> -            if (in_progress) {
> -                *in_progress = true;
> -            }
> -        } else if (rc < 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;
> +    } else if (rc < 0) {
> +        closesocket(sock);
> +        return -1;
> +    }
> +    return sock;
> +}
> +
> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +{
> +    struct addrinfo *res, *e;
> +    int sock = -1;
> +    bool block = qemu_opt_get_bool(opts, "block", 0);
> +
> +    res = inet_parse_connect_opts(opts, errp);
> +    if (!res) {
> +        return -1;
> +    }
> +
> +    if (in_progress) {
> +        *in_progress = false;
> +    }
> +
> +    for (e = res; e != NULL; e = e->ai_next) {
> +        sock = inet_connect_addr(e, block, in_progress, errp);
> +        if (in_progress && *in_progress) {
> +            return sock;

Doesn't this leak res?

> +        } else if (sock >= 0) {
> +            break;
>          }
> -        freeaddrinfo(res);
> -        return sock;
>      }
> -    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> +    if (sock < 0) {
> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> +    }
>      freeaddrinfo(res);
> -    return -1;
> +    return sock;
>  }
>  
>  int inet_dgram_opts(QemuOpts *opts)

What about

    for (e = res; e != NULL; e = e->ai_next) {
        sock = inet_connect_addr(e, block, in_progress, errp);
        if (sock >= 0) {
            break;
        }
    }
    if (sock < 0) {
        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
    }
    freeaddrinfo(res);
    return sock;

Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Orit Wasserman]

On 09/13/2012 03:35 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> From: Michael S. Tsirkin <mst@redhat.com>
>>
>> refactor address resolution code to fix nonblocking connect
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  qemu-sockets.c |  139 +++++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 82 insertions(+), 57 deletions(-)
>>
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 361d890..68e4d30 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -209,32 +209,25 @@ listen:
>>      return slisten;
>>  }
>>  
>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>  {
>> -    struct addrinfo ai,*res,*e;
>> +    struct addrinfo ai, *res;
>> +    int rc;
>>      const char *addr;
>>      const char *port;
>> -    char uaddr[INET6_ADDRSTRLEN+1];
>> -    char uport[33];
>> -    int sock,rc;
>> -    bool block;
>>  
>>      memset(&ai,0, sizeof(ai));
>>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>>      ai.ai_family = PF_UNSPEC;
>>      ai.ai_socktype = SOCK_STREAM;
>>  
>> -    if (in_progress) {
>> -        *in_progress = false;
>> -    }
>> -
>>      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");
>> +        fprintf(stderr,
>> +                "inet_parse_connect_opts: host and/or port not specified\n");
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -        return -1;
>> +        return NULL;
>>      }
>>  
>>      if (qemu_opt_get_bool(opts, "ipv4", 0))
>> @@ -247,57 +240,89 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>          fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>>                  gai_strerror(rc));
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -	return -1;
>> +        return NULL;
>>      }
>> +    return res;
>> +}
>>  
>> -    for (e = res; e != NULL; e = e->ai_next) {
>> -        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
>> -                            uaddr,INET6_ADDRSTRLEN,uport,32,
>> -                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
>> -            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
>> -            continue;
>> -        }
>> -        sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
>> -        if (sock < 0) {
>> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
>> -            inet_strfamily(e->ai_family), strerror(errno));
>> -            continue;
>> +#ifdef _WIN32
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
>> +#else
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS)
>> +#endif
>> +
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, Error **errp)
>> +{
>> +    char uaddr[INET6_ADDRSTRLEN + 1];
>> +    char uport[33];
>> +    int sock, rc;
>> +
>> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
>> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
>> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
>> +        return -1;
>> +    }
> 
> uaddr[] and uport[] are write-only.  Let's drop getnameinfo().
sure
> 
>> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>> +    if (sock < 0) {
>> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>> +                inet_strfamily(addr->ai_family), strerror(errno));
>> +        return -1;
>> +    }
>> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
>> +    if (!block) {
>> +        socket_set_nonblock(sock);
>> +    }
>> +    /* connect to peer */
>> +    do {
>> +        rc = 0;
>> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
>> +            rc = -socket_error();
>>          }
>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -        if (!block) {
>> -            socket_set_nonblock(sock);
>> +    } while (rc == -EINTR);
>> +
>> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> +        if (in_progress) {
>> +            *in_progress = true;
>>          }
>> -        /* connect to peer */
>> -        do {
>> -            rc = 0;
>> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
>> -                rc = -socket_error();
>> -            }
>> -        } while (rc == -EINTR);
>> -
>> -  #ifdef _WIN32
>> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
>> -                       || rc == -WSAEALREADY)) {
>> -  #else
>> -        if (!block && (rc == -EINPROGRESS)) {
>> -  #endif
>> -            if (in_progress) {
>> -                *in_progress = true;
>> -            }
>> -        } else if (rc < 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;
>> +    } else if (rc < 0) {
>> +        closesocket(sock);
>> +        return -1;
>> +    }
>> +    return sock;
>> +}
>> +
>> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +{
>> +    struct addrinfo *res, *e;
>> +    int sock = -1;
>> +    bool block = qemu_opt_get_bool(opts, "block", 0);
>> +
>> +    res = inet_parse_connect_opts(opts, errp);
>> +    if (!res) {
>> +        return -1;
>> +    }
>> +
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>> +
>> +    for (e = res; e != NULL; e = e->ai_next) {
>> +        sock = inet_connect_addr(e, block, in_progress, errp);
>> +        if (in_progress && *in_progress) {
>> +            return sock;
> 
> Doesn't this leak res?
right, will fix it.
> 
>> +        } else if (sock >= 0) {
>> +            break;
>>          }
>> -        freeaddrinfo(res);
>> -        return sock;
>>      }
>> -    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    if (sock < 0) {
>> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    }
>>      freeaddrinfo(res);
>> -    return -1;
>> +    return sock;
>>  }
>>  
>>  int inet_dgram_opts(QemuOpts *opts)
> 
> What about
> 
>     for (e = res; e != NULL; e = e->ai_next) {
>         sock = inet_connect_addr(e, block, in_progress, errp);
>         if (sock >= 0) {
>             break;
>         }
>     }
>     if (sock < 0) {
>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>     }
>     freeaddrinfo(res);
>     return sock;
> 
looks good.

Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Orit Wasserman]

On 09/13/2012 03:35 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> From: Michael S. Tsirkin <mst@redhat.com>
>>
>> refactor address resolution code to fix nonblocking connect
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  qemu-sockets.c |  139 +++++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 82 insertions(+), 57 deletions(-)
>>
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 361d890..68e4d30 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -209,32 +209,25 @@ listen:
>>      return slisten;
>>  }
>>  
>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>  {
>> -    struct addrinfo ai,*res,*e;
>> +    struct addrinfo ai, *res;
>> +    int rc;
>>      const char *addr;
>>      const char *port;
>> -    char uaddr[INET6_ADDRSTRLEN+1];
>> -    char uport[33];
>> -    int sock,rc;
>> -    bool block;
>>  
>>      memset(&ai,0, sizeof(ai));
>>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>>      ai.ai_family = PF_UNSPEC;
>>      ai.ai_socktype = SOCK_STREAM;
>>  
>> -    if (in_progress) {
>> -        *in_progress = false;
>> -    }
>> -
>>      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");
>> +        fprintf(stderr,
>> +                "inet_parse_connect_opts: host and/or port not specified\n");
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -        return -1;
>> +        return NULL;
>>      }
>>  
>>      if (qemu_opt_get_bool(opts, "ipv4", 0))
>> @@ -247,57 +240,89 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>          fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>>                  gai_strerror(rc));
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -	return -1;
>> +        return NULL;
>>      }
>> +    return res;
>> +}
>>  
>> -    for (e = res; e != NULL; e = e->ai_next) {
>> -        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
>> -                            uaddr,INET6_ADDRSTRLEN,uport,32,
>> -                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
>> -            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
>> -            continue;
>> -        }
>> -        sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
>> -        if (sock < 0) {
>> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
>> -            inet_strfamily(e->ai_family), strerror(errno));
>> -            continue;
>> +#ifdef _WIN32
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
>> +#else
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS)
>> +#endif
>> +
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, Error **errp)
>> +{
>> +    char uaddr[INET6_ADDRSTRLEN + 1];
>> +    char uport[33];
>> +    int sock, rc;
>> +
>> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
>> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
>> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
>> +        return -1;
>> +    }
> 
> uaddr[] and uport[] are write-only.  Let's drop getnameinfo().
> 
>> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>> +    if (sock < 0) {
>> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>> +                inet_strfamily(addr->ai_family), strerror(errno));
>> +        return -1;
>> +    }
>> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
>> +    if (!block) {
>> +        socket_set_nonblock(sock);
>> +    }
>> +    /* connect to peer */
>> +    do {
>> +        rc = 0;
>> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
>> +            rc = -socket_error();
>>          }
>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -        if (!block) {
>> -            socket_set_nonblock(sock);
>> +    } while (rc == -EINTR);
>> +
>> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> +        if (in_progress) {
>> +            *in_progress = true;
>>          }
>> -        /* connect to peer */
>> -        do {
>> -            rc = 0;
>> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
>> -                rc = -socket_error();
>> -            }
>> -        } while (rc == -EINTR);
>> -
>> -  #ifdef _WIN32
>> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
>> -                       || rc == -WSAEALREADY)) {
>> -  #else
>> -        if (!block && (rc == -EINPROGRESS)) {
>> -  #endif
>> -            if (in_progress) {
>> -                *in_progress = true;
>> -            }
>> -        } else if (rc < 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;
>> +    } else if (rc < 0) {
>> +        closesocket(sock);
>> +        return -1;
>> +    }
>> +    return sock;
>> +}
>> +
>> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +{
>> +    struct addrinfo *res, *e;
>> +    int sock = -1;
>> +    bool block = qemu_opt_get_bool(opts, "block", 0);
>> +
>> +    res = inet_parse_connect_opts(opts, errp);
>> +    if (!res) {
>> +        return -1;
>> +    }
>> +
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>> +
>> +    for (e = res; e != NULL; e = e->ai_next) {
>> +        sock = inet_connect_addr(e, block, in_progress, errp);
>> +        if (in_progress && *in_progress) {
>> +            return sock;
> 
> Doesn't this leak res?
Actually it doesn't after patch 3 wait_for_connect is the one freeing res.
is it OK to leave it as is ?

Orit
> 
>> +        } else if (sock >= 0) {
>> +            break;
>>          }
>> -        freeaddrinfo(res);
>> -        return sock;
>>      }
>> -    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    if (sock < 0) {
>> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    }
>>      freeaddrinfo(res);
>> -    return -1;
>> +    return sock;
>>  }
>>  
>>  int inet_dgram_opts(QemuOpts *opts)
> 
> What about
> 
>     for (e = res; e != NULL; e = e->ai_next) {
>         sock = inet_connect_addr(e, block, in_progress, errp);
>         if (sock >= 0) {
>             break;
>         }
>     }
>     if (sock < 0) {
>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>     }
>     freeaddrinfo(res);
>     return sock;
> 

Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Michael S. Tsirkin]

On Thu, Sep 13, 2012 at 07:52:35PM +0300, Orit Wasserman wrote:
> >> +    for (e = res; e != NULL; e = e->ai_next) {
> >> +        sock = inet_connect_addr(e, block, in_progress, errp);
> >> +        if (in_progress && *in_progress) {
> >> +            return sock;
> > 
> > Doesn't this leak res?
> Actually it doesn't after patch 3 wait_for_connect is the one freeing res.
> is it OK to leave it as is ?
> 
> Orit

We can't avoid breaking bisect sometimes but let's not do this intentionally.

-- 
MST

Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Markus Armbruster]

One more...

Orit Wasserman <owasserm@redhat.com> writes:

[...]
> +static int inet_connect_addr(struct addrinfo *addr, bool block,
> +                             bool *in_progress, Error **errp)

Parameter errp is unused.

> +{
> +    char uaddr[INET6_ADDRSTRLEN + 1];
> +    char uport[33];
> +    int sock, rc;
> +
> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
> +        return -1;
> +    }
> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
> +    if (sock < 0) {
> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> +                inet_strfamily(addr->ai_family), strerror(errno));
> +        return -1;
> +    }
> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> +    if (!block) {
> +        socket_set_nonblock(sock);
> +    }
> +    /* connect to peer */
> +    do {
> +        rc = 0;
> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
> +            rc = -socket_error();
>          }
> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -        if (!block) {
> -            socket_set_nonblock(sock);
> +    } while (rc == -EINTR);
> +
> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        if (in_progress) {
> +            *in_progress = true;
>          }
> -        /* connect to peer */
> -        do {
> -            rc = 0;
> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
> -                rc = -socket_error();
> -            }
> -        } while (rc == -EINTR);
> -
> -  #ifdef _WIN32
> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
> -                       || rc == -WSAEALREADY)) {
> -  #else
> -        if (!block && (rc == -EINPROGRESS)) {
> -  #endif
> -            if (in_progress) {
> -                *in_progress = true;
> -            }
> -        } else if (rc < 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;
> +    } else if (rc < 0) {
> +        closesocket(sock);
> +        return -1;
> +    }
> +    return sock;
> +}
> +
> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +{
> +    struct addrinfo *res, *e;
> +    int sock = -1;
> +    bool block = qemu_opt_get_bool(opts, "block", 0);
> +
> +    res = inet_parse_connect_opts(opts, errp);
> +    if (!res) {
> +        return -1;
> +    }
> +
> +    if (in_progress) {
> +        *in_progress = false;
> +    }
> +
> +    for (e = res; e != NULL; e = e->ai_next) {
> +        sock = inet_connect_addr(e, block, in_progress, errp);
> +        if (in_progress && *in_progress) {
> +            return sock;
> +        } else if (sock >= 0) {
> +            break;
>          }
> -        freeaddrinfo(res);
> -        return sock;
>      }
> -    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> +    if (sock < 0) {
> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);

Necessary, because inet_connect_addr() doesn't do it.

Suggest to drop inet_connect_addr() parameter errp.

> +    }
>      freeaddrinfo(res);
> -    return -1;
> +    return sock;
>  }
>  
>  int inet_dgram_opts(QemuOpts *opts)

Re: [Qemu-devel] [PATCH v2 1/3] Refactor inet_connect_opts function

[Orit Wasserman]

On 09/13/2012 04:14 PM, Markus Armbruster wrote:
> One more...
> 
> Orit Wasserman <owasserm@redhat.com> writes:
> 
> [...]
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, Error **errp)
> 
> Parameter errp is unused.
> 
>> +{
>> +    char uaddr[INET6_ADDRSTRLEN + 1];
>> +    char uport[33];
>> +    int sock, rc;
>> +
>> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
>> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
>> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
>> +        return -1;
>> +    }
>> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>> +    if (sock < 0) {
>> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>> +                inet_strfamily(addr->ai_family), strerror(errno));
>> +        return -1;
>> +    }
>> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
>> +    if (!block) {
>> +        socket_set_nonblock(sock);
>> +    }
>> +    /* connect to peer */
>> +    do {
>> +        rc = 0;
>> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
>> +            rc = -socket_error();
>>          }
>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -        if (!block) {
>> -            socket_set_nonblock(sock);
>> +    } while (rc == -EINTR);
>> +
>> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> +        if (in_progress) {
>> +            *in_progress = true;
>>          }
>> -        /* connect to peer */
>> -        do {
>> -            rc = 0;
>> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
>> -                rc = -socket_error();
>> -            }
>> -        } while (rc == -EINTR);
>> -
>> -  #ifdef _WIN32
>> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
>> -                       || rc == -WSAEALREADY)) {
>> -  #else
>> -        if (!block && (rc == -EINPROGRESS)) {
>> -  #endif
>> -            if (in_progress) {
>> -                *in_progress = true;
>> -            }
>> -        } else if (rc < 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;
>> +    } else if (rc < 0) {
>> +        closesocket(sock);
>> +        return -1;
>> +    }
>> +    return sock;
>> +}
>> +
>> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +{
>> +    struct addrinfo *res, *e;
>> +    int sock = -1;
>> +    bool block = qemu_opt_get_bool(opts, "block", 0);
>> +
>> +    res = inet_parse_connect_opts(opts, errp);
>> +    if (!res) {
>> +        return -1;
>> +    }
>> +
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>> +
>> +    for (e = res; e != NULL; e = e->ai_next) {
>> +        sock = inet_connect_addr(e, block, in_progress, errp);
>> +        if (in_progress && *in_progress) {
>> +            return sock;
>> +        } else if (sock >= 0) {
>> +            break;
>>          }
>> -        freeaddrinfo(res);
>> -        return sock;
>>      }
>> -    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>> +    if (sock < 0) {
>> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> 
> Necessary, because inet_connect_addr() doesn't do it.
> 
> Suggest to drop inet_connect_addr() parameter errp.
> 
no objections here
>> +    }
>>      freeaddrinfo(res);
>> -    return -1;
>> +    return sock;
>>  }
>>  
>>  int inet_dgram_opts(QemuOpts *opts)

[Qemu-devel] [PATCH v2 2/3] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Orit Wasserman]

No need to add non blocking parameters to the blocking inet_connect

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 migration-tcp.c |    2 +-
 nbd.c           |    2 +-
 qemu-sockets.c  |   24 ++++++++++++++++++++----
 qemu_socket.h   |    4 +++-
 ui/vnc.c        |    2 +-
 5 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index ac891c3..7f6ad98 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -88,7 +88,7 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = inet_connect(host_port, false, &in_progress, errp);
+    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
     if (error_is_set(errp)) {
         migrate_fd_error(s);
         return -1;
diff --git a/nbd.c b/nbd.c
index 0dd60c5..206f75c 100644
--- a/nbd.c
+++ b/nbd.c
@@ -162,7 +162,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, true, NULL, NULL);
+    return inet_connect(address_and_port, NULL);
 }
 
 int tcp_socket_incoming(const char *address, uint16_t port)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 68e4d30..4f5eca8 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -518,16 +518,32 @@ int inet_listen(const char *str, char *ostr, int olen,
     return sock;
 }
 
-int inet_connect(const char *str, bool block, bool *in_progress, Error **errp)
+int inet_connect(const char *str, Error **errp)
 {
     QemuOpts *opts;
     int sock = -1;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
-        if (block) {
-            qemu_opt_set(opts, "block", "on");
-        }
+        qemu_opt_set(opts, "block", "on");
+        sock = inet_connect_opts(opts, NULL, errp);
+    } else {
+        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+    }
+    qemu_opts_del(opts);
+    return sock;
+}
+
+
+int inet_nonblocking_connect(const char *str, bool *in_progress,
+                             Error **errp)
+{
+    QemuOpts *opts;
+    int sock = -1;
+
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    if (inet_parse(opts, str) == 0) {
+        qemu_opt_set(opts, "block", "off");
         sock = inet_connect_opts(opts, in_progress, errp);
     } else {
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
diff --git a/qemu_socket.h b/qemu_socket.h
index 30ae6af..c47f2b0 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -43,7 +43,9 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp);
 int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp);
-int inet_connect(const char *str, bool block, bool *in_progress, Error **errp);
+int inet_connect(const char *str, Error **errp);
+int inet_nonblocking_connect(const char *str, bool *in_progress,
+                             Error **errp);
 int inet_dgram_opts(QemuOpts *opts);
 const char *inet_strfamily(int family);
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 385e345..01b2daf 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3061,7 +3061,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, true, NULL, NULL);
+            vs->lsock = inet_connect(display, NULL);
         if (-1 == vs->lsock) {
             g_free(vs->display);
             vs->display = NULL;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 2/3] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> No need to add non blocking parameters to the blocking inet_connect
>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  migration-tcp.c |    2 +-
>  nbd.c           |    2 +-
>  qemu-sockets.c  |   24 ++++++++++++++++++++----
>  qemu_socket.h   |    4 +++-
>  ui/vnc.c        |    2 +-
>  5 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index ac891c3..7f6ad98 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -88,7 +88,7 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_connect(host_port, false, &in_progress, errp);
> +    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>      if (error_is_set(errp)) {
>          migrate_fd_error(s);
>          return -1;
> diff --git a/nbd.c b/nbd.c
> index 0dd60c5..206f75c 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -162,7 +162,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, true, NULL, NULL);
> +    return inet_connect(address_and_port, NULL);
>  }
>  
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 68e4d30..4f5eca8 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -518,16 +518,32 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
>  
> -int inet_connect(const char *str, bool block, bool *in_progress, Error **errp)
> +int inet_connect(const char *str, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
> -        if (block) {
> -            qemu_opt_set(opts, "block", "on");
> -        }
> +        qemu_opt_set(opts, "block", "on");
> +        sock = inet_connect_opts(opts, NULL, errp);

Let's go one step further: make inet_connect_opts() take a bool block
argument, and drop the stupid QemuOpt "block".  That way, opts are again
for user options only.

> +    } else {
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +    }
> +    qemu_opts_del(opts);
> +    return sock;
> +}
> +
> +
> +int inet_nonblocking_connect(const char *str, bool *in_progress,
> +                             Error **errp)
> +{
> +    QemuOpts *opts;
> +    int sock = -1;
> +
> +    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +    if (inet_parse(opts, str) == 0) {
> +        qemu_opt_set(opts, "block", "off");
>          sock = inet_connect_opts(opts, in_progress, errp);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
[...]

Re: [Qemu-devel] [PATCH v2 2/3] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Orit Wasserman]

On 09/13/2012 03:44 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> No need to add non blocking parameters to the blocking inet_connect
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  migration-tcp.c |    2 +-
>>  nbd.c           |    2 +-
>>  qemu-sockets.c  |   24 ++++++++++++++++++++----
>>  qemu_socket.h   |    4 +++-
>>  ui/vnc.c        |    2 +-
>>  5 files changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration-tcp.c b/migration-tcp.c
>> index ac891c3..7f6ad98 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -88,7 +88,7 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>      s->write = socket_write;
>>      s->close = tcp_close;
>>  
>> -    s->fd = inet_connect(host_port, false, &in_progress, errp);
>> +    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>>      if (error_is_set(errp)) {
>>          migrate_fd_error(s);
>>          return -1;
>> diff --git a/nbd.c b/nbd.c
>> index 0dd60c5..206f75c 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -162,7 +162,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, true, NULL, NULL);
>> +    return inet_connect(address_and_port, NULL);
>>  }
>>  
>>  int tcp_socket_incoming(const char *address, uint16_t port)
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 68e4d30..4f5eca8 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -518,16 +518,32 @@ int inet_listen(const char *str, char *ostr, int olen,
>>      return sock;
>>  }
>>  
>> -int inet_connect(const char *str, bool block, bool *in_progress, Error **errp)
>> +int inet_connect(const char *str, Error **errp)
>>  {
>>      QemuOpts *opts;
>>      int sock = -1;
>>  
>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>      if (inet_parse(opts, str) == 0) {
>> -        if (block) {
>> -            qemu_opt_set(opts, "block", "on");
>> -        }
>> +        qemu_opt_set(opts, "block", "on");
>> +        sock = inet_connect_opts(opts, NULL, errp);
> 
> Let's go one step further: make inet_connect_opts() take a bool block
> argument, and drop the stupid QemuOpt "block".  That way, opts are again
> for user options only.
Very good idea !
> 
>> +    } else {
>> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> +    }
>> +    qemu_opts_del(opts);
>> +    return sock;
>> +}
>> +
>> +
>> +int inet_nonblocking_connect(const char *str, bool *in_progress,
>> +                             Error **errp)
>> +{
>> +    QemuOpts *opts;
>> +    int sock = -1;
>> +
>> +    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>> +    if (inet_parse(opts, str) == 0) {
>> +        qemu_opt_set(opts, "block", "off");
>>          sock = inet_connect_opts(opts, in_progress, errp);
>>      } else {
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> [...]
> 

[Qemu-devel] [PATCH v2 3/3] Fix address handling in inet_nonblocking_connect

[Orit Wasserman]

getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one.  This is common on desktop setups that often have ipv6
configured but not actually working.

To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will have
a negative value

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 migration-tcp.c |   29 +++-------
 qemu-sockets.c  |  169 +++++++++++++++++++++++++++++++++++++++++--------------
 qemu_socket.h   |    9 ++-
 3 files changed, 142 insertions(+), 65 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 7f6ad98..cadea36 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
     return r;
 }
 
-static void tcp_wait_for_connect(void *opaque)
+static void tcp_wait_for_connect(int fd, void *opaque)
 {
     MigrationState *s = opaque;
-    int val, ret;
-    socklen_t valsize = sizeof(val);
 
-    DPRINTF("connect completed\n");
-    do {
-        ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
-    } while (ret == -1 && (socket_error()) == EINTR);
-
-    if (ret < 0) {
+    if (fd < 0) {
+        DPRINTF("migrate connect error\n");
+        s->fd = -1;
         migrate_fd_error(s);
-        return;
-    }
-
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-
-    if (val == 0)
+    } else {
+        DPRINTF("migrate connect success\n");
+        s->fd = fd;
         migrate_fd_connect(s);
-    else {
-        DPRINTF("error connecting %d\n", val);
-        migrate_fd_error(s);
     }
 }
 
@@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
+    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
+                                     &in_progress, errp);
     if (error_is_set(errp)) {
         migrate_fd_error(s);
         return -1;
@@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
 
     if (in_progress) {
         DPRINTF("connect in progress\n");
-        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
     } else {
         migrate_fd_connect(s);
     }
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 4f5eca8..f5d64c8 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
 
 #include "qemu_socket.h"
 #include "qemu-common.h" /* for qemu_isdigit */
+#include "main-loop.h"
 
 #ifndef AI_ADDRCONFIG
 # define AI_ADDRCONFIG 0
@@ -209,41 +210,27 @@ listen:
     return slisten;
 }
 
-static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
-{
-    struct addrinfo ai, *res;
-    int rc;
-    const char *addr;
-    const char *port;
-
-    memset(&ai,0, sizeof(ai));
-    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
-    ai.ai_family = PF_UNSPEC;
-    ai.ai_socktype = SOCK_STREAM;
-
-    addr = qemu_opt_get(opts, "host");
-    port = qemu_opt_get(opts, "port");
-    if (addr == NULL || port == NULL) {
-        fprintf(stderr,
-                "inet_parse_connect_opts: host and/or port not specified\n");
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
-        return NULL;
-    }
-
-    if (qemu_opt_get_bool(opts, "ipv4", 0))
-        ai.ai_family = PF_INET;
-    if (qemu_opt_get_bool(opts, "ipv6", 0))
-        ai.ai_family = PF_INET6;
+#ifdef _WIN32
+#define QEMU_SOCKET_RC_INPROGRESS(rc) \
+    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
+#else
+#define QEMU_SOCKET_RC_INPROGRESS(rc) \
+    ((rc) == -EINPROGRESS)
+#endif
 
-    /* lookup */
-    if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
-        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
-                gai_strerror(rc));
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
-        return NULL;
-    }
-    return res;
-}
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+    int fd;
+    struct addrinfo *addr_list;
+    struct addrinfo *current_addr;
+    ConnectHandler *callback;
+    void *opaque;
+    Error *errp;
+} ConnectState;
+
+static ConnectState connect_state = {
+    .fd = -1,
+};
 
 #ifdef _WIN32
 #define QEMU_SOCKET_RC_INPROGRESS(rc) \
@@ -254,12 +241,17 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
 #endif
 
 static int inet_connect_addr(struct addrinfo *addr, bool block,
-                             bool *in_progress, Error **errp)
+                             IOHandler *handler, bool *in_progress,
+                             Error **errp)
 {
     char uaddr[INET6_ADDRSTRLEN + 1];
     char uport[33];
     int sock, rc;
 
+    if (in_progress) {
+        *in_progress = false;
+    }
+
     if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
                     uaddr, INET6_ADDRSTRLEN, uport, 32,
                     NI_NUMERICHOST | NI_NUMERICSERV)) {
@@ -285,6 +277,8 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
     } while (rc == -EINTR);
 
     if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+        connect_state.fd = sock;
+        qemu_set_fd_handler2(sock, NULL, NULL, handler, &connect_state);
         if (in_progress) {
             *in_progress = true;
         }
@@ -295,6 +289,94 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
     return sock;
 }
 
+static void wait_for_connect(void *opaque)
+{
+    ConnectState *s = opaque;
+    int val = 0, rc = 0;
+    socklen_t valsize = sizeof(val);
+    bool in_progress = false;
+
+    do {
+        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
+    } while (rc == -1 && (socket_error()) == EINTR);
+
+    /* connect succeded */
+    if (!rc && !val) {
+        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+        freeaddrinfo(s->addr_list);
+        if (s->callback) {
+            s->callback(s->fd, s->opaque);
+        }
+        return;
+    }
+
+    if (!rc && val) {
+        rc = -val;
+    }
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+    closesocket(s->fd);
+    if (s->current_addr != NULL && s->current_addr->ai_next != NULL) {
+        s->current_addr = s->current_addr->ai_next;
+        s->fd = inet_connect_addr(s->current_addr, false, wait_for_connect,
+                                  &in_progress, &s->errp);
+        if (in_progress) {
+            return;
+        }
+    }
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+
+    closesocket(s->fd);
+    s->fd = rc;
+    freeaddrinfo(s->addr_list);
+    /* connect failed */
+    if (s->callback) {
+        s->callback(s->fd, s->opaque);
+    }
+    return;
+}
+
+static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
+{
+    struct addrinfo ai, *res;
+    int rc;
+    const char *addr;
+    const char *port;
+
+    memset(&ai, 0, sizeof(ai));
+    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
+    ai.ai_family = PF_UNSPEC;
+    ai.ai_socktype = SOCK_STREAM;
+
+    addr = qemu_opt_get(opts, "host");
+    port = qemu_opt_get(opts, "port");
+    if (addr == NULL || port == NULL) {
+        fprintf(stderr,
+                "inet_parse_connect_opts: host and/or port not specified\n");
+        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        return NULL;
+    }
+
+    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
+        ai.ai_family = PF_INET;
+    }
+    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
+        ai.ai_family = PF_INET6;
+    }
+
+    /* lookup */
+    rc = getaddrinfo(addr, port, &ai, &res);
+    if (rc != 0) {
+        fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
+                gai_strerror(rc));
+        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        return NULL;
+    }
+    return res;
+}
+
+
 int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
 {
     struct addrinfo *res, *e;
@@ -306,12 +388,12 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
         return -1;
     }
 
-    if (in_progress) {
-        *in_progress = false;
-    }
-
     for (e = res; e != NULL; e = e->ai_next) {
-        sock = inet_connect_addr(e, block, in_progress, errp);
+        if (!block) {
+            connect_state.addr_list = res;
+            connect_state.current_addr = e;
+        }
+        sock = inet_connect_addr(e, block, wait_for_connect, in_progress, errp);
         if (in_progress && *in_progress) {
             return sock;
         } else if (sock >= 0) {
@@ -534,9 +616,8 @@ int inet_connect(const char *str, Error **errp)
     return sock;
 }
 
-
-int inet_nonblocking_connect(const char *str, bool *in_progress,
-                             Error **errp)
+int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
+                             void *opaque, bool *in_progress, Error **errp)
 {
     QemuOpts *opts;
     int sock = -1;
@@ -544,6 +625,8 @@ int inet_nonblocking_connect(const char *str, bool *in_progress,
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
         qemu_opt_set(opts, "block", "off");
+        connect_state.callback = callback;
+        connect_state.opaque = opaque;
         sock = inet_connect_opts(opts, in_progress, errp);
     } else {
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
diff --git a/qemu_socket.h b/qemu_socket.h
index c47f2b0..c8431eb 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -38,14 +38,19 @@ void socket_set_block(int fd);
 void socket_set_nonblock(int fd);
 int send_all(int fd, const void *buf, int len1);
 
+/* callback function for nonblocking connect
+ * vaild fd on success, negative error code on failure
+ */
+typedef void ConnectHandler(int fd, void *opaque);
+
 /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
 int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
 int inet_listen(const char *str, char *ostr, int olen,
                 int socktype, int port_offset, Error **errp);
 int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp);
 int inet_connect(const char *str, Error **errp);
-int inet_nonblocking_connect(const char *str, bool *in_progress,
-                             Error **errp);
+int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
+                             void *opaque, bool *in_progress, Error **errp);
 int inet_dgram_opts(QemuOpts *opts);
 const char *inet_strfamily(int family);
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 3/3] Fix address handling in inet_nonblocking_connect

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> getaddrinfo can give us a list of addresses, but we only try to
> connect to the first one. If that fails we never proceed to
> the next one.  This is common on desktop setups that often have ipv6
> configured but not actually working.
>
> To fix this make inet_connect_nonblocking retry connection with a different
> address.
> callers on inet_nonblocking_connect register a callback function that will
> be called when connect opertion completes, in case of failure the fd will have
> a negative value
>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  migration-tcp.c |   29 +++-------
>  qemu-sockets.c  |  169 +++++++++++++++++++++++++++++++++++++++++--------------
>  qemu_socket.h   |    9 ++-
>  3 files changed, 142 insertions(+), 65 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 7f6ad98..cadea36 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
>      return r;
>  }
>  
> -static void tcp_wait_for_connect(void *opaque)
> +static void tcp_wait_for_connect(int fd, void *opaque)
>  {
>      MigrationState *s = opaque;
> -    int val, ret;
> -    socklen_t valsize = sizeof(val);
>  
> -    DPRINTF("connect completed\n");
> -    do {
> -        ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
> -    } while (ret == -1 && (socket_error()) == EINTR);
> -
> -    if (ret < 0) {
> +    if (fd < 0) {
> +        DPRINTF("migrate connect error\n");
> +        s->fd = -1;
>          migrate_fd_error(s);
> -        return;
> -    }
> -
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -
> -    if (val == 0)
> +    } else {
> +        DPRINTF("migrate connect success\n");
> +        s->fd = fd;
>          migrate_fd_connect(s);
> -    else {
> -        DPRINTF("error connecting %d\n", val);
> -        migrate_fd_error(s);
>      }
>  }
>  
> @@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
> +                                     &in_progress, errp);
>      if (error_is_set(errp)) {
>          migrate_fd_error(s);
>          return -1;
> @@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>  
>      if (in_progress) {
>          DPRINTF("connect in progress\n");
> -        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>      } else {
>          migrate_fd_connect(s);
>      }
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 4f5eca8..f5d64c8 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -24,6 +24,7 @@
>  
>  #include "qemu_socket.h"
>  #include "qemu-common.h" /* for qemu_isdigit */
> +#include "main-loop.h"
>  
>  #ifndef AI_ADDRCONFIG
>  # define AI_ADDRCONFIG 0
> @@ -209,41 +210,27 @@ listen:
>      return slisten;
>  }
>  
> -static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
> -{
> -    struct addrinfo ai, *res;
> -    int rc;
> -    const char *addr;
> -    const char *port;
> -
> -    memset(&ai,0, sizeof(ai));
> -    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> -    ai.ai_family = PF_UNSPEC;
> -    ai.ai_socktype = SOCK_STREAM;
> -
> -    addr = qemu_opt_get(opts, "host");
> -    port = qemu_opt_get(opts, "port");
> -    if (addr == NULL || port == NULL) {
> -        fprintf(stderr,
> -                "inet_parse_connect_opts: host and/or port not specified\n");
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> -        return NULL;
> -    }
> -
> -    if (qemu_opt_get_bool(opts, "ipv4", 0))
> -        ai.ai_family = PF_INET;
> -    if (qemu_opt_get_bool(opts, "ipv6", 0))
> -        ai.ai_family = PF_INET6;
> +#ifdef _WIN32
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
> +#else
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS)
> +#endif
>  
> -    /* lookup */
> -    if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> -        return NULL;
> -    }
> -    return res;
> -}

inet_parse_connect_opts() is only moved, not changed, I think.  I like
to keep code motion well away from code changes (separate patch) to
facilitate review.

> +/* Struct to store connect state for non blocking connect */
> +typedef struct ConnectState {
> +    int fd;
> +    struct addrinfo *addr_list;
> +    struct addrinfo *current_addr;
> +    ConnectHandler *callback;
> +    void *opaque;
> +    Error *errp;
> +} ConnectState;
> +
> +static ConnectState connect_state = {
> +    .fd = -1,
> +};

Why does a single connect_state suffice?

>  
>  #ifdef _WIN32
>  #define QEMU_SOCKET_RC_INPROGRESS(rc) \
> @@ -254,12 +241,17 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>  #endif
>  
>  static int inet_connect_addr(struct addrinfo *addr, bool block,
> -                             bool *in_progress, Error **errp)
> +                             IOHandler *handler, bool *in_progress,
> +                             Error **errp)

Sure you want a handler parameter?  It's always wait_for_connect()...

>  {
>      char uaddr[INET6_ADDRSTRLEN + 1];
>      char uport[33];
>      int sock, rc;
>  
> +    if (in_progress) {
> +        *in_progress = false;
> +    }
> +
>      if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>                      uaddr, INET6_ADDRSTRLEN, uport, 32,
>                      NI_NUMERICHOST | NI_NUMERICSERV)) {
> @@ -285,6 +277,8 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
>      } while (rc == -EINTR);
>  
>      if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        connect_state.fd = sock;
> +        qemu_set_fd_handler2(sock, NULL, NULL, handler, &connect_state);
>          if (in_progress) {
>              *in_progress = true;
>          }
> @@ -295,6 +289,94 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
>      return sock;
>  }
>  
> +static void wait_for_connect(void *opaque)
> +{
> +    ConnectState *s = opaque;
> +    int val = 0, rc = 0;
> +    socklen_t valsize = sizeof(val);
> +    bool in_progress = false;
> +
> +    do {
> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
> +    } while (rc == -1 && (socket_error()) == EINTR);

Either parenthesize both operands of && or none.  I prefer none.

> +
> +    /* connect succeded */
> +    if (!rc && !val) {
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +        freeaddrinfo(s->addr_list);
> +        if (s->callback) {
> +            s->callback(s->fd, s->opaque);
> +        }
> +        return;
> +    }
> +
> +    if (!rc && val) {
> +        rc = -val;
> +    }
> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +    closesocket(s->fd);
> +    if (s->current_addr != NULL && s->current_addr->ai_next != NULL) {

How can s->current_addr == NULL happen?

> +        s->current_addr = s->current_addr->ai_next;
> +        s->fd = inet_connect_addr(s->current_addr, false, wait_for_connect,
> +                                  &in_progress, &s->errp);

inet_connect_addr() either

1. completes connect (returns valid fd, sets in_progress to false), or

2. starts connect (returns valid fd, sets in_progress to true), or

3. fails (returns -1 and sets in_progress to false).

> +        if (in_progress) {
> +            return;

Case 2.

> +        }
> +    }

To reach this point, we either ran out of addresses (if not entered), or
connect to the current address completed (case 1), or connect failed
(case 3).

> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +
> +    closesocket(s->fd);
> +    s->fd = rc;
> +    freeaddrinfo(s->addr_list);
> +    /* connect failed */
> +    if (s->callback) {
> +        s->callback(s->fd, s->opaque);
> +    }
> +    return;

Either I'm confused, or this treats completed connect() as failure,
which is wrong.

> +}
> +
> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
> +{
> +    struct addrinfo ai, *res;
> +    int rc;
> +    const char *addr;
> +    const char *port;
> +
> +    memset(&ai, 0, sizeof(ai));
> +    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> +    ai.ai_family = PF_UNSPEC;
> +    ai.ai_socktype = SOCK_STREAM;
> +
> +    addr = qemu_opt_get(opts, "host");
> +    port = qemu_opt_get(opts, "port");
> +    if (addr == NULL || port == NULL) {
> +        fprintf(stderr,
> +                "inet_parse_connect_opts: host and/or port not specified\n");
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        return NULL;
> +    }
> +
> +    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
> +        ai.ai_family = PF_INET;
> +    }
> +    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
> +        ai.ai_family = PF_INET6;
> +    }
> +
> +    /* lookup */
> +    rc = getaddrinfo(addr, port, &ai, &res);
> +    if (rc != 0) {
> +        fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
> +                gai_strerror(rc));
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        return NULL;
> +    }
> +    return res;
> +}
> +
> +
>  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>  {
>      struct addrinfo *res, *e;
> @@ -306,12 +388,12 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>          return -1;
>      }
>  
> -    if (in_progress) {
> -        *in_progress = false;
> -    }
> -
>      for (e = res; e != NULL; e = e->ai_next) {
> -        sock = inet_connect_addr(e, block, in_progress, errp);
> +        if (!block) {
> +            connect_state.addr_list = res;
> +            connect_state.current_addr = e;
> +        }
> +        sock = inet_connect_addr(e, block, wait_for_connect, in_progress, errp);
>          if (in_progress && *in_progress) {
>              return sock;
>          } else if (sock >= 0) {
> @@ -534,9 +616,8 @@ int inet_connect(const char *str, Error **errp)
>      return sock;
>  }
>  
> -
> -int inet_nonblocking_connect(const char *str, bool *in_progress,
> -                             Error **errp)
> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
> +                             void *opaque, bool *in_progress, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
> @@ -544,6 +625,8 @@ int inet_nonblocking_connect(const char *str, bool *in_progress,
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
>          qemu_opt_set(opts, "block", "off");
> +        connect_state.callback = callback;
> +        connect_state.opaque = opaque;
>          sock = inet_connect_opts(opts, in_progress, errp);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> diff --git a/qemu_socket.h b/qemu_socket.h
> index c47f2b0..c8431eb 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -38,14 +38,19 @@ void socket_set_block(int fd);
>  void socket_set_nonblock(int fd);
>  int send_all(int fd, const void *buf, int len1);
>  
> +/* callback function for nonblocking connect
> + * vaild fd on success, negative error code on failure
> + */
> +typedef void ConnectHandler(int fd, void *opaque);
> +
>  /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
>  int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
>  int inet_listen(const char *str, char *ostr, int olen,
>                  int socktype, int port_offset, Error **errp);
>  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp);
>  int inet_connect(const char *str, Error **errp);
> -int inet_nonblocking_connect(const char *str, bool *in_progress,
> -                             Error **errp);
> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
> +                             void *opaque, bool *in_progress, Error **errp);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);

Good stuff!

Re: [Qemu-devel] [PATCH v2 3/3] Fix address handling in inet_nonblocking_connect

[Michael S. Tsirkin]

On Thu, Sep 13, 2012 at 03:22:24PM +0200, Markus Armbruster wrote:
> > +/* Struct to store connect state for non blocking connect */
> > +typedef struct ConnectState {
> > +    int fd;
> > +    struct addrinfo *addr_list;
> > +    struct addrinfo *current_addr;
> > +    ConnectHandler *callback;
> > +    void *opaque;
> > +    Error *errp;
> > +} ConnectState;
> > +
> > +static ConnectState connect_state = {
> > +    .fd = -1,
> > +};
> 
> Why does a single connect_state suffice?

Even if it does for the specific use, better not to
make assumptions about API use. Let's pass connect state
to APIs.

Re: [Qemu-devel] [PATCH v2 3/3] Fix address handling in inet_nonblocking_connect

[Orit Wasserman]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]





On 13 בספט 2012, at 16:30, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 13, 2012 at 03:22:24PM +0200, Markus Armbruster wrote:
>>> +/* Struct to store connect state for non blocking connect */
>>> +typedef struct ConnectState {
>>> +    int fd;
>>> +    struct addrinfo *addr_list;
>>> +    struct addrinfo *current_addr;
>>> +    ConnectHandler *callback;
>>> +    void *opaque;
>>> +    Error *errp;
>>> +} ConnectState;
>>> +
>>> +static ConnectState connect_state = {
>>> +    .fd = -1,
>>> +};
>> 
>> Why does a single connect_state suffice?
> 
> Even if it does for the specific use, better not to
> make assumptions about API use. Let's pass connect state
> to APIs.
I agree,
Orit

[-- Attachment #2: Type: text/html, Size: 2418 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] Fix address handling in inet_nonblocking_connect

[Orit Wasserman]

On 09/13/2012 04:22 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> getaddrinfo can give us a list of addresses, but we only try to
>> connect to the first one. If that fails we never proceed to
>> the next one.  This is common on desktop setups that often have ipv6
>> configured but not actually working.
>>
>> To fix this make inet_connect_nonblocking retry connection with a different
>> address.
>> callers on inet_nonblocking_connect register a callback function that will
>> be called when connect opertion completes, in case of failure the fd will have
>> a negative value
>>
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  migration-tcp.c |   29 +++-------
>>  qemu-sockets.c  |  169 +++++++++++++++++++++++++++++++++++++++++--------------
>>  qemu_socket.h   |    9 ++-
>>  3 files changed, 142 insertions(+), 65 deletions(-)
>>
>> diff --git a/migration-tcp.c b/migration-tcp.c
>> index 7f6ad98..cadea36 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
>>      return r;
>>  }
>>  
>> -static void tcp_wait_for_connect(void *opaque)
>> +static void tcp_wait_for_connect(int fd, void *opaque)
>>  {
>>      MigrationState *s = opaque;
>> -    int val, ret;
>> -    socklen_t valsize = sizeof(val);
>>  
>> -    DPRINTF("connect completed\n");
>> -    do {
>> -        ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>> -    } while (ret == -1 && (socket_error()) == EINTR);
>> -
>> -    if (ret < 0) {
>> +    if (fd < 0) {
>> +        DPRINTF("migrate connect error\n");
>> +        s->fd = -1;
>>          migrate_fd_error(s);
>> -        return;
>> -    }
>> -
>> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> -
>> -    if (val == 0)
>> +    } else {
>> +        DPRINTF("migrate connect success\n");
>> +        s->fd = fd;
>>          migrate_fd_connect(s);
>> -    else {
>> -        DPRINTF("error connecting %d\n", val);
>> -        migrate_fd_error(s);
>>      }
>>  }
>>  
>> @@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>      s->write = socket_write;
>>      s->close = tcp_close;
>>  
>> -    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
>> +                                     &in_progress, errp);
>>      if (error_is_set(errp)) {
>>          migrate_fd_error(s);
>>          return -1;
>> @@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>  
>>      if (in_progress) {
>>          DPRINTF("connect in progress\n");
>> -        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>>      } else {
>>          migrate_fd_connect(s);
>>      }
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 4f5eca8..f5d64c8 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -24,6 +24,7 @@
>>  
>>  #include "qemu_socket.h"
>>  #include "qemu-common.h" /* for qemu_isdigit */
>> +#include "main-loop.h"
>>  
>>  #ifndef AI_ADDRCONFIG
>>  # define AI_ADDRCONFIG 0
>> @@ -209,41 +210,27 @@ listen:
>>      return slisten;
>>  }
>>  
>> -static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>> -{
>> -    struct addrinfo ai, *res;
>> -    int rc;
>> -    const char *addr;
>> -    const char *port;
>> -
>> -    memset(&ai,0, sizeof(ai));
>> -    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>> -    ai.ai_family = PF_UNSPEC;
>> -    ai.ai_socktype = SOCK_STREAM;
>> -
>> -    addr = qemu_opt_get(opts, "host");
>> -    port = qemu_opt_get(opts, "port");
>> -    if (addr == NULL || port == NULL) {
>> -        fprintf(stderr,
>> -                "inet_parse_connect_opts: host and/or port not specified\n");
>> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -        return NULL;
>> -    }
>> -
>> -    if (qemu_opt_get_bool(opts, "ipv4", 0))
>> -        ai.ai_family = PF_INET;
>> -    if (qemu_opt_get_bool(opts, "ipv6", 0))
>> -        ai.ai_family = PF_INET6;
>> +#ifdef _WIN32
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
>> +#else
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS)
>> +#endif
>>  
>> -    /* lookup */
>> -    if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
>> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>> -                gai_strerror(rc));
>> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> -        return NULL;
>> -    }
>> -    return res;
>> -}
> 
> inet_parse_connect_opts() is only moved, not changed, I think.  I like
> to keep code motion well away from code changes (separate patch) to
> facilitate review.
> 
>> +/* Struct to store connect state for non blocking connect */
>> +typedef struct ConnectState {
>> +    int fd;
>> +    struct addrinfo *addr_list;
>> +    struct addrinfo *current_addr;
>> +    ConnectHandler *callback;
>> +    void *opaque;
>> +    Error *errp;
>> +} ConnectState;
>> +
>> +static ConnectState connect_state = {
>> +    .fd = -1,
>> +};
> 
> Why does a single connect_state suffice?
> 
>>  
>>  #ifdef _WIN32
>>  #define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> @@ -254,12 +241,17 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>  #endif
>>  
>>  static int inet_connect_addr(struct addrinfo *addr, bool block,
>> -                             bool *in_progress, Error **errp)
>> +                             IOHandler *handler, bool *in_progress,
>> +                             Error **errp)
> 
> Sure you want a handler parameter?  It's always wait_for_connect()...
> 
>>  {
>>      char uaddr[INET6_ADDRSTRLEN + 1];
>>      char uport[33];
>>      int sock, rc;
>>  
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>> +
>>      if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
>>                      uaddr, INET6_ADDRSTRLEN, uport, 32,
>>                      NI_NUMERICHOST | NI_NUMERICSERV)) {
>> @@ -285,6 +277,8 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
>>      } while (rc == -EINTR);
>>  
>>      if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> +        connect_state.fd = sock;
>> +        qemu_set_fd_handler2(sock, NULL, NULL, handler, &connect_state);
>>          if (in_progress) {
>>              *in_progress = true;
>>          }
>> @@ -295,6 +289,94 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
>>      return sock;
>>  }
>>  
>> +static void wait_for_connect(void *opaque)
>> +{
>> +    ConnectState *s = opaque;
>> +    int val = 0, rc = 0;
>> +    socklen_t valsize = sizeof(val);
>> +    bool in_progress = false;
>> +
>> +    do {
>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>> +    } while (rc == -1 && (socket_error()) == EINTR);
> 
> Either parenthesize both operands of && or none.  I prefer none.
> 
>> +
>> +    /* connect succeded */
>> +    if (!rc && !val) {
>> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> +        freeaddrinfo(s->addr_list);
>> +        if (s->callback) {
>> +            s->callback(s->fd, s->opaque);
>> +        }
>> +        return;
>> +    }
>> +
>> +    if (!rc && val) {
>> +        rc = -val;
>> +    }
>> +
>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> +    closesocket(s->fd);
>> +    if (s->current_addr != NULL && s->current_addr->ai_next != NULL) {
> 
> How can s->current_addr == NULL happen?
can't see a scenario , but it never hurt to check. 
> 
>> +        s->current_addr = s->current_addr->ai_next;
>> +        s->fd = inet_connect_addr(s->current_addr, false, wait_for_connect,
>> +                                  &in_progress, &s->errp);
> 
> inet_connect_addr() either
> 
> 1. completes connect (returns valid fd, sets in_progress to false), or
> 
> 2. starts connect (returns valid fd, sets in_progress to true), or
> 
> 3. fails (returns -1 and sets in_progress to false).
> 
>> +        if (in_progress) {
>> +            return;
> 
> Case 2.
> 
>> +        }
>> +    }
> 
> To reach this point, we either ran out of addresses (if not entered), or
> connect to the current address completed (case 1), or connect failed
> (case 3).
> 
>> +
>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> +
>> +    closesocket(s->fd);
>> +    s->fd = rc;
>> +    freeaddrinfo(s->addr_list);
>> +    /* connect failed */
>> +    if (s->callback) {
>> +        s->callback(s->fd, s->opaque);
>> +    }
>> +    return;
> 
> Either I'm confused, or this treats completed connect() as failure,
> which is wrong.
> 
I think you caught a bug , i will fix it ...
Orit

>> +}
>> +
>> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>> +{
>> +    struct addrinfo ai, *res;
>> +    int rc;
>> +    const char *addr;
>> +    const char *port;
>> +
>> +    memset(&ai, 0, sizeof(ai));
>> +    ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>> +    ai.ai_family = PF_UNSPEC;
>> +    ai.ai_socktype = SOCK_STREAM;
>> +
>> +    addr = qemu_opt_get(opts, "host");
>> +    port = qemu_opt_get(opts, "port");
>> +    if (addr == NULL || port == NULL) {
>> +        fprintf(stderr,
>> +                "inet_parse_connect_opts: host and/or port not specified\n");
>> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> +        return NULL;
>> +    }
>> +
>> +    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
>> +        ai.ai_family = PF_INET;
>> +    }
>> +    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
>> +        ai.ai_family = PF_INET6;
>> +    }
>> +
>> +    /* lookup */
>> +    rc = getaddrinfo(addr, port, &ai, &res);
>> +    if (rc != 0) {
>> +        fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
>> +                gai_strerror(rc));
>> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> +        return NULL;
>> +    }
>> +    return res;
>> +}
>> +
>> +
>>  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>  {
>>      struct addrinfo *res, *e;
>> @@ -306,12 +388,12 @@ int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>          return -1;
>>      }
>>  
>> -    if (in_progress) {
>> -        *in_progress = false;
>> -    }
>> -
>>      for (e = res; e != NULL; e = e->ai_next) {
>> -        sock = inet_connect_addr(e, block, in_progress, errp);
>> +        if (!block) {
>> +            connect_state.addr_list = res;
>> +            connect_state.current_addr = e;
>> +        }
>> +        sock = inet_connect_addr(e, block, wait_for_connect, in_progress, errp);
>>          if (in_progress && *in_progress) {
>>              return sock;
>>          } else if (sock >= 0) {
>> @@ -534,9 +616,8 @@ int inet_connect(const char *str, Error **errp)
>>      return sock;
>>  }
>>  
>> -
>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>> -                             Error **errp)
>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>> +                             void *opaque, bool *in_progress, Error **errp)
>>  {
>>      QemuOpts *opts;
>>      int sock = -1;
>> @@ -544,6 +625,8 @@ int inet_nonblocking_connect(const char *str, bool *in_progress,
>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>      if (inet_parse(opts, str) == 0) {
>>          qemu_opt_set(opts, "block", "off");
>> +        connect_state.callback = callback;
>> +        connect_state.opaque = opaque;
>>          sock = inet_connect_opts(opts, in_progress, errp);
>>      } else {
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>> diff --git a/qemu_socket.h b/qemu_socket.h
>> index c47f2b0..c8431eb 100644
>> --- a/qemu_socket.h
>> +++ b/qemu_socket.h
>> @@ -38,14 +38,19 @@ void socket_set_block(int fd);
>>  void socket_set_nonblock(int fd);
>>  int send_all(int fd, const void *buf, int len1);
>>  
>> +/* callback function for nonblocking connect
>> + * vaild fd on success, negative error code on failure
>> + */
>> +typedef void ConnectHandler(int fd, void *opaque);
>> +
>>  /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
>>  int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp);
>>  int inet_listen(const char *str, char *ostr, int olen,
>>                  int socktype, int port_offset, Error **errp);
>>  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp);
>>  int inet_connect(const char *str, Error **errp);
>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>> -                             Error **errp);
>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>> +                             void *opaque, bool *in_progress, Error **errp);
>>  int inet_dgram_opts(QemuOpts *opts);
>>  const char *inet_strfamily(int family);
> 
> Good stuff!
> 

Re: [Qemu-devel] [PATCH v2 0/3] nonblocking connect address handling cleanup

[Amos Kong]

On 12/09/12 19:12, Orit Wasserman wrote:
> getaddrinfo can give us a list of addresses, but we only try to
> connect to the first one. If that fails we never proceed to
> the next one.  This is common on desktop setups that often have ipv6
> configured but not actually working.
> A simple way to reproduce the problem is migration:
> for the destination use -incoming tcp:0:4444, run migrate -d tcp:localhost:4444
> migration will fail on hosts that have both IPv4 and IPV6 address for localhost.
>
> To fix this, refactor address resolution code and make inet_nonblocking_connect
> retry connection with a different address.


Looks good to me.

Reviewed-by: Amos Kong <akong@redhat.com>
Tested-by: Amos Kong <akong@redhat.com>


> Orit Wasserman (3):
>    Refactor inet_connect_opts function
>    Separate inet_connect into inet_connect (blocking) and
>      inet_nonblocking_connect
>    Fix address handling in inet_nonblocking_connect
>
>   migration-tcp.c |   29 ++-----
>   nbd.c           |    2 +-
>   qemu-sockets.c  |  254 +++++++++++++++++++++++++++++++++++++++++--------------
>   qemu_socket.h   |    9 ++-
>   ui/vnc.c        |    2 +-
>   5 files changed, 208 insertions(+), 88 deletions(-)
>

-- 
			Amos.

Re: [Qemu-devel] [PATCH v2 0/3] nonblocking connect address handling cleanup

[Michael S. Tsirkin]

On Wed, Sep 12, 2012 at 02:12:55PM +0300, Orit Wasserman wrote:
> getaddrinfo can give us a list of addresses, but we only try to
> connect to the first one. If that fails we never proceed to
> the next one.  This is common on desktop setups that often have ipv6
> configured but not actually working.
> A simple way to reproduce the problem is migration:
> for the destination use -incoming tcp:0:4444, run migrate -d tcp:localhost:4444
> migration will fail on hosts that have both IPv4 and IPV6 address for localhost.
> 
> To fix this, refactor address resolution code and make inet_nonblocking_connect
> retry connection with a different address.

Nice improvement over my patch. thanks

For the series:
Acked-by: Michael S. Tsirkin <mst@redhat.com>

Anthony, we got reports offlist from people with such
setups - migration under 1.2 regressed for them.
So I think we need this on 1.2.X as well if
this ever happens.

> Orit Wasserman (3):
>   Refactor inet_connect_opts function
>   Separate inet_connect into inet_connect (blocking) and
>     inet_nonblocking_connect
>   Fix address handling in inet_nonblocking_connect
> 
>  migration-tcp.c |   29 ++-----
>  nbd.c           |    2 +-
>  qemu-sockets.c  |  254 +++++++++++++++++++++++++++++++++++++++++--------------
>  qemu_socket.h   |    9 ++-
>  ui/vnc.c        |    2 +-
>  5 files changed, 208 insertions(+), 88 deletions(-)
> 
> -- 
> 1.7.7.6