[Qemu-devel] IPv6 support for TCP migrations

[Qemu-devel] IPv6 support for TCP migrations

[nick]

Hi,

Currently migration-tcp.c uses the IPv4-only socket functions, making
migrations over IPv6 impossible. Following is a tentative patch that switches
it to use inet_connect() and inet_listen(). 

However, the patch loses the non-blocking connect() behaviour seen with the
previous code. I'm not sure how much of an issue this is - if connect() blocks
here, does it block execution of the VM? 

If so, I guess we need a non-blocking form of inet_connect(), or some way of
replicating the behaviour - it would potentially be needed for my NBD
reconnection patches too? I can see that a blocking connect() might not be an
issue while the KVM process is starting up, but could cause problems if we
try to reconnect while emulation is ongoing.

Thoughts?

/Nick

[Qemu-devel] [PATCH] migration-tcp: Allow incoming and outgoing migrations over IPv6

[nick]

From: Nick Thomas <nick@bytemark.co.uk>

Signed-off-by: Nick Thomas <nick@bytemark.co.uk>
---
 migration-tcp.c |   85 ++++++++-----------------------------------------------
 1 files changed, 12 insertions(+), 73 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index d3d80c9..8ae778e 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -48,46 +48,14 @@ static int tcp_close(FdMigrationState *s)
     return 0;
 }
 
-
-static void tcp_wait_for_connect(void *opaque)
-{
-    FdMigrationState *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 && (s->get_error(s)) == EINTR);
-
-    if (ret < 0) {
-        migrate_fd_error(s);
-        return;
-    }
-
-    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);
-    }
-}
-
 MigrationState *tcp_start_outgoing_migration(Monitor *mon,
                                              const char *host_port,
                                              int64_t bandwidth_limit,
                                              int detach,
-					     int blk,
-					     int inc)
+                                             int blk,
+                                             int inc)
 {
-    struct sockaddr_in addr;
     FdMigrationState *s;
-    int ret;
-
-    if (parse_host_port(&addr, host_port) < 0)
-        return NULL;
 
     s = qemu_mallocz(sizeof(*s));
 
@@ -104,32 +72,20 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
     s->state = MIG_STATE_ACTIVE;
     s->mon = NULL;
     s->bandwidth_limit = bandwidth_limit;
-    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s->fd == -1) {
-        qemu_free(s);
-        return NULL;
-    }
-
-    socket_set_nonblock(s->fd);
 
     if (!detach) {
         migrate_fd_monitor_suspend(s, mon);
     }
 
-    do {
-        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1)
-            ret = -(s->get_error(s));
-
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK)
-            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
-    } while (ret == -EINTR);
+    s->fd = inet_connect(host_port, SOCK_STREAM);
 
-    if (ret < 0 && ret != -EINPROGRESS && ret != -EWOULDBLOCK) {
+    if (s->fd == -1) {
         DPRINTF("connect failed\n");
         migrate_fd_error(s);
-    } else if (ret >= 0)
+    } else {
+        socket_set_nonblock(s->fd);
         migrate_fd_connect(s);
+    }
 
     return &s->mig_state;
 }
@@ -170,34 +126,17 @@ out2:
 
 int tcp_start_incoming_migration(const char *host_port)
 {
-    struct sockaddr_in addr;
-    int val;
     int s;
+    char *ostr = NULL;
 
-    if (parse_host_port(&addr, host_port) < 0) {
-        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
-        return -EINVAL;
-    }
-
-    s = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s == -1)
+    s = inet_listen(host_port, ostr, 0, SOCK_STREAM, 0);
+    if (s == -1) {
         return -socket_error();
-
-    val = 1;
-    setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
-
-    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1)
-        goto err;
-
-    if (listen(s, 1) == -1)
-        goto err;
+    }
 
     qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
                          (void *)(intptr_t)s);
 
     return 0;
-
-err:
-    close(s);
-    return -socket_error();
 }
+
-- 
1.7.0.4

Re: [Qemu-devel] IPv6 support for TCP migrations

[Daniel P. Berrange]

On Wed, May 04, 2011 at 09:39:02AM +0100, nick@bytemark.co.uk wrote:
> Hi,
> 
> Currently migration-tcp.c uses the IPv4-only socket functions, making
> migrations over IPv6 impossible. Following is a tentative patch that switches
> it to use inet_connect() and inet_listen(). 
> 
> However, the patch loses the non-blocking connect() behaviour seen with the
> previous code. I'm not sure how much of an issue this is - if connect() blocks
> here, does it block execution of the VM? 
> 
> If so, I guess we need a non-blocking form of inet_connect(), or some way of
> replicating the behaviour - it would potentially be needed for my NBD
> reconnection patches too? I can see that a blocking connect() might not be an
> issue while the KVM process is starting up, but could cause problems if we
> try to reconnect while emulation is ongoing.
> 
> Thoughts?

