[PATCH v3 0/9] support to migrate with IPv6 address

[PATCH v3 0/9] support to migrate with IPv6 address

[Amos Kong]

Those patches make migration of IPv6 address work, old code 
only support to parse IPv4 address/port, use getaddrinfo()
to get socket addresses infomation.
Last two patches are about spliting IPv6 host/port.

Changes from v1:
- split different changes to small patches, it will be easier to review
- fixed some problem according to Kevin's comment

Changes from v2:
- fix issue of returning real error 
- set s->fd to -1 when parse fails, won't call migrate_fd_error()

---

Amos Kong (9):
      net: introduce tcp_server_start()
      net: use tcp_server_start() for tcp server creation
      net: introduce tcp_client_start()
      net: use tcp_client_start for tcp client creation
      net: refector tcp_*_start functions
      net: use getaddrinfo() in tcp_start_common
      net: introduce parse_host_port_info()
      net: split hostname and service by last colon
      net: support to include ipv6 address by brackets


 migration-tcp.c |   60 ++++++------------------
 net.c           |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/socket.c    |   66 ++++++--------------------
 qemu_socket.h   |    3 +
 4 files changed, 170 insertions(+), 96 deletions(-)

-- 
Amos Kong

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

[Amos Kong]

Those patches make migration of IPv6 address work, old code 
only support to parse IPv4 address/port, use getaddrinfo()
to get socket addresses infomation.
Last two patches are about spliting IPv6 host/port.

Changes from v1:
- split different changes to small patches, it will be easier to review
- fixed some problem according to Kevin's comment

Changes from v2:
- fix issue of returning real error 
- set s->fd to -1 when parse fails, won't call migrate_fd_error()

---

Amos Kong (9):
      net: introduce tcp_server_start()
      net: use tcp_server_start() for tcp server creation
      net: introduce tcp_client_start()
      net: use tcp_client_start for tcp client creation
      net: refector tcp_*_start functions
      net: use getaddrinfo() in tcp_start_common
      net: introduce parse_host_port_info()
      net: split hostname and service by last colon
      net: support to include ipv6 address by brackets


 migration-tcp.c |   60 ++++++------------------
 net.c           |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/socket.c    |   66 ++++++--------------------
 qemu_socket.h   |    3 +
 4 files changed, 170 insertions(+), 96 deletions(-)

-- 
Amos Kong

[PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

Introduce tcp_server_start() by moving original code in
tcp_start_incoming_migration().

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

diff --git a/net.c b/net.c
index c34474f..e90ff23 100644
--- a/net.c
+++ b/net.c
@@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 0;
 }
 
+int tcp_server_start(const char *str, int *fd)
+{
+    int val, ret;
+    struct sockaddr_in saddr;
+
+    if (parse_host_port(&saddr, str) < 0) {
+        error_report("invalid host/port combination: %s", str);
+        return -EINVAL;
+    }
+
+    *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));
+    if (ret < 0) {
+        closesocket(*fd);
+    }
+    return ret;
+}
+
 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..d612793 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen);
 int unix_connect_opts(QemuOpts *opts);
 int unix_connect(const char *path);
 
+int tcp_server_start(const char *str, int *fd);
+
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);
 int socket_init(void);

[Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

Introduce tcp_server_start() by moving original code in
tcp_start_incoming_migration().

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

diff --git a/net.c b/net.c
index c34474f..e90ff23 100644
--- a/net.c
+++ b/net.c
@@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 0;
 }
 
+int tcp_server_start(const char *str, int *fd)
+{
+    int val, ret;
+    struct sockaddr_in saddr;
+
+    if (parse_host_port(&saddr, str) < 0) {
+        error_report("invalid host/port combination: %s", str);
+        return -EINVAL;
+    }
+
+    *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));
+    if (ret < 0) {
+        closesocket(*fd);
+    }
+    return ret;
+}
+
 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..d612793 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen);
 int unix_connect_opts(QemuOpts *opts);
 int unix_connect(const char *path);
 
+int tcp_server_start(const char *str, int *fd);
+
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);
 int socket_init(void);

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Michael Roth]

On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote:
> Introduce tcp_server_start() by moving original code in
> tcp_start_incoming_migration().
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net.c         |   28 ++++++++++++++++++++++++++++
>  qemu_socket.h |    2 ++
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/net.c b/net.c
> index c34474f..e90ff23 100644
> --- a/net.c
> +++ b/net.c
> @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>      return 0;
>  }
> 
> +int tcp_server_start(const char *str, int *fd)
> +{
> +    int val, ret;
> +    struct sockaddr_in saddr;
> +
> +    if (parse_host_port(&saddr, str) < 0) {
> +        error_report("invalid host/port combination: %s", str);
> +        return -EINVAL;
> +    }
> +
> +    *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));
> +    if (ret < 0) {
> +        closesocket(*fd);
> +    }
> +    return ret;
> +}
> +

I would combine this with patch 2, since it provides context for why
this function is being added. Would also do the same for 3 and 4.

I see client the client implementation you need to pass fd back by
reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success,
but here it looks like fd is only value if ret=0, so might as well just
pass back the fd as the function return value.

Also, is there any reason we can't re-use
qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they
serve the same purpose, and already include some of the work from your
PATCH #6.

>  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..d612793 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen);
>  int unix_connect_opts(QemuOpts *opts);
>  int unix_connect(const char *path);
> 
> +int tcp_server_start(const char *str, int *fd);
> +
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
>  int socket_init(void);
> 
> 

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

On 14/03/12 00:39, Michael Roth wrote:
> On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote:
>> Introduce tcp_server_start() by moving original code in
>> tcp_start_incoming_migration().
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   net.c         |   28 ++++++++++++++++++++++++++++
>>   qemu_socket.h |    2 ++
>>   2 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index c34474f..e90ff23 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>>       return 0;
>>   }
>>
>> +int tcp_server_start(const char *str, int *fd)
>> +{
>> +    int val, ret;
>> +    struct sockaddr_in saddr;
>> +
>> +    if (parse_host_port(&saddr, str)<  0) {
>> +        error_report("invalid host/port combination: %s", str);
>> +        return -EINVAL;
>> +    }
>> +
>> +    *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));
>> +    if (ret<  0) {
>> +        closesocket(*fd);
>> +    }
>> +    return ret;
>> +}
>> +
>
> I would combine this with patch 2, since it provides context for why
> this function is being added. Would also do the same for 3 and 4.
>
> I see client the client implementation you need to pass fd back by
> reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success,

ret restores 0 or -socket_error()
  success: 0, -EINPROGRESS
  fail   : ret < 0 && ret !=-EINTR && ret != -EWOULDBLOCK

, it should be -EINPROGRESS

+        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
+        if (ret < 0) {
+            ret = -socket_error();
+            if (ret == -EINPROGRESS) {
+                break;


> but here it looks like fd is only value if ret=0, so might as well just
> pass back the fd as the function return value.

Passed *fd tells us if socket creation success
'ret' would return 0 or -socket_error()

-socket_error() is the error of socket creation/connection, it would be 
used by other functions.

eg: migration-tcp.c:
int tcp_start_incoming_migration(
     ...
     ret = tcp_server_start(host_port, &s);
     if (ret < 0) {
         fprintf(stderr, "tcp_server_start: %s\n", strerror(-ret));

We can also add a error msg in tcp_start_outgoing_migration() when 
tcp_client_start() fails.

> Also, is there any reason we can't re-use
> qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they
> serve the same purpose, and already include some of the work from your
> PATCH #6.

We could not directly use it, there are some difference,
such as tcp_start_incoming_migration() doesn't set socket no-blocked,
but net_socket_listen_init() sets socket no-blocked.

There are some repeated code, so I write a tcp_common_start(), and use 
it in net_socket_listen_init()/
net_socket_connect_init() and tcp_start_incoming_migration()/ 
tcp_start_outgoing_migration()


>>   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..d612793 100644
>> --- a/qemu_socket.h
>> +++ b/qemu_socket.h
>> @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen);
>>   int unix_connect_opts(QemuOpts *opts);
>>   int unix_connect(const char *path);
>>
>> +int tcp_server_start(const char *str, int *fd);
>> +
>>   /* Old, ipv4 only bits.  Don't use for new code. */
>>   int parse_host_port(struct sockaddr_in *saddr, const char *str);
>>   int socket_init(void);
>>
>>
>

-- 
			Amos.

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Michael Roth]

On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote:
> On 14/03/12 00:39, Michael Roth wrote:
> >On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote:
> >>Introduce tcp_server_start() by moving original code in
> >>tcp_start_incoming_migration().
> >>
> >>Signed-off-by: Amos Kong<akong@redhat.com>
> >>---
> >>  net.c         |   28 ++++++++++++++++++++++++++++
> >>  qemu_socket.h |    2 ++
> >>  2 files changed, 30 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/net.c b/net.c
> >>index c34474f..e90ff23 100644
> >>--- a/net.c
> >>+++ b/net.c
> >>@@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
> >>      return 0;
> >>  }
> >>
> >>+int tcp_server_start(const char *str, int *fd)
> >>+{
> >>+    int val, ret;
> >>+    struct sockaddr_in saddr;
> >>+
> >>+    if (parse_host_port(&saddr, str)<  0) {
> >>+        error_report("invalid host/port combination: %s", str);
> >>+        return -EINVAL;
> >>+    }
> >>+
> >>+    *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));
> >>+    if (ret<  0) {
> >>+        closesocket(*fd);
> >>+    }
> >>+    return ret;
> >>+}
> >>+
> >
> >I would combine this with patch 2, since it provides context for why
> >this function is being added. Would also do the same for 3 and 4.
> >
> >I see client the client implementation you need to pass fd back by
> >reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success,
> 
> ret restores 0 or -socket_error()
>  success: 0, -EINPROGRESS
>  fail   : ret < 0 && ret !=-EINTR && ret != -EWOULDBLOCK
> 
> , it should be -EINPROGRESS

