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

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

[Orit Wasserman]

Changes from v2:
	- remove the use of getnameinfo
	- remove errp for inet_connect_addr
	- remove QemuOpt "block"
	- fix errors in wait_for_connect 
	- pass ConnectState as a parameter to allow concurrent connect ops

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.

Michael S. Tsirkin (1):
  Refactor inet_connect_opts function

Orit Wasserman (2):
  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-char.c     |    2 +-
 qemu-sockets.c  |  230 +++++++++++++++++++++++++++++++++++++++----------------
 qemu_socket.h   |   14 +++-
 ui/vnc.c        |    2 +-
 6 files changed, 188 insertions(+), 91 deletions(-)

-- 
1.7.7.6

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

[Orit Wasserman]

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

refactor address resolution code to fix nonblocking connect
remove getnameinfo call

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 |  144 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 81 insertions(+), 63 deletions(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 361d890..939a453 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -209,95 +209,113 @@ listen:
     return slisten;
 }
 
-int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
+#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)
 {
-    struct addrinfo ai,*res,*e;
+    int sock, rc;
+
+    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();
+        }
+    } while (rc == -EINTR);
+
+    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+        if (in_progress) {
+            *in_progress = true;
+        }
+    } else if (rc < 0) {
+        closesocket(sock);
+        return -1;
+    }
+    return sock;
+}
+
+static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
+{
+    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));
+    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))
+    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
         ai.ai_family = PF_INET;
-    if (qemu_opt_get_bool(opts, "ipv6", 0))
+    }
+    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
         ai.ai_family = PF_INET6;
+    }
 
     /* lookup */
-    if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
-        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
+    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 -1;
+        return NULL;
+    }
+    return res;
+}
+
+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) {
-        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;
-        }
-        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
-        if (!block) {
-            socket_set_nonblock(sock);
+        sock = inet_connect_addr(e, block, in_progress);
+        if (sock >= 0) {
+            break;
         }
-        /* 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;
-        }
-        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 v3 1/3] Refactor inet_connect_opts function

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> refactor address resolution code to fix nonblocking connect
> remove getnameinfo call
>
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com> 

> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +#ifdef _WIN32
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
> +#else
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS)
> +#endif

Not specific to this series, but this is used all around in qemu.
Should we put it into osdep.h and refactor?

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

[Amos Kong]

On 14/09/12 02:58, Orit Wasserman wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
>
> refactor address resolution code to fix nonblocking connect
> remove getnameinfo call
>
> 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>

Hi Orit,

Happy new year!

> ---
>   qemu-sockets.c |  144 +++++++++++++++++++++++++++++++------------------------
>   1 files changed, 81 insertions(+), 63 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 361d890..939a453 100644

...

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

trivial comment:

You moved this block to head of inet_connect_addr() in [patch 3/3],
why not do this in this patch?

I know if *in_progress becomes "true", sock is always >= 0, for loop 
will exits.
But moving this block to inet_connect_addr() is clearer.


>       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;
> -        }
> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -        if (!block) {
> -            socket_set_nonblock(sock);
> +        sock = inet_connect_addr(e, block, in_progress);
> +        if (sock >= 0) {
> +            break;
>           }
> -        /* 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;
> -        }
> -        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)
>

-- 
			Amos.

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

[Amos Kong]

On 19/09/12 16:33, Amos Kong wrote:
> On 14/09/12 02:58, Orit Wasserman wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>
>> refactor address resolution code to fix nonblocking connect
>> remove getnameinfo call
>>
>> 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>
>
> Hi Orit,
>
> Happy new year!
>
>> ---
>>   qemu-sockets.c |  144
>> +++++++++++++++++++++++++++++++------------------------
>>   1 files changed, 81 insertions(+), 63 deletions(-)
>>
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 361d890..939a453 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -209,95 +209,113 @@ listen:
>>      return slisten;
>>  }
>>
>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>> +#ifdef _WIN32
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)

^^ Brackets are only be used for first 'rc'

>> +#else
>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>> +    ((rc) == -EINPROGRESS)
>> +#endif

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

In this error path, in_progress is not assigned, but it's used
in tcp_start_outgoing_migration()

Let also set the default value of in_progress to 'false' in
tcp_start_outgoing_migration()

>> +    }
>> +
>
>> +    if (in_progress) {
>> +        *in_progress = false;
>>       }
>
> trivial comment:
>
> You moved this block to head of inet_connect_addr() in [patch 3/3],
> why not do this in this patch?
>
> I know if *in_progress becomes "true", sock is always >= 0, for loop
> will exits.
> But moving this block to inet_connect_addr() is clearer.
>
>
>>       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;
>> -        }
>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -        if (!block) {
>> -            socket_set_nonblock(sock);
>> +        sock = inet_connect_addr(e, block, in_progress);
>> +        if (sock >= 0) {
>> +            break;
>>           }

...

-- 
			Amos.

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

[Orit Wasserman]

On 09/20/2012 05:33 AM, Amos Kong wrote:
> On 19/09/12 16:33, Amos Kong wrote:
>> On 14/09/12 02:58, Orit Wasserman wrote:
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>
>>> refactor address resolution code to fix nonblocking connect
>>> remove getnameinfo call
>>>
>>> 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>
>>
>> Hi Orit,
>>
>> Happy new year!
>>
>>> ---
>>>   qemu-sockets.c |  144
>>> +++++++++++++++++++++++++++++++------------------------
>>>   1 files changed, 81 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>> index 361d890..939a453 100644
>>> --- a/qemu-sockets.c
>>> +++ b/qemu-sockets.c
>>> @@ -209,95 +209,113 @@ listen:
>>>      return slisten;
>>>  }
>>>
>>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>> +#ifdef _WIN32
>>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
> 
> ^^ Brackets are only be used for first 'rc'
good catch !
> 
>>> +#else
>>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>>> +    ((rc) == -EINPROGRESS)
>>> +#endif
> 
>>
>>> +
>>> +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;
> 
> In this error path, in_progress is not assigned, but it's used
> in tcp_start_outgoing_migration()
> 
> Let also set the default value of in_progress to 'false' in
> tcp_start_outgoing_migration()
> 
ok.
>>> +    }
>>> +
>>
>>> +    if (in_progress) {
>>> +        *in_progress = false;
>>>       }
>>
>> trivial comment:
>>
>> You moved this block to head of inet_connect_addr() in [patch 3/3],
>> why not do this in this patch?
>>
>> I know if *in_progress becomes "true", sock is always >= 0, for loop
>> will exits.
>> But moving this block to inet_connect_addr() is clearer.
>>
>>
>>>       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;
>>> -        }
>>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>>> -        if (!block) {
>>> -            socket_set_nonblock(sock);
>>> +        sock = inet_connect_addr(e, block, in_progress);
>>> +        if (sock >= 0) {
>>> +            break;
>>>           }
> 
> ...
> 

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

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> On 19/09/12 16:33, Amos Kong wrote:
>> On 14/09/12 02:58, Orit Wasserman wrote:
>>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>>
>>> refactor address resolution code to fix nonblocking connect
>>> remove getnameinfo call
>>>
>>> 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>
>>
>> Hi Orit,
>>
>> Happy new year!
>>
>>> ---
>>>   qemu-sockets.c |  144
>>> +++++++++++++++++++++++++++++++------------------------
>>>   1 files changed, 81 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>> index 361d890..939a453 100644
>>> --- a/qemu-sockets.c
>>> +++ b/qemu-sockets.c
>>> @@ -209,95 +209,113 @@ listen:
>>>      return slisten;
>>>  }
>>>
>>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
>>> +#ifdef _WIN32
>>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>>> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
>
> ^^ Brackets are only be used for first 'rc'
>
>>> +#else
>>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
>>> +    ((rc) == -EINPROGRESS)
>>> +#endif
>
>>
>>> +
>>> +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;
>
> In this error path, in_progress is not assigned, but it's used
> in tcp_start_outgoing_migration()

Took me a minute to see what you mean.

inet_connect_opts() is called by inet_connect(), which is called by
tcp_start_outgoing_migration(), which uses in_progress.

The other callers of inet_connect() and inet_connect_opts() aren't
affected, because they pass null in_progress.

> Let also set the default value of in_progress to 'false' in
> tcp_start_outgoing_migration()

No, let's make sure the functions that take an in_progress argument
always set it.  That's easier to use safely.

>>> +    }
>>> +
>>
>>> +    if (in_progress) {
>>> +        *in_progress = false;
>>>       }
>>
>> trivial comment:
>>
>> You moved this block to head of inet_connect_addr() in [patch 3/3],
>> why not do this in this patch?
>>
>> I know if *in_progress becomes "true", sock is always >= 0, for loop
>> will exits.
>> But moving this block to inet_connect_addr() is clearer.

Makes inet_connect_addr() always set in_progress, which I like.

[...]

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

[Orit Wasserman]

On 09/19/2012 11:33 AM, Amos Kong wrote:
> On 14/09/12 02:58, Orit Wasserman wrote:
>> From: "Michael S. Tsirkin" <mst@redhat.com>
>>
>> refactor address resolution code to fix nonblocking connect
>> remove getnameinfo call
>>
>> 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>
> 
> Hi Orit,
> 
> Happy new year!
> 
>> ---
>>   qemu-sockets.c |  144 +++++++++++++++++++++++++++++++------------------------
>>   1 files changed, 81 insertions(+), 63 deletions(-)
>>
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 361d890..939a453 100644
> 
> ...
> 
>> +
>> +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;
>>       }
> 
> trivial comment:
> 
> You moved this block to head of inet_connect_addr() in [patch 3/3],
> why not do this in this patch?
> 
> I know if *in_progress becomes "true", sock is always >= 0, for loop will exits.
> But moving this block to inet_connect_addr() is clearer.
> 
sure
> 
>>       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;
>> -        }
>> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>> -        if (!block) {
>> -            socket_set_nonblock(sock);
>> +        sock = inet_connect_addr(e, block, in_progress);
>> +        if (sock >= 0) {
>> +            break;
>>           }
>> -        /* 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;
>> -        }
>> -        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)
>>
> 

[Qemu-devel] [PATCH v3 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
add block parameter for inet_connect_opts instead of using QemuOpt "block".

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 migration-tcp.c |    2 +-
 nbd.c           |    2 +-
 qemu-char.c     |    2 +-
 qemu-sockets.c  |   28 +++++++++++++++++++++-------
 qemu_socket.h   |    7 +++++--
 ui/vnc.c        |    2 +-
 6 files changed, 30 insertions(+), 13 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-char.c b/qemu-char.c
index 767da93..c442952 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
         if (is_listen) {
             fd = inet_listen_opts(opts, 0, NULL);
         } else {
-            fd = inet_connect_opts(opts, NULL, NULL);
+            fd = inet_connect_opts(opts, true, NULL, NULL);
         }
     }
     if (fd < 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 939a453..212075d 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -290,11 +290,11 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
     return res;
 }
 
-int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
+int inet_connect_opts(QemuOpts *opts, bool block, 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) {
@@ -511,17 +511,31 @@ 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");
-        }
-        sock = inet_connect_opts(opts, in_progress, errp);
+        sock = inet_connect_opts(opts, true, 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) {
+        sock = inet_connect_opts(opts, false, in_progress, errp);
     } else {
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
     }
diff --git a/qemu_socket.h b/qemu_socket.h
index 30ae6af..80696aa 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -42,8 +42,11 @@ int send_all(int fd, const void *buf, int len1);
 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_opts(QemuOpts *opts, 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 v3 2/3] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> wrote:
> No need to add non blocking parameters to the blocking inet_connect
> add block parameter for inet_connect_opts instead of using QemuOpt "block".
>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com> 

Re: [Qemu-devel] [PATCH v3 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
> add block parameter for inet_connect_opts instead of using QemuOpt "block".

I believe option "block" in qemu-sockets.c's dummy_opts is now unused,
and should be dropped.  It was added in commit a6ba35b3.

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

[Orit Wasserman]

On 09/20/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
>> add block parameter for inet_connect_opts instead of using QemuOpt "block".
> 
> I believe option "block" in qemu-sockets.c's dummy_opts is now unused,
> and should be dropped.  It was added in commit a6ba35b3.
> 
You are right , I will remove it.

Orit

[Qemu-devel] [PATCH v3 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-char.c     |    2 +-
 qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
 qemu_socket.h   |   13 ++++++--
 4 files changed, 102 insertions(+), 37 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-char.c b/qemu-char.c
index c442952..11cd5ef 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
         if (is_listen) {
             fd = inet_listen_opts(opts, 0, NULL);
         } else {
-            fd = inet_connect_opts(opts, true, NULL, NULL);
+            fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
         }
     }
     if (fd < 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 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
@@ -217,11 +218,69 @@ listen:
     ((rc) == -EINPROGRESS)
 #endif
 
+/* 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 int inet_connect_addr(struct addrinfo *addr, bool block,
-                             bool *in_progress)
+                             bool *in_progress, ConnectState *connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+    ConnectState *s = opaque;
+    int val = 0, rc = 0;
+    socklen_t valsize = sizeof(val);
+    bool in_progress = false;
+
+    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
+
+    do {
+        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
+    } while (rc == -1 && socket_error() == EINTR);
+
+    /* update rc to contain error details */
+    if (!rc && val) {
+        rc = -val;
+    }
+
+    /* connect error */
+    if (rc < 0) {
+        closesocket(s->fd);
+        s->fd = rc;
+    }
+
+    /* try to connect to the next address on the list */
+    while (s->current_addr->ai_next != NULL && s->fd < 0) {
+        s->current_addr = s->current_addr->ai_next;
+        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
+        /* connect in progress */
+        if (in_progress) {
+            return;
+        }
+    }
+
+    freeaddrinfo(s->addr_list);
+    if (s->callback) {
+        s->callback(s->fd, s->opaque);
+    }
+    g_free(s);
+    return;
+}
+
+static int inet_connect_addr(struct addrinfo *addr, bool block,
+                             bool *in_progress, ConnectState *connect_state)
 {
     int sock, rc;
 
+    if (in_progress) {
+        *in_progress = false;
+    }
     sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
     if (sock < 0) {
         fprintf(stderr, "%s: socket(%s): %s\n", __func__,
@@ -241,6 +300,9 @@ 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, wait_for_connect,
+                             connect_state);
         if (in_progress) {
             *in_progress = true;
         }
@@ -259,6 +321,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
     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;
@@ -291,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
 }
 
 int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
-                      Error **errp)
+                      Error **errp, ConnectState *connect_state)
 {
     struct addrinfo *res, *e;
     int sock = -1;
@@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
         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);
