[Qemu-devel] [PATCH v4 0/4] nonblocking connect address handling cleanup

[Qemu-devel] [PATCH v4 0/4] nonblocking connect address handling cleanup

[Orit Wasserman]

Changes from v3:
	- add missing parenthesis QEMU_SOCKET_RC_INPROGRESS macro
	- remove "block" from dummy_opts
	- remove in_progress from external API (inet_connect_opts and 
	  inet_nonblocking_connect)
	- Allocate ConnectState inside inet_connect_opts, this make the 
	  structure internal to qemu-sockets.c
	- fix migrate_fd_cleanup to handle invalid fd.
	
Changes from v2:
	- remove the use of getnameinfo
	- remove errp for inet_connect_addr
	- remove QemuOpt "block"
	- fix errors in wait_for_connect 
	- pass ConnectState as a parameter to allow concurrent connect ops

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.
A simple way to reproduce the problem is migration:
for the destination use -incoming tcp:0:4444, run migrate -d tcp:localhost:4444
migration will fail on hosts that have both IPv4 and IPV6 address for localhost.

To fix this, refactor address resolution code and make inet_nonblocking_connect
retry connection with a different address.

Michael S. Tsirkin (1):
  Refactor inet_connect_opts function

Orit Wasserman (3):
  Separate inet_connect into inet_connect (blocking) and
    inet_nonblocking_connect
  Fix address handling in inet_nonblocking_connect
  Clear handler only for valid fd

 migration-tcp.c |   37 ++------
 migration.c     |    3 +-
 nbd.c           |    2 +-
 qemu-char.c     |    2 +-
 qemu-sockets.c  |  246 +++++++++++++++++++++++++++++++++++++++---------------
 qemu_socket.h   |   45 ++++++++++-
 ui/vnc.c        |    2 +-
 7 files changed, 233 insertions(+), 104 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH v4 1/4] Refactor inet_connect_opts function

[Orit Wasserman]

From: "Michael S. Tsirkin" <mst@redhat.com>

refactor address resolution code to fix nonblocking connect
remove getnameinfo call

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Amos Kong <akong@redhat.com>
Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 qemu-sockets.c |  148 ++++++++++++++++++++++++++++++++------------------------
 1 files changed, 85 insertions(+), 63 deletions(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 361d890..7ca0ac9 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -209,95 +209,117 @@ listen:
     return slisten;
 }
 