I see, I think I was confued by patch #4 where you do a

+    ret = tcp_client_start(host_port, &s->fd);
+    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        DPRINTF("connect in progress");
+        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);

If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to
return EWOULDBLOCK), we should fail it there rather than passing it on to
tcp_wait_for_connect().

> 
> +        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +        if (ret < 0) {
> +            ret = -socket_error();
> +            if (ret == -EINPROGRESS) {
> +                break;
> 
> 
> >but here it looks like fd is only value if ret=0, so might as well just
> >pass back the fd as the function return value.
> 
> Passed *fd tells us if socket creation success
> 'ret' would return 0 or -socket_error()
> 
> -socket_error() is the error of socket creation/connection, it would
> be used by other functions.
> 
> eg: migration-tcp.c:
> int tcp_start_incoming_migration(
>     ...
>     ret = tcp_server_start(host_port, &s);
>     if (ret < 0) {
>         fprintf(stderr, "tcp_server_start: %s\n", strerror(-ret));
> 
> We can also add a error msg in tcp_start_outgoing_migration() when
> tcp_client_start() fails.
> 
> >Also, is there any reason we can't re-use
> >qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they
> >serve the same purpose, and already include some of the work from your
> >PATCH #6.
> 
> We could not directly use it, there are some difference,
> such as tcp_start_incoming_migration() doesn't set socket no-blocked,
> but net_socket_listen_init() sets socket no-blocked.

I think adding a common function with blocking/non-blocking flag and
having inet_listen_opts()/socket_listen_opts() call it with a wrapper
would be reasonable.

A lot of code is being introduced here to solve problems that are
already handled in qemu-sockets.c. inet_listen()/inet_connect()
already handles backeted-enclosed ipv6 addrs, getting port numbers when
there's more than one colon, getaddrinfo()-based connections, and most
importantly it's had ipv6 support from day 1.

Not 100% sure it'll work for what you're doing, but qemu-sockets.c was
specifically added for this type of use-case and is heavilly used
currently (vnc, nbd, Chardev users), so I think we should use it unless
there's a good reason not to.

> 
> There are some repeated code, so I write a tcp_common_start(), and
> use it in net_socket_listen_init()/
> net_socket_connect_init() and tcp_start_incoming_migration()/
> tcp_start_outgoing_migration()
> 
> 
> >>  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..d612793 100644
> >>--- a/qemu_socket.h
> >>+++ b/qemu_socket.h
> >>@@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen);
> >>  int unix_connect_opts(QemuOpts *opts);
> >>  int unix_connect(const char *path);
> >>
> >>+int tcp_server_start(const char *str, int *fd);
> >>+
> >>  /* Old, ipv4 only bits.  Don't use for new code. */
> >>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
> >>  int socket_init(void);
> >>
> >>
> >
> 
> -- 
> 			Amos.
> 

Re: [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

On 14/03/12 22:58, Michael Roth wrote:
> On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote:
>> On 14/03/12 00:39, Michael Roth wrote:
>>> On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote:
>>>> Introduce tcp_server_start() by moving original code in
>>>> tcp_start_incoming_migration().
>>>>
>>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>>> ---
>>>>   net.c         |   28 ++++++++++++++++++++++++++++
>>>>   qemu_socket.h |    2 ++
>>>>   2 files changed, 30 insertions(+), 0 deletions(-)
>>>>
>>>> +int tcp_server_start(const char *str, int *fd)
>>>> +{

>>> I would combine this with patch 2, since it provides context for why
>>> this function is being added. Would also do the same for 3 and 4.
>>>
>>> I see client the client implementation you need to pass fd back by
>>> reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success,
>>
>> ret restores 0 or -socket_error()
>>   success: 0, -EINPROGRESS
>>   fail   : ret<  0&&  ret !=-EINTR&&  ret != -EWOULDBLOCK
>>
>> , it should be -EINPROGRESS
>
> I see, I think I was confued by patch #4 where you do a
>
> +    ret = tcp_client_start(host_port,&s->fd);
> +    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> +        DPRINTF("connect in progress");
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>
> If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to
> return EWOULDBLOCK), we should fail it there rather than passing it on to
> tcp_wait_for_connect().

You are right, it should be :
        if (ret == -EINPROGRESS) {

>>> Also, is there any reason we can't re-use
>>> qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they
>>> serve the same purpose, and already include some of the work from your
>>> PATCH #6.
>>
>> We could not directly use it, there are some difference,
>> such as tcp_start_incoming_migration() doesn't set socket no-blocked,
>> but net_socket_listen_init() sets socket no-blocked.
>
> I think adding a common function with blocking/non-blocking flag and
> having inet_listen_opts()/socket_listen_opts() call it with a wrapper
> would be reasonable.

> A lot of code is being introduced here to solve problems that are
> already handled in qemu-sockets.c. inet_listen()/inet_connect()
> already handles backeted-enclosed ipv6 addrs, getting port numbers when
> there's more than one colon, getaddrinfo()-based connections, and most
> importantly it's had ipv6 support from day 1.

> Not 100% sure it'll work for what you're doing, but qemu-sockets.c was
> specifically added for this type of use-case and is heavilly used
> currently (vnc, nbd, Chardev users), so I think we should use it unless
> there's a good reason not to.

There are many special request for migration, which is not implemented in
inet_listen_opts()/socket_listen_opts(), but many codes can be reused,
I would re-write patches.

Thanks, Amos

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

On 14/03/12 22:58, Michael Roth wrote:
> On Wed, Mar 14, 2012 at 04:33:14PM +0800, Amos Kong wrote:
>> On 14/03/12 00:39, Michael Roth wrote:
>>> On Wed, Mar 07, 2012 at 06:47:45AM +0800, Amos Kong wrote:
>>>> Introduce tcp_server_start() by moving original code in
>>>> tcp_start_incoming_migration().
>>>>
>>>> Signed-off-by: Amos Kong<akong@redhat.com>
>>>> ---
>>>>   net.c         |   28 ++++++++++++++++++++++++++++
>>>>   qemu_socket.h |    2 ++
>>>>   2 files changed, 30 insertions(+), 0 deletions(-)
>>>>
>>>> +int tcp_server_start(const char *str, int *fd)
>>>> +{

>>> I would combine this with patch 2, since it provides context for why
>>> this function is being added. Would also do the same for 3 and 4.
>>>
>>> I see client the client implementation you need to pass fd back by
>>> reference since ret can be set to EINPROGRESS/EWOULDBLOCK on success,
>>
>> ret restores 0 or -socket_error()
>>   success: 0, -EINPROGRESS
>>   fail   : ret<  0&&  ret !=-EINTR&&  ret != -EWOULDBLOCK
>>
>> , it should be -EINPROGRESS
>
> I see, I think I was confued by patch #4 where you do a
>
> +    ret = tcp_client_start(host_port,&s->fd);
> +    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
> +        DPRINTF("connect in progress");
> +        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
>
> If ret == EWOULDBLOCK is a failure (or if the call isn't supposed to
> return EWOULDBLOCK), we should fail it there rather than passing it on to
> tcp_wait_for_connect().

You are right, it should be :
        if (ret == -EINPROGRESS) {

>>> Also, is there any reason we can't re-use
>>> qemu-sockets.c:inet_listen()/qemu-sockets.c:inet_connect()? AFAICT they
>>> serve the same purpose, and already include some of the work from your
>>> PATCH #6.
>>
>> We could not directly use it, there are some difference,
>> such as tcp_start_incoming_migration() doesn't set socket no-blocked,
>> but net_socket_listen_init() sets socket no-blocked.
>
> I think adding a common function with blocking/non-blocking flag and
> having inet_listen_opts()/socket_listen_opts() call it with a wrapper
> would be reasonable.

> A lot of code is being introduced here to solve problems that are
> already handled in qemu-sockets.c. inet_listen()/inet_connect()
> already handles backeted-enclosed ipv6 addrs, getting port numbers when
> there's more than one colon, getaddrinfo()-based connections, and most
> importantly it's had ipv6 support from day 1.

> Not 100% sure it'll work for what you're doing, but qemu-sockets.c was
> specifically added for this type of use-case and is heavilly used
> currently (vnc, nbd, Chardev users), so I think we should use it unless
> there's a good reason not to.

There are many special request for migration, which is not implemented in
inet_listen_opts()/socket_listen_opts(), but many codes can be reused,
I would re-write patches.

Thanks, Amos

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Orit Wasserman]

On 03/07/2012 12:47 AM, Amos Kong wrote:
> Introduce tcp_server_start() by moving original code in
> tcp_start_incoming_migration().
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net.c         |   28 ++++++++++++++++++++++++++++
>  qemu_socket.h |    2 ++
>  2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/net.c b/net.c
> index c34474f..e90ff23 100644
> --- a/net.c
> +++ b/net.c
> @@ -99,6 +99,34 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>      return 0;
>  }
>  
> +int tcp_server_start(const char *str, int *fd)
> +{
> +    int val, ret;
> +    struct sockaddr_in saddr;
> +
> +    if (parse_host_port(&saddr, str) < 0) {
> +        error_report("invalid host/port combination: %s", str);
> +        return -EINVAL;
> +    }
> +
> +    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> +    if (fd < 0) {
> +        perror("socket");

> +        return -1;

return -socket_error();

> +    }
> +    socket_set_nonblock(*fd);

In incoming migration we don't use non-blocking socket.
How about a flag to the function for non-blocking ?

> +
> +    /* allow fast reuse */
> +    val = 1;
> +    setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
> +
> +    ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +    if (ret < 0) {
> +        closesocket(*fd);
> +    }
> +    return ret;

if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)) < 0)
{	
    closesocket(*fd);
    return -socket_error();
}
return 0;

and than you will not need ret

> +}
> +
>  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..d612793 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -54,6 +54,8 @@ int unix_listen(const char *path, char *ostr, int olen);
>  int unix_connect_opts(QemuOpts *opts);
>  int unix_connect(const char *path);
>  
> +int tcp_server_start(const char *str, int *fd);
> +
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
>  int socket_init(void);
> 
> 

Orit

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Paolo Bonzini]

Il 14/03/2012 08:14, Orit Wasserman ha scritto:
> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)) < 0)
> {	
>     closesocket(*fd);
>     return -socket_error();
> }
> return 0;
> 
> and than you will not need ret