-        if (sock >= 0) {
+        if (!block) {
+            connect_state->addr_list = res;
+            connect_state->current_addr = e;
+        }
+        sock = inet_connect_addr(e, block, in_progress, connect_state);
+        if (in_progress && *in_progress) {
+            return sock;
+        } else if (sock >= 0) {
             break;
         }
     }
     if (sock < 0) {
         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
     }
+    g_free(connect_state);
     freeaddrinfo(res);
     return sock;
 }
@@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
-        sock = inet_connect_opts(opts, true, NULL, errp);
+        sock = inet_connect_opts(opts, true, NULL, errp, NULL);
     } else {
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
     }
@@ -526,16 +592,19 @@ 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;
+    ConnectState *connect_state;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     if (inet_parse(opts, str) == 0) {
-        sock = inet_connect_opts(opts, false, in_progress, errp);
+        connect_state = g_malloc0(sizeof(*connect_state));
+        connect_state->callback = callback;
+        connect_state->opaque = opaque;
+        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);
     } else {
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
     }
diff --git a/qemu_socket.h b/qemu_socket.h
index 80696aa..715dc9d 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -38,15 +38,22 @@ 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);
+/* structure for nonblocking connect state */
+typedef struct ConnectState ConnectState;
+
 /* 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 block, bool *in_progress,
-                      Error **errp);
+                      Error **errp, ConnectState *s);
 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 v3 3/3] Fix address handling in inet_nonblocking_connect

[Juan Quintela]

Orit Wasserman <owasserm@redhat.com> 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.
>
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Just thinking out loud to be if I understood this correctly

> +    do {
> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
> +    } while (rc == -1 && socket_error() == EINTR);

"rc"" return the error code of getsockopt()
and "val" returns the error code of the socket if there is one.

> +
> +    /* update rc to contain error details */
> +    if (!rc && val) {
> +        rc = -val;
> +    }

If getsockopt() succeeds, we "reuse" "rc" error code to have the socket
error code.  If you have to resent this, could we improve the comment here?
I have to go to the manual page of getsockopt() that don't likes
SO_ERROR to try to understand what this completely un-intuitive (at
least for me) two lines of code do.


> +
> +    /* connect error */
> +    if (rc < 0) {
> +        closesocket(s->fd);
> +        s->fd = rc;
> +    }

If there is any error (getsockopt or in the socket), we just close the
fd and update the error code.

Head hurts.

Thanks, Juan.

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

[Amos Kong]

On 14/09/12 02:58, 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.
>
> 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


Hi Orit,

We almost get to the destination :)

> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   migration-tcp.c |   29 +++++-----------
>   qemu-char.c     |    2 +-
>   qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   qemu_socket.h   |   13 ++++++--
>   4 files changed, 102 insertions(+), 37 deletions(-)
>

...

> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 212075d..d321c58 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
> @@ -217,11 +218,69 @@ listen:
>       ((rc) == -EINPROGRESS)
>   #endif
>
> +/* 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;

This member is not used.

> +} ConnectState;

# make    (gcc-4.4.6-4.el6.x86_64)
   CC    qemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was here
make: *** [qemu-sockets.o] Error 1


struct ConnectState {
     int fd;
     struct addrinfo *addr_list;
     struct addrinfo *current_addr;
     ConnectHandler *callback;
     void *opaque;
};


> +
>   static int inet_connect_addr(struct addrinfo *addr, bool block,
> -                             bool *in_progress)
> +                             bool *in_progress, ConnectState *connect_state);
> +
> +static void wait_for_connect(void *opaque)
> +{
> +    ConnectState *s = opaque;
> +    int val = 0, rc = 0;
> +    socklen_t valsize = sizeof(val);
> +    bool in_progress = false;
> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +
> +    do {
> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
> +    } while (rc == -1 && socket_error() == EINTR);

# man getsockopt
RETURN VALUE
        On success, zero is returned.  On error, -1 is returned, and 
errno is set appropriately.

.. original code:
..
..     do {
..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, 
&valsize);
..     } while (ret == -1 && (socket_error()) == EINTR);
..
..     if (ret < 0) {  // ret equals to -1, and socket_error() != EINTR

If ret < 0, just set the error state, but the callback function is not 
set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to checking 
current
socket by getsockopt().

But in this patch, we will abandon current socket, and connect next 
address on the list.
We should reserve that block, and move qemu_set_fd_handler2(...NULL..) 
below it.

..      migrate_fd_error(s);
..         return;
..     }
..
..     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);

> +    /* update rc to contain error details */
> +    if (!rc && val) {
> +        rc = -val;

another issue: val >= 0 all the time?

> +    }
> +
> +    /* connect error */
> +    if (rc < 0) {
> +        closesocket(s->fd);
> +        s->fd = rc;
> +    }
> +
> +    /* try to connect to the next address on the list */
> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
> +        s->current_addr = s->current_addr->ai_next;
> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
> +        /* connect in progress */
> +        if (in_progress) {
> +            return;
> +        }
> +    }
> +
> +    freeaddrinfo(s->addr_list);
> +    if (s->callback) {
> +        s->callback(s->fd, s->opaque);
> +    }
> +    g_free(s);
> +    return;
> +}



-- 
			Amos.

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

[Orit Wasserman]