-int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
+#ifdef _WIN32
+#define QEMU_SOCKET_RC_INPROGRESS(rc) \
+    ((rc) == -EINPROGRESS || (rc) == -EWOULDBLOCK || (rc) == -WSAEALREADY)
+#else
+#define QEMU_SOCKET_RC_INPROGRESS(rc) \
+    ((rc) == -EINPROGRESS)
+#endif
+
+static int inet_connect_addr(struct addrinfo *addr, bool block,
+                             bool *in_progress)
 {
-    struct addrinfo ai,*res,*e;
+    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__,
+                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();
+        }
+    } while (rc == -EINTR);
+
+    if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+        if (in_progress) {
+            *in_progress = true;
+        }
+    } else if (rc < 0) {
+        closesocket(sock);
+        return -1;
+    }
+    return sock;
+}
+
+static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
+{
+    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));
+    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");
+        fprintf(stderr,
+                "inet_parse_connect_opts: 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))
+    if (qemu_opt_get_bool(opts, "ipv4", 0)) {
         ai.ai_family = PF_INET;
-    if (qemu_opt_get_bool(opts, "ipv6", 0))
+    }
+    if (qemu_opt_get_bool(opts, "ipv6", 0)) {
         ai.ai_family = PF_INET6;
+    }
 
     /* lookup */
-    if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
-        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
+    rc = getaddrinfo(addr, port, &ai, &res);
+    if (rc != 0) {
+        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;
+}
+
+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) {
-        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;
-        }
-        setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
-        if (!block) {
-            socket_set_nonblock(sock);
-        }
-        /* 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;
+        sock = inet_connect_addr(e, block, in_progress);
+        if (sock >= 0) {
+            break;
         }
-        freeaddrinfo(res);
-        return sock;
     }
-    error_set(errp, QERR_SOCKET_CONNECT_FAILED);
+    if (sock < 0) {
+        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
+    }
     freeaddrinfo(res);
-    return -1;
+    return sock;
 }
 
 int inet_dgram_opts(QemuOpts *opts)
-- 
1.7.7.6

[Qemu-devel] [PATCH v4 2/4] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Orit Wasserman]

No need to add non blocking parameters to the blocking inet_connect
add block parameter for inet_connect_opts instead of using QemuOpt "block".

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 migration-tcp.c |    2 +-
 nbd.c           |    2 +-
 qemu-char.c     |    2 +-
 qemu-sockets.c  |   31 +++++++++++++++++++++----------
 qemu_socket.h   |   30 ++++++++++++++++++++++++++++--
 ui/vnc.c        |    2 +-
 6 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index ac891c3..7f6ad98 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -88,7 +88,7 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = inet_connect(host_port, false, &in_progress, errp);
+    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
     if (error_is_set(errp)) {
         migrate_fd_error(s);
         return -1;
diff --git a/nbd.c b/nbd.c
index 0dd60c5..206f75c 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);
 }
 
 int tcp_socket_incoming(const char *address, uint16_t port)
diff --git a/qemu-char.c b/qemu-char.c
index 7f0f895..13b87b5 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, NULL, NULL);
+            fd = inet_connect_opts(opts, true, NULL, NULL);
         }
     }
     if (fd < 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 7ca0ac9..99a1500 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -54,9 +54,6 @@ static QemuOptsList dummy_opts = {
         },{
             .name = "ipv6",
             .type = QEMU_OPT_BOOL,
-        },{
-            .name = "block",
-            .type = QEMU_OPT_BOOL,
         },
         { /* end if list */ }
     },
@@ -294,11 +291,11 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
     return res;
 }
 
-int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
+int inet_connect_opts(QemuOpts *opts, bool block, 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) {
@@ -515,17 +512,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)
+int inet_connect(const char *str, 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");
-        }
-        sock = inet_connect_opts(opts, in_progress, errp);
+        sock = inet_connect_opts(opts, true, NULL, errp);
+    } else {
+        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+    }
+    qemu_opts_del(opts);
+    return sock;
+}
+
+
+int inet_nonblocking_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) {
+        sock = inet_connect_opts(opts, false, in_progress, errp);
     } else {
         error_set(errp, QERR_SOCKET_CREATE_FAILED);
     }
diff --git a/qemu_socket.h b/qemu_socket.h
index 30ae6af..3247fb7 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -42,8 +42,34 @@ int send_all(int fd, const void *buf, int len1);
 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);
+/**
+ * Connect function
+ * Returns -1 in case of error, fd in case of success
+ * @opts: QEMU options
+ * @block: set true for blocking socket
+ * @in_progress: set to true in case of on going connect
+ * @errp: set in case of an error
+ **/
+int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
+                      Error **errp);
+/**
+ * Blocking connect
+ * Returns -1 in case of error, fd in case of success
+ * @str: address string
+ * @errp: set in case of an error
+ **/
+int inet_connect(const char *str, Error **errp);
+
+/**
+ * Nonblocking connect
+ * Returns -1 in case of error, fd in case of success
+ * @str: address string
+ * @in_progress: set to true in case of on going connect
+ * @errp: set in case of an error
+ **/
+int inet_nonblocking_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..01b2daf 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);
         if (-1 == vs->lsock) {
             g_free(vs->display);
             vs->display = NULL;
-- 
1.7.7.6

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

[Orit Wasserman]

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 |   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;
+};
+
+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;
+
+    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;
 
@@ -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;
         }
@@ -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.
+ *
  * 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.
  * @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.
+ * @opaque: opaque for callback function
  * @errp: set in case of an error
  **/
-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
+ * 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);
-- 
1.7.7.6

[Qemu-devel] [PATCH v4 4/4] Clear handler only for valid fd

[Orit Wasserman]

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
---
 migration.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/migration.c b/migration.c