But closesocket could clobber socket_error(), no?

Paolo

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Paolo Bonzini]

Il 14/03/2012 08:14, Orit Wasserman ha scritto:
> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr)) < 0)
> {	
>     closesocket(*fd);
>     return -socket_error();
> }
> return 0;
> 
> and than you will not need ret

But closesocket could clobber socket_error(), no?

Paolo

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

On 14/03/12 15:27, Paolo Bonzini wrote:
>

Hi Paolo,

> Il 14/03/2012 08:14, Orit Wasserman ha scritto:
>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))<  0)
>> {	
>>      closesocket(*fd);
>>      return -socket_error();
>> }
>> return 0;
>>
>> and than you will not need ret
>
> But closesocket could clobber socket_error(), no?

Yes, it will effect socket_error()

How about this fix ?

     ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
         ret = -socket_error();
         closesocket(*fd);
     }
     return ret;
}

-- 
			Amos.

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Amos Kong]

On 14/03/12 15:27, Paolo Bonzini wrote:
>

Hi Paolo,

> Il 14/03/2012 08:14, Orit Wasserman ha scritto:
>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))<  0)
>> {	
>>      closesocket(*fd);
>>      return -socket_error();
>> }
>> return 0;
>>
>> and than you will not need ret
>
> But closesocket could clobber socket_error(), no?

Yes, it will effect socket_error()

How about this fix ?

     ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
         ret = -socket_error();
         closesocket(*fd);
     }
     return ret;
}

-- 
			Amos.

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Paolo Bonzini]

Il 14/03/2012 08:51, Amos Kong ha scritto:
> 
> 
>     ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>     if (ret < 0) {
>         ret = -socket_error();
>         closesocket(*fd);
>     }
>     return ret;
> }

Looks good.

Paolo

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Paolo Bonzini]

Il 14/03/2012 08:51, Amos Kong ha scritto:
> 
> 
>     ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>     if (ret < 0) {
>         ret = -socket_error();
>         closesocket(*fd);
>     }
>     return ret;
> }

Looks good.

Paolo

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Orit Wasserman]

On 03/14/2012 09:51 AM, Amos Kong wrote:
> On 14/03/12 15:27, Paolo Bonzini wrote:
>>
> 
> Hi Paolo,
> 
>> Il 14/03/2012 08:14, Orit Wasserman ha scritto:
>>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))<  0)
>>> {   
>>>      closesocket(*fd);
>>>      return -socket_error();
>>> }
>>> return 0;
>>>
>>> and than you will not need ret
>>
>> But closesocket could clobber socket_error(), no?

Completely correct.

> 
> Yes, it will effect socket_error()
> 
> How about this fix ?
> 
>     ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>     if (ret < 0) {
>         ret = -socket_error();
>         closesocket(*fd);
>     }
>     return ret;
> }
> 

Looks good.

Orit

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Orit Wasserman]

On 03/14/2012 09:51 AM, Amos Kong wrote:
> On 14/03/12 15:27, Paolo Bonzini wrote:
>>
> 
> Hi Paolo,
> 
>> Il 14/03/2012 08:14, Orit Wasserman ha scritto:
>>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))<  0)
>>> {   
>>>      closesocket(*fd);
>>>      return -socket_error();
>>> }
>>> return 0;
>>>
>>> and than you will not need ret
>>
>> But closesocket could clobber socket_error(), no?

Completely correct.

> 
> Yes, it will effect socket_error()
> 
> How about this fix ?
> 
>     ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>     if (ret < 0) {
>         ret = -socket_error();
>         closesocket(*fd);
>     }
>     return ret;
> }
> 

Looks good.

Orit

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Kevin Wolf]

Am 14.03.2012 08:51, schrieb Amos Kong:
> On 14/03/12 15:27, Paolo Bonzini wrote:
>>
> 
> Hi Paolo,
> 
>> Il 14/03/2012 08:14, Orit Wasserman ha scritto:
>>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))<  0)
>>> {	
>>>      closesocket(*fd);
>>>      return -socket_error();
>>> }
>>> return 0;
>>>
>>> and than you will not need ret
>>
>> But closesocket could clobber socket_error(), no?
> 
> Yes, it will effect socket_error()
> 
> How about this fix ?
> 
>      ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>      if (ret < 0) {
>          ret = -socket_error();
>          closesocket(*fd);
>      }
>      return ret;
> }
> 

But it's still moved (or in this patch copied) code, right?

If so, please move it in one patch, and then fix it in another one on top.

Kevin

Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()

[Kevin Wolf]

