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

[Orit Wasserman]

On 09/24/2012 11:48 AM, Amos Kong wrote:
> On 23/09/12 14:34, Orit Wasserman wrote:
>> On 09/20/2012 06:16 PM, Amos Kong wrote:
>>> ----- Original Message -----
>>>> On 09/19/2012 11:31 AM, Amos Kong wrote:
>>>>> On 14/09/12 02:58, Orit Wasserman wrote:
>>>>>> getaddrinfo can give us a list of addresses, but we only try to
>>>>>> connect to the first one. If that fails we never proceed to
>>>>>> the next one.  This is common on desktop setups that often have
>>>>>> ipv6
>>>>>> configured but not actually working.
>>>>>>
>>>>>> To fix this make inet_connect_nonblocking retry connection with a
>>>>>> different
>>>>>> address.
>>>>>> callers on inet_nonblocking_connect register a callback function
>>>>>> that will
>>>>>> be called when connect opertion completes, in case of failure the
>>>>>> fd will have
>>>>>> a negative value
>>>>>
>>>>>
>>>>> Hi Orit,
>>>>>
>>>>> We almost get to the destination :)
>>>>>
>>>>>> Signed-off-by: Orit Wasserman <address@hidden>
>>>>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>>>>> ---
>>>>>>    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
> 
>