Re: [Qemu-devel] [PATCH for-2.7 2/2] net: make socket connect non-blocking again

["Marc-André Lureau" <mlureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

Hi

----- Original Message -----
> On Tue, Aug 23, 2016 at 06:43:54PM +0800, Cao jin wrote:
> > Hi,
> > I just noticed you still using the old-style non-blocking connect(which
> > will
> > still block when query DNS), which will be removed (suggested by Daniel),
> > but the patch is still on the way. So I guess maybe you should switch to
> > QIOChannel way.
> > 
> > FYI:
> > http://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg06386.html
> > http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg00046.html
> 
> Yes, switching to QIOChannel would be good, but that is certainly not
> something that's acceptable for 2.7.0. We need Marc-André's fix in the
> short term for this release. If someone wants to do a QIOChannel
> conversion for 2.8.0/2.9.0/etc that's an option...

After looking at this a bit, it would need significant effort for net/socket to switch fully to qiochannel, so it's certainly not for 2.7. However, we could use it just for the connect step for now (see attached patch)

(tbh, I am not sure how well that net/socket async code is being handled in case of cancellation etc..)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-net-make-socket-connect-non-blocking-again.patch --]
[-- Type: text/x-patch; name=0001-net-make-socket-connect-non-blocking-again.patch, Size: 4437 bytes --]

From ad81f6d913bdb09b6f7c781c1e55ac42228f7c4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com>
Date: Tue, 23 Aug 2016 13:33:30 +0400
Subject: [PATCH] net: make socket connect non-blocking again
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since commit 7e8449594c929, the socket connect code is blocking, because
calling socket_connect() without callback is blocking. Make it
non-blocking by adding a callback. Unfortunately, the callback needs
many local variables that are not easy to get rid of (it can't easily
create the qemu_new_net_client() earlier). By adding the callback, the
socket is also made non-blocking. If the socket attempt succeeded
immediately, the callback is still being called.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/socket.c | 91 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 61 insertions(+), 30 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 645bcb0..ca0c0b7 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -33,6 +33,7 @@
 #include "qemu/sockets.h"
 #include "qemu/iov.h"
 #include "qemu/main-loop.h"
+#include "io/channel-socket.h"
 
 typedef struct NetSocketState {
     NetClientState nc;
@@ -517,53 +518,83 @@ static int net_socket_listen_init(NetClientState *peer,
     return 0;
 }
 
-static int net_socket_connect_init(NetClientState *peer,
-                                   const char *model,
-                                   const char *name,
-                                   const char *host_str)
+typedef struct {
+    QIOChannelSocket *qio_socket;
+    NetClientState *peer;
+    SocketAddress *saddr;
+    char *model;
+    char *name;
+} socket_connect_data;
+
+static void socket_connect_data_free(void *data)
 {
+    socket_connect_data *c = data;
+
+    qapi_free_SocketAddress(c->saddr);
+    object_unref(OBJECT(c->qio_socket));
+    g_free(c->model);
+    g_free(c->name);
+    g_free(c);
+}
+
+static void net_socket_connect_cb(Object *source,
+                                  Error *err,
+                                  gpointer opaque)
+{
+    socket_connect_data *c = opaque;
+    int fd;
     NetSocketState *s;
-    int fd = -1, ret = -1;
     char *addr_str = NULL;
-    SocketAddress *saddr = NULL;
     Error *local_error = NULL;
 
-    saddr = socket_parse(host_str, &local_error);
-    if (saddr == NULL) {
-        error_report_err(local_error);
-        return -1;
-    }
-
-    fd = socket_connect(saddr, &local_error, NULL, NULL);
-    if (fd < 0) {
+    addr_str = socket_address_to_string(c->saddr, &local_error);
+    if (addr_str == NULL) {
         error_report_err(local_error);
-        goto end;
+        return;
     }
 
-    qemu_set_nonblock(fd);
-
-    s = net_socket_fd_init(peer, model, name, fd, true);
+    fd = c->qio_socket->fd;
+    c->qio_socket->fd = -1;
+    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
     if (!s) {
-        goto end;
-    }
-
-    addr_str = socket_address_to_string(saddr, &local_error);
-    if (addr_str == NULL) {
-        error_report_err(local_error);
+        closesocket(fd);
         goto end;
     }
 
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),
              "socket: connect to %s", addr_str);
-    ret = 0;
 
 end:
-    if (ret == -1 && fd >= 0) {
-        closesocket(fd);
-    }
-    qapi_free_SocketAddress(saddr);
     g_free(addr_str);
-    return ret;
+}
+
+static int net_socket_connect_init(NetClientState *peer,
+                                   const char *model,
+                                   const char *name,
+                                   const char *host_str)
+{
+    socket_connect_data *c = g_new0(socket_connect_data, 1);
+    Error *local_error = NULL;
+
+    c->qio_socket = qio_channel_socket_new();
+    c->peer = peer;
+    c->model = g_strdup(model);
+    c->name = g_strdup(name);
+    c->saddr = socket_parse(host_str, &local_error);
+    if (c->saddr == NULL) {
+        goto err;
+    }
+
+    qio_channel_socket_connect_async(c->qio_socket, c->saddr,
+                                     net_socket_connect_cb,
+                                     c, socket_connect_data_free);
+
+    return 0;
+
+err:
+    error_report_err(local_error);
+    socket_connect_data_free(c);
+    return -1;
 }
 
 static int net_socket_mcast_init(NetClientState *peer,
-- 
2.9.0