[v3,3/3] Fix address handling in inet_nonblocking_connect

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

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.

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

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

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

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.

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

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

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

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

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

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


[...]

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

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

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.

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

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