On 09/19/2012 11:31 AM, Amos Kong wrote:
> On 14/09/12 02:58, 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.
>>
>> 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
> 
> 
> Hi Orit,
> 
> We almost get to the destination :)
> 
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   migration-tcp.c |   29 +++++-----------
>>   qemu-char.c     |    2 +-
>>   qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>   qemu_socket.h   |   13 ++++++--
>>   4 files changed, 102 insertions(+), 37 deletions(-)
>>
> 
> ...
> 
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 212075d..d321c58 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
>> @@ -217,11 +218,69 @@ listen:
>>       ((rc) == -EINPROGRESS)
>>   #endif
>>
>> +/* 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;
> 
> This member is not used.
I will remove it.
> 
>> +} ConnectState;
> 
> # make    (gcc-4.4.6-4.el6.x86_64)
>   CC    qemu-sockets.o
> qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
> qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was here
> make: *** [qemu-sockets.o] Error 1
> 
> 
> struct ConnectState {
>     int fd;
>     struct addrinfo *addr_list;
>     struct addrinfo *current_addr;
>     ConnectHandler *callback;
>     void *opaque;
> };
> 
> 
>> +
>>   static int inet_connect_addr(struct addrinfo *addr, bool block,
>> -                             bool *in_progress)
>> +                             bool *in_progress, ConnectState *connect_state);
>> +
>> +static void wait_for_connect(void *opaque)
>> +{
>> +    ConnectState *s = opaque;
>> +    int val = 0, rc = 0;
>> +    socklen_t valsize = sizeof(val);
>> +    bool in_progress = false;
>> +
>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> +
>> +    do {
>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>> +    } while (rc == -1 && socket_error() == EINTR);
> 
> # man getsockopt
> RETURN VALUE
>        On success, zero is returned.  On error, -1 is returned, and errno is set appropriately.
> 
> .. original code:
> ..
> ..     do {
> ..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
> ..     } while (ret == -1 && (socket_error()) == EINTR);
> ..
> ..     if (ret < 0) {  // ret equals to -1, and socket_error() != EINTR
> 
> If ret < 0, just set the error state, but the callback function is not set to NULL.
> Then tcp_wait_for_connect() will be called again, and retry to checking current
> socket by getsockopt().
> 
> But in this patch, we will abandon current socket, and connect next address on the list.
> We should reserve that block, and move qemu_set_fd_handler2(...NULL..) below it.
I'm sorry I don't get you point, it makes no difference where is qemu_set_fd_handlers(...NULL...).
rc < 0 happens in two cases:
1. val != 0 - connect error
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new socket fd.

> 
> ..      migrate_fd_error(s);
> ..         return;
> ..     }
> ..
> ..     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> 
>> +    /* update rc to contain error details */
>> +    if (!rc && val) {
>> +        rc = -val;
> 
> another issue: val >= 0 all the time?
yes, I can had a check anyway.
> 
>> +    }
>> +
>> +    /* connect error */
>> +    if (rc < 0) {
>> +        closesocket(s->fd);
>> +        s->fd = rc;
>> +    }
>> +
>> +    /* try to connect to the next address on the list */
>> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> +        s->current_addr = s->current_addr->ai_next;
>> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
>> +        /* connect in progress */
>> +        if (in_progress) {
>> +            return;
>> +        }
>> +    }
>> +
>> +    freeaddrinfo(s->addr_list);
>> +    if (s->callback) {
>> +        s->callback(s->fd, s->opaque);
>> +    }
>> +    g_free(s);
>> +    return;
>> +}
> 
> 
> 

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

[Amos Kong]

----- Original Message -----
> On 09/19/2012 11:31 AM, Amos Kong wrote:
> > On 14/09/12 02:58, 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.
> >>
> >> 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
> > 
> > 
> > Hi Orit,
> > 
> > We almost get to the destination :)
> > 
> >> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> ---
> >>   migration-tcp.c |   29 +++++-----------
> >>   qemu-char.c     |    2 +-
> >>   qemu-sockets.c  |   95
> >>   +++++++++++++++++++++++++++++++++++++++++++++++-------
> >>   qemu_socket.h   |   13 ++++++--
> >>   4 files changed, 102 insertions(+), 37 deletions(-)
> >>
> > 
> > ...
> > 
> >> diff --git a/qemu-sockets.c b/qemu-sockets.c
> >> index 212075d..d321c58 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
> >> @@ -217,11 +218,69 @@ listen:
> >>       ((rc) == -EINPROGRESS)
> >>   #endif
> >>
> >> +/* 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;
> > 
> > This member is not used.
> I will remove it.
> > 
> >> +} ConnectState;
> > 
> > # make    (gcc-4.4.6-4.el6.x86_64)
> >   CC    qemu-sockets.o
> > qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
> > qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
> > here
> > make: *** [qemu-sockets.o] Error 1
> > 
> > 
> > struct ConnectState {
> >     int fd;
> >     struct addrinfo *addr_list;
> >     struct addrinfo *current_addr;
> >     ConnectHandler *callback;
> >     void *opaque;
> > };
> > 
> > 
> >> +
> >>   static int inet_connect_addr(struct addrinfo *addr, bool block,
> >> -                             bool *in_progress)
> >> +                             bool *in_progress, ConnectState
> >> *connect_state);
> >> +
> >> +static void wait_for_connect(void *opaque)
> >> +{
> >> +    ConnectState *s = opaque;
> >> +    int val = 0, rc = 0;
> >> +    socklen_t valsize = sizeof(val);
> >> +    bool in_progress = false;
> >> +
> >> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> >> +
> >> +    do {
> >> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
> >> &val, &valsize);
> >> +    } while (rc == -1 && socket_error() == EINTR);
> > 
> > # man getsockopt
> > RETURN VALUE
> >        On success, zero is returned.  On error, -1 is returned, and
> >        errno is set appropriately.
> > 
> > .. original code:
> > ..
> > ..     do {
> > ..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
> > &val, &valsize);
> > ..     } while (ret == -1 && (socket_error()) == EINTR);
> > ..
> > ..     if (ret < 0) {  // ret equals to -1, and socket_error() !=
> > EINTR
> > 
> > If ret < 0, just set the error state, but the callback function is
> > not set to NULL.
> > Then tcp_wait_for_connect() will be called again, and retry to
> > checking current
> > socket by getsockopt().
> > 
> > But in this patch, we will abandon current socket, and connect next
> > address on the list.
> > We should reserve that block, and move
> > qemu_set_fd_handler2(...NULL..) below it.
> I'm sorry I don't get you point, it makes no difference where is
> qemu_set_fd_handlers(...NULL...).
> rc < 0 happens in two cases:
> 1. val != 0 - connect error

It seems rc is 0 in this case.

> 2. getsockopt failed for reason different than EINTR .
> In both cases we can only move to the next addr in the list,
> inet_connect_addr will call qemu_set_fd_handlers with the new socket
> fd.

In the past, if rc < 0, we will return without setting fd handler to NULL,
tcp_wait_for_connect() will be re-called. the original socket (current addr)
will be re-checked by getsockopt()

But in your patch, it will retry next addr in the list.


> > 
> > ..      migrate_fd_error(s);
> > ..         return;
> > ..     }
> > ..
> > ..     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> > 
> >> +    /* update rc to contain error details */
> >> +    if (!rc && val) {
> >> +        rc = -val;
> > 
> > another issue: val >= 0 all the time?
> yes, I can had a check anyway.
> > 
> >> +    }
> >> +
> >> +    /* connect error */
> >> +    if (rc < 0) {
> >> +        closesocket(s->fd);
> >> +        s->fd = rc;
> >> +    }
> >> +
> >> +    /* try to connect to the next address on the list */
> >> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
> >> +        s->current_addr = s->current_addr->ai_next;
> >> +        s->fd = inet_connect_addr(s->current_addr, false,
> >> &in_progress, s);
> >> +        /* connect in progress */
> >> +        if (in_progress) {
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    freeaddrinfo(s->addr_list);
> >> +    if (s->callback) {
> >> +        s->callback(s->fd, s->opaque);
> >> +    }
> >> +    g_free(s);
> >> +    return;
> >> +}
> > 
> > 
> > 
> 
> 
> 

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

[Orit Wasserman]

On 09/20/2012 06:16 PM, Amos Kong wrote:
> ----- Original Message -----
>> On 09/19/2012 11:31 AM, Amos Kong wrote:
>>> On 14/09/12 02:58, 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.
>>>>
>>>> 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
>>>
>>>
>>> Hi Orit,
>>>
>>> We almost get to the destination :)
>>>
>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>   migration-tcp.c |   29 +++++-----------
>>>>   qemu-char.c     |    2 +-
>>>>   qemu-sockets.c  |   95
>>>>   +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>   qemu_socket.h   |   13 ++++++--
>>>>   4 files changed, 102 insertions(+), 37 deletions(-)
>>>>
>>>
>>> ...
>>>
>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>>> index 212075d..d321c58 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
>>>> @@ -217,11 +218,69 @@ listen:
>>>>       ((rc) == -EINPROGRESS)
>>>>   #endif
>>>>
>>>> +/* 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;
>>>
>>> This member is not used.
>> I will remove it.
>>>
>>>> +} ConnectState;
>>>
>>> # make    (gcc-4.4.6-4.el6.x86_64)
>>>   CC    qemu-sockets.o
>>> qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
>>> qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
>>> here
>>> make: *** [qemu-sockets.o] Error 1
>>>
>>>
>>> struct ConnectState {
>>>     int fd;
>>>     struct addrinfo *addr_list;
>>>     struct addrinfo *current_addr;
>>>     ConnectHandler *callback;
>>>     void *opaque;
>>> };
>>>
>>>
>>>> +
>>>>   static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> -                             bool *in_progress)
>>>> +                             bool *in_progress, ConnectState
>>>> *connect_state);
>>>> +
>>>> +static void wait_for_connect(void *opaque)
>>>> +{
>>>> +    ConnectState *s = opaque;
>>>> +    int val = 0, rc = 0;
>>>> +    socklen_t valsize = sizeof(val);
>>>> +    bool in_progress = false;
>>>> +
>>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> +
>>>> +    do {
>>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
>>>> &val, &valsize);
>>>> +    } while (rc == -1 && socket_error() == EINTR);
>>>
>>> # man getsockopt
>>> RETURN VALUE
>>>        On success, zero is returned.  On error, -1 is returned, and
>>>        errno is set appropriately.
>>>
>>> .. original code:
>>> ..
>>> ..     do {
>>> ..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
>>> &val, &valsize);
>>> ..     } while (ret == -1 && (socket_error()) == EINTR);
>>> ..
>>> ..     if (ret < 0) {  // ret equals to -1, and socket_error() !=
>>> EINTR
>>>
>>> If ret < 0, just set the error state, but the callback function is
>>> not set to NULL.
>>> Then tcp_wait_for_connect() will be called again, and retry to
>>> checking current
>>> socket by getsockopt().
>>>
>>> But in this patch, we will abandon current socket, and connect next
>>> address on the list.
>>> We should reserve that block, and move
>>> qemu_set_fd_handler2(...NULL..) below it.
>> I'm sorry I don't get you point, it makes no difference where is
>> qemu_set_fd_handlers(...NULL...).
>> rc < 0 happens in two cases:
>> 1. val != 0 - connect error
> 
> It seems rc is 0 in this case.
> 
>> 2. getsockopt failed for reason different than EINTR .
>> In both cases we can only move to the next addr in the list,
>> inet_connect_addr will call qemu_set_fd_handlers with the new socket
>> fd.
> 
> In the past, if rc < 0, we will return without setting fd handler to NULL,
> tcp_wait_for_connect() will be re-called. the original socket (current addr)
> will be re-checked by getsockopt()
> 
> But in your patch, it will retry next addr in the list.
> 
In the past we didn't have the address list so we could not continue to the next one,
now we can.

Orit 
> 
>>>
>>> ..      migrate_fd_error(s);
>>> ..         return;
>>> ..     }
>>> ..
>>> ..     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>
>>>> +    /* update rc to contain error details */
>>>> +    if (!rc && val) {
>>>> +        rc = -val;
>>>
>>> another issue: val >= 0 all the time?
>> yes, I can had a check anyway.
>>>
>>>> +    }
>>>> +
>>>> +    /* connect error */
>>>> +    if (rc < 0) {
>>>> +        closesocket(s->fd);
>>>> +        s->fd = rc;
>>>> +    }
>>>> +
>>>> +    /* try to connect to the next address on the list */
>>>> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>>>> +        s->current_addr = s->current_addr->ai_next;
>>>> +        s->fd = inet_connect_addr(s->current_addr, false,
>>>> &in_progress, s);
>>>> +        /* connect in progress */
>>>> +        if (in_progress) {
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    freeaddrinfo(s->addr_list);
>>>> +    if (s->callback) {
>>>> +        s->callback(s->fd, s->opaque);
>>>> +    }
>>>> +    g_free(s);
>>>> +    return;
>>>> +}
>>>
>>>
>>>
>>
>>
>>
> 

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