index 1edeec5..c20a2fe 100644
--- a/migration.c
+++ b/migration.c
@@ -240,8 +240,6 @@ static int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;
 
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-
     if (s->file) {
         DPRINTF("closing file\n");
         ret = qemu_fclose(s->file);
@@ -249,6 +247,7 @@ static int migrate_fd_cleanup(MigrationState *s)
     }
 
     if (s->fd != -1) {
+        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
         close(s->fd);
         s->fd = -1;
     }
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v4 0/4] nonblocking connect address handling cleanup

[Michael S. Tsirkin]

On Sun, Sep 23, 2012 at 04:49:03PM +0200, Orit Wasserman wrote:
> Changes from v3:
> 	- add missing parenthesis QEMU_SOCKET_RC_INPROGRESS macro
> 	- remove "block" from dummy_opts
> 	- remove in_progress from external API (inet_connect_opts and 
> 	  inet_nonblocking_connect)
> 	- Allocate ConnectState inside inet_connect_opts, this make the 
> 	  structure internal to qemu-sockets.c
> 	- fix migrate_fd_cleanup to handle invalid fd.
> 	
> Changes from v2:
> 	- remove the use of getnameinfo
> 	- remove errp for inet_connect_addr
> 	- remove QemuOpt "block"
> 	- fix errors in wait_for_connect 
> 	- pass ConnectState as a parameter to allow concurrent connect ops
> 
> 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.
> A simple way to reproduce the problem is migration:
> for the destination use -incoming tcp:0:4444, run migrate -d tcp:localhost:4444
> migration will fail on hosts that have both IPv4 and IPV6 address for localhost.
> 
> To fix this, refactor address resolution code and make inet_nonblocking_connect
> retry connection with a different address.
> 
> Michael S. Tsirkin (1):
>   Refactor inet_connect_opts function
> 
> Orit Wasserman (3):
>   Separate inet_connect into inet_connect (blocking) and
>     inet_nonblocking_connect
>   Fix address handling in inet_nonblocking_connect
>   Clear handler only for valid fd

For the series:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

This fixes regression in 1.2 so should go on a stable branch too I
guess.

>  migration-tcp.c |   37 ++------
>  migration.c     |    3 +-
>  nbd.c           |    2 +-
>  qemu-char.c     |    2 +-
>  qemu-sockets.c  |  246 +++++++++++++++++++++++++++++++++++++++---------------
>  qemu_socket.h   |   45 ++++++++++-
>  ui/vnc.c        |    2 +-
>  7 files changed, 233 insertions(+), 104 deletions(-)
> 
> -- 
> 1.7.7.6

