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

[Amos Kong <akong@redhat.com>]

----- 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 <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.
> for rc < 0 and errno != EINTR there is no point of re-trying
> getsockopt again.

Ok, got it.