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

[Amos Kong]

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;
+        }
+    }
+}
+
+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;
-- 
1.7.1