Skip to contentSkip to conversation

Allow ipv6 for migration

PATCH Superseded Juan Quintela 2 participants 7 messages Mar 16, 2011

Subsystems (2)
Networking Sockets

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Juan Quintela

author
#0 lore raw

cover Allow ipv6 for migration

Hi

1st patch moves migration to use getaddrinfo() instead of parse_host_port(). This allows us to use ipv6 addresses. As an extra bonus, now we can use names from /etc/services.

Code for net_socket_listen_init() and net_socket_connect_init() was almost identical to migration one, so also changed that ones. The difference were small based on how error codes were handled. After discussing with Anthony, it appears that the right ones are the migration ones.

Tested (the migration code) with:
- "tcp:foo:4444" ipv4 name
- "tcp:foo6:4444" ipv6 name
- "tcp:0:4444"
- "tcp:foo6:iqobject" (this was an unused entry on my machine /etc/services)
iqobject 48619/tcp # iqobject
- "tcp::4444": let the kernel make a choice

Please review.

Later, Juan.

Juan Quintela (2):
Use getaddrinfo for migration
net/socket: allow ipv6 for net_socket_listen_init and
net_socket_connect_init

 migration-tcp.c |   53 ++++++-------------------
 net.c           |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/socket.c    |   60 +++++------------------------
 qemu_socket.h   |    3 +
 4 files changed, 140 insertions(+), 89 deletions(-)
Signature · 1 line
1.7.4
author
#1 lore raw

PATCH 1/2 Use getaddrinfo for migration

This allows us to use ipv4/ipv6 for migration addresses. Once there, it also uses /etc/services names (it came free).

Signed-off-by Juan Quintela <[email protected]>
---
 migration-tcp.c |   53 ++++++-------------------
 net.c           |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu_socket.h   |    3 +
 3 files changed, 129 insertions(+), 40 deletions(-)
migration-tcp.c +13 -40
index b55f419..2fa496a 100644
@@ -48,8 +48,6 @@ static int tcp_close(FdMigrationState *s)
     }
     return 0;
 }
-
-
 static void tcp_wait_for_connect(void *opaque)
 {
     FdMigrationState *s = opaque;

@@ -83,13 +81,10 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
 					     int blk,
 					     int inc)
 {
-    struct sockaddr_in addr;
     FdMigrationState *s;
+    int fd;
     int ret;
 
-    if (parse_host_port(&addr, host_port) < 0)
-        return NULL;
-
     s = qemu_mallocz(sizeof(*s));
 
     s->get_error = socket_errno;

@@ -105,33 +100,22 @@ 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);
-
-    if (ret < 0 && ret != -EINPROGRESS && ret != -EWOULDBLOCK) {
+    ret = tcp_client_start(host_port, &fd);
+    s->fd = fd;
+    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        DPRINTF("connect in progress");
+        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
+    } else if (ret < 0) {
         DPRINTF("connect failed\n");
         migrate_fd_error(s);
-    } else if (ret >= 0)
+    } else {
         migrate_fd_connect(s);
-
+    }
     return &s->mig_state;
 }
 

