Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 23/09/12 14:34, Orit Wasserman wrote:
On 09/20/2012 06:16 PM, Amos Kong wrote:
- Original Message -
On 09/19/2012 11:31 AM, Amos Kong wrote:
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have
ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a
different
address.
callers on inet_nonblocking_connect register a callback function
that will
be called when connect opertion completes, in case of failure the
fd will have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
I will remove it.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and
errno is set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error() !=
EINTR
If ret 0, just set the error state, but the callback function is
not set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to
checking current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect next
address on the list.
We should reserve that block, and move
qemu_set_fd_handler2(...NULL..) below it.
I'm sorry I don't get you point, it makes no difference where is
qemu_set_fd_handlers(...NULL...).
rc 0 happens in two cases:
1. val != 0 - connect error
It seems rc is 0 in this case.
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new socket
fd.
In the past, if rc 0, we will return without setting fd handler to NULL,
tcp_wait_for_connect() will be re-called. the original socket (current addr)
will be re-checked by getsockopt()
But in your patch, it will retry next addr in the list.
In the past we didn't have the address list so we could not continue to the
next one,
now we can.
Yes, we should try to connect next addr if current address is not available.
But rc 0 doesn't mean current is not available.
rc 0 means fail to get socket option, we need to retry to _get_
socket option.
--
Amos.
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/24/2012 11:48 AM, Amos Kong wrote:
On 23/09/12 14:34, Orit Wasserman wrote:
On 09/20/2012 06:16 PM, Amos Kong wrote:
- Original Message -
On 09/19/2012 11:31 AM, Amos Kong wrote:
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have
ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a
different
address.
callers on inet_nonblocking_connect register a callback function
that will
be called when connect opertion completes, in case of failure the
fd will have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
I will remove it.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and
errno is set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error() !=
EINTR
If ret 0, just set the error state, but the callback function is
not set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to
checking current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect next
address on the list.
We should reserve that block, and move
qemu_set_fd_handler2(...NULL..) below it.
I'm sorry I don't get you point, it makes no difference where is
qemu_set_fd_handlers(...NULL...).
rc 0 happens in two cases:
1. val != 0 - connect error
It seems rc is 0 in this case.
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new socket
fd.
In the past, if rc 0, we will return without setting fd handler to NULL,
tcp_wait_for_connect() will be re-called. the original socket (current addr)
will be re-checked by getsockopt()
But in your patch, it will retry next addr in the list.
In the past we didn't have the address list so we could not continue to the
next one,
now we can.
Yes, we should try to connect next addr if current address is not available.
But rc 0 doesn't mean current is not available.
rc 0 means fail to get socket option, we need to retry to _get_ socket
option.
for rc 0 and errno != EINTR there is no point of re-trying getsockopt again.
Orit
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
- Original Message -
On 09/24/2012 11:48 AM, Amos Kong wrote:
On 23/09/12 14:34, Orit Wasserman wrote:
On 09/20/2012 06:16 PM, Amos Kong wrote:
- Original Message -
On 09/19/2012 11:31 AM, Amos Kong wrote:
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try
to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often
have
ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection
with a
different
address.
callers on inet_nonblocking_connect register a callback
function
that will
be called when connect opertion completes, in case of failure
the
fd will have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
I will remove it.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef
‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’
was
here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool
block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is
returned, and
errno is set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void
*)
val, valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error()
!=
EINTR
If ret 0, just set the error state, but the callback function
is
not set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to
checking current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect
next
address on the list.
We should reserve that block, and move
qemu_set_fd_handler2(...NULL..) below it.
I'm sorry I don't get you point, it makes no difference where is
qemu_set_fd_handlers(...NULL...).
rc 0 happens in two cases:
1. val != 0 - connect error
It seems rc is 0 in this case.
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new
socket
fd.
In the past, if rc 0, we will return without setting fd handler
to NULL,
tcp_wait_for_connect() will be re-called. the original socket
(current addr)
will be re-checked by getsockopt()
But in your patch, it will retry next addr in the list.
In the past we didn't have the address list so we could not
continue to the next one,
now we can.
Yes, we should try to connect next addr if current address is not
available.
But rc 0 doesn't mean current is not available.
rc 0 means fail to get socket option, we need to retry to _get_
socket option.
for rc 0 and errno != EINTR there is no point of re-trying
getsockopt again.
Ok, got it.
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/20/2012 06:16 PM, Amos Kong wrote:
- Original Message -
On 09/19/2012 11:31 AM, Amos Kong wrote:
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have
ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a
different
address.
callers on inet_nonblocking_connect register a callback function
that will
be called when connect opertion completes, in case of failure the
fd will have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
I will remove it.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and
errno is set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error() !=
EINTR
If ret 0, just set the error state, but the callback function is
not set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to
checking current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect next
address on the list.
We should reserve that block, and move
qemu_set_fd_handler2(...NULL..) below it.
I'm sorry I don't get you point, it makes no difference where is
qemu_set_fd_handlers(...NULL...).
rc 0 happens in two cases:
1. val != 0 - connect error
It seems rc is 0 in this case.
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new socket
fd.
In the past, if rc 0, we will return without setting fd handler to NULL,
tcp_wait_for_connect() will be re-called. the original socket (current addr)
will be re-checked by getsockopt()
But in your patch, it will retry next addr in the list.
In the past we didn't have the address list so we could not continue to the
next one,
now we can.
Orit
.. migrate_fd_error(s);
.. return;
.. }
..
.. qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
another issue: val = 0 all the time?
yes, I can had a check anyway.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+s-current_addr = s-current_addr-ai_next;
+s-fd = inet_connect_addr(s-current_addr, false,
in_progress, s);
+/* connect in progress */
+if (in_progress) {
+return;
+}
+}
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/21/2012 11:03 AM, Markus Armbruster wrote:
Orit Wasserman owass...@redhat.com writes:
On 09/20/2012 04:14 PM, Markus Armbruster wrote:
Orit Wasserman owass...@redhat.com writes:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will
have
a negative value
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index 7f6ad98..cadea36 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
return r;
}
-static void tcp_wait_for_connect(void *opaque)
+static void tcp_wait_for_connect(int fd, void *opaque)
{
MigrationState *s = opaque;
-int val, ret;
-socklen_t valsize = sizeof(val);
-DPRINTF(connect completed\n);
-do {
-ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
-} while (ret == -1 (socket_error()) == EINTR);
-
-if (ret 0) {
+if (fd 0) {
+DPRINTF(migrate connect error\n);
+s-fd = -1;
migrate_fd_error(s);
-return;
-}
-
-qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
-
-if (val == 0)
+} else {
+DPRINTF(migrate connect success\n);
+s-fd = fd;
migrate_fd_connect(s);
-else {
-DPRINTF(error connecting %d\n, val);
-migrate_fd_error(s);
}
}
@@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s,
const char *host_port,
s-write = socket_write;
s-close = tcp_close;
-s-fd = inet_nonblocking_connect(host_port, in_progress, errp);
+s-fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
+ in_progress, errp);
if (error_is_set(errp)) {
migrate_fd_error(s);
return -1;
@@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s,
const char *host_port,
if (in_progress) {
DPRINTF(connect in progress\n);
-qemu_set_fd_handler2(s-fd, NULL, NULL, tcp_wait_for_connect, s);
} else {
migrate_fd_connect(s);
}
diff --git a/qemu-char.c b/qemu-char.c
index c442952..11cd5ef 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState
*qemu_chr_open_socket(QemuOpts *opts)
if (is_listen) {
fd = inet_listen_opts(opts, 0, NULL);
} else {
-fd = inet_connect_opts(opts, true, NULL, NULL);
+fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
}
}
if (fd 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
+} ConnectState;
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
+} while (rc == -1 socket_error() == EINTR);
+
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
Would rc = -1 suffice? I'd find that clearer.
I guess so, I want the errno for more detailed error message
but those will come in another patch set and I can handle it than.
I agree that using -1 will make the code much cleaner.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
Orit Wasserman owass...@redhat.com writes:
On 09/20/2012 04:14 PM, Markus Armbruster wrote:
Orit Wasserman owass...@redhat.com writes:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will
have
a negative value
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index 7f6ad98..cadea36 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
return r;
}
-static void tcp_wait_for_connect(void *opaque)
+static void tcp_wait_for_connect(int fd, void *opaque)
{
MigrationState *s = opaque;
-int val, ret;
-socklen_t valsize = sizeof(val);
-DPRINTF(connect completed\n);
-do {
-ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
-} while (ret == -1 (socket_error()) == EINTR);
-
-if (ret 0) {
+if (fd 0) {
+DPRINTF(migrate connect error\n);
+s-fd = -1;
migrate_fd_error(s);
-return;
-}
-
-qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
-
-if (val == 0)
+} else {
+DPRINTF(migrate connect success\n);
+s-fd = fd;
migrate_fd_connect(s);
-else {
-DPRINTF(error connecting %d\n, val);
-migrate_fd_error(s);
}
}
@@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
s-write = socket_write;
s-close = tcp_close;
-s-fd = inet_nonblocking_connect(host_port, in_progress, errp);
+s-fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
+ in_progress, errp);
if (error_is_set(errp)) {
migrate_fd_error(s);
return -1;
@@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
if (in_progress) {
DPRINTF(connect in progress\n);
-qemu_set_fd_handler2(s-fd, NULL, NULL, tcp_wait_for_connect, s);
} else {
migrate_fd_connect(s);
}
diff --git a/qemu-char.c b/qemu-char.c
index c442952..11cd5ef 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts
*opts)
if (is_listen) {
fd = inet_listen_opts(opts, 0, NULL);
} else {
-fd = inet_connect_opts(opts, true, NULL, NULL);
+fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
}
}
if (fd 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
+} ConnectState;
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
+} while (rc == -1 socket_error() == EINTR);
+
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
Would rc = -1 suffice? I'd find that clearer.
I guess so, I want the errno for more detailed error message
but those will come in another patch set and I can handle it than.
I agree that using -1 will make the code much cleaner.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
@@ -526,16 +592,19 @@ int inet_connect(const char *str, Error **errp)
return sock;
}
-
-int inet_nonblocking_connect(const char *str, bool *in_progress,
- Error **errp)
+int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
+ void *opaque, bool *in_progress, Error **errp)
{
Would be nice to have some documentation here.
Something like on immediate success or immediate
failure, in_progress is set to false, in that case
callback is not invoked.
If you look at it this way, this API is hard to
use right. I'd like to suggest we get rid of
in_progress flag: return -1 on error and
return =0 and invoke callback on immediate success.
--
MST
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/20/2012 09:03 AM, Michael S. Tsirkin wrote:
@@ -526,16 +592,19 @@ int inet_connect(const char *str, Error **errp)
return sock;
}
-
-int inet_nonblocking_connect(const char *str, bool *in_progress,
- Error **errp)
+int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
+ void *opaque, bool *in_progress, Error **errp)
{
Would be nice to have some documentation here.
Something like on immediate success or immediate
failure, in_progress is set to false, in that case
callback is not invoked.
of course.
If you look at it this way, this API is hard to
use right. I'd like to suggest we get rid of
in_progress flag: return -1 on error and
return =0 and invoke callback on immediate success.
we can even take it further and always invoke the callback
(even for immediate error), this way the user of the function
can put all the error/success handling in the callback function.
Orit
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On Thu, Sep 20, 2012 at 11:57:37AM +0300, Orit Wasserman wrote:
On 09/20/2012 09:03 AM, Michael S. Tsirkin wrote:
@@ -526,16 +592,19 @@ int inet_connect(const char *str, Error **errp)
return sock;
}
-
-int inet_nonblocking_connect(const char *str, bool *in_progress,
- Error **errp)
+int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
+ void *opaque, bool *in_progress, Error
**errp)
{
Would be nice to have some documentation here.
Something like on immediate success or immediate
failure, in_progress is set to false, in that case
callback is not invoked.
of course.
If you look at it this way, this API is hard to
use right. I'd like to suggest we get rid of
in_progress flag: return -1 on error and
return =0 and invoke callback on immediate success.
we can even take it further and always invoke the callback
(even for immediate error), this way the user of the function
can put all the error/success handling in the callback function.
Orit
Yes but I don't think this is a good idea:
there's value in reporting immediate error
directly to the user when command was invoked,
this is more user-friendly.
Not so for immediate success since that is
only a step in the migration process.
--
MST
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/20/2012 12:37 PM, Michael S. Tsirkin wrote:
On Thu, Sep 20, 2012 at 11:57:37AM +0300, Orit Wasserman wrote:
On 09/20/2012 09:03 AM, Michael S. Tsirkin wrote:
@@ -526,16 +592,19 @@ int inet_connect(const char *str, Error **errp)
return sock;
}
-
-int inet_nonblocking_connect(const char *str, bool *in_progress,
- Error **errp)
+int inet_nonblocking_connect(const char *str, ConnectHandler *callback,
+ void *opaque, bool *in_progress, Error
**errp)
{
Would be nice to have some documentation here.
Something like on immediate success or immediate
failure, in_progress is set to false, in that case
callback is not invoked.
of course.
If you look at it this way, this API is hard to
use right. I'd like to suggest we get rid of
in_progress flag: return -1 on error and
return =0 and invoke callback on immediate success.
we can even take it further and always invoke the callback
(even for immediate error), this way the user of the function
can put all the error/success handling in the callback function.
Orit
Yes but I don't think this is a good idea:
there's value in reporting immediate error
directly to the user when command was invoked,
this is more user-friendly.
Not so for immediate success since that is
only a step in the migration process.
OK
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/19/2012 11:31 AM, Amos Kong wrote:
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will
have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
I will remove it.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and errno is
set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error() != EINTR
If ret 0, just set the error state, but the callback function is not set to
NULL.
Then tcp_wait_for_connect() will be called again, and retry to checking
current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect next address
on the list.
We should reserve that block, and move qemu_set_fd_handler2(...NULL..) below
it.
I'm sorry I don't get you point, it makes no difference where is
qemu_set_fd_handlers(...NULL...).
rc 0 happens in two cases:
1. val != 0 - connect error
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new socket fd.
.. migrate_fd_error(s);
.. return;
.. }
..
.. qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
another issue: val = 0 all the time?
yes, I can had a check anyway.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+s-current_addr = s-current_addr-ai_next;
+s-fd = inet_connect_addr(s-current_addr, false, in_progress, s);
+/* connect in progress */
+if (in_progress) {
+return;
+}
+}
+
+freeaddrinfo(s-addr_list);
+if (s-callback) {
+s-callback(s-fd, s-opaque);
+}
+g_free(s);
+return;
+}
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
Orit Wasserman owass...@redhat.com writes:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will have
a negative value
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95 +++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index 7f6ad98..cadea36 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
return r;
}
-static void tcp_wait_for_connect(void *opaque)
+static void tcp_wait_for_connect(int fd, void *opaque)
{
MigrationState *s = opaque;
-int val, ret;
-socklen_t valsize = sizeof(val);
-DPRINTF(connect completed\n);
-do {
-ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
-} while (ret == -1 (socket_error()) == EINTR);
-
-if (ret 0) {
+if (fd 0) {
+DPRINTF(migrate connect error\n);
+s-fd = -1;
migrate_fd_error(s);
-return;
-}
-
-qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
-
-if (val == 0)
+} else {
+DPRINTF(migrate connect success\n);
+s-fd = fd;
migrate_fd_connect(s);
-else {
-DPRINTF(error connecting %d\n, val);
-migrate_fd_error(s);
}
}
@@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
s-write = socket_write;
s-close = tcp_close;
-s-fd = inet_nonblocking_connect(host_port, in_progress, errp);
+s-fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
+ in_progress, errp);
if (error_is_set(errp)) {
migrate_fd_error(s);
return -1;
@@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
if (in_progress) {
DPRINTF(connect in progress\n);
-qemu_set_fd_handler2(s-fd, NULL, NULL, tcp_wait_for_connect, s);
} else {
migrate_fd_connect(s);
}
diff --git a/qemu-char.c b/qemu-char.c
index c442952..11cd5ef 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts
*opts)
if (is_listen) {
fd = inet_listen_opts(opts, 0, NULL);
} else {
-fd = inet_connect_opts(opts, true, NULL, NULL);
+fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
}
}
if (fd 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
+} ConnectState;
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState *connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
+} while (rc == -1 socket_error() == EINTR);
+
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
Would rc = -1 suffice? I'd find that clearer.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+s-current_addr = s-current_addr-ai_next;
+s-fd = inet_connect_addr(s-current_addr, false, in_progress, s);
+/* connect in progress */
+if (in_progress) {
+return;
+}
+}
+
+
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 09/20/2012 04:14 PM, Markus Armbruster wrote:
Orit Wasserman owass...@redhat.com writes:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will
have
a negative value
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index 7f6ad98..cadea36 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
return r;
}
-static void tcp_wait_for_connect(void *opaque)
+static void tcp_wait_for_connect(int fd, void *opaque)
{
MigrationState *s = opaque;
-int val, ret;
-socklen_t valsize = sizeof(val);
-DPRINTF(connect completed\n);
-do {
-ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
-} while (ret == -1 (socket_error()) == EINTR);
-
-if (ret 0) {
+if (fd 0) {
+DPRINTF(migrate connect error\n);
+s-fd = -1;
migrate_fd_error(s);
-return;
-}
-
-qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
-
-if (val == 0)
+} else {
+DPRINTF(migrate connect success\n);
+s-fd = fd;
migrate_fd_connect(s);
-else {
-DPRINTF(error connecting %d\n, val);
-migrate_fd_error(s);
}
}
@@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
s-write = socket_write;
s-close = tcp_close;
-s-fd = inet_nonblocking_connect(host_port, in_progress, errp);
+s-fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
+ in_progress, errp);
if (error_is_set(errp)) {
migrate_fd_error(s);
return -1;
@@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
if (in_progress) {
DPRINTF(connect in progress\n);
-qemu_set_fd_handler2(s-fd, NULL, NULL, tcp_wait_for_connect, s);
} else {
migrate_fd_connect(s);
}
diff --git a/qemu-char.c b/qemu-char.c
index c442952..11cd5ef 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts
*opts)
if (is_listen) {
fd = inet_listen_opts(opts, 0, NULL);
} else {
-fd = inet_connect_opts(opts, true, NULL, NULL);
+fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
}
}
if (fd 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
+} ConnectState;
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
+} while (rc == -1 socket_error() == EINTR);
+
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
Would rc = -1 suffice? I'd find that clearer.
I guess so, I want the errno for more detailed error message
but those will come in another patch set and I can handle it than.
I agree that using -1 will make the code much cleaner.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
- Original Message -
On 09/19/2012 11:31 AM, Amos Kong wrote:
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have
ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a
different
address.
callers on inet_nonblocking_connect register a callback function
that will
be called when connect opertion completes, in case of failure the
fd will have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95
+++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
I will remove it.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was
here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState
*connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and
errno is set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *)
val, valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error() !=
EINTR
If ret 0, just set the error state, but the callback function is
not set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to
checking current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect next
address on the list.
We should reserve that block, and move
qemu_set_fd_handler2(...NULL..) below it.
I'm sorry I don't get you point, it makes no difference where is
qemu_set_fd_handlers(...NULL...).
rc 0 happens in two cases:
1. val != 0 - connect error
It seems rc is 0 in this case.
2. getsockopt failed for reason different than EINTR .
In both cases we can only move to the next addr in the list,
inet_connect_addr will call qemu_set_fd_handlers with the new socket
fd.
In the past, if rc 0, we will return without setting fd handler to NULL,
tcp_wait_for_connect() will be re-called. the original socket (current addr)
will be re-checked by getsockopt()
But in your patch, it will retry next addr in the list.
.. migrate_fd_error(s);
.. return;
.. }
..
.. qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
another issue: val = 0 all the time?
yes, I can had a check anyway.
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+s-current_addr = s-current_addr-ai_next;
+s-fd = inet_connect_addr(s-current_addr, false,
in_progress, s);
+/* connect in progress */
+if (in_progress) {
+return;
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
On 14/09/12 02:58, Orit Wasserman wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will have
a negative value
Hi Orit,
We almost get to the destination :)
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95 +++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
...
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
This member is not used.
+} ConnectState;
# make(gcc-4.4.6-4.el6.x86_64)
CCqemu-sockets.o
qemu-sockets.c:229: error: redefinition of typedef ‘ConnectState’
qemu_socket.h:46: note: previous declaration of ‘ConnectState’ was here
make: *** [qemu-sockets.o] Error 1
struct ConnectState {
int fd;
struct addrinfo *addr_list;
struct addrinfo *current_addr;
ConnectHandler *callback;
void *opaque;
};
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState *connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val, valsize);
+} while (rc == -1 socket_error() == EINTR);
# man getsockopt
RETURN VALUE
On success, zero is returned. On error, -1 is returned, and
errno is set appropriately.
.. original code:
..
.. do {
.. ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
.. } while (ret == -1 (socket_error()) == EINTR);
..
.. if (ret 0) { // ret equals to -1, and socket_error() != EINTR
If ret 0, just set the error state, but the callback function is not
set to NULL.
Then tcp_wait_for_connect() will be called again, and retry to checking
current
socket by getsockopt().
But in this patch, we will abandon current socket, and connect next
address on the list.
We should reserve that block, and move qemu_set_fd_handler2(...NULL..)
below it.
.. migrate_fd_error(s);
.. return;
.. }
..
.. qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
another issue: val = 0 all the time?
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+s-current_addr = s-current_addr-ai_next;
+s-fd = inet_connect_addr(s-current_addr, false, in_progress, s);
+/* connect in progress */
+if (in_progress) {
+return;
+}
+}
+
+freeaddrinfo(s-addr_list);
+if (s-callback) {
+s-callback(s-fd, s-opaque);
+}
+g_free(s);
+return;
+}
--
Amos.
Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
Orit Wasserman owass...@redhat.com wrote:
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will have
a negative value
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
Reviewed-by: Juan Quintela quint...@redhat.com
Just thinking out loud to be if I understood this correctly
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val,
valsize);
+} while (rc == -1 socket_error() == EINTR);
rc return the error code of getsockopt()
and val returns the error code of the socket if there is one.
+
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
+}
If getsockopt() succeeds, we reuse rc error code to have the socket
error code. If you have to resent this, could we improve the comment here?
I have to go to the manual page of getsockopt() that don't likes
SO_ERROR to try to understand what this completely un-intuitive (at
least for me) two lines of code do.
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
If there is any error (getsockopt or in the socket), we just close the
fd and update the error code.
Head hurts.
Thanks, Juan.
[Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
getaddrinfo can give us a list of addresses, but we only try to
connect to the first one. If that fails we never proceed to
the next one. This is common on desktop setups that often have ipv6
configured but not actually working.
To fix this make inet_connect_nonblocking retry connection with a different
address.
callers on inet_nonblocking_connect register a callback function that will
be called when connect opertion completes, in case of failure the fd will have
a negative value
Signed-off-by: Orit Wasserman owass...@redhat.com
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
migration-tcp.c | 29 +---
qemu-char.c |2 +-
qemu-sockets.c | 95 +++---
qemu_socket.h | 13 ++--
4 files changed, 102 insertions(+), 37 deletions(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index 7f6ad98..cadea36 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -53,29 +53,18 @@ static int tcp_close(MigrationState *s)
return r;
}
-static void tcp_wait_for_connect(void *opaque)
+static void tcp_wait_for_connect(int fd, void *opaque)
{
MigrationState *s = opaque;
-int val, ret;
-socklen_t valsize = sizeof(val);
-DPRINTF(connect completed\n);
-do {
-ret = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val, valsize);
-} while (ret == -1 (socket_error()) == EINTR);
-
-if (ret 0) {
+if (fd 0) {
+DPRINTF(migrate connect error\n);
+s-fd = -1;
migrate_fd_error(s);
-return;
-}
-
-qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
-
-if (val == 0)
+} else {
+DPRINTF(migrate connect success\n);
+s-fd = fd;
migrate_fd_connect(s);
-else {
-DPRINTF(error connecting %d\n, val);
-migrate_fd_error(s);
}
}
@@ -88,7 +77,8 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
s-write = socket_write;
s-close = tcp_close;
-s-fd = inet_nonblocking_connect(host_port, in_progress, errp);
+s-fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
+ in_progress, errp);
if (error_is_set(errp)) {
migrate_fd_error(s);
return -1;
@@ -96,7 +86,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const
char *host_port,
if (in_progress) {
DPRINTF(connect in progress\n);
-qemu_set_fd_handler2(s-fd, NULL, NULL, tcp_wait_for_connect, s);
} else {
migrate_fd_connect(s);
}
diff --git a/qemu-char.c b/qemu-char.c
index c442952..11cd5ef 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2459,7 +2459,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts
*opts)
if (is_listen) {
fd = inet_listen_opts(opts, 0, NULL);
} else {
-fd = inet_connect_opts(opts, true, NULL, NULL);
+fd = inet_connect_opts(opts, true, NULL, NULL, NULL);
}
}
if (fd 0) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 212075d..d321c58 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -24,6 +24,7 @@
#include qemu_socket.h
#include qemu-common.h /* for qemu_isdigit */
+#include main-loop.h
#ifndef AI_ADDRCONFIG
# define AI_ADDRCONFIG 0
@@ -217,11 +218,69 @@ listen:
((rc) == -EINPROGRESS)
#endif
+/* Struct to store connect state for non blocking connect */
+typedef struct ConnectState {
+int fd;
+struct addrinfo *addr_list;
+struct addrinfo *current_addr;
+ConnectHandler *callback;
+void *opaque;
+Error *errp;
+} ConnectState;
+
static int inet_connect_addr(struct addrinfo *addr, bool block,
- bool *in_progress)
+ bool *in_progress, ConnectState *connect_state);
+
+static void wait_for_connect(void *opaque)
+{
+ConnectState *s = opaque;
+int val = 0, rc = 0;
+socklen_t valsize = sizeof(val);
+bool in_progress = false;
+
+qemu_set_fd_handler2(s-fd, NULL, NULL, NULL, NULL);
+
+do {
+rc = getsockopt(s-fd, SOL_SOCKET, SO_ERROR, (void *) val, valsize);
+} while (rc == -1 socket_error() == EINTR);
+
+/* update rc to contain error details */
+if (!rc val) {
+rc = -val;
+}
+
+/* connect error */
+if (rc 0) {
+closesocket(s-fd);
+s-fd = rc;
+}
+
+/* try to connect to the next address on the list */
+while (s-current_addr-ai_next != NULL s-fd 0) {
+s-current_addr = s-current_addr-ai_next;
+s-fd = inet_connect_addr(s-current_addr, false, in_progress, s);
+/* connect in progress */
+if (in_progress) {
+return;
+}
+}
+
+freeaddrinfo(s-addr_list);
+if (s-callback) {
+s-callback(s-fd, s-opaque);
+}
+g_free(s);
+return;
+}
+
+static int inet_connect_addr(struct addrinfo *addr, bool block,
+ bool *in_progress, ConnectState