Am 14.03.2012 08:51, schrieb Amos Kong:
> On 14/03/12 15:27, Paolo Bonzini wrote:
>>
> 
> Hi Paolo,
> 
>> Il 14/03/2012 08:14, Orit Wasserman ha scritto:
>>> if (bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr))<  0)
>>> {	
>>>      closesocket(*fd);
>>>      return -socket_error();
>>> }
>>> return 0;
>>>
>>> and than you will not need ret
>>
>> But closesocket could clobber socket_error(), no?
> 
> Yes, it will effect socket_error()
> 
> How about this fix ?
> 
>      ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>      if (ret < 0) {
>          ret = -socket_error();
>          closesocket(*fd);
>      }
>      return ret;
> }
> 

But it's still moved (or in this patch copied) code, right?

If so, please move it in one patch, and then fix it in another one on top.

Kevin

[PATCH v3 2/9] net: use tcp_server_start() for tcp server creation

[Amos Kong]

Use tcp_server_start in those two functions:
 tcp_start_incoming_migration()
 net_socket_listen_init()

Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   21 +++++----------------
 net/socket.c    |   23 +++--------------------
 2 files changed, 8 insertions(+), 36 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 35a5781..ecadd10 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -157,28 +157,17 @@ 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) {
+        fprintf(stderr, "tcp_server_start: %s\n", strerror(-ret));
+        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/socket.c b/net/socket.c
index 0bcf229..5feb3d2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -403,31 +403,14 @@ 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");
+        error_report("tcp_server_start: %s", strerror(-ret));
         g_free(s);
-        closesocket(fd);
         return -1;
     }
     ret = listen(fd, 0);

[Qemu-devel] [PATCH v3 2/9] net: use tcp_server_start() for tcp server creation

[Amos Kong]

Use tcp_server_start in those two functions:
 tcp_start_incoming_migration()
 net_socket_listen_init()

Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   21 +++++----------------
 net/socket.c    |   23 +++--------------------
 2 files changed, 8 insertions(+), 36 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 35a5781..ecadd10 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -157,28 +157,17 @@ 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) {
+        fprintf(stderr, "tcp_server_start: %s\n", strerror(-ret));
+        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/socket.c b/net/socket.c
index 0bcf229..5feb3d2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -403,31 +403,14 @@ 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");
+        error_report("tcp_server_start: %s", strerror(-ret));
         g_free(s);
-        closesocket(fd);
         return -1;
     }
     ret = listen(fd, 0);

[PATCH v3 3/9] net: introduce tcp_client_start()

[Amos Kong]

Introduce tcp_client_start() by moving original code in
tcp_start_outgoing_migration().

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

diff --git a/net.c b/net.c
index e90ff23..9afb0d1 100644
--- a/net.c
+++ b/net.c
@@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd)
     return ret;
 }
 
+int tcp_client_start(const char *str, int *fd)
+{
+    struct sockaddr_in saddr;
+    int ret;
+
+    *fd = -1;
+    if (parse_host_port(&saddr, str) < 0) {
+        error_report("invalid host/port combination: %s", str);
+        return -EINVAL;
+    }
+
+    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
+    if (fd < 0) {
+        perror("socket");
+        return -1;
+    }
+    socket_set_nonblock(*fd);
+
+    for (;;) {
+        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
+        if (ret < 0) {
+            ret = -socket_error();
+            if (ret == -EINPROGRESS) {
+                break;
+#ifdef _WIN32
+            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+                break;
+#endif
+            } else if (ret != -EINTR && ret != -EWOULDBLOCK) {
+                perror("connect");
+                closesocket(*fd);
+                return ret;
+            }
+        } else {
+            break;
+        }
+    }
+
+    return ret;
+}
+
 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 d612793..9246578 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -55,6 +55,7 @@ int unix_connect_opts(QemuOpts *opts);
 int unix_connect(const char *path);
 
 int tcp_server_start(const char *str, int *fd);
+int tcp_client_start(const char *str, int *fd);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);

[Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()

[Amos Kong]

Introduce tcp_client_start() by moving original code in
tcp_start_outgoing_migration().

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

diff --git a/net.c b/net.c
index e90ff23..9afb0d1 100644
--- a/net.c
+++ b/net.c
@@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd)
     return ret;
 }
 
+int tcp_client_start(const char *str, int *fd)
+{
+    struct sockaddr_in saddr;
+    int ret;
+
+    *fd = -1;
+    if (parse_host_port(&saddr, str) < 0) {
+        error_report("invalid host/port combination: %s", str);
+        return -EINVAL;
+    }
+
+    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
+    if (fd < 0) {
+        perror("socket");
+        return -1;
+    }
+    socket_set_nonblock(*fd);
+
+    for (;;) {
+        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
+        if (ret < 0) {
+            ret = -socket_error();
+            if (ret == -EINPROGRESS) {
+                break;
+#ifdef _WIN32
+            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+                break;
+#endif
+            } else if (ret != -EINTR && ret != -EWOULDBLOCK) {
+                perror("connect");
+                closesocket(*fd);
+                return ret;
+            }
+        } else {
+            break;
+        }
+    }
+
+    return ret;
+}
+
 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 d612793..9246578 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -55,6 +55,7 @@ int unix_connect_opts(QemuOpts *opts);
 int unix_connect(const char *path);
 
 int tcp_server_start(const char *str, int *fd);
+int tcp_client_start(const char *str, int *fd);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);

Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()

[Michael Roth]

