[Qemu-devel] [PATCH 0/4] support to migrate with IPv6 address

[Qemu-devel] [PATCH 0/4] support to migrate with IPv6 address

[Amos Kong]

Those four patches make migration of IPv6 address work.
Use getaddrinfo() to socket addresses infomation.

---

Amos Kong (4):
      Use getaddrinfo for migration
      net/socket: allow ipv6 for net_socket_listen_init and socket_connect_init
      net: split hostname and service by last colon
      net: support to include ipv6 address by brackets


 migration-tcp.c |   60 ++++++++--------------------
 net.c           |  116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/socket.c    |   60 +++++-----------------------
 qemu_socket.h   |    3 +
 4 files changed, 145 insertions(+), 94 deletions(-)

-- 
Amos Kong

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

[Amos Kong]

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>
Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   60 ++++++++-----------------------
 net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu_socket.h   |    3 ++
 3 files changed, 127 insertions(+), 44 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 35a5781..644bb8f 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -81,43 +81,27 @@ static void tcp_wait_for_connect(void *opaque)
 
 int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
 {
-    struct sockaddr_in addr;
     int ret;
-
-    ret = parse_host_port(&addr, host_port);
-    if (ret < 0) {
-        return ret;
-    }
+    int fd;
 
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s->fd == -1) {
-        DPRINTF("Unable to open socket");
-        return -socket_error();
-    }
-
-    socket_set_nonblock(s->fd);
-
-    do {
-        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1) {
-            ret = -socket_error();
-        }
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
-            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
-            return 0;
-        }
-    } while (ret == -EINTR);
-
-    if (ret < 0) {
+    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);
+        if (ret != -EINVAL) {
+            migrate_fd_error(s);
+        }
         return ret;
+    } else {
+        migrate_fd_connect(s);
     }
-    migrate_fd_connect(s);
     return 0;
 }
 