[Amos Kong]

On 23/09/12 14:34, Orit Wasserman wrote:
> On 09/20/2012 06:16 PM, Amos Kong wrote:
>> ----- Original Message -----
>>> On 09/19/2012 11:31 AM, Amos Kong wrote:
>>>> On 14/09/12 02:58, 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.
>>>>>
>>>>> 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
>>>>
>>>>
>>>> Hi Orit,
>>>>
>>>> We almost get to the destination :)
>>>>
>>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>    migration-tcp.c |   29 +++++-----------
>>>>>    qemu-char.c     |    2 +-
>>>>>    qemu-sockets.c  |   95
>>>>>    +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>>    qemu_socket.h   |   13 ++++++--
>>>>>    4 files changed, 102 insertions(+), 37 deletions(-)
>>>>>
>>>>
>>>> ...
>>>>
>>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>>>> index 212075d..d321c58 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
>>>>> @@ -217,11 +218,69 @@ listen:
>>>>>        ((rc) == -EINPROGRESS)
>>>>>    #endif
>>>>>
>>>>> +/* 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;
>>>>
>>>> This member is not used.
>>> I will remove it.
>>>>
>>>>> +} ConnectState;
>>>>
>>>> # make    (gcc-4.4.6-4.el6.x86_64)
>>>>    CC    qemu-sockets.o
>>>> qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
>>>> qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
>>>> here
>>>> make: *** [qemu-sockets.o] Error 1
>>>>
>>>>
>>>> struct ConnectState {
>>>>      int fd;
>>>>      struct addrinfo *addr_list;
>>>>      struct addrinfo *current_addr;
>>>>      ConnectHandler *callback;
>>>>      void *opaque;
>>>> };
>>>>
>>>>
>>>>> +
>>>>>    static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>>> -                             bool *in_progress)
>>>>> +                             bool *in_progress, ConnectState
>>>>> *connect_state);
>>>>> +
>>>>> +static void wait_for_connect(void *opaque)
>>>>> +{
>>>>> +    ConnectState *s = opaque;
>>>>> +    int val = 0, rc = 0;
>>>>> +    socklen_t valsize = sizeof(val);
>>>>> +    bool in_progress = false;
>>>>> +
>>>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>>> +
>>>>> +    do {
>>>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
>>>>> &val, &valsize);
>>>>> +    } while (rc == -1 && socket_error() == EINTR);
>>>>
>>>> # man getsockopt
>>>> RETURN VALUE
>>>>         On success, zero is returned.  On error, -1 is returned, and
>>>>         errno is set appropriately.
>>>>
>>>> .. original code:
>>>> ..
>>>> ..     do {
>>>> ..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
>>>> &val, &valsize);
>>>> ..     } while (ret == -1 && (socket_error()) == EINTR);
>>>> ..
>>>> ..     if (ret < 0) {  // ret equals to -1, and socket_error() !=
>>>> EINTR
>>>>
>>>> If ret < 0, just set the error state, but the callback function is
>>>> not set to NULL.
>>>> Then tcp_wait_for_connect() will be called again, and retry to
>>>> checking current
>>>> socket by getsockopt().
>>>>
>>>> But in this patch, we will abandon current socket, and connect next
>>>> address on the list.
>>>> We should reserve that block, and move
>>>> qemu_set_fd_handler2(...NULL..) below it.
>>> I'm sorry I don't get you point, it makes no difference where is
>>> qemu_set_fd_handlers(...NULL...).
>>> rc < 0 happens in two cases:
>>> 1. val != 0 - connect error
>>
>> It seems rc is 0 in this case.
>>
>>> 2. getsockopt failed for reason different than EINTR .
>>> In both cases we can only move to the next addr in the list,
>>> inet_connect_addr will call qemu_set_fd_handlers with the new socket
>>> fd.
>>
>> In the past, if rc < 0, we will return without setting fd handler to NULL,
>> tcp_wait_for_connect() will be re-called. the original socket (current addr)
>> will be re-checked by getsockopt()
>>
>> But in your patch, it will retry next addr in the list.
>>
> In the past we didn't have the address list so we could not continue to the next one,
> now we can.

Yes, we should try to connect next addr if current address is not available.
But rc < 0 doesn't mean current is not available.

"rc < 0" means fail to get socket option, we need to retry to _get_ 
socket option.


-- 
			Amos.

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

[Orit Wasserman]

On 09/24/2012 11:48 AM, Amos Kong wrote:
> On 23/09/12 14:34, Orit Wasserman wrote:
>> On 09/20/2012 06:16 PM, Amos Kong wrote:
>>> ----- Original Message -----
>>>> On 09/19/2012 11:31 AM, Amos Kong wrote:
>>>>> On 14/09/12 02:58, 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.
>>>>>>
>>>>>> 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
>>>>>
>>>>>
>>>>> Hi Orit,
>>>>>
>>>>> We almost get to the destination :)
>>>>>
>>>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>> ---
>>>>>>    migration-tcp.c |   29 +++++-----------
>>>>>>    qemu-char.c     |    2 +-
>>>>>>    qemu-sockets.c  |   95
>>>>>>    +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>>>    qemu_socket.h   |   13 ++++++--
>>>>>>    4 files changed, 102 insertions(+), 37 deletions(-)
>>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>>>>> index 212075d..d321c58 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
>>>>>> @@ -217,11 +218,69 @@ listen:
>>>>>>        ((rc) == -EINPROGRESS)
>>>>>>    #endif
>>>>>>
>>>>>> +/* 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;
>>>>>
>>>>> This member is not used.
>>>> I will remove it.
>>>>>
>>>>>> +} ConnectState;
>>>>>
>>>>> # make    (gcc-4.4.6-4.el6.x86_64)
>>>>>    CC    qemu-sockets.o
>>>>> qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
>>>>> qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
>>>>> here
>>>>> make: *** [qemu-sockets.o] Error 1
>>>>>
>>>>>
>>>>> struct ConnectState {
>>>>>      int fd;
>>>>>      struct addrinfo *addr_list;
>>>>>      struct addrinfo *current_addr;
>>>>>      ConnectHandler *callback;
>>>>>      void *opaque;
>>>>> };
>>>>>
>>>>>
>>>>>> +
>>>>>>    static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>>>> -                             bool *in_progress)
>>>>>> +                             bool *in_progress, ConnectState
>>>>>> *connect_state);
>>>>>> +
>>>>>> +static void wait_for_connect(void *opaque)
>>>>>> +{
>>>>>> +    ConnectState *s = opaque;
>>>>>> +    int val = 0, rc = 0;
>>>>>> +    socklen_t valsize = sizeof(val);
>>>>>> +    bool in_progress = false;
>>>>>> +
>>>>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>>>> +
>>>>>> +    do {
>>>>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
>>>>>> &val, &valsize);
>>>>>> +    } while (rc == -1 && socket_error() == EINTR);
>>>>>
>>>>> # man getsockopt
>>>>> RETURN VALUE
>>>>>         On success, zero is returned.  On error, -1 is returned, and
>>>>>         errno is set appropriately.
>>>>>
>>>>> .. original code:
>>>>> ..
>>>>> ..     do {
>>>>> ..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
>>>>> &val, &valsize);
>>>>> ..     } while (ret == -1 && (socket_error()) == EINTR);
>>>>> ..
>>>>> ..     if (ret < 0) {  // ret equals to -1, and socket_error() !=
>>>>> EINTR
>>>>>
>>>>> If ret < 0, just set the error state, but the callback function is
>>>>> not set to NULL.
>>>>> Then tcp_wait_for_connect() will be called again, and retry to
>>>>> checking current
>>>>> socket by getsockopt().
>>>>>
>>>>> But in this patch, we will abandon current socket, and connect next
>>>>> address on the list.
>>>>> We should reserve that block, and move
>>>>> qemu_set_fd_handler2(...NULL..) below it.
>>>> I'm sorry I don't get you point, it makes no difference where is
>>>> qemu_set_fd_handlers(...NULL...).
>>>> rc < 0 happens in two cases:
>>>> 1. val != 0 - connect error
>>>
>>> It seems rc is 0 in this case.
>>>
>>>> 2. getsockopt failed for reason different than EINTR .
>>>> In both cases we can only move to the next addr in the list,
>>>> inet_connect_addr will call qemu_set_fd_handlers with the new socket
>>>> fd.
>>>
>>> In the past, if rc < 0, we will return without setting fd handler to NULL,
>>> tcp_wait_for_connect() will be re-called. the original socket (current addr)
>>> will be re-checked by getsockopt()
>>>
>>> But in your patch, it will retry next addr in the list.
>>>
>> In the past we didn't have the address list so we could not continue to the next one,
>> now we can.
> 
> Yes, we should try to connect next addr if current address is not available.
> But rc < 0 doesn't mean current is not available.
> 
> "rc < 0" means fail to get socket option, we need to retry to _get_ socket option.
for rc < 0 and errno != EINTR there is no point of re-trying getsockopt again.

Orit
> 
> 

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

[Amos Kong]

----- Original Message -----
> On 09/24/2012 11:48 AM, Amos Kong wrote:
> > On 23/09/12 14:34, Orit Wasserman wrote:
> >> On 09/20/2012 06:16 PM, Amos Kong wrote:
> >>> ----- Original Message -----
> >>>> On 09/19/2012 11:31 AM, Amos Kong wrote:
> >>>>> On 14/09/12 02:58, 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.
> >>>>>>
> >>>>>> 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
> >>>>>
> >>>>>
> >>>>> Hi Orit,
> >>>>>
> >>>>> We almost get to the destination :)
> >>>>>
> >>>>>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>> ---
> >>>>>>    migration-tcp.c |   29 +++++-----------
> >>>>>>    qemu-char.c     |    2 +-
> >>>>>>    qemu-sockets.c  |   95
> >>>>>>    +++++++++++++++++++++++++++++++++++++++++++++++-------
> >>>>>>    qemu_socket.h   |   13 ++++++--
> >>>>>>    4 files changed, 102 insertions(+), 37 deletions(-)
> >>>>>>
> >>>>>
> >>>>> ...
> >>>>>
> >>>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
> >>>>>> index 212075d..d321c58 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
> >>>>>> @@ -217,11 +218,69 @@ listen:
> >>>>>>        ((rc) == -EINPROGRESS)
> >>>>>>    #endif
> >>>>>>
> >>>>>> +/* 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;
> >>>>>
> >>>>> This member is not used.
> >>>> I will remove it.
> >>>>>
> >>>>>> +} ConnectState;
> >>>>>
> >>>>> # make    (gcc-4.4.6-4.el6.x86_64)
> >>>>>    CC    qemu-sockets.o
> >>>>> qemu-sockets.c:229: error: redefinition of typedef
> >>>>> ‘ConnectState’
> >>>>> qemu_socket.h:46: note: previous declaration of ‘ConnectState’
> >>>>> was
> >>>>> here
> >>>>> make: *** [qemu-sockets.o] Error 1
> >>>>>
> >>>>>
> >>>>> struct ConnectState {
> >>>>>      int fd;
> >>>>>      struct addrinfo *addr_list;
> >>>>>      struct addrinfo *current_addr;
> >>>>>      ConnectHandler *callback;
> >>>>>      void *opaque;
> >>>>> };
> >>>>>
> >>>>>
> >>>>>> +
> >>>>>>    static int inet_connect_addr(struct addrinfo *addr, bool
> >>>>>>    block,
> >>>>>> -                             bool *in_progress)
> >>>>>> +                             bool *in_progress, ConnectState
> >>>>>> *connect_state);
> >>>>>> +
> >>>>>> +static void wait_for_connect(void *opaque)
> >>>>>> +{
> >>>>>> +    ConnectState *s = opaque;
> >>>>>> +    int val = 0, rc = 0;
> >>>>>> +    socklen_t valsize = sizeof(val);
> >>>>>> +    bool in_progress = false;
> >>>>>> +
> >>>>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> >>>>>> +
> >>>>>> +    do {
> >>>>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *)
> >>>>>> &val, &valsize);
> >>>>>> +    } while (rc == -1 && socket_error() == EINTR);
> >>>>>
> >>>>> # man getsockopt
> >>>>> RETURN VALUE
> >>>>>         On success, zero is returned.  On error, -1 is
> >>>>>         returned, and
> >>>>>         errno is set appropriately.
> >>>>>
> >>>>> .. original code:
> >>>>> ..
> >>>>> ..     do {
> >>>>> ..         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void
> >>>>> *)
> >>>>> &val, &valsize);
> >>>>> ..     } while (ret == -1 && (socket_error()) == EINTR);
> >>>>> ..
> >>>>> ..     if (ret < 0) {  // ret equals to -1, and socket_error()
> >>>>> !=
> >>>>> EINTR
> >>>>>
> >>>>> If ret < 0, just set the error state, but the callback function
> >>>>> is
> >>>>> not set to NULL.
> >>>>> Then tcp_wait_for_connect() will be called again, and retry to
> >>>>> checking current
> >>>>> socket by getsockopt().
> >>>>>
> >>>>> But in this patch, we will abandon current socket, and connect
> >>>>> next
> >>>>> address on the list.
> >>>>> We should reserve that block, and move
> >>>>> qemu_set_fd_handler2(...NULL..) below it.
> >>>> I'm sorry I don't get you point, it makes no difference where is
> >>>> qemu_set_fd_handlers(...NULL...).
> >>>> rc < 0 happens in two cases:
> >>>> 1. val != 0 - connect error
> >>>
> >>> It seems rc is 0 in this case.
> >>>
> >>>> 2. getsockopt failed for reason different than EINTR .
> >>>> In both cases we can only move to the next addr in the list,
> >>>> inet_connect_addr will call qemu_set_fd_handlers with the new
> >>>> socket
> >>>> fd.
> >>>
> >>> In the past, if rc < 0, we will return without setting fd handler
> >>> to NULL,
> >>> tcp_wait_for_connect() will be re-called. the original socket
> >>> (current addr)
> >>> will be re-checked by getsockopt()
> >>>
> >>> But in your patch, it will retry next addr in the list.
> >>>
> >> In the past we didn't have the address list so we could not
> >> continue to the next one,
> >> now we can.
> > 
> > Yes, we should try to connect next addr if current address is not
> > available.
> > But rc < 0 doesn't mean current is not available.
> > 
> > "rc < 0" means fail to get socket option, we need to retry to _get_
> > socket option.
> for rc < 0 and errno != EINTR there is no point of re-trying
> getsockopt again.

Ok, got it.

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

[Michael S. Tsirkin]

> @@ -526,16 +592,19 @@ 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)
>  {

Would be nice to have some documentation here.
Something like "on immediate success or immediate
failure, in_progress is set to false, in that case
callback is not invoked".

If you look at it this way, this API is hard to
use right. I'd like to suggest we get rid of
in_progress flag: return -1 on error and
return >=0 and invoke callback on immediate success.

-- 
MST

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

[Orit Wasserman]

On 09/20/2012 09:03 AM, Michael S. Tsirkin wrote:
>> @@ -526,16 +592,19 @@ 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)
>>  {
> 
> Would be nice to have some documentation here.
> Something like "on immediate success or immediate
> failure, in_progress is set to false, in that case
> callback is not invoked".
of course.
> 
> If you look at it this way, this API is hard to
> use right. I'd like to suggest we get rid of
> in_progress flag: return -1 on error and
> return >=0 and invoke callback on immediate success.
> 
we can even take it further and always invoke the callback 
(even for immediate error), this way the user of the function
can put all the error/success handling in the callback function.

Orit

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

[Michael S. Tsirkin]

On Thu, Sep 20, 2012 at 11:57:37AM +0300, Orit Wasserman wrote:
> On 09/20/2012 09:03 AM, Michael S. Tsirkin wrote:
> >> @@ -526,16 +592,19 @@ 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)
> >>  {
> > 
> > Would be nice to have some documentation here.
> > Something like "on immediate success or immediate
> > failure, in_progress is set to false, in that case
> > callback is not invoked".
> of course.
> > 
> > If you look at it this way, this API is hard to
> > use right. I'd like to suggest we get rid of
> > in_progress flag: return -1 on error and
> > return >=0 and invoke callback on immediate success.
> > 
> we can even take it further and always invoke the callback 
> (even for immediate error), this way the user of the function
> can put all the error/success handling in the callback function.
> 
> Orit

Yes but I don't think this is a good idea:
there's value in reporting immediate error
directly to the user when command was invoked,
this is more user-friendly.
Not so for immediate success since that is
only a step in the migration process.


-- 
MST

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

[Orit Wasserman]

On 09/20/2012 12:37 PM, Michael S. Tsirkin wrote:
> On Thu, Sep 20, 2012 at 11:57:37AM +0300, Orit Wasserman wrote:
>> On 09/20/2012 09:03 AM, Michael S. Tsirkin wrote:
>>>> @@ -526,16 +592,19 @@ 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)
>>>>  {
>>>
>>> Would be nice to have some documentation here.
>>> Something like "on immediate success or immediate
>>> failure, in_progress is set to false, in that case
>>> callback is not invoked".
>> of course.
>>>
>>> If you look at it this way, this API is hard to
>>> use right. I'd like to suggest we get rid of
>>> in_progress flag: return -1 on error and
>>> return >=0 and invoke callback on immediate success.
>>>
>> we can even take it further and always invoke the callback 
>> (even for immediate error), this way the user of the function
>> can put all the error/success handling in the callback function.
>>
>> Orit
> 
> Yes but I don't think this is a good idea:
> there's value in reporting immediate error
> directly to the user when command was invoked,
> this is more user-friendly.
> Not so for immediate success since that is
> only a step in the migration process.
> 
> 
OK

Re: [Qemu-devel] [PATCH v3 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-char.c     |    2 +-
>  qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  qemu_socket.h   |   13 ++++++--
>  4 files changed, 102 insertions(+), 37 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-char.c b/qemu-char.c
> index c442952..11cd5ef 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>          if (is_listen) {
>              fd = inet_listen_opts(opts, 0, NULL);
>          } else {
> -            fd = inet_connect_opts(opts, true, NULL, NULL);
> +            fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
>          }
>      }
>      if (fd < 0) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 212075d..d321c58 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
> @@ -217,11 +218,69 @@ listen:
>      ((rc) == -EINPROGRESS)
>  #endif
>  
> +/* 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 int inet_connect_addr(struct addrinfo *addr, bool block,
> -                             bool *in_progress)
> +                             bool *in_progress, ConnectState *connect_state);
> +
> +static void wait_for_connect(void *opaque)
> +{
> +    ConnectState *s = opaque;
> +    int val = 0, rc = 0;
> +    socklen_t valsize = sizeof(val);
> +    bool in_progress = false;
> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +
> +    do {
> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
> +    } while (rc == -1 && socket_error() == EINTR);
> +
> +    /* update rc to contain error details */
> +    if (!rc && val) {
> +        rc = -val;

Would rc = -1 suffice?  I'd find that clearer.

> +    }
> +
> +    /* connect error */
> +    if (rc < 0) {
> +        closesocket(s->fd);
> +        s->fd = rc;
> +    }
> +
> +    /* try to connect to the next address on the list */
> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
> +        s->current_addr = s->current_addr->ai_next;
> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
> +        /* connect in progress */
> +        if (in_progress) {
> +            return;
> +        }
> +    }
> +
> +    freeaddrinfo(s->addr_list);
> +    if (s->callback) {
> +        s->callback(s->fd, s->opaque);
> +    }
> +    g_free(s);
> +    return;
> +}
> +
> +static int inet_connect_addr(struct addrinfo *addr, bool block,
> +                             bool *in_progress, ConnectState *connect_state)