Re: [Qemu-devel] [PATCH v4 4/4] Clear handler only for valid fd

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  migration.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 1edeec5..c20a2fe 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -240,8 +240,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>  {
>      int ret = 0;
>  
> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> -
>      if (s->file) {
>          DPRINTF("closing file\n");
>          ret = qemu_fclose(s->file);
> @@ -249,6 +247,7 @@ static int migrate_fd_cleanup(MigrationState *s)
>      }
>  
>      if (s->fd != -1) {
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>          close(s->fd);
>          s->fd = -1;
>      }

As far as I can see, qemu_set_fd_handler2() treats invalid file
descriptor -1 just like any other.  If it's in io_handlers, it gets
deleted, else it's a nop.  Thus, the old code works.  But I agree
passing invalid file descriptors is abusing the interface.

I'm not sufficiently familiar with the migration code to judge whether
moving the handler reset down is safe.

Re: [Qemu-devel] [PATCH v4 2/4] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> No need to add non blocking parameters to the blocking inet_connect
> add block parameter for inet_connect_opts instead of using QemuOpt "block".
>
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> ---
>  migration-tcp.c |    2 +-
>  nbd.c           |    2 +-
>  qemu-char.c     |    2 +-
>  qemu-sockets.c  |   31 +++++++++++++++++++++----------
>  qemu_socket.h   |   30 ++++++++++++++++++++++++++++--
>  ui/vnc.c        |    2 +-
>  6 files changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/migration-tcp.c b/migration-tcp.c
> index ac891c3..7f6ad98 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -88,7 +88,7 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_connect(host_port, false, &in_progress, errp);
> +    s->fd = inet_nonblocking_connect(host_port, &in_progress, errp);
>      if (error_is_set(errp)) {
>          migrate_fd_error(s);
>          return -1;
> diff --git a/nbd.c b/nbd.c
> index 0dd60c5..206f75c 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);
>  }
>  
>  int tcp_socket_incoming(const char *address, uint16_t port)
> diff --git a/qemu-char.c b/qemu-char.c
> index 7f0f895..13b87b5 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, NULL, NULL);
> +            fd = inet_connect_opts(opts, true, NULL, NULL);
>          }
>      }
>      if (fd < 0) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 7ca0ac9..99a1500 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -54,9 +54,6 @@ static QemuOptsList dummy_opts = {
>          },{
>              .name = "ipv6",
>              .type = QEMU_OPT_BOOL,
> -        },{
> -            .name = "block",
> -            .type = QEMU_OPT_BOOL,
>          },
>          { /* end if list */ }
>      },
> @@ -294,11 +291,11 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>      return res;
>  }
>  
> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp)
> +int inet_connect_opts(QemuOpts *opts, bool block, 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) {
> @@ -515,17 +512,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)
> +int inet_connect(const char *str, 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");
> -        }
> -        sock = inet_connect_opts(opts, in_progress, errp);
> +        sock = inet_connect_opts(opts, true, NULL, errp);
> +    } else {
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +    }
> +    qemu_opts_del(opts);
> +    return sock;
> +}
> +
> +
> +int inet_nonblocking_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) {
> +        sock = inet_connect_opts(opts, false, in_progress, errp);
>      } else {
>          error_set(errp, QERR_SOCKET_CREATE_FAILED);
>      }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 30ae6af..3247fb7 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -42,8 +42,34 @@ int send_all(int fd, const void *buf, int len1);
>  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);
> +/**
> + * Connect function
> + * Returns -1 in case of error, fd in case of success
> + * @opts: QEMU options
> + * @block: set true for blocking socket
> + * @in_progress: set to true in case of on going connect

ongoing

> + * @errp: set in case of an error
> + **/

We usually tack function comments to the function definition, not the
declaration.  Matter of taste.  I like it next to the definition,
because that improves the comment's chances to get updated along with
the function.

> +int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
> +                      Error **errp);
> +/**
> + * Blocking connect
> + * Returns -1 in case of error, fd in case of success
> + * @str: address string
> + * @errp: set in case of an error
> + **/
> +int inet_connect(const char *str, Error **errp);
> +
> +/**
> + * Nonblocking connect
> + * Returns -1 in case of error, fd in case of success
> + * @str: address string
> + * @in_progress: set to true in case of on going connect

ongoing

> + * @errp: set in case of an error
> + **/
> +int inet_nonblocking_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..01b2daf 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);
>          if (-1 == vs->lsock) {
>              g_free(vs->display);
>              vs->display = NULL;

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

[Markus Armbruster]

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 |   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);

Re: [Qemu-devel] [PATCH v4 0/4] nonblocking connect address handling cleanup

[Markus Armbruster]

Orit Wasserman <owasserm@redhat.com> writes:

> Changes from v3:
> 	- add missing parenthesis QEMU_SOCKET_RC_INPROGRESS macro
> 	- remove "block" from dummy_opts
> 	- remove in_progress from external API (inet_connect_opts and 
> 	  inet_nonblocking_connect)
> 	- Allocate ConnectState inside inet_connect_opts, this make the 
> 	  structure internal to qemu-sockets.c
> 	- fix migrate_fd_cleanup to handle invalid fd.
> 	
> Changes from v2:
> 	- remove the use of getnameinfo
> 	- remove errp for inet_connect_addr
> 	- remove QemuOpt "block"
> 	- fix errors in wait_for_connect 
> 	- pass ConnectState as a parameter to allow concurrent connect ops
>
> 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.
> A simple way to reproduce the problem is migration:
> for the destination use -incoming tcp:0:4444, run migrate -d tcp:localhost:4444
> migration will fail on hosts that have both IPv4 and IPV6 address for localhost.
>
> To fix this, refactor address resolution code and make inet_nonblocking_connect
> retry connection with a different address.

