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

[Orit Wasserman <owasserm@redhat.com>]

On 09/20/2012 04:14 PM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> 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 <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/migration-tcp.c b/migration-tcp.c
>> index 7f6ad98..cadea36 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -53,29 +53,18 @@ 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);
>>      }
>>  }
>>  
>> @@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>      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,
>> +                                     &in_progress, errp);
>>      if (error_is_set(errp)) {
>>          migrate_fd_error(s);
>>          return -1;
>> @@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>>  
>>      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);
>>      }
>> diff --git a/qemu-char.c b/qemu-char.c
>> index c442952..11cd5ef 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2459,7 +2459,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, true, NULL, NULL, NULL);
>>          }
>>      }
>>      if (fd < 0) {
>> 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;
>> +} ConnectState;
>> +
>>  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);
>> +
>> +    /* update rc to contain error details */
>> +    if (!rc && val) {
>> +        rc = -val;
> 
> Would rc = -1 suffice?  I'd find that clearer.
I guess so, I want the errno for more detailed error message
but those will come in another patch set and I can handle it than.
I agree that using -1 will make the code much cleaner.
> 
>> +    }
>> +
>> +    /* 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;
>> +}
>> +
>> +static int inet_connect_addr(struct addrinfo *addr, bool block,
>> +                             bool *in_progress, ConnectState *connect_state)
> 
> connect_state is needed only for non-blocking connect, isn't it?  Could
> we drop block and simply use connect_state == NULL instead?
it is a good idea !
> 
>>  {
>>      int sock, rc;
>>  
>> +    if (in_progress) {
>> +        *in_progress = false;
>> +    }
>>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>>      if (sock < 0) {
>>          fprintf(stderr, "%s: socket(%s): %s\n", __func__,
>> @@ -241,6 +300,9 @@ static int inet_connect_addr(struct addrinfo *addr, bool block,
>>      } while (rc == -EINTR);
>>  
>>      if (!block && 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;
>>          }
>> @@ -259,6 +321,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,7 +354,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>>  }
>>  
>>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>> -                      Error **errp)
>> +                      Error **errp, ConnectState *connect_state)
> 
> Same as for inet_connect_addr(): could we drop block and simply use
> connect_state == NULL instead?
> 
>>  {
>>      struct addrinfo *res, *e;
>>      int sock = -1;
>> @@ -301,19 +364,22 @@ int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>          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 (!block) {
>> +            connect_state->addr_list = res;
>> +            connect_state->current_addr = e;
>> +        }
>> +        sock = inet_connect_addr(e, block, in_progress, connect_state);
>> +        if (in_progress && *in_progress) {
>> +            return sock;
>> +        } else if (sock >= 0) {
>>              break;
>>          }
>>      }
>>      if (sock < 0) {
>>          error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>      }
> 
> Testing in_progress is fishy: whether the caller passes in_progress or
> not shouldn't affect what this function does, only how it reports what
> it did.
> 
> inet_connect_addr() either
> 
> 1. completes connect (returns valid fd, sets *in_progress to false), or
> 
> 2. starts connect (returns valid fd, sets *in_progress to true), or
> 
> 3. fails (returns -1 and sets *in_progress to false).
> 
> In case 3, we want to try the next address.  If there is none, fail.
> 
> In cases 1 and 2, we want to break the loop and return sock.
> 
> What about:
> 
>     for (e = res; sock < 0 && e != NULL; e = e->ai_next) {
>         if (!block) {
>             connect_state->addr_list = res;
>             connect_state->current_addr = e;
>         }
>         sock = inet_connect_addr(e, block, in_progress, connect_state);
>     }
>     if (sock < 0) {
>         error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>     }
>     freeaddrinfo(res);
>     return sock;
> 
>> +    g_free(connect_state);
> 
> connect_state isn't allocated in this function, are you sure you want to
> free it here?  If yes, are you sure you want to free it only sometimes?
> 
inet_nonblocking connect allocate connect_state.
In case connect succeeded/failed immediately than we need to free it (i.e in_progress is true),
that the case here.
I will move it to inet_nonblocking_connect when I remove in_progress flag.

We still need to free in two places in the code ,the second is in tcp_wait_for_connect.

>>      freeaddrinfo(res);
>>      return sock;
>>  }
>> @@ -518,7 +584,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, true, NULL, errp, NULL);
>>      } else {
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>      }
>> @@ -526,16 +592,19 @@ 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, bool *in_progress, Error **errp)
>>  {
>>      QemuOpts *opts;
>>      int sock = -1;
>> +    ConnectState *connect_state;
>>  
>>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>>      if (inet_parse(opts, str) == 0) {
>> -        sock = inet_connect_opts(opts, false, in_progress, errp);
>> +        connect_state = g_malloc0(sizeof(*connect_state));
>> +        connect_state->callback = callback;
>> +        connect_state->opaque = opaque;
>> +        sock = inet_connect_opts(opts, false, in_progress, errp, connect_state);
> 
> May leak connect_state, because inet_connect_opts() doesn't always free
> it.
it is freed by tcp_wait_for_connect after it calls the user callback function

Orit

> 
>>      } else {
>>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>>      }
>> diff --git a/qemu_socket.h b/qemu_socket.h
>> index 80696aa..715dc9d 100644
>> --- a/qemu_socket.h
>> +++ b/qemu_socket.h
>> @@ -38,15 +38,22 @@ 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);
>> +/* structure for nonblocking connect state */
>> +typedef struct ConnectState ConnectState;
>> +
>>  /* 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);
>>  int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>> -                      Error **errp);
>> +                      Error **errp, ConnectState *s);
>>  int inet_connect(const char *str, Error **errp);
>> -int inet_nonblocking_connect(const char *str, bool *in_progress,
>> -                             Error **errp);
>> +int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
>> +                             void *opaque, bool *in_progress, Error **errp);
>>  int inet_dgram_opts(QemuOpts *opts);
>>  const char *inet_strfamily(int family);