connect_state is needed only for non-blocking connect, isn't it?  Could
we drop block and simply use connect_state == NULL instead?

>  {
>      int sock, rc;
>  
> +    if (in_progress) {
> +        *in_progress = false;
> +    }
>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>      if (sock < 0) {
>          fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> @@ -241,6 +300,9 @@ 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, wait_for_connect,
> +                             connect_state);
>          if (in_progress) {
>              *in_progress = true;
>          }
> @@ -259,6 +321,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>      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;
> @@ -291,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>  }
>  
>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
> -                      Error **errp)
> +                      Error **errp, ConnectState *connect_state)

Same as for inet_connect_addr(): could we drop block and simply use
connect_state == NULL instead?

>  {
>      struct addrinfo *res, *e;
>      int sock = -1;
> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>          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);
> -        if (sock >= 0) {
> +        if (!block) {
> +            connect_state->addr_list = res;
> +            connect_state->current_addr = e;
> +        }
> +        sock = inet_connect_addr(e, block, in_progress, connect_state);
> +        if (in_progress && *in_progress) {
> +            return sock;
> +        } else if (sock >= 0) {
>              break;
>          }
>      }
>      if (sock < 0) {
>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>      }

Testing in_progress is fishy: whether the caller passes in_progress or
not shouldn't affect what this function does, only how it reports what
it did.

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

