* [Qemu-devel] [PATCH v3 0/9] support to migrate with IPv6 address
@ 2012-03-06 22:47 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:47 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:47 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:47 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-06 22:47 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:47 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
(?)
@ 2012-03-13 16:39 ` Michael Roth
2012-03-14 8:33 ` Amos Kong
-1 siblings, 1 reply; 45+ messages in thread
From: Michael Roth @ 2012-03-13 16:39 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-13 16:39 ` Michael Roth
@ 2012-03-14 8:33 ` Amos Kong
2012-03-14 14:58 ` Michael Roth
0 siblings, 1 reply; 45+ messages in thread
From: Amos Kong @ 2012-03-14 8:33 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 8:33 ` Amos Kong
@ 2012-03-14 14:58 ` Michael Roth
2012-03-16 10:47 ` [Qemu-devel] " Amos Kong
0 siblings, 1 reply; 45+ messages in thread
From: Michael Roth @ 2012-03-14 14:58 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 14:58 ` Michael Roth
@ 2012-03-16 10:47 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-16 10:47 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-16 10:47 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-16 10:47 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
(?)
(?)
@ 2012-03-14 7:14 ` Orit Wasserman
2012-03-14 7:27 ` Paolo Bonzini
-1 siblings, 1 reply; 45+ messages in thread
From: Orit Wasserman @ 2012-03-14 7:14 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 7:14 ` Orit Wasserman
@ 2012-03-14 7:27 ` Paolo Bonzini
0 siblings, 0 replies; 45+ messages in thread
From: Paolo Bonzini @ 2012-03-14 7:27 UTC (permalink / raw)
To: Orit Wasserman
Cc: Amos Kong, aliguori, kvm, quintela, jasowang, qemu-devel, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-14 7:27 ` Paolo Bonzini
0 siblings, 0 replies; 45+ messages in thread
From: Paolo Bonzini @ 2012-03-14 7:27 UTC (permalink / raw)
To: Orit Wasserman
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, laine, Amos Kong
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 7:27 ` Paolo Bonzini
@ 2012-03-14 7:51 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-14 7:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Orit Wasserman, aliguori, kvm, quintela, jasowang, qemu-devel, laine
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-14 7:51 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-14 7:51 UTC (permalink / raw)
To: Paolo Bonzini
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, Orit Wasserman, laine
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 7:51 ` Amos Kong
@ 2012-03-14 8:28 ` Paolo Bonzini
-1 siblings, 0 replies; 45+ messages in thread
From: Paolo Bonzini @ 2012-03-14 8:28 UTC (permalink / raw)
To: Amos Kong
Cc: Orit Wasserman, aliguori, kvm, quintela, jasowang, qemu-devel, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-14 8:28 ` Paolo Bonzini
0 siblings, 0 replies; 45+ messages in thread
From: Paolo Bonzini @ 2012-03-14 8:28 UTC (permalink / raw)
To: Amos Kong
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, Orit Wasserman, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 7:51 ` Amos Kong
@ 2012-03-14 10:03 ` Orit Wasserman
-1 siblings, 0 replies; 45+ messages in thread
From: Orit Wasserman @ 2012-03-14 10:03 UTC (permalink / raw)
To: Amos Kong
Cc: Paolo Bonzini, aliguori, kvm, quintela, jasowang, qemu-devel, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-14 10:03 ` Orit Wasserman
0 siblings, 0 replies; 45+ messages in thread
From: Orit Wasserman @ 2012-03-14 10:03 UTC (permalink / raw)
To: Amos Kong
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, laine, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
2012-03-14 7:51 ` Amos Kong
@ 2012-03-14 11:39 ` Kevin Wolf
-1 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2012-03-14 11:39 UTC (permalink / raw)
To: Amos Kong
Cc: Paolo Bonzini, aliguori, kvm, quintela, jasowang, qemu-devel,
Orit Wasserman, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/9] net: introduce tcp_server_start()
@ 2012-03-14 11:39 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2012-03-14 11:39 UTC (permalink / raw)
To: Amos Kong
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, Orit Wasserman,
laine, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 2/9] net: use tcp_server_start() for tcp server creation
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:47 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:47 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 2/9] net: use tcp_server_start() for tcp server creation
@ 2012-03-06 22:47 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:47 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 3/9] net: introduce tcp_client_start()
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()
2012-03-06 22:48 ` [Qemu-devel] " Amos Kong
(?)
@ 2012-03-13 18:35 ` Michael Roth
2012-03-14 10:19 ` Amos Kong
-1 siblings, 1 reply; 45+ messages in thread
From: Michael Roth @ 2012-03-13 18:35 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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);
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()
2012-03-13 18:35 ` Michael Roth
@ 2012-03-14 10:19 ` Amos Kong
2012-03-14 15:30 ` Michael Roth
0 siblings, 1 reply; 45+ messages in thread
From: Amos Kong @ 2012-03-14 10:19 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()
2012-03-14 10:19 ` Amos Kong
@ 2012-03-14 15:30 ` Michael Roth
0 siblings, 0 replies; 45+ messages in thread
From: Michael Roth @ 2012-03-14 15:30 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/9] net: introduce tcp_client_start()
2012-03-06 22:48 ` [Qemu-devel] " Amos Kong
(?)
(?)
@ 2012-03-14 7:31 ` Orit Wasserman
-1 siblings, 0 replies; 45+ messages in thread
From: Orit Wasserman @ 2012-03-14 7:31 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, laine
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 4/9] net: use tcp_client_start for tcp client creation
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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")) {
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 4/9] net: use tcp_client_start for tcp client creation
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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")) {
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 5/9] net: refector tcp_*_start functions
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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];
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 5/9] net: refector tcp_*_start functions
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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];
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 6/9] net: use getaddrinfo() in tcp_start_common
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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;
}
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 6/9] net: use getaddrinfo() in tcp_start_common
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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;
}
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 7/9] net: introduce parse_host_port_info()
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 7/9] net: introduce parse_host_port_info()
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 8/9] net: split hostname and service by last colon
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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;
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 8/9] net: split hostname and service by last colon
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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;
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 8/9] net: split hostname and service by last colon
2012-03-06 22:48 ` [Qemu-devel] " Amos Kong
(?)
@ 2012-03-13 19:34 ` Michael Roth
-1 siblings, 0 replies; 45+ messages in thread
From: Michael Roth @ 2012-03-13 19:34 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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;
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 9/9] net: support to include ipv6 address by brackets
2012-03-06 22:47 ` [Qemu-devel] " Amos Kong
@ 2012-03-06 22:48 ` Amos Kong
-1 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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)
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH v3 9/9] net: support to include ipv6 address by brackets
@ 2012-03-06 22:48 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-06 22:48 UTC (permalink / raw)
To: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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)
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 9/9] net: support to include ipv6 address by brackets
2012-03-06 22:48 ` [Qemu-devel] " Amos Kong
(?)
@ 2012-03-13 19:47 ` Michael Roth
2012-03-14 9:58 ` [Qemu-devel] " Amos Kong
-1 siblings, 1 reply; 45+ messages in thread
From: Michael Roth @ 2012-03-13 19:47 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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)
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 9/9] net: support to include ipv6 address by brackets
2012-03-13 19:47 ` Michael Roth
@ 2012-03-14 9:58 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-14 9:58 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 9/9] net: support to include ipv6 address by brackets
@ 2012-03-14 9:58 ` Amos Kong
0 siblings, 0 replies; 45+ messages in thread
From: Amos Kong @ 2012-03-14 9:58 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH v3 9/9] net: support to include ipv6 address by brackets
2012-03-14 9:58 ` [Qemu-devel] " Amos Kong
(?)
@ 2012-03-14 15:38 ` Michael Roth
-1 siblings, 0 replies; 45+ messages in thread
From: Michael Roth @ 2012-03-14 15:38 UTC (permalink / raw)
To: Amos Kong; +Cc: aliguori, kvm, quintela, jasowang, qemu-devel, owasserm, laine
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.
>
^ permalink raw reply [flat|nested] 45+ messages in thread