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

[Amos Kong]

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 <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>   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.

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

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

> +    }
> +
> +    /* 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;
> +}



--
                        Amos.