In case 3, we want to try the next address.  If there is none, fail.

In cases 1 and 2, we want to break the loop and return sock.

What about:

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

> +    g_free(connect_state);

connect_state isn't allocated in this function, are you sure you want to
free it here?  If yes, are you sure you want to free it only sometimes?

>      freeaddrinfo(res);
>      return sock;
>  }
> @@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
> -        sock = inet_connect_opts(opts, true, NULL, errp);
> +        sock = inet_connect_opts(opts, true, NULL, errp, NULL);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
> @@ -526,16 +592,19 @@ 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;
> +    ConnectState *connect_state;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
> -        sock = inet_connect_opts(opts, false, in_progress, errp);
> +        connect_state = g_malloc0(sizeof(*connect_state));
> +        connect_state->callback = callback;
> +        connect_state->opaque = opaque;
> +        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);

May leak connect_state, because inet_connect_opts() doesn't always free
it.

>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 80696aa..715dc9d 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -38,15 +38,22 @@ 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);
> +/* structure for nonblocking connect state */
> +typedef struct ConnectState ConnectState;
> +
>  /* 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 block, bool *in_progress,
> -                      Error **errp);
> +                      Error **errp, ConnectState *s);
>  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);

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

[Orit Wasserman]

On 09/20/2012 04:14 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-char.c     |    2 +-
>>  qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>  qemu_socket.h   |   13 ++++++--
>>  4 files changed, 102 insertions(+), 37 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-char.c b/qemu-char.c
>> index c442952..11cd5ef 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>>          if (is_listen) {
>>              fd = inet_listen_opts(opts, 0, NULL);
>>          } else {
>> -            fd = inet_connect_opts(opts, true, NULL, NULL);
>> +            fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
>>          }
>>      }
>>      if (fd < 0) {
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index 212075d..d321c58 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
>> @@ -217,11 +218,69 @@ listen:
>>      ((rc) == -EINPROGRESS)
>>  #endif
>>  
>> +/* 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 int inet_connect_addr(struct addrinfo *addr, bool block,
>> -                             bool *in_progress)
>> +                             bool *in_progress, ConnectState *connect_state);
>> +
>> +static void wait_for_connect(void *opaque)
>> +{
>> +    ConnectState *s = opaque;
>> +    int val = 0, rc = 0;
>> +    socklen_t valsize = sizeof(val);
>> +    bool in_progress = false;
>> +
>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> +
>> +    do {
>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>> +    } while (rc == -1 && socket_error() == EINTR);
>> +
>> +    /* update rc to contain error details */
>> +    if (!rc && val) {
>> +        rc = -val;
> 
> Would rc = -1 suffice?  I'd find that clearer.
I guess so, I want the errno for more detailed error message
but those will come in another patch set and I can handle it than.
I agree that using -1 will make the code much cleaner.
> 
>> +    }
>> +
>> +    /* connect error */
>> +    if (rc < 0) {
>> +        closesocket(s->fd);
>> +        s->fd = rc;
>> +    }
>> +
>> +    /* try to connect to the next address on the list */
>> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> +        s->current_addr = s->current_addr->ai_next;
>> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
>> +        /* connect in progress */
>> +        if (in_progress) {
>> +            return;
>> +        }
>> +    }
>> +
>> +    freeaddrinfo(s->addr_list);
>> +    if (s->callback) {
>> +        s->callback(s->fd, s->opaque);
>> +    }
>> +    g_free(s);
>> +    return;
>> +}
>> +
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, ConnectState *connect_state)
> 
> connect_state is needed only for non-blocking connect, isn't it?  Could
> we drop block and simply use connect_state == NULL instead?
it is a good idea !
> 
>>  {
>>      int sock, rc;
>>  
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>>      if (sock < 0) {
>>          fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>> @@ -241,6 +300,9 @@ 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, wait_for_connect,
>> +                             connect_state);
>>          if (in_progress) {
>>              *in_progress = true;
>>          }
>> @@ -259,6 +321,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>      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;
>> @@ -291,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>  }
>>  
>>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>> -                      Error **errp)
>> +                      Error **errp, ConnectState *connect_state)
> 
> Same as for inet_connect_addr(): could we drop block and simply use
> connect_state == NULL instead?
> 
>>  {
>>      struct addrinfo *res, *e;
>>      int sock = -1;
>> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>          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);
>> -        if (sock >= 0) {
>> +        if (!block) {
>> +            connect_state->addr_list = res;
>> +            connect_state->current_addr = e;
>> +        }
>> +        sock = inet_connect_addr(e, block, in_progress, connect_state);
>> +        if (in_progress && *in_progress) {
>> +            return sock;
>> +        } else if (sock >= 0) {
>>              break;
>>          }
>>      }
>>      if (sock < 0) {
>>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>      }
> 
> Testing in_progress is fishy: whether the caller passes in_progress or
> not shouldn't affect what this function does, only how it reports what
> it did.
> 
> 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).
> 
> In case 3, we want to try the next address.  If there is none, fail.
> 
> In cases 1 and 2, we want to break the loop and return sock.
> 
> What about:
> 
>     for (e = res; sock < 0 && e != NULL; e = e->ai_next) {
>         if (!block) {
>             connect_state->addr_list = res;
>             connect_state->current_addr = e;
>         }
>         sock = inet_connect_addr(e, block, in_progress, connect_state);
>     }
>     if (sock < 0) {
>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>     }
>     freeaddrinfo(res);
>     return sock;
> 
>> +    g_free(connect_state);
> 
> connect_state isn't allocated in this function, are you sure you want to
> free it here?  If yes, are you sure you want to free it only sometimes?
> 
inet_nonblocking connect allocate connect_state.
In case connect succeeded/failed immediately than we need to free it (i.e in_progress is true),
that the case here.
I will move it to inet_nonblocking_connect when I remove in_progress flag.

We still need to free in two places in the code ,the second is in tcp_wait_for_connect.

>>      freeaddrinfo(res);
>>      return sock;
>>  }
>> @@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
>>  
>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>      if (inet_parse(opts, str) == 0) {
>> -        sock = inet_connect_opts(opts, true, NULL, errp);
>> +        sock = inet_connect_opts(opts, true, NULL, errp, NULL);
>>      } else {
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>      }
>> @@ -526,16 +592,19 @@ 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;
>> +    ConnectState *connect_state;
>>  
>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>      if (inet_parse(opts, str) == 0) {
>> -        sock = inet_connect_opts(opts, false, in_progress, errp);
>> +        connect_state = g_malloc0(sizeof(*connect_state));
>> +        connect_state->callback = callback;
>> +        connect_state->opaque = opaque;
>> +        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);
> 
> May leak connect_state, because inet_connect_opts() doesn't always free
> it.
it is freed by tcp_wait_for_connect after it calls the user callback function

Orit

