[Qemu-devel] [PATCH v2 0/1] Enable full IPv4/IPv6 dual stack support in chardevs

[Qemu-devel] [PATCH v2 0/1] Enable full IPv4/IPv6 dual stack support in chardevs

[Daniel P. Berrange]

Previously posted as part of a larger series:

  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg02064.html

The common code has merged, so this is just the patch that needs to go via the
chardev maintainer's tree.

With this change, the chardev socket server honours multiple IP addresses
returned by getaddrinfo instead of only binding on the first. This makes dual
stack work fully, and also improves support with multi-homed hosts.

No changes in v1, except for rebasing.

Daniel P. Berrange (1):
  chardev: convert the socket server to QIONetListener

 chardev/char-socket.c | 72 +++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/1] chardev: convert the socket server to QIONetListener

[Daniel P. Berrange]

Instead of creating a QIOChannelSocket directly for the chardev
server socket, use a QIONetListener. This provides the ability
to listen on multiple sockets at the same time, so enables
full support for IPv4/IPv6 dual stack.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 chardev/char-socket.c | 72 +++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 53eda8ef00..2d2252ab0a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -25,6 +25,7 @@
 #include "chardev/char.h"
 #include "io/channel-socket.h"
 #include "io/channel-tls.h"
+#include "io/net-listener.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
@@ -40,8 +41,7 @@ typedef struct {
     Chardev parent;
     QIOChannel *ioc; /* Client I/O channel */
     QIOChannelSocket *sioc; /* Client master channel */
-    QIOChannelSocket *listen_ioc;
-    guint listen_tag;
+    QIONetListener *listener;
     QCryptoTLSCreds *tls_creds;
     int connected;
     int max_size;
@@ -93,9 +93,9 @@ static void check_report_connect_error(Chardev *chr,
     qemu_chr_socket_restart_timer(chr);
 }
 
-static gboolean tcp_chr_accept(QIOChannel *chan,
-                               GIOCondition cond,
-                               void *opaque);
+static void tcp_chr_accept(QIONetListener *listener,
+                           QIOChannelSocket *cioc,
+                           void *opaque);
 
 static int tcp_chr_read_poll(void *opaque);
 static void tcp_chr_disconnect(Chardev *chr);
@@ -401,9 +401,9 @@ static void tcp_chr_disconnect(Chardev *chr)
 
     tcp_chr_free_connection(chr);
 
-    if (s->listen_ioc && s->listen_tag == 0) {
-        s->listen_tag = qio_channel_add_watch(
-            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
+    if (s->listener) {
+        qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
+                                         chr, NULL);
     }
     update_disconnected_filename(s);
     if (emit_close) {
@@ -702,9 +702,8 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
     if (s->do_nodelay) {
         qio_channel_set_delay(s->ioc, false);
     }
-    if (s->listen_tag) {
-        g_source_remove(s->listen_tag);
-        s->listen_tag = 0;
+    if (s->listener) {
+        qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
     }
 
     if (s->tls_creds) {
@@ -736,24 +735,14 @@ static int tcp_chr_add_client(Chardev *chr, int fd)
     return ret;
 }
 
-static gboolean tcp_chr_accept(QIOChannel *channel,
-                               GIOCondition cond,
-                               void *opaque)
+static void tcp_chr_accept(QIONetListener *listener,
+                           QIOChannelSocket *cioc,
+                           void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
-    QIOChannelSocket *sioc;
-
-    sioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(channel),
-                                     NULL);
-    if (!sioc) {
-        return TRUE;
-    }
-
-    tcp_chr_new_client(chr, sioc);
 
-    object_unref(OBJECT(sioc));
-
-    return TRUE;
+    tcp_chr_set_client_ioc_name(chr, cioc);
+    tcp_chr_new_client(chr, cioc);
 }
 
 static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
@@ -767,9 +756,10 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
         if (s->is_listen) {
             info_report("QEMU waiting for connection on: %s",
                         chr->filename);
-            qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
-            tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
-            qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
+            sioc = qio_net_listener_wait_client(s->listener);
+            tcp_chr_set_client_ioc_name(chr, sioc);
+            tcp_chr_new_client(chr, sioc);
+            object_unref(OBJECT(sioc));
         } else {
             sioc = qio_channel_socket_new();
             tcp_chr_set_client_ioc_name(chr, sioc);
@@ -797,12 +787,9 @@ static void char_socket_finalize(Object *obj)
         s->reconnect_timer = 0;
     }
     qapi_free_SocketAddress(s->addr);
-    if (s->listen_tag) {
-        g_source_remove(s->listen_tag);
-        s->listen_tag = 0;
-    }
-    if (s->listen_ioc) {
-        object_unref(OBJECT(s->listen_ioc));
+    if (s->listener) {
+        qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
+        object_unref(OBJECT(s->listener));
     }
     if (s->tls_creds) {
         object_unref(OBJECT(s->tls_creds));
@@ -935,29 +922,28 @@ static void qmp_chardev_open_socket(Chardev *chr,
     } else {
         if (s->is_listen) {
             char *name;
-            sioc = qio_channel_socket_new();
+            s->listener = qio_net_listener_new();
 
             name = g_strdup_printf("chardev-tcp-listener-%s", chr->label);
-            qio_channel_set_name(QIO_CHANNEL(sioc), name);
+            qio_net_listener_set_name(s->listener, name);
             g_free(name);
 
-            if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
+            if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
+                object_unref(OBJECT(s->listener));
                 goto error;
             }
 
             qapi_free_SocketAddress(s->addr);
-            s->addr = socket_local_address(sioc->fd, errp);
+            s->addr = socket_local_address(s->listener->sioc[0]->fd, errp);
             update_disconnected_filename(s);
 
-            s->listen_ioc = sioc;
             if (is_waitconnect &&
                 qemu_chr_wait_connected(chr, errp) < 0) {
                 return;
             }
             if (!s->ioc) {
-                s->listen_tag = qio_channel_add_watch(
-                    QIO_CHANNEL(s->listen_ioc), G_IO_IN,
-                    tcp_chr_accept, chr, NULL);
+                qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
+                                                 chr, NULL);
             }
         } else if (qemu_chr_wait_connected(chr, errp) < 0) {
             goto error;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 1/1] chardev: convert the socket server to QIONetListener

[Paolo Bonzini]

On 18/12/2017 14:54, Daniel P. Berrange wrote:
> Instead of creating a QIOChannelSocket directly for the chardev
> server socket, use a QIONetListener. This provides the ability
> to listen on multiple sockets at the same time, so enables
> full support for IPv4/IPv6 dual stack.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 72 +++++++++++++++++++++------------------------------
>  1 file changed, 29 insertions(+), 43 deletions(-)

Queued, thanks.

As a follow-up, it's probably worth creating two new functions for
respectively tcp_chr_set_client_ioc_name+tcp_chr_new_client and
tcp_chr_set_client_ioc_name+qio_channel_socket_connect_async
(with tcp_chr_set_client_ioc_name inlined).

Paolo

Re: [Qemu-devel] [PATCH v2 1/1] chardev: convert the socket server to QIONetListener

[Paolo Bonzini]

On 19/12/2017 09:48, Paolo Bonzini wrote:
> On 18/12/2017 14:54, Daniel P. Berrange wrote:
>> Instead of creating a QIOChannelSocket directly for the chardev
>> server socket, use a QIONetListener. This provides the ability
>> to listen on multiple sockets at the same time, so enables
>> full support for IPv4/IPv6 dual stack.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>  chardev/char-socket.c | 72 +++++++++++++++++++++------------------------------
>>  1 file changed, 29 insertions(+), 43 deletions(-)
> 
> Queued, thanks.

I think this has to be squashed in to avoid use-after-free issues left and right
(visible with test-hmp):

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 2d2252a..630a7f2 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -930,6 +930,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
 
             if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
                 object_unref(OBJECT(s->listener));
+                s->listener = NULL;
                 goto error;
             }
 

Paolo

> 
> As a follow-up, it's probably worth creating two new functions for
> respectively tcp_chr_set_client_ioc_name+tcp_chr_new_client and
> tcp_chr_set_client_ioc_name+qio_channel_socket_connect_async
> (with tcp_chr_set_client_ioc_name inlined).
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v2 1/1] chardev: convert the socket server to QIONetListener

[Daniel P. Berrange]

On Wed, Dec 20, 2017 at 05:23:17PM +0100, Paolo Bonzini wrote:
> On 19/12/2017 09:48, Paolo Bonzini wrote:
> > On 18/12/2017 14:54, Daniel P. Berrange wrote:
> >> Instead of creating a QIOChannelSocket directly for the chardev
> >> server socket, use a QIONetListener. This provides the ability
> >> to listen on multiple sockets at the same time, so enables
> >> full support for IPv4/IPv6 dual stack.
> >>
> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >> ---
> >>  chardev/char-socket.c | 72 +++++++++++++++++++++------------------------------
> >>  1 file changed, 29 insertions(+), 43 deletions(-)
> > 
> > Queued, thanks.
> 
> I think this has to be squashed in to avoid use-after-free issues left and right
> (visible with test-hmp):
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 2d2252a..630a7f2 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -930,6 +930,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>  
>              if (qio_net_listener_open_sync(s->listener, s->addr, errp) < 0) {
>                  object_unref(OBJECT(s->listener));
> +                s->listener = NULL;
>                  goto error;
>              }

Yes, that makes sense given the context, though I can't reproduce the test
failures myself yet. Any, please do squash it into the patch, as it makes
sense. Reviewed-by: Daniel P. Berrange <berrange@redhat.com>



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|