Good work, I'm down to nitpicking now :)

Re: [Qemu-devel] [PATCH v4 4/4] Clear handler only for valid fd

[Orit Wasserman]

On 09/24/2012 11:45 AM, Markus Armbruster wrote:
> Orit Wasserman <owasserm@redhat.com> writes:
> 
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> ---
>>  migration.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 1edeec5..c20a2fe 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -240,8 +240,6 @@ static int migrate_fd_cleanup(MigrationState *s)
>>  {
>>      int ret = 0;
>>  
>> -    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>> -
>>      if (s->file) {
>>          DPRINTF("closing file\n");
>>          ret = qemu_fclose(s->file);
>> @@ -249,6 +247,7 @@ static int migrate_fd_cleanup(MigrationState *s)
>>      }
>>  
>>      if (s->fd != -1) {
>> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>          close(s->fd);
>>          s->fd = -1;
>>      }
> 
> As far as I can see, qemu_set_fd_handler2() treats invalid file
> descriptor -1 just like any other.  If it's in io_handlers, it gets
> deleted, else it's a nop.  Thus, the old code works.
Not any more, there was an assert(fd >=0) added in commit bbdd2ad0814ea0911076419ea21b7957505cf1cc
recently.

  But I agree
> passing invalid file descriptors is abusing the interface.
> 
> I'm not sufficiently familiar with the migration code to judge whether
> moving the handler reset down is safe.
> 
I can keep the call in the same location if you think it is safer.

Orit

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

[Amos Kong]

On 23/09/12 22:49, Orit Wasserman wrote:
> 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 |   37 +++++--------------
>   qemu-char.c     |    2 +-
>   qemu-sockets.c  |  108 ++++++++++++++++++++++++++++++++++++++++++++++---------
>   qemu_socket.h   |   33 ++++++++++++-----
>   4 files changed, 125 insertions(+), 55 deletions(-)

...

> -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));

Repeatedly malloc insider for loop without releasing.

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

We should free() connect_state here, or moving allocate code before for 
loop (don't forget to check callback != NULL ;)

Amos.

>       }
>       if (sock < 0) {
>           error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>       }
> +    g_free(connect_state);
>       freeaddrinfo(res);
>       return sock;
>   }


-- 
			Amos.

Re: [Qemu-devel] [PATCH v4 4/4] Clear handler only for valid fd

[Paolo Bonzini]

Il 24/09/2012 11:58, Orit Wasserman ha scritto:
>>> >>      if (s->fd != -1) {
>>> >> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>> >>          close(s->fd);
>>> >>          s->fd = -1;
>>> >>      }
>> > 
>> > As far as I can see, qemu_set_fd_handler2() treats invalid file
>> > descriptor -1 just like any other.  If it's in io_handlers, it gets
>> > deleted, else it's a nop.  Thus, the old code works.
> Not any more, there was an assert(fd >=0) added in commit bbdd2ad0814ea0911076419ea21b7957505cf1cc
> recently.
> 
>> > I'm not sufficiently familiar with the migration code to judge whether
>> > moving the handler reset down is safe.
>> > 
> I can keep the call in the same location if you think it is safer.

I think it's okay as you did, given how the code looks like now.

I plan to make qemu_fclose take care of closing the file descriptor, but
then I'll adjust the code myself.  I just want this series to go in. :)))

Paolo

Re: [Qemu-devel] [PATCH v4 2/4] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Paolo Bonzini]

Il 24/09/2012 11:53, Markus Armbruster ha scritto:
> ongoing
> 
>> > + * @errp: set in case of an error
>> > + **/
> We usually tack function comments to the function definition, not the
> declaration.  Matter of taste.  I like it next to the definition,
> because that improves the comment's chances to get updated along with
> the function.