FWIW, Juan Quintela also posted a set of patches to add IPv6 support for
migration a few weeks back, but unfortunately they don't appear to have
been merged yet:

  http://www.mail-archive.com/qemu-devel@nongnu.org/msg58954.html

IIUC, Juan's patches don't have the blocking connect() problem you
mention.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] IPv6 support for TCP migrations

[Nicholas Thomas]

On Wed, 2011-05-04 at 11:13 +0100, Daniel P. Berrange wrote:
> On Wed, May 04, 2011 at 09:39:02AM +0100, nick@bytemark.co.uk wrote:
> > Hi,
> > 
> > Currently migration-tcp.c uses the IPv4-only socket functions, making
> > migrations over IPv6 impossible. Following is a tentative patch that switches
> > it to use inet_connect() and inet_listen(). 
> > 
> > However, the patch loses the non-blocking connect() behaviour seen with the
> > previous code. I'm not sure how much of an issue this is - if connect() blocks
> > here, does it block execution of the VM? 
> > 
> > If so, I guess we need a non-blocking form of inet_connect(), or some way of
> > replicating the behaviour - it would potentially be needed for my NBD
> > reconnection patches too? I can see that a blocking connect() might not be an
> > issue while the KVM process is starting up, but could cause problems if we
> > try to reconnect while emulation is ongoing.
> > 
> > Thoughts?
> 
> FWIW, Juan Quintela also posted a set of patches to add IPv6 support for
> migration a few weeks back, but unfortunately they don't appear to have
> been merged yet:
> 
>   http://www.mail-archive.com/qemu-devel@nongnu.org/msg58954.html
> 
> IIUC, Juan's patches don't have the blocking connect() problem you
> mention.

Hi,

Those patches look closer than mine, yes, although I spotted a problem
in tcp_start_common() of that patch series (Juan CC'd):

> +static int tcp_start_common(const char *str, int *fd, bool server)
> +{
> +    char hostname[512];
> +    const char *service;
> +    const char *name;
> +    struct addrinfo hints;
> +    struct addrinfo *result, *rp;
> +    int s;
> +    int sfd;
> +    int ret = -EINVAL;
> +
> +    *fd = -1;
> +    service = str;
> +    if (get_str_sep(hostname, sizeof(hostname), &service, ':') < 0) {
> +        return -EINVAL;
> +    }
> +    if (server && strlen(hostname) == 0) {
> +        name = NULL;
> +    } else {
> +        name = hostname;
> +    }

I think this will fail when specifying an IPv6 *address* and port, e.g:

migrate -d [::1]:5000

We already have code that does this correctly in qemu-sockets.c - 
inet_parse() - which is primarily what I was trying to get the
benefit of, in my patch. 

> +
> +    /* Obtain address(es) matching host/port */
> +
> +    memset(&hints, 0, sizeof(struct addrinfo));
> +    hints.ai_family = AF_UNSPEC;     /* Allow IPv4 or IPv6 */
> +    hints.ai_socktype = SOCK_STREAM; /* Datagram socket */
> +
> +    if (server) {
> +        hints.ai_flags = AI_PASSIVE;
> +    }
> +
> +    s = getaddrinfo(name, service, &hints, &result);
> +    if (s != 0) {
> +        fprintf(stderr, "qemu: getaddrinfo: %s\n", gai_strerror(s));
> +        return -EINVAL;
> +    }
> +
> +    /* getaddrinfo() returns a list of address structures.
> +       Try each address until we successfully bind/connect).
> +       If socket(2) (or bind/connect(2)) fails, we (close the socket
> +       and) try the next address. */
> +
> +    for (rp = result; rp != NULL; rp = rp->ai_next) {
> +        sfd = qemu_socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);
> +        if (sfd == -1) {
> +            ret = -errno;
> +            continue;
> +        }
> +        socket_set_nonblock(sfd);
> +        if (server) {
> +            ret = tcp_server_bind(sfd, rp);
> +        } else {
> +            ret = tcp_client_connect(sfd, rp);
> +        }
> +        if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> +            *fd = sfd;
> +            break;                  /* Success */
> +        }
> +        close(sfd);
> +    }
> +
> +    freeaddrinfo(result);
> +    return ret;
> +}