[Qemu-devel] [PATCH 0/2] Allow ipv6 for migration

[Qemu-devel] [PATCH 0/2] Allow ipv6 for migration

[Juan Quintela]

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

-- 
1.7.4

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

[Juan Quintela]

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 <quintela@redhat.com>
---
 migration-tcp.c |   53 ++++++-------------------
 net.c           |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu_socket.h   |    3 +
 3 files changed, 129 insertions(+), 40 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index b55f419..2fa496a 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -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;

diff --git a/net.c b/net.c
index ddcca97..8347e17 100644
--- a/net.c
+++ b/net.c
@@ -94,6 +94,119 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 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,
+               sizeof(val));
+
+    ret = bind(fd, rp->ai_addr, rp->ai_addrlen);
+
+    if (ret == -1) {
+        ret = -socket_error();
+    }
+    return ret;
+
+}
+
+static int tcp_client_connect(int fd, struct addrinfo *rp)
+{
+    int ret;
+
+    do {
+        ret = connect(fd, rp->ai_addr, rp->ai_addrlen);
+        if (ret == -1) {
+            ret = -socket_error();
+        }
+    } while (ret == -EINTR);
+
+    return ret;
+}
+
+
+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;
+    }
+
+    /* 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);
+    }
+
+    if (rp == NULL) {               /* No address succeeded */
+        fprintf(stderr, "Could not connect\n");
+    }
+
+    freeaddrinfo(result);
+    return ret;
+}
+
+int tcp_server_start(const char *str, int *fd)
+{
+    return tcp_start_common(str, fd, true);
+}
+
+int tcp_client_start(const char *str, int *fd)
+{
+    return tcp_start_common(str, fd, false);
+}
+
 int parse_host_port(struct sockaddr_in *saddr, const char *str)
 {
     char buf[512];
diff --git a/qemu_socket.h b/qemu_socket.h
index 180e4db..c399d96 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -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 */
-- 
1.7.4

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

[Peter Maydell]

On 16 March 2011 21:01, Juan Quintela <quintela@redhat.com> wrote:
> +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,
> +               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?)

> +static int tcp_start_common(const char *str, int *fd, bool server)
> +{
[...]
> +    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

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

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 March 2011 21:01, Juan Quintela <quintela@redhat.com> wrote:
>> +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,
>> +               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?)

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


>> +static int tcp_start_common(const char *str, int *fd, bool server)
>> +{
> [...]
>> +    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.

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.

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

[Juan Quintela]

Remove use of parse_host_port.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 net/socket.c |   60 ++++++++++-----------------------------------------------
 1 files changed, 11 insertions(+), 49 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 7337f4f..a28dfdd 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -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)
-- 
1.7.4

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

[Peter Maydell]

On 16 March 2011 21:01, Juan Quintela <quintela@redhat.com> wrote:
> -    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

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

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 March 2011 21:01, Juan Quintela <quintela@redhat.com> wrote:
>> -    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.