[Qemu-devel] [PATCH for-2.7 0/2] Fix net socket connect regressions

[Qemu-devel] [PATCH for-2.7 0/2] Fix net socket connect regressions

[Marc-André Lureau]

Hi,

Commit 7e8449594c929 introduced multiple regressions. The most
important is that the socket is actually not the one connected, due to
wrong usage of socket_connect(). I fixed that along with some leak
fixes, that could eventually be splitted.

Secondly, connect is no longer non-blocking. The second patch is my
attempt to fix it.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1368447

Please review asap

Marc-André Lureau (2):
  net: fix socket connect
  net: make socket connect non-blocking again

 net/socket.c | 102 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 59 insertions(+), 43 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH for-2.7 1/2] net: fix socket connect

[Marc-André Lureau]

Commit 7e8449594c929 changed net socket code to use socket_*() functions
from include/qemu/sockets.h. However, the change broke the connection
part, because socket_connect() returns a connected socket that is
being ignored. Use socket_connect() return value appropriately and fix
leaks on error path.

Note that after this change, the function is still blocking.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1368447

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

diff --git a/net/socket.c b/net/socket.c
index 17e635d..645bcb0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -501,6 +501,7 @@ static int net_socket_listen_init(NetClientState *peer,
 
     ret = socket_listen(saddr, &local_error);
     if (ret < 0) {
+        qapi_free_SocketAddress(saddr);
         error_report_err(local_error);
         return -1;
     }
@@ -522,9 +523,9 @@ static int net_socket_connect_init(NetClientState *peer,
                                    const char *host_str)
 {
     NetSocketState *s;
-    int fd, connected, ret;
-    char *addr_str;
-    SocketAddress *saddr;
+    int fd = -1, ret = -1;
+    char *addr_str = NULL;
+    SocketAddress *saddr = NULL;
     Error *local_error = NULL;
 
     saddr = socket_parse(host_str, &local_error);
@@ -533,45 +534,36 @@ static int net_socket_connect_init(NetClientState *peer,
         return -1;
     }
 
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
+    fd = socket_connect(saddr, &local_error, NULL, NULL);
     if (fd < 0) {
-        perror("socket");
-        return -1;
+        error_report_err(local_error);
+        goto end;
     }
+
     qemu_set_nonblock(fd);
-    connected = 0;
-    for(;;) {
-        ret = socket_connect(saddr, &local_error, NULL, NULL);
-        if (ret < 0) {
-            if (errno == EINTR || errno == EWOULDBLOCK) {
-                /* continue */
-            } else if (errno == EINPROGRESS ||
-                       errno == EALREADY ||
-                       errno == EINVAL) {
-                break;
-            } else {
-                error_report_err(local_error);
-                closesocket(fd);
-                return -1;
-            }
-        } else {
-            connected = 1;
-            break;
-        }
+
+    s = net_socket_fd_init(peer, model, name, fd, true);
+    if (!s) {
+        goto end;
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected);
-    if (!s)
-        return -1;
 
     addr_str = socket_address_to_string(saddr, &local_error);
-    if (addr_str == NULL)
-        return -1;
+    if (addr_str == NULL) {
+        error_report_err(local_error);
+        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 0;
+    return ret;
 }
 
 static int net_socket_mcast_init(NetClientState *peer,
-- 
2.9.0

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

[Marc-André Lureau]

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 | 82 +++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 29 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 645bcb0..982c8de 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -517,53 +517,77 @@ 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 {
+    NetClientState *peer;
+    SocketAddress *saddr;
+    char *model;
+    char *name;
+} socket_connect_data;
+
+static void socket_connect_data_free(socket_connect_data *c)
 {
+    qapi_free_SocketAddress(c->saddr);
+    g_free(c->model);
+    g_free(c->name);
+    g_free(c);
+}
+
+static void net_socket_connected(int fd, Error *err, void *opaque)
+{
+    socket_connect_data *c = opaque;
     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);
+        closesocket(fd);
         goto end;
     }
 
-    qemu_set_nonblock(fd);
-
-    s = net_socket_fd_init(peer, model, name, fd, true);
+    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;
+    socket_connect_data_free(c);
+}
+
+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);
+    int fd = -1;
+    Error *local_error = NULL;
+
+    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;
+    }
+
+    fd = socket_connect(c->saddr, &local_error, net_socket_connected, c);
+    if (fd < 0) {
+        goto err;
+    }
+
+    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

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

[Cao jin]

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

     Cao jin

On 08/23/2016 05:45 PM, Marc-André Lureau wrote:
> 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 | 82 +++++++++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 53 insertions(+), 29 deletions(-)
>

> +
> +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);
> +    int fd = -1;
> +    Error *local_error = NULL;
> +
> +    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;
> +    }
> +
> +    fd = socket_connect(c->saddr, &local_error, net_socket_connected, c);
> +    if (fd < 0) {
> +        goto err;
> +    }
> +
> +    return 0;
> +
> +err:
> +    error_report_err(local_error);
> +    socket_connect_data_free(c);
> +    return -1;
>   }
>
>   static int net_socket_mcast_init(NetClientState *peer,
>

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

[Daniel P. Berrange]

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...


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Marc-André Lureau]

[-- 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

Re: [Qemu-devel] [PATCH for-2.7 0/2] Fix net socket connect regressions

[Paolo Bonzini]

On 23/08/2016 11:45, Marc-André Lureau wrote:
> Hi,
> 
> Commit 7e8449594c929 introduced multiple regressions. The most
> important is that the socket is actually not the one connected, due to
> wrong usage of socket_connect(). I fixed that along with some leak
> fixes, that could eventually be splitted.
> 
> Secondly, connect is no longer non-blocking. The second patch is my
> attempt to fix it.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1368447

I'll send a revert, and your patch for 2.8.

Paolo