> 
>>      } else {
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>      }
>> diff --git a/qemu_socket.h b/qemu_socket.h
>> index 80696aa..715dc9d 100644
>> --- a/qemu_socket.h
>> +++ b/qemu_socket.h
>> @@ -38,15 +38,22 @@ 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);
>> +/* structure for nonblocking connect state */
>> +typedef struct ConnectState ConnectState;
>> +
>>  /* 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 block, bool *in_progress,
>> -                      Error **errp);
>> +                      Error **errp, ConnectState *s);
>>  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);

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

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> On 09/20/2012 04:14 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-char.c     |    2 +-
>>>  qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>  qemu_socket.h   |   13 ++++++--
>>>  4 files changed, 102 insertions(+), 37 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-char.c b/qemu-char.c
>>> index c442952..11cd5ef 100644
>>> --- a/qemu-char.c
>>> +++ b/qemu-char.c
>>> @@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>>>          if (is_listen) {
>>>              fd = inet_listen_opts(opts, 0, NULL);
>>>          } else {
>>> -            fd = inet_connect_opts(opts, true, NULL, NULL);
>>> +            fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
>>>          }
>>>      }
>>>      if (fd < 0) {
>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>> index 212075d..d321c58 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
>>> @@ -217,11 +218,69 @@ listen:
>>>      ((rc) == -EINPROGRESS)
>>>  #endif
>>>  
>>> +/* 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 int inet_connect_addr(struct addrinfo *addr, bool block,
>>> -                             bool *in_progress)
>>> +                             bool *in_progress, ConnectState *connect_state);
>>> +
>>> +static void wait_for_connect(void *opaque)
>>> +{
>>> +    ConnectState *s = opaque;
>>> +    int val = 0, rc = 0;
>>> +    socklen_t valsize = sizeof(val);
>>> +    bool in_progress = false;
>>> +
>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>> +
>>> +    do {
>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>>> +    } while (rc == -1 && socket_error() == EINTR);
>>> +
>>> +    /* update rc to contain error details */
>>> +    if (!rc && val) {
>>> +        rc = -val;
>> 
>> Would rc = -1 suffice?  I'd find that clearer.
> I guess so, I want the errno for more detailed error message
> but those will come in another patch set and I can handle it than.
> I agree that using -1 will make the code much cleaner.
>> 
>>> +    }
>>> +
>>> +    /* connect error */
>>> +    if (rc < 0) {
>>> +        closesocket(s->fd);
>>> +        s->fd = rc;
>>> +    }
>>> +
>>> +    /* try to connect to the next address on the list */
>>> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>>> +        s->current_addr = s->current_addr->ai_next;
>>> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
>>> +        /* connect in progress */
>>> +        if (in_progress) {
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    freeaddrinfo(s->addr_list);
>>> +    if (s->callback) {
>>> +        s->callback(s->fd, s->opaque);
>>> +    }
>>> +    g_free(s);
>>> +    return;
>>> +}
>>> +
>>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>>> +                             bool *in_progress, ConnectState *connect_state)
>> 
>> connect_state is needed only for non-blocking connect, isn't it?  Could
>> we drop block and simply use connect_state == NULL instead?
> it is a good idea !
>> 
>>>  {
>>>      int sock, rc;
>>>  
>>> +    if (in_progress) {
>>> +        *in_progress = false;
>>> +    }
>>>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>>>      if (sock < 0) {
>>>          fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>>> @@ -241,6 +300,9 @@ 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, wait_for_connect,
>>> +                             connect_state);
>>>          if (in_progress) {
>>>              *in_progress = true;
>>>          }
>>> @@ -259,6 +321,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>      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;
>>> @@ -291,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>  }
>>>  
>>>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>> -                      Error **errp)
>>> +                      Error **errp, ConnectState *connect_state)
>> 
>> Same as for inet_connect_addr(): could we drop block and simply use
>> connect_state == NULL instead?
>> 
>>>  {
>>>      struct addrinfo *res, *e;
>>>      int sock = -1;
>>> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>          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);
>>> -        if (sock >= 0) {
>>> +        if (!block) {
>>> +            connect_state->addr_list = res;
>>> +            connect_state->current_addr = e;
>>> +        }
>>> +        sock = inet_connect_addr(e, block, in_progress, connect_state);
>>> +        if (in_progress && *in_progress) {
>>> +            return sock;
>>> +        } else if (sock >= 0) {
>>>              break;
>>>          }
>>>      }
>>>      if (sock < 0) {
>>>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>>      }
>> 
>> Testing in_progress is fishy: whether the caller passes in_progress or
>> not shouldn't affect what this function does, only how it reports what
>> it did.
>> 
>> 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).
>> 
>> In case 3, we want to try the next address.  If there is none, fail.
>> 
>> In cases 1 and 2, we want to break the loop and return sock.
>> 
>> What about:
>> 
>>     for (e = res; sock < 0 && e != NULL; e = e->ai_next) {
>>         if (!block) {
>>             connect_state->addr_list = res;
>>             connect_state->current_addr = e;
>>         }
>>         sock = inet_connect_addr(e, block, in_progress, connect_state);
>>     }
>>     if (sock < 0) {
>>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>     }
>>     freeaddrinfo(res);
>>     return sock;
>> 
>>> +    g_free(connect_state);
>> 
>> connect_state isn't allocated in this function, are you sure you want to
>> free it here?  If yes, are you sure you want to free it only sometimes?
>> 
> inet_nonblocking connect allocate connect_state.
> In case connect succeeded/failed immediately than we need to free it
> (i.e in_progress is true),
> that the case here.
> I will move it to inet_nonblocking_connect when I remove in_progress flag.
>
> We still need to free in two places in the code ,the second is in
> tcp_wait_for_connect.

Do you mean wait_for_connect()?

Here's how I now think it's designed to work: inet_connect_opts() frees
connect_state.  Except when connect_state->callback is going to be
called, it's freed only after the callback returns.  In no case, the
caller or its callback function need to free it.

In short, inet_connect_opts()'s caller only allocates, the free is
automatic.  Needs mention in function comment.

Ways to avoid splitting alloc/free responsibility between
inet_connect_opts() and its callers:

1. Move free into caller code.  Probably a bad idea, because it needs to
be done by the caller when in_progress, else by the callback, which
feels error-prone.

2. Move allocation into inet_connect_opts(), replace connect_state
argument by whatever the caller needs put in connect_state.  I'm not
saying you should do this, just throwing out the idea; use it if you
like it.

>>>      freeaddrinfo(res);
>>>      return sock;
>>>  }
>>> @@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
>>>  
>>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>      if (inet_parse(opts, str) == 0) {
>>> -        sock = inet_connect_opts(opts, true, NULL, errp);
>>> +        sock = inet_connect_opts(opts, true, NULL, errp, NULL);
>>>      } else {
>>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>>      }
>>> @@ -526,16 +592,19 @@ 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;
>>> +    ConnectState *connect_state;
>>>  
>>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>      if (inet_parse(opts, str) == 0) {
>>> -        sock = inet_connect_opts(opts, false, in_progress, errp);
>>> +        connect_state = g_malloc0(sizeof(*connect_state));
>>> +        connect_state->callback = callback;
>>> +        connect_state->opaque = opaque;
>>> +        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);
>> 
>> May leak connect_state, because inet_connect_opts() doesn't always free
>> it.
> it is freed by tcp_wait_for_connect after it calls the user callback function

wait_for_connect(), actually.  You're right.

Now I actually understand how this works, let me have another try at
pointing out allocation errors:

    int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
                          Error **errp, ConnectState *connect_state)
    {
        struct addrinfo *res, *e;
        int sock = -1;

        res = inet_parse_connect_opts(opts, errp);
        if (!res) {

Leaks connect_state.

            return -1;
        }

        for (e = res; e != NULL; e = e->ai_next) {
            if (!block) {
                connect_state->addr_list = res;
                connect_state->current_addr = e;
            }
            sock = inet_connect_addr(e, block, in_progress, connect_state);
            if (in_progress && *in_progress) {

If inet_connect_addr() only started the connect, it arranged for
wait_for_connect() to run when the connect completes.
wait_for_connect() frees connect_state.

If in_progress, we detect this correctly, and refrain from freeing
connect_state.

If !in_progress, we fail to detect it, and free connect_state().  When
wait_for_connect() runs, we get a use-after-free, and a double-free.

Possible fixes:

1. Document caller must pass non-null in_progress for non-blocking
connect.  Recommend assert().

2. Stick "if (!in_progress) in_progress = &local_in_progress;" before
the loop.

3. Move the free into caller code, as described above (still not
thrilled about that idea).

I'd recommend 1. if you think passing null in_progress for non-blocking
connect makes no sense.  I doubt that's the case.

Else I'd recommend 2.

                return sock;
            } else if (sock >= 0) {
                break;
            }
        }
        if (sock < 0) {
            error_set(errp, QERR_SOCKET_CONNECT_FAILED);
        }
        g_free(connect_state);
        freeaddrinfo(res);
        return sock;
    }


[...]

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

[Orit Wasserman]

