Re: [Qemu-devel] [PATCH] migration: address handling cleanup

[Orit Wasserman]

On 09/11/2012 10:33 AM, Amos Kong wrote:
> From: Michael S. Tsirkin <address@hidden>
> 
> 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, refactor address resolution code and make tcp migration
> retry connection with a different address.
> 
> This also drops argument from inet_connect.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> Signed-off-by: Amos Kong <address@hidden>
> ---
>  migration-tcp.c |   70 +++++++++++++++++++------
>  migration.c     |    5 ++
>  migration.h     |    3 +
>  nbd.c           |    2 +-
>  qemu-sockets.c  |  153 
> ++++++++++++++++++++++++++++++++++---------------------
>  qemu_socket.h   |    6 ++-
>  ui/vnc.c        |    2 +-
>  7 files changed, 163 insertions(+), 78 deletions(-)
> 
> diff --git a/migration-tcp.c b/migration-tcp.c
> index ac891c3..1c1996b 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -30,6 +30,8 @@
>      do { } while (0)
>  #endif
>  
> +static void tcp_wait_for_connect(void *opaque);
> +
>  static int socket_errno(MigrationState *s)
>  {
>      return socket_error();
> @@ -53,13 +55,43 @@ static int tcp_close(MigrationState *s)
>      return r;
>  }
>  
> +/*
> + * Try to connect to next address on list.
> + */
> +static void
> +tcp_next_connect_migration(MigrationState *s, bool *in_progress, Error 
> **errp)
> +{
> +    *in_progress = false;
> +    migrate_fd_retry(s);
> +    while (s->next_addr) {
> +        s->fd = inet_connect_addr(s->next_addr, false, in_progress, errp);
> +        s->next_addr = s->next_addr->ai_next;
> +        if (*in_progress) {
> +            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
> +            return;
> +        }
> +    }
> +}
> +
Hi,
This code is not migration specific, it should be part of qemu-sockets.c.
I really don't see a good reason to add next_addr to migration state.
I think we need to add a callback for non-blocking connect that is called when 
the operation is done
(it will need the status of the operation), another option is two callback one 
for success and one for error.
I personally would create a separate function inet_connect_nonblocking but that 
is up to you.
Regards,
Orit
> +static int tcp_connect_done(MigrationState *s)
> +{
> +    freeaddrinfo(s->addr_list);
> +    if (s->fd < 0) {
> +        migrate_fd_error(s);
> +        return -1;
> +    }
> +
> +    migrate_fd_connect(s);
> +    return 0;
> +}
> +
>  static void tcp_wait_for_connect(void *opaque)
>  {
>      MigrationState *s = opaque;
>      int val, ret;
> +    bool in_progress;
>      socklen_t valsize = sizeof(val);
>  
> -    DPRINTF("connect completed\n");
>      do {
>          ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, 
> &valsize);
>      } while (ret == -1 && (socket_error()) == EINTR);
> @@ -69,39 +101,45 @@ static void tcp_wait_for_connect(void *opaque)
>          return;
>      }
>  
> +    DPRINTF("connect completed: %d\n", val);
> +
>      qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>  
> -    if (val == 0)
> -        migrate_fd_connect(s);
> -    else {
> -        DPRINTF("error connecting %d\n", val);
> -        migrate_fd_error(s);
> +    if (val != 0) {
> +        if (!s->next_addr) {
> +            DPRINTF("all addresses could not be connected\n");
> +            freeaddrinfo(s->addr_list);
> +            migrate_fd_error(s);
> +            return;
> +        }
> +
> +        tcp_next_connect_migration(s, &in_progress, NULL);
> +        if (in_progress) {
> +            return;
> +        }
>      }
> +    tcp_connect_done(s);
>  }
>  
>  int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>                                   Error **errp)
>  {
> -    bool in_progress;
> +    bool in_progress = false;
>  
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
> +    s->next_addr = s->addr_list = inet_parse_connect(host_port, errp);
>  
> -    s->fd = inet_connect(host_port, false, &in_progress, errp);
> -    if (error_is_set(errp)) {
> -        migrate_fd_error(s);
> -        return -1;
> -    }
> -
> +    s->fd = inet_connect_addr(s->next_addr, false, &in_progress, errp);
> +    s->next_addr = s->next_addr->ai_next;
>      if (in_progress) {
>          DPRINTF("connect in progress\n");
>          qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
> -    } else {
> -        migrate_fd_connect(s);
> +        return 0;
>      }
>  
> -    return 0;
> +    return tcp_connect_done(s);
>  }
>  
>  static void tcp_accept_incoming_migration(void *opaque)
> diff --git a/migration.c b/migration.c
> index 1edeec5..04c3057 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -256,6 +256,11 @@ static int migrate_fd_cleanup(MigrationState *s)
>      return ret;
>  }
>  
> +int migrate_fd_retry(MigrationState *s)
> +{
> +    return migrate_fd_cleanup(s);
> +}
> +
>  void migrate_fd_error(MigrationState *s)
>  {
>      DPRINTF("setting error state\n");
> diff --git a/migration.h b/migration.h
> index a9852fc..23eb15f 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -33,6 +33,8 @@ struct MigrationState
>      int64_t bandwidth_limit;
>      QEMUFile *file;
>      int fd;
> +    struct addrinfo *addr_list;
> +    struct addrinfo *next_addr;
>      int state;
>      int (*get_error)(MigrationState *s);
>      int (*close)(MigrationState *s);
> @@ -72,6 +74,7 @@ int fd_start_incoming_migration(const char *path);
>  int fd_start_outgoing_migration(MigrationState *s, const char *fdname);
>  
>  void migrate_fd_error(MigrationState *s);
> +int migrate_fd_retry(MigrationState *s);
>  
>  void migrate_fd_connect(MigrationState *s);
>  
> diff --git a/nbd.c b/nbd.c
> index 0dd60c5..417412a 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -162,7 +162,7 @@ int tcp_socket_outgoing(const char *address, uint16_t 
> port)
>  
>  int tcp_socket_outgoing_spec(const char *address_and_port)
>  {
> -    return inet_connect(address_and_port, true, NULL, NULL);
> +    return inet_connect(address_and_port, NULL, NULL);
>  }
>  
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 361d890..26d754c 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -209,32 +209,24 @@ listen:
>      return slisten;
>  }
>  
> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>  {
> -    struct addrinfo ai,*res,*e;
> +    struct addrinfo ai, *res;
> +    int rc;
>      const char *addr;
>      const char *port;
> -    char uaddr[INET6_ADDRSTRLEN+1];
> -    char uport[33];
> -    int sock,rc;
> -    bool block;
>  
>      memset(&ai,0, sizeof(ai));
>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>      ai.ai_family = PF_UNSPEC;
>      ai.ai_socktype = SOCK_STREAM;
>  
> -    if (in_progress) {
> -        *in_progress = false;
> -    }
> -
>      addr = qemu_opt_get(opts, "host");
>      port = qemu_opt_get(opts, "port");
> -    block = qemu_opt_get_bool(opts, "block", 0);
>      if (addr == NULL || port == NULL) {
>          fprintf(stderr, "inet_connect: host and/or port not specified\n");
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> -        return -1;
> +        return NULL;
>      }
>  
>      if (qemu_opt_get_bool(opts, "ipv4", 0))
> @@ -247,57 +239,85 @@ int inet_connect_opts(QemuOpts *opts, bool 
> *in_progress, Error **errp)
>          fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
>                  gai_strerror(rc));
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> -     return -1;
> +        return NULL;
>      }
> +    return res;
> +}
>  
> -    for (e = res; e != NULL; e = e->ai_next) {
> -        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> -                            uaddr,INET6_ADDRSTRLEN,uport,32,
> -                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
> -            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
> -            continue;
> -        }
> -        sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
> -        if (sock < 0) {
> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
> -            inet_strfamily(e->ai_family), strerror(errno));
> -            continue;
> +#ifdef _WIN32
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY)
> +#else
> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \
> +    ((rc) == -EINPROGRESS)
> +#endif
> +
> +int inet_connect_addr(struct addrinfo *addr, bool block, bool *in_progress,
> +                      Error **errp)
> +{
> +    char uaddr[INET6_ADDRSTRLEN + 1];
> +    char uport[33];
> +    int sock, rc;
> +
> +    if (getnameinfo((struct sockaddr *)addr->ai_addr, addr->ai_addrlen,
> +                    uaddr, INET6_ADDRSTRLEN, uport, 32,
> +                    NI_NUMERICHOST | NI_NUMERICSERV)) {
> +        fprintf(stderr, "%s: getnameinfo: oops\n", __func__);
> +        return -1;
> +    }
> +    sock = qemu_socket(addr->ai_family, addr->ai_socktype, 
> addr->ai_protocol);
> +    if (sock < 0) {
> +        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> +                inet_strfamily(addr->ai_family), strerror(errno));
> +        return -1;
> +    }
> +    setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> +    if (!block) {
> +        socket_set_nonblock(sock);
> +    }
> +    /* connect to peer */
> +    do {
> +        rc = 0;
> +        if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
> +            rc = -socket_error();
>          }
> -        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> -        if (!block) {
> -            socket_set_nonblock(sock);
> +    } while (rc == -EINTR);
> +
> +    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        if (in_progress) {
> +            *in_progress = true;
>          }
> -        /* connect to peer */
> -        do {
> -            rc = 0;
> -            if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) {
> -                rc = -socket_error();
> -            }
> -        } while (rc == -EINTR);
> -
> -  #ifdef _WIN32
> -        if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK
> -                       || rc == -WSAEALREADY)) {
> -  #else
> -        if (!block && (rc == -EINPROGRESS)) {
> -  #endif
> -            if (in_progress) {
> -                *in_progress = true;
> -            }
> -        } else if (rc < 0) {
> -            if (NULL == e->ai_next)
> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", 
> __FUNCTION__,
> -                        inet_strfamily(e->ai_family),
> -                        e->ai_canonname, uaddr, uport, strerror(errno));
> -            closesocket(sock);
> -            continue;
> +    } else if (rc < 0) {
> +        closesocket(sock);
> +        return -1;
> +    }
> +    return sock;
> +}
> +
> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +{
> +    struct addrinfo *res, *e;
> +    int sock = -1;
> +    bool block = qemu_opt_get_bool(opts, "block", 0);
> +
> +    res = inet_parse_connect_opts(opts, errp);
> +    if (!res) {
> +        return -1;
> +    }
> +
> +    if (in_progress) {
> +        *in_progress = false;
> +    }
> +
> +    for (e = res; e != NULL; e = e->ai_next) {
> +        sock = inet_connect_addr(e, block, in_progress, errp);
> +        if (sock >= 0) {
> +            break;
>          }
> -        freeaddrinfo(res);
> -        return sock;
>      }
>      error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>      freeaddrinfo(res);
> -    return -1;
> +    return sock;
>  }
>  
>  int inet_dgram_opts(QemuOpts *opts)
> @@ -493,16 +513,31 @@ int inet_listen(const char *str, char *ostr, int olen,
>      return sock;
>  }
>  
> -int inet_connect(const char *str, bool block, bool *in_progress, Error 
> **errp)
> +struct addrinfo *inet_parse_connect(const char *str, Error **errp)
> +{
> +    QemuOpts *opts;
> +    struct addrinfo *res;
> +
> +    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +    if (inet_parse(opts, str) == 0) {
> +        qemu_opt_set(opts, "block", "on");
> +        res = inet_parse_connect_opts(opts, errp);
> +    } else {
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        res = NULL;
> +    }
> +    qemu_opts_del(opts);
> +    return res;
> +}
> +
> +int inet_connect(const char *str, bool *in_progress, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
> -        if (block) {
> -            qemu_opt_set(opts, "block", "on");
> -        }
> +        qemu_opt_set(opts, "block", "on");
>          sock = inet_connect_opts(opts, in_progress, errp);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 30ae6af..a69bfb8 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -43,7 +43,11 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, 
> Error **errp);
>  int inet_listen(const char *str, char *ostr, int olen,
>                  int socktype, int port_offset, Error **errp);
>  int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp);
> -int inet_connect(const char *str, bool block, bool *in_progress, Error 
> **errp);
> +struct addrinfo *inet_parse_connect(const char *str, Error **errp);
> +int inet_connect_addr(struct addrinfo *addr, bool block, bool *in_progress,
> +                      Error **errp);
> +
> +int inet_connect(const char *str, bool *in_progress, Error **errp);
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 385e345..5ab8ecc 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3061,7 +3061,7 @@ int vnc_display_open(DisplayState *ds, const char 
> *display)
>          if (strncmp(display, "unix:", 5) == 0)
>              vs->lsock = unix_connect(display+5);
>          else
> -            vs->lsock = inet_connect(display, true, NULL, NULL);
> +            vs->lsock = inet_connect(display, NULL, NULL);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;
>