On Wed, Mar 07, 2012 at 06:48:03AM +0800, Amos Kong wrote:
> Introduce tcp_client_start() by moving original code in
> tcp_start_outgoing_migration().
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net.c         |   41 +++++++++++++++++++++++++++++++++++++++++
>  qemu_socket.h |    1 +
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/net.c b/net.c
> index e90ff23..9afb0d1 100644
> --- a/net.c
> +++ b/net.c
> @@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd)
>      return ret;
>  }
> 
> +int tcp_client_start(const char *str, int *fd)
> +{
> +    struct sockaddr_in saddr;
> +    int ret;
> +
> +    *fd = -1;
> +    if (parse_host_port(&saddr, str) < 0) {
> +        error_report("invalid host/port combination: %s", str);
> +        return -EINVAL;
> +    }
> +
> +    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> +    if (fd < 0) {
> +        perror("socket");
> +        return -1;
> +    }
> +    socket_set_nonblock(*fd);
> +
> +    for (;;) {
> +        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +        if (ret < 0) {
> +            ret = -socket_error();
> +            if (ret == -EINPROGRESS) {
> +                break;

The previous implementation and your next patch seem to be expecting a break on
-EWOULDBLOCK/-EAGAIN as well. Was the behavior changed on purpose?  I'm not
sure what the proper handling is for -EAGAIN: whether a non-blocking connect()
can eventually succeed or not. I suspect that it's not, but that previously we
treated it synonymously with -EINPROGRESS, then eventually got an error via
getsockopt() before failing the migration. If so, we're now changing the
behavior to retry until successful, but given the man page entry I don't
think that's a good idea since you might block indefinitely:

 EAGAIN No  more  free local ports or insufficient
              entries in the routing cache.  For AF_INET
              see        the        description       of
              /proc/sys/net/ipv4/ip_local_port_range
              ip(7)  for  information on how to increase
              the number of local ports.

> +#ifdef _WIN32
> +            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
> +                break;
> +#endif
> +            } else if (ret != -EINTR && ret != -EWOULDBLOCK) {
> +                perror("connect");
> +                closesocket(*fd);
> +                return ret;
> +            }
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  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 d612793..9246578 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -55,6 +55,7 @@ int unix_connect_opts(QemuOpts *opts);
>  int unix_connect(const char *path);
> 
>  int tcp_server_start(const char *str, int *fd);
> +int tcp_client_start(const char *str, int *fd);
> 
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
> 
> 

Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()

[Amos Kong]

On 14/03/12 02:35, Michael Roth wrote:
> On Wed, Mar 07, 2012 at 06:48:03AM +0800, Amos Kong wrote:
>> Introduce tcp_client_start() by moving original code in
>> tcp_start_outgoing_migration().
>>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   net.c         |   41 +++++++++++++++++++++++++++++++++++++++++
>>   qemu_socket.h |    1 +
>>   2 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index e90ff23..9afb0d1 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd)
>>       return ret;
>>   }
>>
>> +int tcp_client_start(const char *str, int *fd)
>> +{

...

Hi Michael,


>> +    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>> +    if (fd<  0) {
>> +        perror("socket");
>> +        return -1;
>> +    }
>> +    socket_set_nonblock(*fd);
>> +
>> +    for (;;) {
>> +        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
>> +        if (ret<  0) {
>> +            ret = -socket_error();
>> +            if (ret == -EINPROGRESS) {
>> +                break;
>
> The previous implementation and your next patch seem to be expecting a break on
> -EWOULDBLOCK/-EAGAIN as well. Was the behavior changed on purpose?

In original tcp_start_outgoing_migration():
   break:  -EINPROGRES
   cont :  -EINTR or -EWOULDBLOCK

In original net_socket_connect_init():
   break:  -EINPROGRES or -EWOULDBLOCK
   cont :  -EINTR


http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_15.html
EWOULDBLOCK
     socket has nonblocking mode set, and there are no pending 
connections immediately available.

So continue to re-connect if EWOULDBLOCK or EINTR returned by 
socket_error() in tcp_client_start()


>   I'm not
> sure what the proper handling is for -EAGAIN: whether a non-blocking connect()
> can eventually succeed or not. I suspect that it's not, but that previously we
> treated it synonymously with -EINPROGRESS, then eventually got an error via
> getsockopt() before failing the migration. If so, we're now changing the
> behavior to retry until successful, but given the man page entry I don't
> think that's a good idea since you might block indefinitely:
>
>   EAGAIN No  more  free local ports or insufficient
>                entries in the routing cache.  For AF_INET
>                see        the        description       of
>                /proc/sys/net/ipv4/ip_local_port_range
>                ip(7)  for  information on how to increase
>                the number of local ports.


We didn't process EAGAIN specially, you mean EINTR ?


>
>> +#ifdef _WIN32
>> +            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
>> +                break;
>> +#endif
>> +            } else if (ret != -EINTR&&  ret != -EWOULDBLOCK) {
>> +                perror("connect");
>> +                closesocket(*fd);
>> +                return ret;

-EAGAIN would go this path.


>> +            }
>> +        } else {
>> +            break;
>> +        }
>> +    }
>> +
>> +    return ret;
>> +}
>> +

-- 
			Amos.

Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()

[Michael Roth]

On Wed, Mar 14, 2012 at 06:19:48PM +0800, Amos Kong wrote:
> On 14/03/12 02:35, Michael Roth wrote:
> >On Wed, Mar 07, 2012 at 06:48:03AM +0800, Amos Kong wrote:
> >>Introduce tcp_client_start() by moving original code in
> >>tcp_start_outgoing_migration().
> >>
> >>Signed-off-by: Amos Kong<akong@redhat.com>
> >>---
> >>  net.c         |   41 +++++++++++++++++++++++++++++++++++++++++
> >>  qemu_socket.h |    1 +
> >>  2 files changed, 42 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/net.c b/net.c
> >>index e90ff23..9afb0d1 100644
> >>--- a/net.c
> >>+++ b/net.c
> >>@@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd)
> >>      return ret;
> >>  }
> >>
> >>+int tcp_client_start(const char *str, int *fd)
> >>+{
> 
> ...
> 
> Hi Michael,
> 
> 
> >>+    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> >>+    if (fd<  0) {
> >>+        perror("socket");
> >>+        return -1;
> >>+    }
> >>+    socket_set_nonblock(*fd);
> >>+
> >>+    for (;;) {
> >>+        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
> >>+        if (ret<  0) {
> >>+            ret = -socket_error();
> >>+            if (ret == -EINPROGRESS) {
> >>+                break;
> >
> >The previous implementation and your next patch seem to be expecting a break on
> >-EWOULDBLOCK/-EAGAIN as well. Was the behavior changed on purpose?
> 
> In original tcp_start_outgoing_migration():
>   break:  -EINPROGRES
>   cont :  -EINTR or -EWOULDBLOCK
> 
> In original net_socket_connect_init():
>   break:  -EINPROGRES or -EWOULDBLOCK
>   cont :  -EINTR
> 
> 
> http://www.cs.utah.edu/dept/old/texinfo/glibc-manual-0.02/library_15.html
> EWOULDBLOCK
>     socket has nonblocking mode set, and there are no pending
> connections immediately available.
> 
> So continue to re-connect if EWOULDBLOCK or EINTR returned by
> socket_error() in tcp_client_start()
> 

That seems to be for accept(), not connect(). And in the accept()/incoming
case I don't think it's an issue to keep retrying.

On the connect()/outgoing case I think we need to be careful because we can
hang both the monitor and the guest indefinitely if there's an issue affecting
outgoing connection attempts on the source-side. It's much safer to fail in
this situation rather than loop indefinitely, and originally that's what the
code did, albeit somewhat indirectly. That behavior is changed with your
implementation:

tcp_start_outgoing_migration() originally:
    ...
    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);
    ...

tcp_start_output_migration() with your changes:
    ...
    ret = tcp_client_start(host_port, &s->fd);
    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
        DPRINTF("connect in progress");
        qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);

tcp_client_start():

    static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
    {
        int ret;
    
        do {
            ret = connect(fd, (struct sockaddr *)saddr, sizeof(*saddr));
            if (ret == -1) {
                ret = -socket_error();
            }
        } while (ret == -EINTR || ret == -EWOULDBLOCK);

> 
> >  I'm not
> >sure what the proper handling is for -EAGAIN: whether a non-blocking connect()
> >can eventually succeed or not. I suspect that it's not, but that previously we
> >treated it synonymously with -EINPROGRESS, then eventually got an error via
> >getsockopt() before failing the migration. If so, we're now changing the
> >behavior to retry until successful, but given the man page entry I don't
> >think that's a good idea since you might block indefinitely:
> >
> >  EAGAIN No  more  free local ports or insufficient
> >               entries in the routing cache.  For AF_INET
> >               see        the        description       of
> >               /proc/sys/net/ipv4/ip_local_port_range
> >               ip(7)  for  information on how to increase
> >               the number of local ports.
> 
> 
> We didn't process EAGAIN specially, you mean EINTR ?

I was referring to the EWOULDBLOCK handling, but on linux at least,
EAGAIN == EWOULDBLOCK.

> 
> 
> >
> >>+#ifdef _WIN32
> >>+            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
> >>+                break;
> >>+#endif
> >>+            } else if (ret != -EINTR&&  ret != -EWOULDBLOCK) {
> >>+                perror("connect");
> >>+                closesocket(*fd);
> >>+                return ret;
> 
> -EAGAIN would go this path.

When EAGAIN == EWOULDBLOCK, it would loop, and I'm not aware of any hosts where
this won't be the case. BSD maybe?

> 
> 
> >>+            }
> >>+        } else {
> >>+            break;
> >>+        }
> >>+    }
> >>+
> >>+    return ret;
> >>+}
> >>+
> 
> -- 
> 			Amos.
> 

Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()

[Orit Wasserman]

On 03/07/2012 12:48 AM, Amos Kong wrote:
> Introduce tcp_client_start() by moving original code in
> tcp_start_outgoing_migration().
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  net.c         |   41 +++++++++++++++++++++++++++++++++++++++++
>  qemu_socket.h |    1 +
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/net.c b/net.c
> index e90ff23..9afb0d1 100644
> --- a/net.c
> +++ b/net.c
> @@ -127,6 +127,47 @@ int tcp_server_start(const char *str, int *fd)
>      return ret;
>  }
>  
> +int tcp_client_start(const char *str, int *fd)
> +{
> +    struct sockaddr_in saddr;
> +    int ret;
> +
> +    *fd = -1;
> +    if (parse_host_port(&saddr, str) < 0) {
> +        error_report("invalid host/port combination: %s", str);
> +        return -EINVAL;
> +    }
> +
> +    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> +    if (fd < 0) {
> +        perror("socket");
> +        return -1;

return -socket_error();

> +    }
> +    socket_set_nonblock(*fd);
> +
> +    for (;;) {
> +        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +        if (ret < 0) {
> +            ret = -socket_error();
> +            if (ret == -EINPROGRESS) {
> +                break;
> +#ifdef _WIN32
> +            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
> +                break;
> +#endif
> +            } else if (ret != -EINTR && ret != -EWOULDBLOCK) {
> +                perror("connect");
> +                closesocket(*fd);
> +                return ret;
> +            }
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  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 d612793..9246578 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -55,6 +55,7 @@ int unix_connect_opts(QemuOpts *opts);
>  int unix_connect(const char *path);
>  
>  int tcp_server_start(const char *str, int *fd);
> +int tcp_client_start(const char *str, int *fd);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
> 
> 

Orit

[PATCH v3 4/9] net: use tcp_client_start for tcp client creation

[Amos Kong]

Use tcp_client_start() in those two functions:
 tcp_start_outgoing_migration()
 net_socket_connect_init()

Changes from v2:
- return real error in net_socket_connect_init(), not always -1

Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   39 +++++++++++----------------------------
 net/socket.c    |   43 ++++++++++++-------------------------------
 2 files changed, 23 insertions(+), 59 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index ecadd10..b74be1c 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -81,43 +81,26 @@ 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;
-    }
-
     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, &s->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 (s->fd != -1) {
+            migrate_fd_error(s);
+        }
         return ret;
+    } else {
+        migrate_fd_connect(s);
     }
-    migrate_fd_connect(s);
+
     return 0;
 }
 