Not necessarily, see main-loop.h for example.

/me would like more of these...

Paolo

>> > +int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>> > +                      Error **errp);

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

[Orit Wasserman]

On 09/24/2012 12:10 PM, Michael S. Tsirkin wrote:
> On Sun, Sep 23, 2012 at 04:49:06PM +0200, Orit Wasserman wrote:
>> 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
> 
> typo
> 
>> + */
>> +typedef void ConnectHandler(int fd, void *opaque);
> 
> Can we rename this NonBlockingConnectHandler?
Of course.
> 
>> +
>>  /* New, ipv6-ready socket helper functions, see qemu-sockets.c */
> 
> BTW let's kill this comment.
> 

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

[Orit Wasserman]

On 09/24/2012 12:05 PM, Amos Kong wrote:
> On 23/09/12 22:49, Orit Wasserman wrote:
>> 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 |   37 +++++--------------
>>   qemu-char.c     |    2 +-
>>   qemu-sockets.c  |  108 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>   qemu_socket.h   |   33 ++++++++++++-----
>>   4 files changed, 125 insertions(+), 55 deletions(-)
> 
> ...
> 
>> -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));
> 
> Repeatedly malloc insider for loop without releasing.
> 
oops ...
I will move it outside of the loop.
>> +            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;
>>           }
> 
> We should free() connect_state here, or moving allocate code before for loop (don't forget to check callback != NULL ;)
> 
right.
> Amos.
> 
>>       }
>>       if (sock < 0) {
>>           error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>>       }
>> +    g_free(connect_state);
>>       freeaddrinfo(res);
>>       return sock;
>>   }
> 
> 

Re: [Qemu-devel] [PATCH v4 4/4] Clear handler only for valid fd

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 24/09/2012 11:58, Orit Wasserman ha scritto:
>>>> >>      if (s->fd != -1) {
>>>> >> +        qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
>>>> >>          close(s->fd);
>>>> >>          s->fd = -1;
>>>> >>      }
>>> > 
>>> > As far as I can see, qemu_set_fd_handler2() treats invalid file
>>> > descriptor -1 just like any other.  If it's in io_handlers, it gets
>>> > deleted, else it's a nop.  Thus, the old code works.
>> Not any more, there was an assert(fd >=0) added in commit
>> bbdd2ad0814ea0911076419ea21b7957505cf1cc
>> recently.
>> 
>>> > I'm not sufficiently familiar with the migration code to judge whether
>>> > moving the handler reset down is safe.
>>> > 
>> I can keep the call in the same location if you think it is safer.

I have no idea, and...

> I think it's okay as you did, given how the code looks like now.

... I'm prepared to take Paolo's word for it.

[...]

Re: [Qemu-devel] [PATCH v4 2/4] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Orit Wasserman]

On 09/24/2012 12:07 PM, Paolo Bonzini wrote:
> Il 24/09/2012 11:53, Markus Armbruster ha scritto:
>> ongoing
>>
>>>> + * @errp: set in case of an error
>>>> + **/
>> We usually tack function comments to the function definition, not the
>> declaration.  Matter of taste.  I like it next to the definition,
>> because that improves the comment's chances to get updated along with
>> the function.
> 
> Not necessarily, see main-loop.h for example.
> 
> /me would like more of these...
too late :) 
> 
> Paolo
> 
>>>> +int inet_connect_opts(QemuOpts *opts, bool block, bool *in_progress,
>>>> +                      Error **errp);
> 

Re: [Qemu-devel] [PATCH v4 2/4] Separate inet_connect into inet_connect (blocking) and inet_nonblocking_connect

[Paolo Bonzini]

Il 24/09/2012 15:37, Orit Wasserman ha scritto:
>>> >> We usually tack function comments to the function definition, not the
>>> >> declaration.  Matter of taste.  I like it next to the definition,
>>> >> because that improves the comment's chances to get updated along with
>>> >> the function.
>> > 
>> > Not necessarily, see main-loop.h for example.
>> > 
>> > /me would like more of these...
> too late :) 

No big deal!

Paolo