On 09/21/2012 11:03 AM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> On 09/20/2012 04:14 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-char.c     |    2 +-
>>>>  qemu-sockets.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  qemu_socket.h   |   13 ++++++--
>>>>  4 files changed, 102 insertions(+), 37 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-char.c b/qemu-char.c
>>>> index c442952..11cd5ef 100644
>>>> --- a/qemu-char.c
>>>> +++ b/qemu-char.c
>>>> @@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>>>>          if (is_listen) {
>>>>              fd = inet_listen_opts(opts, 0, NULL);
>>>>          } else {
>>>> -            fd = inet_connect_opts(opts, true, NULL, NULL);
>>>> +            fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
>>>>          }
>>>>      }
>>>>      if (fd < 0) {
>>>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>>>> index 212075d..d321c58 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
>>>> @@ -217,11 +218,69 @@ listen:
>>>>      ((rc) == -EINPROGRESS)
>>>>  #endif
>>>>  
>>>> +/* 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 int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> -                             bool *in_progress)
>>>> +                             bool *in_progress, ConnectState *connect_state);
>>>> +
>>>> +static void wait_for_connect(void *opaque)
>>>> +{
>>>> +    ConnectState *s = opaque;
>>>> +    int val = 0, rc = 0;
>>>> +    socklen_t valsize = sizeof(val);
>>>> +    bool in_progress = false;
>>>> +
>>>> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> +
>>>> +    do {
>>>> +        rc = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
>>>> +    } while (rc == -1 && socket_error() == EINTR);
>>>> +
>>>> +    /* update rc to contain error details */
>>>> +    if (!rc && val) {
>>>> +        rc = -val;
>>>
>>> Would rc = -1 suffice?  I'd find that clearer.
>> I guess so, I want the errno for more detailed error message
>> but those will come in another patch set and I can handle it than.
>> I agree that using -1 will make the code much cleaner.
>>>
>>>> +    }
>>>> +
>>>> +    /* connect error */
>>>> +    if (rc < 0) {
>>>> +        closesocket(s->fd);
>>>> +        s->fd = rc;
>>>> +    }
>>>> +
>>>> +    /* try to connect to the next address on the list */
>>>> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
>>>> +        s->current_addr = s->current_addr->ai_next;
>>>> +        s->fd = inet_connect_addr(s->current_addr, false, &in_progress, s);
>>>> +        /* connect in progress */
>>>> +        if (in_progress) {
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    freeaddrinfo(s->addr_list);
>>>> +    if (s->callback) {
>>>> +        s->callback(s->fd, s->opaque);
>>>> +    }
>>>> +    g_free(s);
>>>> +    return;
>>>> +}
>>>> +
>>>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>>>> +                             bool *in_progress, ConnectState *connect_state)
>>>
>>> connect_state is needed only for non-blocking connect, isn't it?  Could
>>> we drop block and simply use connect_state == NULL instead?
>> it is a good idea !
>>>
>>>>  {
>>>>      int sock, rc;
>>>>  
>>>> +    if (in_progress) {
>>>> +        *in_progress = false;
>>>> +    }
>>>>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>>>>      if (sock < 0) {
>>>>          fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>>>> @@ -241,6 +300,9 @@ 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, wait_for_connect,
>>>> +                             connect_state);
>>>>          if (in_progress) {
>>>>              *in_progress = true;
>>>>          }
>>>> @@ -259,6 +321,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>>      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;
>>>> @@ -291,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>>>  }
>>>>  
>>>>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>> -                      Error **errp)
>>>> +                      Error **errp, ConnectState *connect_state)
>>>
>>> Same as for inet_connect_addr(): could we drop block and simply use
>>> connect_state == NULL instead?
>>>
>>>>  {
>>>>      struct addrinfo *res, *e;
>>>>      int sock = -1;
>>>> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>>          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);
>>>> -        if (sock >= 0) {
>>>> +        if (!block) {
>>>> +            connect_state->addr_list = res;
>>>> +            connect_state->current_addr = e;
>>>> +        }
>>>> +        sock = inet_connect_addr(e, block, in_progress, connect_state);
>>>> +        if (in_progress && *in_progress) {
>>>> +            return sock;
>>>> +        } else if (sock >= 0) {
>>>>              break;
>>>>          }
>>>>      }
>>>>      if (sock < 0) {
>>>>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>>>      }
>>>
>>> Testing in_progress is fishy: whether the caller passes in_progress or
>>> not shouldn't affect what this function does, only how it reports what
>>> it did.
>>>
>>> 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).
>>>
>>> In case 3, we want to try the next address.  If there is none, fail.
>>>
>>> In cases 1 and 2, we want to break the loop and return sock.
>>>
>>> What about:
>>>
>>>     for (e = res; sock < 0 && e != NULL; e = e->ai_next) {
>>>         if (!block) {
>>>             connect_state->addr_list = res;
>>>             connect_state->current_addr = e;
>>>         }
>>>         sock = inet_connect_addr(e, block, in_progress, connect_state);
>>>     }
>>>     if (sock < 0) {
>>>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>>     }
>>>     freeaddrinfo(res);
>>>     return sock;
>>>
>>>> +    g_free(connect_state);
>>>
>>> connect_state isn't allocated in this function, are you sure you want to
>>> free it here?  If yes, are you sure you want to free it only sometimes?
>>>
>> inet_nonblocking connect allocate connect_state.
>> In case connect succeeded/failed immediately than we need to free it
>> (i.e in_progress is true),
>> that the case here.
>> I will move it to inet_nonblocking_connect when I remove in_progress flag.
>>
>> We still need to free in two places in the code ,the second is in
>> tcp_wait_for_connect.
> 
> Do you mean wait_for_connect()?
> 
> Here's how I now think it's designed to work: inet_connect_opts() frees
> connect_state.  Except when connect_state->callback is going to be
> called, it's freed only after the callback returns.  In no case, the
> caller or its callback function need to free it.
> 
> In short, inet_connect_opts()'s caller only allocates, the free is
> automatic.  Needs mention in function comment.
> 
> Ways to avoid splitting alloc/free responsibility between
> inet_connect_opts() and its callers:
> 
> 1. Move free into caller code.  Probably a bad idea, because it needs to
> be done by the caller when in_progress, else by the callback, which
> feels error-prone.
> 
> 2. Move allocation into inet_connect_opts(), replace connect_state
> argument by whatever the caller needs put in connect_state.  I'm not
> saying you should do this, just throwing out the idea; use it if you
> like it.
> 
>>>>      freeaddrinfo(res);
>>>>      return sock;
>>>>  }
>>>> @@ -518,7 +584,7 @@ int inet_connect(const char *str, Error **errp)
>>>>  
>>>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>>      if (inet_parse(opts, str) == 0) {
>>>> -        sock = inet_connect_opts(opts, true, NULL, errp);
>>>> +        sock = inet_connect_opts(opts, true, NULL, errp, NULL);
>>>>      } else {
>>>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>>>      }
>>>> @@ -526,16 +592,19 @@ 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;
>>>> +    ConnectState *connect_state;
>>>>  
>>>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>>>      if (inet_parse(opts, str) == 0) {
>>>> -        sock = inet_connect_opts(opts, false, in_progress, errp);
>>>> +        connect_state = g_malloc0(sizeof(*connect_state));
>>>> +        connect_state->callback = callback;
>>>> +        connect_state->opaque = opaque;
>>>> +        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);
>>>
>>> May leak connect_state, because inet_connect_opts() doesn't always free
>>> it.
>> it is freed by tcp_wait_for_connect after it calls the user callback function
> 
> wait_for_connect(), actually.  You're right.
> 
> Now I actually understand how this works, let me have another try at
> pointing out allocation errors:
> 
>     int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>                           Error **errp, ConnectState *connect_state)
>     {
>         struct addrinfo *res, *e;
>         int sock = -1;
> 
>         res = inet_parse_connect_opts(opts, errp);
>         if (!res) {
> 
> Leaks connect_state.
right missed it, I will add a free here
> 
>             return -1;
>         }
> 
>         for (e = res; e != NULL; e = e->ai_next) {
>             if (!block) {
>                 connect_state->addr_list = res;
>                 connect_state->current_addr = e;
>             }
>             sock = inet_connect_addr(e, block, in_progress, connect_state);
>             if (in_progress && *in_progress) {
> 
> If inet_connect_addr() only started the connect, it arranged for
> wait_for_connect() to run when the connect completes.
> wait_for_connect() frees connect_state.
> 
> If in_progress, we detect this correctly, and refrain from freeing
> connect_state.
> 
> If !in_progress, we fail to detect it, and free connect_state().  When
> wait_for_connect() runs, we get a use-after-free, and a double-free.
> 
> Possible fixes:
> 
> 1. Document caller must pass non-null in_progress for non-blocking
> connect.  Recommend assert().
> 
> 2. Stick "if (!in_progress) in_progress = &local_in_progress;" before
> the loop.
> 
> 3. Move the free into caller code, as described above (still not
> thrilled about that idea).
> 
> I'd recommend 1. if you think passing null in_progress for non-blocking
> connect makes no sense.  I doubt that's the case.
> 
> Else I'd recommend 2.
> 
>                 return sock;
>             } else if (sock >= 0) {
>                 break;
>             }
>         }
>         if (sock < 0) {
>             error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>         }
>         g_free(connect_state);
>         freeaddrinfo(res);
>         return sock;
>     }
> 
> 
> [...]
> 
Well I removed in_progress from inet_nonblocking_connect as mst requested, this actually helps with 
this issue as it becomes a local var in inet_connect_opts.

Orit

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

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> Changes from v2:
> 	- remove the use of getnameinfo
> 	- remove errp for inet_connect_addr
> 	- remove QemuOpt "block"
> 	- fix errors in wait_for_connect 
> 	- pass ConnectState as a parameter to allow concurrent connect ops
>
> 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.

Almost there for connect.

I'm afraid we have a similar problem with listen: we bind only on the
first address that works.  Shouldn't we bind all of them?

http://www.akkadia.org/drepper/userapi-ipv6.html

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

[Orit Wasserman]

On 09/20/2012 04:19 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> Changes from v2:
>> 	- remove the use of getnameinfo
>> 	- remove errp for inet_connect_addr
>> 	- remove QemuOpt "block"
>> 	- fix errors in wait_for_connect 
>> 	- pass ConnectState as a parameter to allow concurrent connect ops
>>
>> 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.
> 
> Almost there for connect.
> 
> I'm afraid we have a similar problem with listen: we bind only on the
> first address that works.  Shouldn't we bind all of them?
> 
> http://www.akkadia.org/drepper/userapi-ipv6.html
> 
yes listen should be fixed but lets do it in a separate patch set.

Orit

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

[Paolo Bonzini]

Il 20/09/2012 16:55, Orit Wasserman ha scritto:
> > Almost there for connect.
> > 
> > I'm afraid we have a similar problem with listen: we bind only on the
> > first address that works.  Shouldn't we bind all of them?
> > 
> > http://www.akkadia.org/drepper/userapi-ipv6.html
> 
> yes listen should be fixed but lets do it in a separate patch set.

It's also much more complicated with the current API because you need an
array of file descriptors rather than just one.

Paolo

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

[Michael S. Tsirkin]

On Thu, Sep 20, 2012 at 05:12:41PM +0200, Paolo Bonzini wrote:
> Il 20/09/2012 16:55, Orit Wasserman ha scritto:
> > > Almost there for connect.
> > > 
> > > I'm afraid we have a similar problem with listen: we bind only on the
> > > first address that works.  Shouldn't we bind all of them?
> > > 
> > > http://www.akkadia.org/drepper/userapi-ipv6.html
> > 
> > yes listen should be fixed but lets do it in a separate patch set.
> 
> It's also much more complicated with the current API because you need an
> array of file descriptors rather than just one.
> 
> Paolo

As a quick hack we can listen on ipv6 only
which is a superset.
I agree address independence would be better.

-- 
MST

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

[Michael S. Tsirkin]

On Thu, Sep 20, 2012 at 05:55:14PM +0300, Orit Wasserman wrote:
> On 09/20/2012 04:19 PM, Markus Armbruster wrote:
> > Orit Wasserman <owasserm@redhat.com> writes:
> > 
> >> Changes from v2:
> >> 	- remove the use of getnameinfo
> >> 	- remove errp for inet_connect_addr
> >> 	- remove QemuOpt "block"
> >> 	- fix errors in wait_for_connect 
> >> 	- pass ConnectState as a parameter to allow concurrent connect ops
> >>
> >> 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.
> > 
> > Almost there for connect.
> > 
> > I'm afraid we have a similar problem with listen: we bind only on the
> > first address that works.  Shouldn't we bind all of them?
> > 
> > http://www.akkadia.org/drepper/userapi-ipv6.html
> > 
> yes listen should be fixed but lets do it in a separate patch set.
> 
> Orit

One useful hack is to set  AI_ADDRCONFIG hint, this way
you don't get ipv6 addresses if your system does not
have any configured, this results in faster connections
on some boxes.

http://linux.die.net/man/3/getaddrinfo

This probably should be a separate patch too,
if for no other reason than that it makes failures
in connect much harder to trigger and test.

-- 
MST

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

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> On 09/20/2012 04:19 PM, Markus Armbruster wrote:
>> Orit Wasserman <owasserm@redhat.com> writes:
>> 
>>> Changes from v2:
>>> 	- remove the use of getnameinfo
>>> 	- remove errp for inet_connect_addr
>>> 	- remove QemuOpt "block"
>>> 	- fix errors in wait_for_connect 
>>> 	- pass ConnectState as a parameter to allow concurrent connect ops
>>>
>>> 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.
>> 
>> Almost there for connect.
>> 
>> I'm afraid we have a similar problem with listen: we bind only on the
>> first address that works.  Shouldn't we bind all of them?
>> 
>> http://www.akkadia.org/drepper/userapi-ipv6.html
>> 
> yes listen should be fixed but lets do it in a separate patch set.

Absolutely.