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

[Amos Kong <akong@redhat.com>]

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

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

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


-- 
			Amos.