diff --git a/net/socket.c b/net/socket.c
index 5feb3d2..0ecbc84 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -434,41 +434,22 @@ 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");
-        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;
+    ret = tcp_client_start(host_str, &fd);
+    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        connected = 0;
 #ifdef _WIN32
-            } else if (err == WSAEALREADY || err == WSAEINVAL) {
-                break;
+    } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+        connected = 0;
 #endif
-            } else {
-                perror("connect");
-                closesocket(fd);
-                return -1;
-            }
-        } else {
-            connected = 1;
-            break;
-        }
+    } else if (ret < 0) {
+        return ret;
+    } else {
+        connected = 1;
     }
+
     s = net_socket_fd_init(vlan, model, name, fd, connected);
     if (!s)
         return -1;
@@ -621,7 +602,7 @@ int net_init_socket(QemuOpts *opts,
 
         connect = qemu_opt_get(opts, "connect");
 
-        if (net_socket_connect_init(vlan, "socket", name, connect) == -1) {
+        if (net_socket_connect_init(vlan, "socket", name, connect) < 0) {
             return -1;
         }
     } else if (qemu_opt_get(opts, "mcast")) {

[Qemu-devel] [PATCH v3 4/9] net: use tcp_client_start for tcp client creation

[Amos Kong]

Use tcp_client_start() in those two functions:
 tcp_start_outgoing_migration()
 net_socket_connect_init()

Changes from v2:
- return real error in net_socket_connect_init(), not always -1

Signed-off-by: Amos Kong <akong@redhat.com>
---
 migration-tcp.c |   39 +++++++++++----------------------------
 net/socket.c    |   43 ++++++++++++-------------------------------
 2 files changed, 23 insertions(+), 59 deletions(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index ecadd10..b74be1c 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -81,43 +81,26 @@ 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;
-    }
-
     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, &s->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 (s->fd != -1) {
+            migrate_fd_error(s);
+        }
         return ret;
+    } else {
+        migrate_fd_connect(s);
     }
-    migrate_fd_connect(s);
+
     return 0;
 }
 
diff --git a/net/socket.c b/net/socket.c
index 5feb3d2..0ecbc84 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -434,41 +434,22 @@ 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");
-        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;
+    ret = tcp_client_start(host_str, &fd);
+    if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        connected = 0;
 #ifdef _WIN32
-            } else if (err == WSAEALREADY || err == WSAEINVAL) {
-                break;
+    } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+        connected = 0;
 #endif
-            } else {
-                perror("connect");
-                closesocket(fd);
-                return -1;
-            }
-        } else {
-            connected = 1;
-            break;
-        }
+    } else if (ret < 0) {
+        return ret;
+    } else {
+        connected = 1;
     }
+
     s = net_socket_fd_init(vlan, model, name, fd, connected);
     if (!s)
         return -1;