@@ -157,28 +141,16 @@ out2:
 
 int tcp_start_incoming_migration(const char *host_port)
 {
-    struct sockaddr_in addr;
-    int val;
+    int ret;
     int s;
 
     DPRINTF("Attempting to start an incoming migration\n");
 
-    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) {
-        return -socket_error();
+    ret = tcp_server_start(host_port, &s);
+    if (ret < 0) {
+        return ret;
     }
 
-    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 c34474f..f63014c 100644
--- a/net.c
+++ b/net.c
@@ -99,6 +99,114 @@ 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 ret;
+    int val = 1;
+
+    /* 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);
+    }
+
+    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 fe4cf6c..6024496 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -58,4 +58,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 */

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

[Kevin Wolf]

Am 10.02.2012 07:27, schrieb Amos Kong:
> 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>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  migration-tcp.c |   60 ++++++++-----------------------
>  net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu_socket.h   |    3 ++
>  3 files changed, 127 insertions(+), 44 deletions(-)

This patch is making too many changes at once:

1. Move code around
2. Remove duplicated code between client and server
3. Replace parse_host_port() with an open-coded version
4. Modify the open-coded parse_host_port() to use getaddrinfo instead of
inet_aton/gethostbyname (Why can't we just change parse_host_port? Are
there valid reasons to use the old implementation? Which?)

This makes it rather hard to review.

> diff --git a/migration-tcp.c b/migration-tcp.c
> index 35a5781..644bb8f 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -81,43 +81,27 @@ static void tcp_wait_for_connect(void *opaque)
>  
>  int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
>  {
> -    struct sockaddr_in addr;
>      int ret;
> -
> -    ret = parse_host_port(&addr, host_port);
> -    if (ret < 0) {
> -        return ret;
> -    }
> +    int fd;
>  
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> -    if (s->fd == -1) {
> -        DPRINTF("Unable to open socket");
> -        return -socket_error();
> -    }
> -
> -    socket_set_nonblock(s->fd);
> -
> -    do {
> -        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
> -        if (ret == -1) {
> -            ret = -socket_error();
> -        }
> -        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> -            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
> -            return 0;
> -        }
> -    } while (ret == -EINTR);
> -
> -    if (ret < 0) {
> +    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);
> +        if (ret != -EINVAL) {
> +            migrate_fd_error(s);
> +        }

This looks odd. It is probably needed to keep the old behaviour where a
failed parse_host_port() wouldn't call migrate_fd_error(). Is this
really required? Maybe I'm missing something, but the caller can't know
which failure case we had and if migrate_fd_error() has been called.

>          return ret;
> +    } else {
> +        migrate_fd_connect(s);
>      }
> -    migrate_fd_connect(s);
>      return 0;
>  }
>  
> @@ -157,28 +141,16 @@ out2:
>  
>  int tcp_start_incoming_migration(const char *host_port)
>  {
> -    struct sockaddr_in addr;
> -    int val;
> +    int ret;
>      int s;
>  
>      DPRINTF("Attempting to start an incoming migration\n");
>  
> -    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) {
> -        return -socket_error();
> +    ret = tcp_server_start(host_port, &s);
> +    if (ret < 0) {
> +        return ret;
>      }
>  
> -    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 c34474f..f63014c 100644
> --- a/net.c
> +++ b/net.c
> @@ -99,6 +99,114 @@ 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 ret;
> +    int val = 1;
> +
> +    /* 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;
> +    }

Separating host name and port at the first colon looks wrong for IPv6.

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

error_report?

Kevin

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

[Amos Kong]

On 24/02/12 17:08, Kevin Wolf wrote:
> Am 10.02.2012 07:27, schrieb Amos Kong:
>> 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>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   migration-tcp.c |   60 ++++++++-----------------------
>>   net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu_socket.h   |    3 ++
>>   3 files changed, 127 insertions(+), 44 deletions(-)
>
> This patch is making too many changes at once:
>
> 1. Move code around
> 2. Remove duplicated code between client and server
> 3. Replace parse_host_port() with an open-coded version

> 4. Modify the open-coded parse_host_port() to use getaddrinfo instead of
> inet_aton/gethostbyname (Why can't we just change parse_host_port? Are
> there valid reasons to use the old implementation? Which?)

tcp_common_start() needs to use the address list which is got by 
getaddrinfo(),

But I agree with verifying host/port by getaddrinfo() in parse_host_port.


> This makes it rather hard to review.
>
>> diff --git a/migration-tcp.c b/migration-tcp.c
>> index 35a5781..644bb8f 100644
>> --- a/migration-tcp.c
>> +++ b/migration-tcp.c
>> @@ -81,43 +81,27 @@ static void tcp_wait_for_connect(void *opaque)
>>
>>   int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
>>   {
>> -    struct sockaddr_in addr;
>>       int ret;
>> -
>> -    ret = parse_host_port(&addr, host_port);
>> -    if (ret<  0) {
>> -        return ret;
>> -    }
>> +    int fd;
>>
>>       s->get_error = socket_errno;
>>       s->write = socket_write;
>>       s->close = tcp_close;
>>
>> -    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>> -    if (s->fd == -1) {
>> -        DPRINTF("Unable to open socket");
>> -        return -socket_error();
>> -    }
>> -
>> -    socket_set_nonblock(s->fd);
>> -
>> -    do {
>> -        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
>> -        if (ret == -1) {
>> -            ret = -socket_error();
>> -        }
>> -        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
>> -            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>> -            return 0;
>> -        }
>> -    } while (ret == -EINTR);
>> -
>> -    if (ret<  0) {
>> +    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);
>> +        if (ret != -EINVAL) {
>> +            migrate_fd_error(s);
>> +        }
>
> This looks odd. It is probably needed to keep the old behaviour where a
> failed parse_host_port() wouldn't call migrate_fd_error(). Is this
> really required? Maybe I'm missing something, but the caller can't know
> which failure case we had and if migrate_fd_error() has been called.

getaddrinfo() is used in tcp_common_start(), -EINVAL will be returned
when getaddrinfo() doesn't return 0.


>>           return ret;
>> +    } else {
>> +        migrate_fd_connect(s);
>>       }
>> -    migrate_fd_connect(s);
>>       return 0;
>>   }
>>
>> @@ -157,28 +141,16 @@ out2:
>>
>>   int tcp_start_incoming_migration(const char *host_port)
>>   {
>> -    struct sockaddr_in addr;
>> -    int val;
>> +    int ret;
>>       int s;
>>
>>       DPRINTF("Attempting to start an incoming migration\n");
>>
>> -    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) {
>> -        return -socket_error();
>> +    ret = tcp_server_start(host_port,&s);
>> +    if (ret<  0) {
>> +        return ret;
>>       }
>>
>> -    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 c34474f..f63014c 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -99,6 +99,114 @@ 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 ret;
>> +    int val = 1;
>> +
>> +    /* 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;
>> +    }
>
> Separating host name and port at the first colon looks wrong for IPv6.

I fixed this in [PATCH 3/4] net: split hostname and service by last colon

>> +    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));
>
> error_report?

yes

> Kevin

-- 
			Amos.

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

[Kevin Wolf]

Am 02.03.2012 04:33, schrieb Amos Kong:
> On 24/02/12 17:08, Kevin Wolf wrote:
>> Am 10.02.2012 07:27, schrieb Amos Kong:
>>> 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>
>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>> ---
>>>   migration-tcp.c |   60 ++++++++-----------------------
>>>   net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qemu_socket.h   |    3 ++
>>>   3 files changed, 127 insertions(+), 44 deletions(-)
>>
>> This patch is making too many changes at once:
>>
>> 1. Move code around
>> 2. Remove duplicated code between client and server
>> 3. Replace parse_host_port() with an open-coded version
> 
>> 4. Modify the open-coded parse_host_port() to use getaddrinfo instead of
>> inet_aton/gethostbyname (Why can't we just change parse_host_port? Are
>> there valid reasons to use the old implementation? Which?)
> 
> tcp_common_start() needs to use the address list which is got by 
> getaddrinfo(),
> 
> But I agree with verifying host/port by getaddrinfo() in parse_host_port.

The new parse_host_port() could return an address list. If it's the
right thing to use it here, it's probably right to use it in all other
places as well.

>> This makes it rather hard to review.
>>
>>> diff --git a/migration-tcp.c b/migration-tcp.c
>>> index 35a5781..644bb8f 100644
>>> --- a/migration-tcp.c
>>> +++ b/migration-tcp.c
>>> @@ -81,43 +81,27 @@ static void tcp_wait_for_connect(void *opaque)
>>>
>>>   int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
>>>   {
>>> -    struct sockaddr_in addr;
>>>       int ret;
>>> -
>>> -    ret = parse_host_port(&addr, host_port);
>>> -    if (ret<  0) {
>>> -        return ret;
>>> -    }
>>> +    int fd;
>>>
>>>       s->get_error = socket_errno;
>>>       s->write = socket_write;
>>>       s->close = tcp_close;
>>>
>>> -    s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>> -    if (s->fd == -1) {
>>> -        DPRINTF("Unable to open socket");
>>> -        return -socket_error();
>>> -    }
>>> -
>>> -    socket_set_nonblock(s->fd);
>>> -
>>> -    do {
>>> -        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
>>> -        if (ret == -1) {
>>> -            ret = -socket_error();
>>> -        }
>>> -        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
>>> -            qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>>> -            return 0;
>>> -        }
>>> -    } while (ret == -EINTR);
>>> -
>>> -    if (ret<  0) {
>>> +    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);
>>> +        if (ret != -EINVAL) {
>>> +            migrate_fd_error(s);
>>> +        }
>>
>> This looks odd. It is probably needed to keep the old behaviour where a
>> failed parse_host_port() wouldn't call migrate_fd_error(). Is this
>> really required? Maybe I'm missing something, but the caller can't know
>> which failure case we had and if migrate_fd_error() has been called.
> 
> getaddrinfo() is used in tcp_common_start(), -EINVAL will be returned
> when getaddrinfo() doesn't return 0.

This is an implementation detail of tcp_common_start(). What does
-EINVAL mean at the tcp_client_start() level? Should it be documented
with a comment?

Why must migrate_fd_error() not be called when getaddrinfo() failed?

>>>           return ret;
>>> +    } else {
>>> +        migrate_fd_connect(s);
>>>       }
>>> -    migrate_fd_connect(s);
>>>       return 0;
>>>   }
>>>
>>> @@ -157,28 +141,16 @@ out2:
>>>
>>>   int tcp_start_incoming_migration(const char *host_port)
>>>   {
>>> -    struct sockaddr_in addr;
>>> -    int val;
>>> +    int ret;
>>>       int s;
>>>
>>>       DPRINTF("Attempting to start an incoming migration\n");
>>>
>>> -    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) {
>>> -        return -socket_error();
>>> +    ret = tcp_server_start(host_port,&s);
>>> +    if (ret<  0) {
>>> +        return ret;
>>>       }
>>>
>>> -    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 c34474f..f63014c 100644
>>> --- a/net.c
>>> +++ b/net.c
>>> @@ -99,6 +99,114 @@ 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 ret;
>>> +    int val = 1;
>>> +
>>> +    /* 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;
>>> +    }
>>
>> Separating host name and port at the first colon looks wrong for IPv6.
> 
> I fixed this in [PATCH 3/4] net: split hostname and service by last colon

Noticed it later. That's why I suggested reordering the patches in my
reply to PATCH 0/4.

Kevin

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

[Kevin Wolf]

Am 10.02.2012 07:27, schrieb Amos Kong:
> 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>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  migration-tcp.c |   60 ++++++++-----------------------
>  net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu_socket.h   |    3 ++
>  3 files changed, 127 insertions(+), 44 deletions(-)

> @@ -157,28 +141,16 @@ out2:
>  
>  int tcp_start_incoming_migration(const char *host_port)
>  {
> -    struct sockaddr_in addr;
> -    int val;
> +    int ret;
>      int s;
>  
>      DPRINTF("Attempting to start an incoming migration\n");
>  
> -    if (parse_host_port(&addr, host_port) < 0) {
> -        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
> -        return -EINVAL;
> -    }

Oh, and this case doesn't print an error message any more now.

Kevin

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

[Amos Kong]

On 24/02/12 17:34, Kevin Wolf wrote:
> Am 10.02.2012 07:27, schrieb Amos Kong:
>> 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>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   migration-tcp.c |   60 ++++++++-----------------------
>>   net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qemu_socket.h   |    3 ++
>>   3 files changed, 127 insertions(+), 44 deletions(-)
>
>> @@ -157,28 +141,16 @@ out2:
>>
>>   int tcp_start_incoming_migration(const char *host_port)
>>   {
>> -    struct sockaddr_in addr;
>> -    int val;
>> +    int ret;
>>       int s;
>>
>>       DPRINTF("Attempting to start an incoming migration\n");
>>
>> -    if (parse_host_port(&addr, host_port)<  0) {
>> -        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
>> -        return -EINVAL;
>> -    }
>
> Oh, and this case doesn't print an error message any more now.

The check work is done in tcp_start_common()

tcp_start_incoming_migration()
  -> tcp_client_start()
      -> tcp_start_common()

-- 
			Amos.

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

[Kevin Wolf]

Am 02.03.2012 03:50, schrieb Amos Kong:
> On 24/02/12 17:34, Kevin Wolf wrote:
>> Am 10.02.2012 07:27, schrieb Amos Kong:
>>> 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>
>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>> ---
>>>   migration-tcp.c |   60 ++++++++-----------------------
>>>   net.c           |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qemu_socket.h   |    3 ++
>>>   3 files changed, 127 insertions(+), 44 deletions(-)
>>
>>> @@ -157,28 +141,16 @@ out2:
>>>
>>>   int tcp_start_incoming_migration(const char *host_port)
>>>   {
>>> -    struct sockaddr_in addr;
>>> -    int val;
>>> +    int ret;
>>>       int s;
>>>
>>>       DPRINTF("Attempting to start an incoming migration\n");
>>>
>>> -    if (parse_host_port(&addr, host_port)<  0) {
>>> -        fprintf(stderr, "invalid host/port combination: %s\n", host_port);
>>> -        return -EINVAL;
>>> -    }
>>
>> Oh, and this case doesn't print an error message any more now.
> 
> The check work is done in tcp_start_common()
> 
> tcp_start_incoming_migration()
>   -> tcp_client_start()
>       -> tcp_start_common()

Yes, but it only return -EINVAL without printing an error message, so
the failure case is silent now.

Kevin

Re: [Qemu-devel] Use getaddrinfo for migration

[Michael Tokarev]

Not a reply to the patch but a general observation.

I noticed that the tcp migration uses gethostname
(or getaddrinfo after this patch) from the main
thread - is it really the way to go?  Note that
DNS query which is done may block for a large amount
of time.  Is it really safe in this context?  Should
it resolve the name in a separate thread, allowing
guest to run while it is doing that?

This question is important for me because right now
I'm evaluating a network-connected block device driver
which should do failover, so it will have to resolve
alternative name(s) at runtime (especially since list
of available targets is dynamic).

>From one point, _usually_, the delay there is very
small since it is unlikely you'll do migration or
failover overseas, so most likely you'll have the
answer from DNS handy.  But from another point, if
the DNS is malfunctioning right at that time (eg,
one of the two DNS resolvers is being rebooted),
the delay even from local DNS may be noticeable.

Thanks,

/mjt

Re: [Qemu-devel] Use getaddrinfo for migration

[Daniel P. Berrange]

On Fri, Mar 02, 2012 at 02:25:36PM +0400, Michael Tokarev wrote:
> Not a reply to the patch but a general observation.
> 
> I noticed that the tcp migration uses gethostname
> (or getaddrinfo after this patch) from the main
> thread - is it really the way to go?  Note that
> DNS query which is done may block for a large amount
> of time.  Is it really safe in this context?  Should
> it resolve the name in a separate thread, allowing
> guest to run while it is doing that?
> 
> This question is important for me because right now
> I'm evaluating a network-connected block device driver
> which should do failover, so it will have to resolve
> alternative name(s) at runtime (especially since list
> of available targets is dynamic).
> 
> From one point, _usually_, the delay there is very
> small since it is unlikely you'll do migration or
> failover overseas, so most likely you'll have the
> answer from DNS handy.  But from another point, if
> the DNS is malfunctioning right at that time (eg,
> one of the two DNS resolvers is being rebooted),
> the delay even from local DNS may be noticeable.

Yes, I think you are correct - QEMU should take care to ensure that
DNS resolution can not block the QEMU event loop thread.

There is the GLib extension (getaddrinfo_a) which does async DNS
resolution, but for sake of portability it is probably better
to use a thread to do it.

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] Use getaddrinfo for migration

[Amos Kong]

On 02/03/12 18:41, Daniel P. Berrange wrote:
> On Fri, Mar 02, 2012 at 02:25:36PM +0400, Michael Tokarev wrote:
>> Not a reply to the patch but a general observation.
>>
>> I noticed that the tcp migration uses gethostname
>> (or getaddrinfo after this patch) from the main
>> thread - is it really the way to go?  Note that
>> DNS query which is done may block for a large amount
>> of time.  Is it really safe in this context?  Should
>> it resolve the name in a separate thread, allowing
>> guest to run while it is doing that?
>>
>> This question is important for me because right now
>> I'm evaluating a network-connected block device driver
>> which should do failover, so it will have to resolve
>> alternative name(s) at runtime (especially since list
>> of available targets is dynamic).
>>
>>  From one point, _usually_, the delay there is very
>> small since it is unlikely you'll do migration or
>> failover overseas, so most likely you'll have the
>> answer from DNS handy.  But from another point, if
>> the DNS is malfunctioning right at that time (eg,
>> one of the two DNS resolvers is being rebooted),
>> the delay even from local DNS may be noticeable.
>
> Yes, I think you are correct - QEMU should take care to ensure that
> DNS resolution can not block the QEMU event loop thread.
>
> There is the GLib extension (getaddrinfo_a) which does async DNS
> resolution, but for sake of portability it is probably better
> to use a thread to do it.


I've prepared a V2 according to Kevin's comment,
https://github.com/kongove/qemu/commits/master

But I don't know how to process the getaddrinfo issue,
which steps should be done by a thread?
anyone can give a hint? thanks.

== migrate steps ==
0. main_loop, qemu_iohandler_poll
1. get migration command from qemu monitor
2. parse host/port, get an address list by getaddrinfo()
3. connect server
4. check status and return to main_loop (step 0)

(VMstate data is transmitted in background)

main_loop_wait()
...
  \- do_migrate()
    \- tcp_start_outgoing_migration()
      \- tcp_client_start()
        \- parse_host_port_info()
          \- getaddrinfo()


-- 
			Amos.

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

[Amos Kong]

Remove use of parse_host_port.
More SO_SOCKADDR changes.

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

diff --git a/net/socket.c b/net/socket.c
index d4c2002..439a69c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -403,29 +403,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 = g_malloc0(sizeof(NetSocketListenState));
 
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
-        g_free(s);
-        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");
+        fprintf(stderr, "tcp_server_start: %s\n", strerror(ret));
         g_free(s);
         closesocket(fd);
         return -1;
@@ -451,41 +435,19 @@ 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;
+    } else {
+        connected = 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;
-        }
-    }
     s = net_socket_fd_init(vlan, model, name, fd, connected);
     if (!s)
         return -1;

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

[Kevin Wolf]

Am 10.02.2012 07:27, schrieb Amos Kong:
> Remove use of parse_host_port.
> More SO_SOCKADDR changes.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net/socket.c |   60 +++++++++++-----------------------------------------------
>  1 files changed, 11 insertions(+), 49 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index d4c2002..439a69c 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -403,29 +403,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 = g_malloc0(sizeof(NetSocketListenState));
>  
> -    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> -    if (fd < 0) {
> -        perror("socket");
> -        g_free(s);
> -        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");
> +        fprintf(stderr, "tcp_server_start: %s\n", strerror(ret));

error_report, and it should be strerror(-ret).

>          g_free(s);
>          closesocket(fd);
>          return -1;
> @@ -451,41 +435,19 @@ 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);

There is no open fd in this case. The only time that fd != -1 and ret <
0 is for -EINPROGRESS and -EWOULDBLOCK.

>          return -1;
> +    } else {
> +        connected = 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

This part is lost without replacement?

> -            } else {
> -                perror("connect");
> -                closesocket(fd);

tcp_client_start uses close() instead of closesocket() in error case,
should probably be changed.

Kevin

[Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Amos Kong]

IPv6 address contains colons, parse will be wrong.

    [2312::8274]:5200

Signed-off-by: Amos Kong <akong@redhat.com>
---
 net.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net.c b/net.c
index f63014c..9e1ef9e 100644
--- a/net.c
+++ b/net.c
@@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     const char *p, *p1;
     int len;
     p = *pp;
-    p1 = strchr(p, sep);
+    p1 = strrchr(p, sep);
     if (!p1)
         return -1;
     len = p1 - p;

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Kevin Wolf]

Am 10.02.2012 07:27, schrieb Amos Kong:
> IPv6 address contains colons, parse will be wrong.
> 
>     [2312::8274]:5200
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net.c b/net.c
> index f63014c..9e1ef9e 100644
> --- a/net.c
> +++ b/net.c
> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>      const char *p, *p1;
>      int len;
>      p = *pp;
> -    p1 = strchr(p, sep);
> +    p1 = strrchr(p, sep);
>      if (!p1)
>          return -1;
>      len = p1 - p;

And what if the port isn't specified? I think you would erroneously
interpret the last part of the IP address as port.

Kevin

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Amos Kong]

On 24/02/12 17:29, Kevin Wolf wrote:
> Am 10.02.2012 07:27, schrieb Amos Kong:
>> IPv6 address contains colons, parse will be wrong.
>>
>>      [2312::8274]:5200
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   net.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index f63014c..9e1ef9e 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>>       const char *p, *p1;
>>       int len;
>>       p = *pp;
>> -    p1 = strchr(p, sep);
>> +    p1 = strrchr(p, sep);
>>       if (!p1)
>>           return -1;
>>       len = p1 - p;
>
> And what if the port isn't specified? I think you would erroneously
> interpret the last part of the IP address as port.

IPv6 address have paired colons, need more precision check.

-- 
			Amos.

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Amos Kong]

On 02/03/12 11:38, Amos Kong wrote:
>>> --- a/net.c
>>> +++ b/net.c
>>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size,
>>> const char **pp, int sep)
>>>       const char *p, *p1;
>>>       int len;
>>>       p = *pp;
>>> -    p1 = strchr(p, sep);
>>> +    p1 = strrchr(p, sep);
>>>       if (!p1)
>>>           return -1;
>>>       len = p1 - p;
>>
>> And what if the port isn't specified? I think you would erroneously
>> interpret the last part of the IP address as port.

Hi Kevin, port must be specified in '-incoming' parameters and migrate 
monitor cmd.

  qemu-kvm ... -incoming tcp:$host:$port
  (qemu) migrate -d tcp:$host:$port


If use boot up guest by wrong cmdline, qemu will report an error msg.

# ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -boot n -incoming 
tcp:2312::8272 -monitor stdio
qemu-system-x86_64: qemu: getaddrinfo: Name or service not known
tcp_server_start: Invalid argument
Migration failed. Exit code tcp:2312::8272(-22), exiting.

> IPv6 address have paired colons, need more precision check.

------

parse_host_port() are used in four functions in net/socket.c:
  tcp_start_outgoing_migration()
  tcp_start_incoming_migration()
  net_socket_mcast_init()
  net_socket_udp_init()

The argument type of parse_host_port() needs to be changed
if we replace inet_aton/gethostbyname by getaddrinfo.

So I will not change parse_host_port(), and verify Ipv6 addr
in tcp_start_common without parse_host_port.


-- 
			Amos.

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Kevin Wolf]

Am 02.03.2012 10:58, schrieb Amos Kong:
> On 02/03/12 11:38, Amos Kong wrote:
>>>> --- a/net.c
>>>> +++ b/net.c
>>>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size,
>>>> const char **pp, int sep)
>>>>       const char *p, *p1;
>>>>       int len;
>>>>       p = *pp;
>>>> -    p1 = strchr(p, sep);
>>>> +    p1 = strrchr(p, sep);
>>>>       if (!p1)
>>>>           return -1;
>>>>       len = p1 - p;
>>>
>>> And what if the port isn't specified? I think you would erroneously
>>> interpret the last part of the IP address as port.
> 
> Hi Kevin, port must be specified in '-incoming' parameters and migrate 
> monitor cmd.
> 
>   qemu-kvm ... -incoming tcp:$host:$port
>   (qemu) migrate -d tcp:$host:$port
> 
> 
> If use boot up guest by wrong cmdline, qemu will report an error msg.
> 
> # ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -boot n -incoming 
> tcp:2312::8272 -monitor stdio
> qemu-system-x86_64: qemu: getaddrinfo: Name or service not known
> tcp_server_start: Invalid argument
> Migration failed. Exit code tcp:2312::8272(-22), exiting.

Which is because 2312: isn't a valid IP address, right? But what if you
have something like 2312::1234:8272? If you misinterpret the 8272 as a
port number, the remaining address is still a valid IPv6 address.

> parse_host_port() are used in four functions in net/socket.c:
>   tcp_start_outgoing_migration()
>   tcp_start_incoming_migration()
>   net_socket_mcast_init()
>   net_socket_udp_init()
> 
> The argument type of parse_host_port() needs to be changed
> if we replace inet_aton/gethostbyname by getaddrinfo.
> 
> So I will not change parse_host_port(), and verify Ipv6 addr
> in tcp_start_common without parse_host_port.

Why not change the prototype of parse_host_port() and modify all callers?

Kevin

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Laine Stump]

On 03/02/2012 05:35 AM, Kevin Wolf wrote:
> Am 02.03.2012 10:58, schrieb Amos Kong:
>> On 02/03/12 11:38, Amos Kong wrote:
>>>>> --- a/net.c
>>>>> +++ b/net.c
>>>>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size,
>>>>> const char **pp, int sep)
>>>>>       const char *p, *p1;
>>>>>       int len;
>>>>>       p = *pp;
>>>>> -    p1 = strchr(p, sep);
>>>>> +    p1 = strrchr(p, sep);
>>>>>       if (!p1)
>>>>>           return -1;
>>>>>       len = p1 - p;
>>>> And what if the port isn't specified? I think you would erroneously
>>>> interpret the last part of the IP address as port.
>> Hi Kevin, port must be specified in '-incoming' parameters and migrate 
>> monitor cmd.
>>
>>   qemu-kvm ... -incoming tcp:$host:$port
>>   (qemu) migrate -d tcp:$host:$port
>>
>>
>> If use boot up guest by wrong cmdline, qemu will report an error msg.
>>
>> # ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -boot n -incoming 
>> tcp:2312::8272 -monitor stdio
>> qemu-system-x86_64: qemu: getaddrinfo: Name or service not known
>> tcp_server_start: Invalid argument
>> Migration failed. Exit code tcp:2312::8272(-22), exiting.
> Which is because 2312: isn't a valid IP address, right? But what if you
> have something like 2312::1234:8272? If you misinterpret the 8272 as a
> port number, the remaining address is still a valid IPv6 address.

This is made irrelevant by PATCH 4/4, which allows for the IP address to
be placed inside brackets:

   [2312::8272]:port

(at least it's irrelevant if your documentation *requires* brackets for
all numeric ipv6-address:port pairs, which is strongly recommended by
RFC 5952). It really is impossible to disambiguate the meaning of the
final ":nnnn" unless you require these brackets (or 1) require full
specification of all potential colons in the IPv6 address or require
that the port *always* be specified, neither of which seem acceptable to
me).

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Kevin Wolf]

Am 02.03.2012 20:54, schrieb Laine Stump:
> On 03/02/2012 05:35 AM, Kevin Wolf wrote:
>> Am 02.03.2012 10:58, schrieb Amos Kong:
>>> On 02/03/12 11:38, Amos Kong wrote:
>>>>>> --- a/net.c
>>>>>> +++ b/net.c
>>>>>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int buf_size,
>>>>>> const char **pp, int sep)
>>>>>>       const char *p, *p1;
>>>>>>       int len;
>>>>>>       p = *pp;
>>>>>> -    p1 = strchr(p, sep);
>>>>>> +    p1 = strrchr(p, sep);
>>>>>>       if (!p1)
>>>>>>           return -1;
>>>>>>       len = p1 - p;
>>>>> And what if the port isn't specified? I think you would erroneously
>>>>> interpret the last part of the IP address as port.
>>> Hi Kevin, port must be specified in '-incoming' parameters and migrate 
>>> monitor cmd.
>>>
>>>   qemu-kvm ... -incoming tcp:$host:$port
>>>   (qemu) migrate -d tcp:$host:$port
>>>
>>>
>>> If use boot up guest by wrong cmdline, qemu will report an error msg.
>>>
>>> # ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -boot n -incoming 
>>> tcp:2312::8272 -monitor stdio
>>> qemu-system-x86_64: qemu: getaddrinfo: Name or service not known
>>> tcp_server_start: Invalid argument
>>> Migration failed. Exit code tcp:2312::8272(-22), exiting.
>> Which is because 2312: isn't a valid IP address, right? But what if you
>> have something like 2312::1234:8272? If you misinterpret the 8272 as a
>> port number, the remaining address is still a valid IPv6 address.
> 
> This is made irrelevant by PATCH 4/4, which allows for the IP address to
> be placed inside brackets:
> 
>    [2312::8272]:port
> 
> (at least it's irrelevant if your documentation *requires* brackets for
> all numeric ipv6-address:port pairs, which is strongly recommended by
> RFC 5952). It really is impossible to disambiguate the meaning of the
> final ":nnnn" unless you require these brackets (or 1) require full
> specification of all potential colons in the IPv6 address or require
> that the port *always* be specified, neither of which seem acceptable to
> me).

Here you're actually explaining why it's not irrelevant. You don't want
to enforce port numbers, so 2312::1234:8272 must be interpreted as an
IPv6 address without a port. This code however would take 8727 as the
port and 2312::1234 as the IPv6 address, which is not what you expected
(even after brackets are allowed - they don't make a difference because
the example doesn't use brackets).

Kevin

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Amos Kong]

----- Original Message -----
> Am 02.03.2012 20:54, schrieb Laine Stump:
> > On 03/02/2012 05:35 AM, Kevin Wolf wrote:
> >> Am 02.03.2012 10:58, schrieb Amos Kong:
> >>> On 02/03/12 11:38, Amos Kong wrote:
> >>>>>> --- a/net.c
> >>>>>> +++ b/net.c
> >>>>>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int
> >>>>>> buf_size,
> >>>>>> const char **pp, int sep)
> >>>>>>       const char *p, *p1;
> >>>>>>       int len;
> >>>>>>       p = *pp;
> >>>>>> -    p1 = strchr(p, sep);
> >>>>>> +    p1 = strrchr(p, sep);
> >>>>>>       if (!p1)
> >>>>>>           return -1;
> >>>>>>       len = p1 - p;
> >>>>> And what if the port isn't specified? I think you would
> >>>>> erroneously
> >>>>> interpret the last part of the IP address as port.
> >>> Hi Kevin, port must be specified in '-incoming' parameters and
> >>> migrate
> >>> monitor cmd.
> >>>
> >>>   qemu-kvm ... -incoming tcp:$host:$port
> >>>   (qemu) migrate -d tcp:$host:$port
> >>>
> >>>
> >>> If use boot up guest by wrong cmdline, qemu will report an error
> >>> msg.
> >>>
> >>> # ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -boot n
> >>> -incoming
> >>> tcp:2312::8272 -monitor stdio
> >>> qemu-system-x86_64: qemu: getaddrinfo: Name or service not known
> >>> tcp_server_start: Invalid argument
> >>> Migration failed. Exit code tcp:2312::8272(-22), exiting.
> >> Which is because 2312: isn't a valid IP address, right? But what
> >> if you
> >> have something like 2312::1234:8272? If you misinterpret the 8272
> >> as a
> >> port number, the remaining address is still a valid IPv6 address.
> > 
> > This is made irrelevant by PATCH 4/4, which allows for the IP
> > address to
> > be placed inside brackets:
> > 
> >    [2312::8272]:port
> > 
> > (at least it's irrelevant if your documentation *requires* brackets
> > for
> > all numeric ipv6-address:port pairs, which is strongly recommended
> > by
> > RFC 5952). It really is impossible to disambiguate the meaning of
> > the
> > final ":nnnn" unless you require these brackets (or 1) require full
> > specification of all potential colons in the IPv6 address or
> > require
> > that the port *always* be specified, neither of which seem
> > acceptable to
> > me).
> 
> Here you're actually explaining why it's not irrelevant. You don't
> want
> to enforce port numbers, so 2312::1234:8272 must be interpreted as an
> IPv6 address without a port. This code however would take 8727 as the
> port and 2312::1234 as the IPv6 address, which is not what you
> expected
> (even after brackets are allowed - they don't make a difference
> because
> the example doesn't use brackets).

In the migration context, host/port are all necessary, so it's right to parse "8272" to a port.
However, for IPv6 brackets must be mandatory if you require a port.


BTW, the DNS delay issue existed in the past (gethostbyname()), it should be fixed by another patchset.
I will post my V2 (without fix of DNS delay) later.

> 
> Kevin
> 

Re: [Qemu-devel] [PATCH 3/4] net: split hostname and service by last colon

[Kevin Wolf]

Am 05.03.2012 09:59, schrieb Amos Kong:
> ----- Original Message -----
>> Am 02.03.2012 20:54, schrieb Laine Stump:
>>> On 03/02/2012 05:35 AM, Kevin Wolf wrote:
>>>> Am 02.03.2012 10:58, schrieb Amos Kong:
>>>>> On 02/03/12 11:38, Amos Kong wrote:
>>>>>>>> --- a/net.c
>>>>>>>> +++ b/net.c
>>>>>>>> @@ -84,7 +84,7 @@ static int get_str_sep(char *buf, int
>>>>>>>> buf_size,
>>>>>>>> const char **pp, int sep)
>>>>>>>>       const char *p, *p1;
>>>>>>>>       int len;
>>>>>>>>       p = *pp;
>>>>>>>> -    p1 = strchr(p, sep);
>>>>>>>> +    p1 = strrchr(p, sep);
>>>>>>>>       if (!p1)
>>>>>>>>           return -1;
>>>>>>>>       len = p1 - p;
>>>>>>> And what if the port isn't specified? I think you would
>>>>>>> erroneously
>>>>>>> interpret the last part of the IP address as port.
>>>>> Hi Kevin, port must be specified in '-incoming' parameters and
>>>>> migrate
>>>>> monitor cmd.
>>>>>
>>>>>   qemu-kvm ... -incoming tcp:$host:$port
>>>>>   (qemu) migrate -d tcp:$host:$port
>>>>>
>>>>>
>>>>> If use boot up guest by wrong cmdline, qemu will report an error
>>>>> msg.
>>>>>
>>>>> # ./x86_64-softmmu/qemu-system-x86_64 --enable-kvm -boot n
>>>>> -incoming
>>>>> tcp:2312::8272 -monitor stdio
>>>>> qemu-system-x86_64: qemu: getaddrinfo: Name or service not known
>>>>> tcp_server_start: Invalid argument
>>>>> Migration failed. Exit code tcp:2312::8272(-22), exiting.
>>>> Which is because 2312: isn't a valid IP address, right? But what
>>>> if you
>>>> have something like 2312::1234:8272? If you misinterpret the 8272
>>>> as a
>>>> port number, the remaining address is still a valid IPv6 address.
>>>
>>> This is made irrelevant by PATCH 4/4, which allows for the IP
>>> address to
>>> be placed inside brackets:
>>>
>>>    [2312::8272]:port
>>>
>>> (at least it's irrelevant if your documentation *requires* brackets
>>> for
>>> all numeric ipv6-address:port pairs, which is strongly recommended
>>> by
>>> RFC 5952). It really is impossible to disambiguate the meaning of
>>> the
>>> final ":nnnn" unless you require these brackets (or 1) require full
>>> specification of all potential colons in the IPv6 address or
>>> require
>>> that the port *always* be specified, neither of which seem
>>> acceptable to
>>> me).
>>
>> Here you're actually explaining why it's not irrelevant. You don't
>> want
>> to enforce port numbers, so 2312::1234:8272 must be interpreted as an
>> IPv6 address without a port. This code however would take 8727 as the
>> port and 2312::1234 as the IPv6 address, which is not what you
>> expected
>> (even after brackets are allowed - they don't make a difference
>> because
>> the example doesn't use brackets).
> 
> In the migration context, host/port are all necessary, so it's right to parse "8272" to a port.
> However, for IPv6 brackets must be mandatory if you require a port.

Makes sense.

> BTW, the DNS delay issue existed in the past (gethostbyname()), it should be fixed by another patchset.
> I will post my V2 (without fix of DNS delay) later.

Yes, I agree.

Kevin

[Qemu-devel] [PATCH 4/4] net: support to include ipv6 address by brackets

[Amos Kong]

That method of representing an IPv6 address with a port is
discouraged because of its ambiguity. Referencing to RFC5952,
the recommended format is:

     [2312::8274]:5200

test status: Successed
listen side: qemu-kvm .... -incoming tcp:[2312::8274]:5200
client side: qemu-kvm ...
             (qemu) migrate -d tcp:[2312::8274]:5200

Signed-off-by: Amos Kong <akong@redhat.com>
---
 net.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net.c b/net.c
index 9e1ef9e..cccdb6b 100644
--- a/net.c
+++ b/net.c
@@ -88,6 +88,12 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     if (!p1)
         return -1;
     len = p1 - p;
+    /* remove brackets which includes hostname */
+    if (*p == '[' && *(p1-1) == ']') {
+        p += 1;
+        len -= 2;
+    }
+
     p1++;
     if (buf_size > 0) {
         if (len > buf_size - 1)

Re: [Qemu-devel] [PATCH 0/4] support to migrate with IPv6 address

[Kevin Wolf]

Am 10.02.2012 07:26, schrieb Amos Kong:
> Those four patches make migration of IPv6 address work.
> Use getaddrinfo() to socket addresses infomation.
> 
> ---
> 
> Amos Kong (4):
>       Use getaddrinfo for migration
>       net/socket: allow ipv6 for net_socket_listen_init and socket_connect_init
>       net: split hostname and service by last colon
>       net: support to include ipv6 address by brackets

I think it would be better to split the patches differently:

1. Create tcp_server_start by moving code. Keep using parse_host_port
and the old loop around connect.
2. Unify all TCP servers to use this code
3. Introduce the client code and the convert all clients. Up to here,
this should be pure refactoring work.
4. Only now start changing the logic. First thing is getaddrinfo. Don't
change from IPv4 to IPv6 yet.
5. Make all changes required for IPv6 (getaddrinfo arguments, address
parsing, etc.)

Having step-by-step conversions with an explanation of each step in the
commit message makes it so much easier to understand the commits.

For the comments on details see the replies to the individual patches.

Kevin