@@ -171,25 +155,14 @@ out2:
 
 int tcp_start_incoming_migration(const char *host_port)
 {
-    struct sockaddr_in addr;
-    int val;
+    int ret;
     int s;
 
-    if (parse_host_port(&addr, host_port) < 0) {
-        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
-        return -EINVAL;
+    ret = tcp_server_start(host_port, &s);
+    if (ret < 0) {
+        return ret;
     }
 
-    s = qemu_socket(PF_INET, 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;
net.c +113 -0

119 lines not on this page · raw message

qemu_socket.h +3 -0
index 180e4db..c399d96 100644
@@ -56,4 +56,7 @@ int unix_connect(const char *path);
 int parse_host_port(struct sockaddr_in *saddr, const char *str);
 int socket_init(void);
 
+int tcp_client_start(const char *str, int *fd);
+int tcp_server_start(const char *str, int *fd);
+
 #endif /* QEMU_SOCKET_H */
Signature · 1 line
1.7.4
#2 lore raw

Re: [Qemu-devel] [PATCH 1/2] Use getaddrinfo for migration

Quoted 7 lines · Juan Quintela
net.c +7 -0
+static int tcp_server_bind(int fd, struct addrinfo *rp)
+{
+    int val = 1;
+    int ret;
+
+    /* allow fast reuse */
+    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val,

net.c static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) · line 113

Quoted 10 lines
net.c +10 -0
+               sizeof(val));
+
+    ret = bind(fd, rp->ai_addr, rp->ai_addrlen);
+
+    if (ret == -1) {
+        ret = -socket_error();
+    }
+    return ret;
+
+}

I know this is just moved code, but does this do the right thing on Windows? Windows semantics for SO_REUSEADDR are completely different from the Unix ones:
http://blogs.msdn.com/b/wndp/archive/2005/08/03/anthony-jones.aspx
http://msdn.microsoft.com/en-us/library/ms740618%28VS.85%29.aspx

Roughly, Unix default behaviour is like Windows SO_EXCLUSIVEADDRUSE; Unix SO_REUSEADDR is like Windows default behaviour; and Windows SO_REUSEADDR behaviour is really weird and you don't want it.

(I think Cygwin has a workaround for this to give unix semantics, but Windows qemu builds as a mingw app, so we get the MS semantics, right?)

Quoted 2 lines
net.c +2 -0
+static int tcp_start_common(const char *str, int *fd, bool server)
+{

[...]

Quoted 5 lines

+    s = getaddrinfo(name, service, &hints, &result);
+    if (s != 0) {
+        fprintf(stderr, "qemu: getaddrinfo: %s\n", gai_strerror(s));
+        return -EINVAL;
+    }

It seems a bit odd to have a low level utility routine printing diagnostics to stderr rather than just returning the error details to its caller somehow. Ditto the other fprintf later.

-- PMM

author
#3 lore raw

[Qemu-devel] Re: [PATCH 1/2] Use getaddrinfo for migration

Quoted 25 lines · Peter Maydell
Quoted 7 lines · Juan Quintela
net.c +7 -0
+static int tcp_server_bind(int fd, struct addrinfo *rp)
+{
+    int val = 1;
+    int ret;
+
+    /* allow fast reuse */
+    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val,

net.c static int get_str_sep(char *buf, int buf_size, const char **pp, int sep) · line 113

Quoted 10 lines
net.c +10 -0
+               sizeof(val));
+
+    ret = bind(fd, rp->ai_addr, rp->ai_addrlen);
+
+    if (ret == -1) {
+        ret = -socket_error();
+    }
+    return ret;
+
+}

I know this is just moved code, but does this do the right thing on Windows? Windows semantics for SO_REUSEADDR are completely different from the Unix ones:
http://blogs.msdn.com/b/wndp/archive/2005/08/03/anthony-jones.aspx
http://msdn.microsoft.com/en-us/library/ms740618%28VS.85%29.aspx

Quoted 7 lines

Roughly, Unix default behaviour is like Windows SO_EXCLUSIVEADDRUSE; Unix SO_REUSEADDR is like Windows default behaviour; and Windows SO_REUSEADDR behaviour is really weird and you don't want it.

(I think Cygwin has a workaround for this to give unix semantics, but Windows qemu builds as a mingw app, so we get the MS semantics, right?)

No clue really, but again, this is the code that existed there, we can fix on top of it.

Quoted 9 lines
Quoted 2 lines
net.c +2 -0
+static int tcp_start_common(const char *str, int *fd, bool server)
+{

[...]

Quoted 5 lines

+    s = getaddrinfo(name, service, &hints, &result);
+    if (s != 0) {
+        fprintf(stderr, "qemu: getaddrinfo: %s\n", gai_strerror(s));
+        return -EINVAL;
+    }

Quoted 3 lines

It seems a bit odd to have a low level utility routine printing diagnostics to stderr rather than just returning the error details to its caller somehow. Ditto the other fprintf later.

Previous code did it. I am not clear what to do here. The way to get an error for getaddrinfo is to return one string with gai_strerror().

Suggestions appreciated.

Later, Jaun.

author
#4 lore raw

PATCH 2/2 net/socket: allow ipv6 for net_socket_listen_init and net_socket_connect_init

In reply to Juan Quintela

Remove use of parse_host_port.

Signed-off-by Juan Quintela <[email protected]>
---
 net/socket.c |   60 ++++++++++-----------------------------------------------
 1 files changed, 11 insertions(+), 49 deletions(-)
net/socket.c +11 -49
index 7337f4f..a28dfdd 100644
@@ -392,28 +392,13 @@ static int net_socket_listen_init(VLANState *vlan,
                                   const char *host_str)
 {
     NetSocketListenState *s;
-    int fd, val, ret;
-    struct sockaddr_in saddr;
-
-    if (parse_host_port(&saddr, host_str) < 0)
-        return -1;
+    int fd, ret;
 
     s = qemu_mallocz(sizeof(NetSocketListenState));
 
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
-        return -1;
-    }
-    socket_set_nonblock(fd);
-
-    /* allow fast reuse */
-    val = 1;
-    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
-
-    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
+    ret = tcp_server_start(host_str, &fd);
     if (ret < 0) {
-        perror("bind");
+        perror("tcp_server_start");
         return -1;
     }
     ret = listen(fd, 0);

@@ -435,40 +420,17 @@ static int net_socket_connect_init(VLANState *vlan,
                                    const char *host_str)
 {
     NetSocketState *s;
-    int fd, connected, ret, err;
+    int fd, connected, ret;
     struct sockaddr_in saddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
-        return -1;
-
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
+    ret = tcp_client_start(host_str, &fd);
+    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        connected = 0;
+    } else if (ret < 0) {
+        closesocket(fd);
         return -1;
-    }
-    socket_set_nonblock(fd);
-
-    connected = 0;
-    for(;;) {
-        ret = connect(fd, (struct sockaddr *)&saddr, sizeof(saddr));
-        if (ret < 0) {
-            err = socket_error();
-            if (err == EINTR || err == EWOULDBLOCK) {
-            } else if (err == EINPROGRESS) {
-                break;
-#ifdef _WIN32
-            } else if (err == WSAEALREADY || err == WSAEINVAL) {
-                break;
-#endif
-            } else {
-                perror("connect");
-                closesocket(fd);
-                return -1;
-            }
-        } else {
-            connected = 1;
-            break;
-        }
+    } else {
+        connected = 1;
     }
     s = net_socket_fd_init(vlan, model, name, fd, connected);
     if (!s)
Signature · 1 line
1.7.4
#5 lore raw

Re: [Qemu-devel] [PATCH 2/2] net/socket: allow ipv6 for net_socket_listen_init and net_socket_connect_init

Quoted 7 lines · Juan Quintela

-    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
+    ret = tcp_server_start(host_str, &fd);
    if (ret < 0) {
-        perror("bind");
+        perror("tcp_server_start");
        return -1;
    }

It looks like tcp_server_start() returns an error code rather than setting errno, so isn't perror() wrong here?

-- PMM

author
#6 lore raw

[Qemu-devel] Re: [PATCH 2/2] net/socket: allow ipv6 for net_socket_listen_init and net_socket_connect_init

Quoted 11 lines · Peter Maydell
Quoted 7 lines · Juan Quintela

-    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
+    ret = tcp_server_start(host_str, &fd);
    if (ret < 0) {
-        perror("bind");
+        perror("tcp_server_start");
        return -1;
    }

It looks like tcp_server_start() returns an error code rather than setting errno, so isn't perror() wrong here?

You are right O:-)

I am not clear about what to do here. Basically we have:
- migration code: uses DPRINTF()
- sockets: use perror() left and right.

Any good idea on what to do everywhere?

For this case, I will just put strerrno(), easy enough.

Thanks a lot, Juan.