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

[Amos Kong <akong@redhat.com>]

----- 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.


> > 
> > ..      migrate_fd_error(s);
> > ..         return;
> > ..     }
> > ..
> > ..     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> > 
> >> +    /* update rc to contain error details */
> >> +    if (!rc && val) {
> >> +        rc = -val;
> > 
> > another issue: val >= 0 all the time?
> yes, I can had a check anyway.
> > 
> >> +    }
> >> +
> >> +    /* connect error */
> >> +    if (rc < 0) {
> >> +        closesocket(s->fd);
> >> +        s->fd = rc;
> >> +    }
> >> +
> >> +    /* try to connect to the next address on the list */
> >> +    while (s->current_addr->ai_next != NULL && s->fd < 0) {
> >> +        s->current_addr = s->current_addr->ai_next;
> >> +        s->fd = inet_connect_addr(s->current_addr, false,
> >> &in_progress, s);
> >> +        /* connect in progress */
> >> +        if (in_progress) {
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    freeaddrinfo(s->addr_list);
> >> +    if (s->callback) {
> >> +        s->callback(s->fd, s->opaque);
> >> +    }
> >> +    g_free(s);
> >> +    return;
> >> +}
> > 
> > 
> > 
> 
> 
>