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

[Orit Wasserman <owasserm@redhat.com>]

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