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

[Orit Wasserman <owasserm@redhat.com>]

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;
+    }
+
+    /* 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)
 {
     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)
 {
     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);
     }
+    g_free(connect_state);
     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);
     } 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);
 
-- 
1.7.7.6