----- Original Message -----
> 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.
Ok, got it.
