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

[Orit Wasserman <owasserm@redhat.com>]

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