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

[Markus Armbruster]

Orit Wasserman <address@hidden> writes:

> 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
>
> Signed-off-by: Orit Wasserman <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
>  migration-tcp.c |   37 +++++--------------
>  qemu-char.c     |    2 +-
>  qemu-sockets.c  |  108 
> ++++++++++++++++++++++++++++++++++++++++++++++---------
>  qemu_socket.h   |   33 ++++++++++++-----
>  4 files changed, 125 insertions(+), 55 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 7f6ad98..a15c2b8 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -53,54 +53,35 @@ static int tcp_close(MigrationState *s)
>      return r;
>  }
>  
> -static void tcp_wait_for_connect(void *opaque)
> +static void tcp_wait_for_connect(int fd, void *opaque)
>  {
>      MigrationState *s = opaque;
> -    int val, ret;
> -    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);
> -
> -    if (ret < 0) {
> +    if (fd < 0) {
> +        DPRINTF("migrate connect error\n");
> +        s->fd = -1;
>          migrate_fd_error(s);
> -        return;
> -    }
> -
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -
> -    if (val == 0)
> +    } else {
> +        DPRINTF("migrate connect success\n");
> +        s->fd = fd;
>          migrate_fd_connect(s);
> -    else {
> -        DPRINTF("error connecting %d\n", val);
> -        migrate_fd_error(s);
>      }
>  }
>  
>  int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>                                   Error **errp)
>  {
> -    bool in_progress;
> -
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
> +                                     errp);
>      if (error_is_set(errp)) {
>          migrate_fd_error(s);
>          return -1;
>      }
>  
> -    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;
>  }
>  
> diff --git a/qemu-char.c b/qemu-char.c
> index 13b87b5..b082bae 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2456,7 +2456,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts 
> *opts)
>          if (is_listen) {
>              fd = inet_listen_opts(opts, 0, NULL);
>          } else {
> -            fd = inet_connect_opts(opts, true, NULL, NULL);
> +            fd = inet_connect_opts(opts, NULL, NULL, NULL);
>          }
>      }
>      if (fd < 0) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 99a1500..9a4be15 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
> @@ -214,8 +215,64 @@ listen:
>      ((rc) == -EINPROGRESS)
>  #endif
>  
> -static int inet_connect_addr(struct addrinfo *addr, bool block,
> -                             bool *in_progress)
> +/* Struct to store connect state for non blocking connect */
> +typedef struct ConnectState ConnectState;
> +
> +struct ConnectState {
> +    int fd;
> +    struct addrinfo *addr_list;
> +    struct addrinfo *current_addr;
> +    ConnectHandler *callback;
> +    void *opaque;
> +};

Suggest not to separate the typedef and the struct declaration.

> +
> +static int inet_connect_addr(struct addrinfo *addr, 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;

Dead initialization.

> +
> +    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);
> +
> +    /* update rc to contain error */
> +    if (!rc && val) {
> +        rc = -1;
> +    }
> +
> +    /* 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, &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;
> +}
> +
> +static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> +                             ConnectState *connect_state)
>  {
>      int sock, rc;
>  
       if (in_progress) {
           *in_progress = false;
       }

All callers always pass non-null in_progress.  Suggest to drop the test.

> @@ -230,7 +287,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool 
> block,
>          return -1;
>      }
>      setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> -    if (!block) {
> +    if (connect_state != NULL) {
>          socket_set_nonblock(sock);
>      }
>      /* connect to peer */
> @@ -241,7 +298,10 @@ static int inet_connect_addr(struct addrinfo *addr, bool 
> block,
>          }
>      } while (rc == -EINTR);
>  
> -    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        connect_state->fd = sock;
> +        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
> +                             connect_state);
>          if (in_progress) {
>              *in_progress = true;
>          }

Another superfluous test of in_progress.