@@ -621,7 +602,7 @@ int net_init_socket(QemuOpts *opts,
 
         connect = qemu_opt_get(opts, "connect");
 
-        if (net_socket_connect_init(vlan, "socket", name, connect) == -1) {
+        if (net_socket_connect_init(vlan, "socket", name, connect) < 0) {
             return -1;
         }
     } else if (qemu_opt_get(opts, "mcast")) {

[PATCH v3 5/9] net: refector tcp_*_start functions

[Amos Kong]

There are some repeated code for tcp_server_start()
and tcp_client_start().

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

diff --git a/net.c b/net.c
index 9afb0d1..b05c881 100644
--- a/net.c
+++ b/net.c
@@ -99,38 +99,41 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 0;
 }
 
-int tcp_server_start(const char *str, int *fd)
+static int tcp_server_bind(int fd, struct sockaddr_in *saddr)
 {
-    int val, ret;
-    struct sockaddr_in saddr;
+    int ret;
+    int val = 1;
 
-    if (parse_host_port(&saddr, str) < 0) {
-        error_report("invalid host/port combination: %s", str);
-        return -EINVAL;
-    }
+    /* allow fast reuse */
+    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
 
-    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
-        return -1;
+    ret = bind(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+
+    if (ret == -1) {
+        ret = -socket_error();
     }
-    socket_set_nonblock(*fd);
+    return ret;
 
-    /* allow fast reuse */
-    val = 1;
-    setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
+}
+
+static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
+{
+    int ret;
+
+    do {
+        ret = connect(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+        if (ret == -1) {
+            ret = -socket_error();
+        }
+    } while (ret == -EINTR || ret == -EWOULDBLOCK);
 
-    ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
-    if (ret < 0) {
-        closesocket(*fd);
-    }
     return ret;
 }
 
-int tcp_client_start(const char *str, int *fd)
+static int tcp_start_common(const char *str, int *fd, bool server)
 {
+    int ret = -EINVAL;
     struct sockaddr_in saddr;
-    int ret;
 
     *fd = -1;
     if (parse_host_port(&saddr, str) < 0) {
@@ -145,29 +148,35 @@ int tcp_client_start(const char *str, int *fd)
     }
     socket_set_nonblock(*fd);
 
-    for (;;) {
-        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
-        if (ret < 0) {
-            ret = -socket_error();
-            if (ret == -EINPROGRESS) {
-                break;
+    if (server) {
+        ret = tcp_server_bind(*fd, &saddr);
+    } else {
+        ret = tcp_client_connect(*fd, &saddr);
+    }
+
 #ifdef _WIN32
-            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
-                break;
+    if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+        return ret;                  /* Success */
+    }
 #endif
-            } else if (ret != -EINTR && ret != -EWOULDBLOCK) {
-                perror("connect");
-                closesocket(*fd);
-                return ret;
-            }
-        } else {
-            break;
-        }
+    if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        return ret;                  /* Success */
     }
 
+    closesocket(*fd);
     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];

[Qemu-devel] [PATCH v3 5/9] net: refector tcp_*_start functions

[Amos Kong]

There are some repeated code for tcp_server_start()
and tcp_client_start().

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

diff --git a/net.c b/net.c
index 9afb0d1..b05c881 100644
--- a/net.c
+++ b/net.c
@@ -99,38 +99,41 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 0;
 }
 
-int tcp_server_start(const char *str, int *fd)
+static int tcp_server_bind(int fd, struct sockaddr_in *saddr)
 {
-    int val, ret;
-    struct sockaddr_in saddr;
+    int ret;
+    int val = 1;
 
-    if (parse_host_port(&saddr, str) < 0) {
-        error_report("invalid host/port combination: %s", str);
-        return -EINVAL;
-    }
+    /* allow fast reuse */
+    setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
 
-    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
-        return -1;
+    ret = bind(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+
+    if (ret == -1) {
+        ret = -socket_error();
     }
-    socket_set_nonblock(*fd);
+    return ret;
 
-    /* allow fast reuse */
-    val = 1;
-    setsockopt(*fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
+}
+
+static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
+{
+    int ret;
+
+    do {
+        ret = connect(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+        if (ret == -1) {
+            ret = -socket_error();
+        }
+    } while (ret == -EINTR || ret == -EWOULDBLOCK);
 
-    ret = bind(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
-    if (ret < 0) {
-        closesocket(*fd);
-    }
     return ret;
 }
 
-int tcp_client_start(const char *str, int *fd)
+static int tcp_start_common(const char *str, int *fd, bool server)
 {
+    int ret = -EINVAL;
     struct sockaddr_in saddr;
-    int ret;
 
     *fd = -1;
     if (parse_host_port(&saddr, str) < 0) {
@@ -145,29 +148,35 @@ int tcp_client_start(const char *str, int *fd)
     }
     socket_set_nonblock(*fd);
 
-    for (;;) {
-        ret = connect(*fd, (struct sockaddr *)&saddr, sizeof(saddr));
-        if (ret < 0) {
-            ret = -socket_error();
-            if (ret == -EINPROGRESS) {
-                break;
+    if (server) {
+        ret = tcp_server_bind(*fd, &saddr);
+    } else {
+        ret = tcp_client_connect(*fd, &saddr);
+    }
+
 #ifdef _WIN32
-            } else if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
-                break;
+    if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+        return ret;                  /* Success */
+    }
 #endif
-            } else if (ret != -EINTR && ret != -EWOULDBLOCK) {
-                perror("connect");
-                closesocket(*fd);
-                return ret;
-            }
-        } else {
-            break;
-        }
+    if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+        return ret;                  /* Success */
     }
 
+    closesocket(*fd);
     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];

[PATCH v3 6/9] net: use getaddrinfo() in tcp_start_common

[Amos Kong]

Migrating with IPv6 address exists problem, gethostbyname()/inet_aton()
could not translate IPv6 address/port simply, so use getaddrinfo()
in tcp_start_common to translate network address and service.
We can get an address list by getaddrinfo().

Userlevel IPv6 Programming Introduction:
http://www.akkadia.org/drepper/userapi-ipv6.html

Reference RFC 3493, Basic Socket Interface Extensions for IPv6

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

diff --git a/net.c b/net.c
index b05c881..de1db8c 100644
--- a/net.c
+++ b/net.c
@@ -99,7 +99,7 @@ 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 sockaddr_in *saddr)
+static int tcp_server_bind(int fd, struct addrinfo *rp)
 {
     int ret;
     int val = 1;
@@ -107,7 +107,7 @@ static int tcp_server_bind(int fd, struct sockaddr_in *saddr)
     /* allow fast reuse */
     setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
 
-    ret = bind(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+    ret = bind(fd, rp->ai_addr, rp->ai_addrlen);
 
     if (ret == -1) {
         ret = -socket_error();
@@ -116,12 +116,12 @@ static int tcp_server_bind(int fd, struct sockaddr_in *saddr)
 
 }
 
-static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
+static int tcp_client_connect(int fd, struct addrinfo *rp)
 {
     int ret;
 
     do {
-        ret = connect(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+        ret = connect(fd, rp->ai_addr, rp->ai_addrlen);
         if (ret == -1) {
             ret = -socket_error();
         }
@@ -132,38 +132,75 @@ static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
 
 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;
-    struct sockaddr_in saddr;
 
     *fd = -1;
-    if (parse_host_port(&saddr, str) < 0) {
+    service = str;
+
+    if (get_str_sep(hostname, sizeof(hostname), &service, ':') < 0) {
         error_report("invalid host/port combination: %s", str);
         return -EINVAL;
     }
-
-    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
-        return -1;
+    if (server && strlen(hostname) == 0) {
+        name = NULL;
+    } else {
+        name = hostname;
     }
-    socket_set_nonblock(*fd);
+
+    /* 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) {
-        ret = tcp_server_bind(*fd, &saddr);
-    } else {
-        ret = tcp_client_connect(*fd, &saddr);
+        hints.ai_flags = AI_PASSIVE;
     }
 
-#ifdef _WIN32
-    if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
-        return ret;                  /* Success */
+    s = getaddrinfo(name, service, &hints, &result);
+    if (s != 0) {
+        error_report("qemu: getaddrinfo: %s", 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);
+        }
+#ifdef _WIN32
+        if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+            *fd = sfd;
+            break;                  /* Success */
+        }
 #endif
-    if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
-        return ret;                  /* Success */
+        if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+            *fd = sfd;
+            break;                  /* Success */
+        }
+        closesocket(sfd);
     }
 
-    closesocket(*fd);
+    freeaddrinfo(result);
     return ret;
 }
 

[Qemu-devel] [PATCH v3 6/9] net: use getaddrinfo() in tcp_start_common

[Amos Kong]

Migrating with IPv6 address exists problem, gethostbyname()/inet_aton()
could not translate IPv6 address/port simply, so use getaddrinfo()
in tcp_start_common to translate network address and service.
We can get an address list by getaddrinfo().

Userlevel IPv6 Programming Introduction:
http://www.akkadia.org/drepper/userapi-ipv6.html

Reference RFC 3493, Basic Socket Interface Extensions for IPv6

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

diff --git a/net.c b/net.c
index b05c881..de1db8c 100644
--- a/net.c
+++ b/net.c
@@ -99,7 +99,7 @@ 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 sockaddr_in *saddr)
+static int tcp_server_bind(int fd, struct addrinfo *rp)
 {
     int ret;
     int val = 1;
@@ -107,7 +107,7 @@ static int tcp_server_bind(int fd, struct sockaddr_in *saddr)
     /* allow fast reuse */
     setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
 
-    ret = bind(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+    ret = bind(fd, rp->ai_addr, rp->ai_addrlen);
 
     if (ret == -1) {
         ret = -socket_error();
@@ -116,12 +116,12 @@ static int tcp_server_bind(int fd, struct sockaddr_in *saddr)
 
 }
 
-static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
+static int tcp_client_connect(int fd, struct addrinfo *rp)
 {
     int ret;
 
     do {
-        ret = connect(fd, (struct sockaddr *)saddr, sizeof(*saddr));
+        ret = connect(fd, rp->ai_addr, rp->ai_addrlen);
         if (ret == -1) {
             ret = -socket_error();
         }
@@ -132,38 +132,75 @@ static int tcp_client_connect(int fd, struct sockaddr_in *saddr)
 
 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;
-    struct sockaddr_in saddr;
 
     *fd = -1;
-    if (parse_host_port(&saddr, str) < 0) {
+    service = str;
+
+    if (get_str_sep(hostname, sizeof(hostname), &service, ':') < 0) {
         error_report("invalid host/port combination: %s", str);
         return -EINVAL;
     }
-
-    *fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (fd < 0) {
-        perror("socket");
-        return -1;
+    if (server && strlen(hostname) == 0) {
+        name = NULL;
+    } else {
+        name = hostname;
     }
-    socket_set_nonblock(*fd);
+
+    /* 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) {
-        ret = tcp_server_bind(*fd, &saddr);
-    } else {
-        ret = tcp_client_connect(*fd, &saddr);
+        hints.ai_flags = AI_PASSIVE;
     }
 
-#ifdef _WIN32
-    if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
-        return ret;                  /* Success */
+    s = getaddrinfo(name, service, &hints, &result);
+    if (s != 0) {
+        error_report("qemu: getaddrinfo: %s", 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);
+        }
+#ifdef _WIN32
+        if (ret == -WSAEALREADY || ret == -WSAEINVAL) {
+            *fd = sfd;
+            break;                  /* Success */
+        }
 #endif
-    if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
-        return ret;                  /* Success */
+        if (ret >= 0 || ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
+            *fd = sfd;
+            break;                  /* Success */
+        }
+        closesocket(sfd);
     }
 
-    closesocket(*fd);
+    freeaddrinfo(result);
     return ret;
 }
 

[PATCH v3 7/9] net: introduce parse_host_port_info()

[Amos Kong]

int parse_host_port(struct sockaddr_in *saddr, const char *str)
Parsed address info will be restored into 'saddr', it only support ipv4.
This function is used by net_socket_mcast_init() and net_socket_udp_init().

int parse_host_port_info(struct addrinfo *result, const char *str)
Parsed address info will be restored into 'result', it's an address list.
It can be used to parse IPv6 address/port.

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

diff --git a/net.c b/net.c
index de1db8c..2518e5f 100644
--- a/net.c
+++ b/net.c
@@ -130,18 +130,15 @@ static int tcp_client_connect(int fd, struct addrinfo *rp)
     return ret;
 }
 
-static int tcp_start_common(const char *str, int *fd, bool server)
+static int parse_host_port_info(struct addrinfo **result, const char *str,
+                                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) {
@@ -164,12 +161,29 @@ static int tcp_start_common(const char *str, int *fd, bool server)
         hints.ai_flags = AI_PASSIVE;
     }
 
-    s = getaddrinfo(name, service, &hints, &result);
+    s = getaddrinfo(name, service, &hints, result);
     if (s != 0) {
         error_report("qemu: getaddrinfo: %s", gai_strerror(s));
         return -EINVAL;
     }
 
+    return 0;
+}
+
+static int tcp_start_common(const char *str, int *fd, bool server)
+{
+    struct addrinfo *rp;
+    int sfd;
+    int ret = -EINVAL;
+    struct addrinfo *result;
+
+    *fd = -1;
+
+    ret = parse_host_port_info(&result, str, server);
+    if (ret < 0) {
+        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

[Qemu-devel] [PATCH v3 7/9] net: introduce parse_host_port_info()

[Amos Kong]

int parse_host_port(struct sockaddr_in *saddr, const char *str)
Parsed address info will be restored into 'saddr', it only support ipv4.
This function is used by net_socket_mcast_init() and net_socket_udp_init().

int parse_host_port_info(struct addrinfo *result, const char *str)
Parsed address info will be restored into 'result', it's an address list.
It can be used to parse IPv6 address/port.

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

diff --git a/net.c b/net.c
index de1db8c..2518e5f 100644
--- a/net.c
+++ b/net.c
@@ -130,18 +130,15 @@ static int tcp_client_connect(int fd, struct addrinfo *rp)
     return ret;
 }
 
-static int tcp_start_common(const char *str, int *fd, bool server)
+static int parse_host_port_info(struct addrinfo **result, const char *str,
+                                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) {
@@ -164,12 +161,29 @@ static int tcp_start_common(const char *str, int *fd, bool server)
         hints.ai_flags = AI_PASSIVE;
     }
 
-    s = getaddrinfo(name, service, &hints, &result);
+    s = getaddrinfo(name, service, &hints, result);
     if (s != 0) {
         error_report("qemu: getaddrinfo: %s", gai_strerror(s));
         return -EINVAL;
     }
 
+    return 0;
+}
+
+static int tcp_start_common(const char *str, int *fd, bool server)
+{
+    struct addrinfo *rp;
+    int sfd;
+    int ret = -EINVAL;
+    struct addrinfo *result;
+
+    *fd = -1;
+
+    ret = parse_host_port_info(&result, str, server);
+    if (ret < 0) {
+        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

[PATCH v3 8/9] 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 2518e5f..d6ce1fa 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;

[Qemu-devel] [PATCH v3 8/9] 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 2518e5f..d6ce1fa 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 v3 8/9] net: split hostname and service by last colon

[Michael Roth]

On Wed, Mar 07, 2012 at 06:48:48AM +0800, Amos Kong wrote:
> 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 2518e5f..d6ce1fa 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);

Some callers expect get_str_sep() to split from the front,
net/slirp.c:net_slirp_hostfwd_remove() for example.

Would add a seperate helper, or replace it with a wrapper around a more
generic implementation.
>      if (!p1)
>          return -1;
>      len = p1 - p;
> 
> 

[PATCH v3 9/9] 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

For IPv6 brackets must be mandatory if you require a port.

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 d6ce1fa..499ed1d 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)

[Qemu-devel] [PATCH v3 9/9] 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

For IPv6 brackets must be mandatory if you require a port.

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 d6ce1fa..499ed1d 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 v3 9/9] net: support to include ipv6 address by brackets

[Michael Roth]

On Wed, Mar 07, 2012 at 06:48:57AM +0800, Amos Kong wrote:
> That method of representing an IPv6 address with a port is

I'm not sure what "that" is referencing. I assumed the previous patch
but the representation seems to be the same?

> discouraged because of its ambiguity. Referencing to RFC5952,
> the recommended format is:
> 
>      [2312::8274]:5200
> 
> For IPv6 brackets must be mandatory if you require a port.
> 
> 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 d6ce1fa..499ed1d 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;
> +    }

Sorry, looking again I guess net/slirp.c actually has it's own copy of
get_str_sep(), so modifying this doesn't look like it would break
anything currently. It might cause some confusion though :). And I think
the special handling for brackets should be done in
parse_host_port_info() since get_str_sep() is pretty generically named.

> +
>      p1++;
>      if (buf_size > 0) {
>          if (len > buf_size - 1)
> 
> 

Re: [PATCH v3 9/9] net: support to include ipv6 address by brackets

[Amos Kong]

On 14/03/12 03:47, Michael Roth wrote:
> On Wed, Mar 07, 2012 at 06:48:57AM +0800, Amos Kong wrote:
>> That method of representing an IPv6 address with a port is
>
> I'm not sure what "that" is referencing.

2312::8274:5200  (5200 is a port)

> I assumed the previous patch
> but the representation seems to be the same?

2312::8274:5200

[2312::8274]:5200

The second one is better.

>> discouraged because of its ambiguity. Referencing to RFC5952,
>> the recommended format is:
>>
>>       [2312::8274]:5200
>>
>> For IPv6 brackets must be mandatory if you require a port.
>>
>> 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 d6ce1fa..499ed1d 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;
>> +    }
>
> Sorry, looking again I guess net/slirp.c actually has it's own copy of
> get_str_sep(), so modifying this doesn't look like it would break
> anything currently.

> It might cause some confusion though :). And I think
> the special handling for brackets should be done in
> parse_host_port_info() since get_str_sep() is pretty generically named.

