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 <owass...@redhat.com>
>>>>>> Signed-off-by: Michael S. Tsirkin <m...@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
>
>