> @@ -260,6 +320,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts 
> *opts, Error **errp)
>      const char *port;
>  
>      memset(&ai, 0, sizeof(ai));
> +
>      ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
>      ai.ai_family = PF_UNSPEC;
>      ai.ai_socktype = SOCK_STREAM;
> @@ -291,30 +352,42 @@ static struct addrinfo 
> *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>      return res;
>  }
>  
> -int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
> -                      Error **errp)
> +int inet_connect_opts(QemuOpts *opts, Error **errp, ConnectHandler *callback,
> +                      void *opaque)
>  {
>      struct addrinfo *res, *e;
>      int sock = -1;
> +    bool in_progress = false;
> +    ConnectState *connect_state = NULL;
>  
>      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);
> -        if (sock >= 0) {
> +        if (callback != NULL) {
> +            connect_state = g_malloc0(sizeof(*connect_state));
> +            connect_state->addr_list = res;
> +            connect_state->current_addr = e;
> +            connect_state->callback = callback;
> +            connect_state->opaque = opaque;
> +        }
> +        sock = inet_connect_addr(e, &in_progress, connect_state);
> +        if (in_progress) {
> +            return sock;
> +        } else if (sock >= 0) {
> +            /* non blocking socket immediate success, call callback */
> +            if (callback != NULL) {
> +                callback(sock, opaque);
> +            }
>              break;
>          }
>      }
>      if (sock < 0) {
>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>      }
> +    g_free(connect_state);
>      freeaddrinfo(res);
>      return sock;
>  }
> @@ -519,7 +592,7 @@ int inet_connect(const char *str, Error **errp)
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
> -        sock = inet_connect_opts(opts, true, NULL, errp);
> +        sock = inet_connect_opts(opts, errp, NULL, NULL);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
> @@ -527,16 +600,17 @@ int inet_connect(const char *str, Error **errp)
>      return sock;
>  }
>  
> -
> -int inet_nonblocking_connect(const char *str, bool *in_progress,
> -                             Error **errp)
> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
> +                             void *opaque, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock = -1;
>  
> +    g_assert(callback != NULL);
> +
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      if (inet_parse(opts, str) == 0) {
> -        sock = inet_connect_opts(opts, false, in_progress, errp);
> +        sock = inet_connect_opts(opts, errp, callback, opaque);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 3247fb7..da93509 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -38,20 +38,31 @@ void socket_set_block(int fd);
>  void socket_set_nonblock(int fd);
>  int send_all(int fd, const void *buf, int len1);
>  
> +/* callback function for nonblocking connect
> + * vaild fd on success, negative error code on failure
> + */
> +typedef void ConnectHandler(int fd, void *opaque);
> +
>  /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
>  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);
>  /**
>   * Connect function
> + * Use callback for nonblocking connect.
> + *

With all the talk on non-blocking below, this sentence feels redundant.

>   * Returns -1 in case of error, fd in case of success
> + * For non-blocking connect calls the callback function
> + * with fd in case of success or -1 in case of error.

Suggests callback is *always* called, which isn't true: it's only called
when when inet_connect_opts() succeeds and returns the fd.

>   * @opts: QEMU options
> - * @block: set true for blocking socket
> - * @in_progress: set to true in case of on going connect
> + * @callback: callback function that is called when connect completes,
> + *            not NULL for nonblocking.

A few negations too many for my taste %-}

> + * @opaque: opaque for callback function
>   * @errp: set in case of an error
>   **/

Let me try:

/**
 * Create a socket and connect it to an address.
 *
 * @opts: QEMU options, recognized parameters strings "host" and "port",
 *        bools "ipv4" and "ipv6".
 * @errp: set on error
 * @callback: callback function for non-blocking connect
 * @opaque: opaque for callback function
 *
 * Returns: -1 on error, file descriptor on success.
 *
 * If @callback is non-null, the connect is non-blocking.  If this
 * function succeeds, callback will be called when the connection
 * completes, with the file descriptor on success, or -1 on error.
 */

Also closer to Gtk-doc style we seem to be adopting.

I dislike the use of errp here, but let's ignore that for now.

> -int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
> -                      Error **errp);
> +int inet_connect_opts(QemuOpts *opts, Error **errp, ConnectHandler *callback,
> +                      void *opaque);
> +
>  /**
>   * Blocking connect
>   * Returns -1 in case of error, fd in case of success
> @@ -61,14 +72,18 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool 
> *in_progress,
>  int inet_connect(const char *str, Error **errp);
>  
>  /**
> - * Nonblocking connect
> - * Returns -1 in case of error, fd in case of success
> + * Non-blocking connect

Incorrect spelling "nonblocking" is from PATCH 2/4.  Suggest to fix it
there.

> + * Returns -1 in case of immediate error.
> + * Calls the callback function with fd in case of success or -1 in case of
> + * error.
>   * @str: address string
> - * @in_progress: set to true in case of on going connect
> + * @callback: callback function that is called when connect completes,
> + *            cannot be NULL.
> + * @opaque: opaque for callback function
>   * @errp: set in case of an error
>   **/
> -int inet_nonblocking_connect(const char *str, bool *in_progress,
> -                             Error **errp);
> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
> +                             void *opaque, Error **errp);
>  
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);