agree.

>> +
>>       p1++;
>>       if (buf_size>  0) {
>>           if (len>  buf_size - 1)
>>
>>

-- 
			Amos.

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

[Amos Kong]

On 14/03/12 03:47, Michael Roth wrote:
> On Wed, Mar 07, 2012 at 06:48:57AM +0800, Amos Kong wrote:
>> That method of representing an IPv6 address with a port is
>
> I'm not sure what "that" is referencing.

2312::8274:5200  (5200 is a port)

> I assumed the previous patch
> but the representation seems to be the same?

2312::8274:5200

[2312::8274]:5200

The second one is better.

>> discouraged because of its ambiguity. Referencing to RFC5952,
>> the recommended format is:
>>
>>       [2312::8274]:5200
>>
>> For IPv6 brackets must be mandatory if you require a port.
>>
>> 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 d6ce1fa..499ed1d 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;
>> +    }
>
> Sorry, looking again I guess net/slirp.c actually has it's own copy of
> get_str_sep(), so modifying this doesn't look like it would break
> anything currently.

> It might cause some confusion though :). And I think
> the special handling for brackets should be done in
> parse_host_port_info() since get_str_sep() is pretty generically named.

agree.

>> +
>>       p1++;
>>       if (buf_size>  0) {
>>           if (len>  buf_size - 1)
>>
>>

-- 
			Amos.

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

[Michael Roth]

On Wed, Mar 14, 2012 at 05:58:20PM +0800, Amos Kong wrote:
> On 14/03/12 03:47, Michael Roth wrote:
> >On Wed, Mar 07, 2012 at 06:48:57AM +0800, Amos Kong wrote:
> >>That method of representing an IPv6 address with a port is
> >
> >I'm not sure what "that" is referencing.
> 
> 2312::8274:5200  (5200 is a port)
> 
> >I assumed the previous patch
> >but the representation seems to be the same?
> 
> 2312::8274:5200
> 
> [2312::8274]:5200
> 
> The second one is better.

The commit message for the previous patch also uses "[2312::8274]:5200"

If that format wasn't supported till this patch you should fix up the
example in patch 8 to be "2312::8274:5200". Small nit, but it gives the
impression [host]:port was already supported in some form, which is all
the more confusing because [host]:port *is* already supported in some
form, depending on where you are in QEMU :)

> 
> >>discouraged because of its ambiguity. Referencing to RFC5952,
> >>the recommended format is:
> >>
> >>      [2312::8274]:5200
> >>
> >>For IPv6 brackets must be mandatory if you require a port.
> >>
> >>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 d6ce1fa..499ed1d 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;
> >>+    }
> >
> >Sorry, looking again I guess net/slirp.c actually has it's own copy of
> >get_str_sep(), so modifying this doesn't look like it would break
> >anything currently.
> 
> >It might cause some confusion though :). And I think
> >the special handling for brackets should be done in
> >parse_host_port_info() since get_str_sep() is pretty generically named.
> 
> agree.
> 
> >>+
> >>      p1++;
> >>      if (buf_size>  0) {
> >>          if (len>  buf_size - 1)
> >>
> >>
> 
> -- 
> 			Amos.
>