qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking


From: Amos Kong
Subject: Re: [Qemu-devel] [PATCH v3 3/3] Fix address handling in inet_nonblocking_connect
Date: Mon, 24 Sep 2012 17:48:35 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0

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 <address@hidden>
Signed-off-by: Michael S. Tsirkin <address@hidden>
---
   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)
   CC    qemu-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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]