[Qemu-devel] [PATCH v2 0/9] chardev: qio related non-default context support

[Qemu-devel] [PATCH v2 0/9] chardev: qio related non-default context support

[Peter Xu]

Based-on: <20180305064324.9238-1-peterx@redhat.com>

This series is based on the QIO part:
  [PATCH v3 0/6] qio: general non-default GMainContext support

v2:
- fix the reported problem by patchew in patch 5
- added some r-bs from Marc-Andre

Please review, thanks.

Peter Xu (9):
  vl: export machine_init_done
  chardev: fix leak in tcp_chr_telnet_init_io()
  chardev: update net listener gcontext
  chardev: allow telnet gsource to switch gcontext
  chardev: introduce chr_machine_done hook
  chardev: use chardev's gcontext for async connect
  chardev: tcp: postpone async connection setup
  chardev: tcp: let TLS run on chardev context
  chardev: tcp: postpone TLS work until machine done

 chardev/char-mux.c         |  33 ++++++++--
 chardev/char-socket.c      | 153 ++++++++++++++++++++++++++++++++++-----------
 chardev/char.c             |  43 +++++--------
 include/chardev/char-mux.h |   2 -
 include/chardev/char.h     |   2 +
 include/sysemu/sysemu.h    |   2 +
 stubs/machine-init-done.c  |   2 +
 tests/test-char.c          |   1 -
 vl.c                       |   4 +-
 9 files changed, 169 insertions(+), 73 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/9] vl: export machine_init_done

[Peter Xu]

We have that variable but not exported.  Export that so modules can have
a way to poke on whether machine init has finished.

Meanwhile, set that up even before calling the notifiers, so that
notifiers who may depend on this field will get a correct answer.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/sysemu/sysemu.h   | 2 ++
 stubs/machine-init-done.c | 2 ++
 vl.c                      | 4 ++--
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 77bb3da582..3f0f35610b 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -86,6 +86,8 @@ void qemu_system_guest_panicked(GuestPanicInformation *info);
 void qemu_add_exit_notifier(Notifier *notify);
 void qemu_remove_exit_notifier(Notifier *notify);
 
+extern bool machine_init_done;
+
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
diff --git a/stubs/machine-init-done.c b/stubs/machine-init-done.c
index 9a0d62514f..4121f1709b 100644
--- a/stubs/machine-init-done.c
+++ b/stubs/machine-init-done.c
@@ -2,6 +2,8 @@
 #include "qemu-common.h"
 #include "sysemu/sysemu.h"
 
+bool machine_init_done = true;
+
 void qemu_add_machine_init_done_notifier(Notifier *notify)
 {
 }
diff --git a/vl.c b/vl.c
index a33ac008fb..57777e8d47 100644
--- a/vl.c
+++ b/vl.c
@@ -2712,7 +2712,7 @@ static void qemu_run_exit_notifiers(void)
     notifier_list_notify(&exit_notifiers, NULL);
 }
 
-static bool machine_init_done;
+bool machine_init_done;
 
 void qemu_add_machine_init_done_notifier(Notifier *notify)
 {
@@ -2729,8 +2729,8 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify)
 
 static void qemu_run_machine_init_done_notifiers(void)
 {
-    notifier_list_notify(&machine_init_done_notifiers, NULL);
     machine_init_done = true;
+    notifier_list_notify(&machine_init_done_notifiers, NULL);
 }
 
 static const QEMUOption *lookup_opt(int argc, char **argv,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 1/9] vl: export machine_init_done

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:12PM +0800, Peter Xu wrote:
> We have that variable but not exported.  Export that so modules can have
> a way to poke on whether machine init has finished.
> 
> Meanwhile, set that up even before calling the notifiers, so that
> notifiers who may depend on this field will get a correct answer.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/sysemu/sysemu.h   | 2 ++
>  stubs/machine-init-done.c | 2 ++
>  vl.c                      | 4 ++--
>  3 files changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <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 :|

[Qemu-devel] [PATCH v2 2/9] chardev: fix leak in tcp_chr_telnet_init_io()

[Peter Xu]

Need to free TCPChardevTelnetInit when session established.

Since at it, switch to use G_SOURCE_* macros.

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 113feaf948..205ee377a4 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -591,19 +591,23 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
             ret = 0;
         } else {
             tcp_chr_disconnect(init->chr);
-            return FALSE;
+            goto end;
         }
     }
     init->buflen -= ret;
 
     if (init->buflen == 0) {
         tcp_chr_connect(init->chr);
-        return FALSE;
+        goto end;
     }
 
     memmove(init->buf, init->buf + ret, init->buflen);
 
-    return TRUE;
+    return G_SOURCE_CONTINUE;
+
+end:
+    g_free(init);
+    return G_SOURCE_REMOVE;
 }
 
 static void tcp_chr_telnet_init(Chardev *chr)
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/9] chardev: update net listener gcontext

[Peter Xu]

TCP chardevs can be using QIO network listeners working in the
background when in listening mode.  However the network listeners are
always running in main context.  This can race with chardevs that are
running in non-main contexts.

To solve this, we need to re-setup the net listeners in
tcp_chr_update_read_handler() with the newly cached gcontext.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 205ee377a4..5aa01e15ff 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -422,8 +422,8 @@ static void tcp_chr_disconnect(Chardev *chr)
     tcp_chr_free_connection(chr);
 
     if (s->listener) {
-        qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
-                                         chr, NULL);
+        qio_net_listener_set_client_func_full(s->listener, tcp_chr_accept,
+                                              chr, NULL, chr->gcontext);
     }
     update_disconnected_filename(s);
     if (emit_close) {
@@ -559,6 +559,16 @@ static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
+    if (s->listener) {
+        /*
+         * It's possible that chardev context is changed in
+         * qemu_chr_be_update_read_handlers().  Reset it for QIO net
+         * listener if there is.
+         */
+        qio_net_listener_set_client_func_full(s->listener, tcp_chr_accept,
+                                              chr, NULL, chr->gcontext);
+    }
+
     if (!s->connected) {
         return;
     }
@@ -743,7 +753,8 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
         qio_channel_set_delay(s->ioc, false);
     }
     if (s->listener) {
-        qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
+        qio_net_listener_set_client_func_full(s->listener, NULL, NULL,
+                                              NULL, chr->gcontext);
     }
 
     if (s->tls_creds) {
@@ -824,7 +835,8 @@ static void char_socket_finalize(Object *obj)
     tcp_chr_reconn_timer_cancel(s);
     qapi_free_SocketAddress(s->addr);
     if (s->listener) {
-        qio_net_listener_set_client_func(s->listener, NULL, NULL, NULL);
+        qio_net_listener_set_client_func_full(s->listener, NULL, NULL,
+                                              NULL, chr->gcontext);
         object_unref(OBJECT(s->listener));
     }
     if (s->tls_creds) {
@@ -980,8 +992,10 @@ static void qmp_chardev_open_socket(Chardev *chr,
                 return;
             }
             if (!s->ioc) {
-                qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
-                                                 chr, NULL);
+                qio_net_listener_set_client_func_full(s->listener,
+                                                      tcp_chr_accept,
+                                                      chr, NULL,
+                                                      chr->gcontext);
             }
         } else if (qemu_chr_wait_connected(chr, errp) < 0) {
             goto error;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 3/9] chardev: update net listener gcontext

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:14PM +0800, Peter Xu wrote:
> TCP chardevs can be using QIO network listeners working in the
> background when in listening mode.  However the network listeners are
> always running in main context.  This can race with chardevs that are
> running in non-main contexts.
> 
> To solve this, we need to re-setup the net listeners in
> tcp_chr_update_read_handler() with the newly cached gcontext.
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)

Reviewed-by: Daniel P. Berrangé <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 :|

[Qemu-devel] [PATCH v2 4/9] chardev: allow telnet gsource to switch gcontext

[Peter Xu]

It was originally created by qio_channel_add_watch() so it's always
assigning the task to main context.  Now we use the new API called
qio_channel_add_watch_source() so that we get the GSource handle rather
than the tag ID.

Meanwhile, caching the gsource and TCPChardevTelnetInit (which holds the
handshake data) in SocketChardev.telnet_source so that we can also do
dynamic context switch when update read handlers.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 67 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 16 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 5aa01e15ff..c22b3f330c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -39,6 +39,11 @@
 
 #define TCP_MAX_FDS 16
 
+typedef struct {
+    char buf[21];
+    size_t buflen;
+} TCPChardevTelnetInit;
+
 typedef struct {
     Chardev parent;
     QIOChannel *ioc; /* Client I/O channel */
@@ -59,6 +64,8 @@ typedef struct {
     bool is_listen;
     bool is_telnet;
     bool is_tn3270;
+    GSource *telnet_source;
+    TCPChardevTelnetInit *telnet_init;
 
     GSource *reconnect_timer;
     int64_t reconnect_time;
@@ -69,6 +76,7 @@ typedef struct {
     OBJECT_CHECK(SocketChardev, (obj), TYPE_CHARDEV_SOCKET)
 
 static gboolean socket_reconnect_timeout(gpointer opaque);
+static void tcp_chr_telnet_init(Chardev *chr);
 
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
@@ -555,6 +563,15 @@ static void tcp_chr_connect(void *opaque)
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 
+static void tcp_chr_telnet_destroy(SocketChardev *s)
+{
+    if (s->telnet_source) {
+        g_source_destroy(s->telnet_source);
+        g_source_unref(s->telnet_source);
+        s->telnet_source = NULL;
+    }
+}
+
 static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
@@ -569,6 +586,10 @@ static void tcp_chr_update_read_handler(Chardev *chr)
                                               chr, NULL, chr->gcontext);
     }
 
+    if (s->telnet_source) {
+        tcp_chr_telnet_init(CHARDEV(s));
+    }
+
     if (!s->connected) {
         return;
     }
@@ -582,32 +603,30 @@ static void tcp_chr_update_read_handler(Chardev *chr)
     }
 }
 
-typedef struct {
-    Chardev *chr;
-    char buf[21];
-    size_t buflen;
-} TCPChardevTelnetInit;
-
 static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
                                        GIOCondition cond G_GNUC_UNUSED,
                                        gpointer user_data)
 {
-    TCPChardevTelnetInit *init = user_data;
+    SocketChardev *s = user_data;
+    Chardev *chr = CHARDEV(s);
+    TCPChardevTelnetInit *init = s->telnet_init;
     ssize_t ret;
 
+    assert(init);
+
     ret = qio_channel_write(ioc, init->buf, init->buflen, NULL);
     if (ret < 0) {
         if (ret == QIO_CHANNEL_ERR_BLOCK) {
             ret = 0;
         } else {
-            tcp_chr_disconnect(init->chr);
+            tcp_chr_disconnect(chr);
             goto end;
         }
     }
     init->buflen -= ret;
 
     if (init->buflen == 0) {
-        tcp_chr_connect(init->chr);
+        tcp_chr_connect(chr);
         goto end;
     }
 
@@ -616,16 +635,30 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
     return G_SOURCE_CONTINUE;
 
 end:
-    g_free(init);
+    g_free(s->telnet_init);
+    s->telnet_init = NULL;
+    g_source_unref(s->telnet_source);
+    s->telnet_source = NULL;
     return G_SOURCE_REMOVE;
 }
 
 static void tcp_chr_telnet_init(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
-    TCPChardevTelnetInit *init = g_new0(TCPChardevTelnetInit, 1);
+    TCPChardevTelnetInit *init;
     size_t n = 0;
 
+    /* Destroy existing task */
+    tcp_chr_telnet_destroy(s);
+
+    if (s->telnet_init) {
+        /* We are possibly during a handshake already */
+        goto cont;
+    }
+
+    s->telnet_init = g_new0(TCPChardevTelnetInit, 1);
+    init = s->telnet_init;
+
 #define IACSET(x, a, b, c)                      \
     do {                                        \
         x[n++] = a;                             \
@@ -633,7 +666,6 @@ static void tcp_chr_telnet_init(Chardev *chr)
         x[n++] = c;                             \
     } while (0)
 
-    init->chr = chr;
     if (!s->is_tn3270) {
         init->buflen = 12;
         /* Prep the telnet negotion to put telnet in binary,
@@ -656,10 +688,11 @@ static void tcp_chr_telnet_init(Chardev *chr)
 
 #undef IACSET
 
-    qio_channel_add_watch(
-        s->ioc, G_IO_OUT,
-        tcp_chr_telnet_init_io,
-        init, NULL);
+cont:
+    s->telnet_source = qio_channel_add_watch_source(s->ioc, G_IO_OUT,
+                                                    tcp_chr_telnet_init_io,
+                                                    s, NULL,
+                                                    chr->gcontext);
 }
 
 
@@ -834,6 +867,8 @@ static void char_socket_finalize(Object *obj)
     tcp_chr_free_connection(chr);
     tcp_chr_reconn_timer_cancel(s);
     qapi_free_SocketAddress(s->addr);
+    tcp_chr_telnet_destroy(s);
+    g_free(s->telnet_init);
     if (s->listener) {
         qio_net_listener_set_client_func_full(s->listener, NULL, NULL,
                                               NULL, chr->gcontext);
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 4/9] chardev: allow telnet gsource to switch gcontext

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:15PM +0800, Peter Xu wrote:
> It was originally created by qio_channel_add_watch() so it's always
> assigning the task to main context.  Now we use the new API called
> qio_channel_add_watch_source() so that we get the GSource handle rather
> than the tag ID.
> 
> Meanwhile, caching the gsource and TCPChardevTelnetInit (which holds the
> handshake data) in SocketChardev.telnet_source so that we can also do
> dynamic context switch when update read handlers.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 67 +++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 16 deletions(-)

Reviewed-by: Daniel P. Berrangé <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 :|

[Qemu-devel] [PATCH v2 5/9] chardev: introduce chr_machine_done hook

[Peter Xu]

Introduce ChardevClass.chr_machine_done() hook so that chardevs can run
customized procedures after machine init.

There was an existing mux user already that did similar thing but used a
raw machine done notifier.  Generalize it into a framework, and let the
mux chardevs provide such a class-specific hook to achieve the same
thing.  Then we can move the mux related code to the char-mux.c file.

Since at it, replace the mux_realized variable with the global
machine_init_done varible.

This notifier framework will be further leverged by other type of
chardevs soon.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-mux.c         | 33 +++++++++++++++++++++++++++++----
 chardev/char.c             | 43 +++++++++++++++++--------------------------
 include/chardev/char-mux.h |  2 --
 include/chardev/char.h     |  2 ++
 tests/test-char.c          |  1 -
 5 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index d48e78103a..1b925c8dec 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -27,6 +27,7 @@
 #include "qemu/option.h"
 #include "chardev/char.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
 #include "chardev/char-mux.h"
 
 /* MUX driver for serial I/O splitting */
@@ -230,14 +231,12 @@ static void mux_chr_read(void *opaque, const uint8_t *buf, int size)
         }
 }
 
-bool muxes_realized;
-
 void mux_chr_send_all_event(Chardev *chr, int event)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
     int i;
 
-    if (!muxes_realized) {
+    if (!machine_init_done) {
         return;
     }
 
@@ -327,7 +326,7 @@ static void qemu_chr_open_mux(Chardev *chr,
     /* only default to opened state if we've realized the initial
      * set of muxes
      */
-    *be_opened = muxes_realized;
+    *be_opened = machine_init_done;
     qemu_chr_fe_init(&d->chr, drv, errp);
 }
 
@@ -347,6 +346,31 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
     mux->chardev = g_strdup(chardev);
 }
 
+/**
+ * Called after processing of default and command-line-specified
+ * chardevs to deliver CHR_EVENT_OPENED events to any FEs attached
+ * to a mux chardev. This is done here to ensure that
+ * output/prompts/banners are only displayed for the FE that has
+ * focus when initial command-line processing/machine init is
+ * completed.
+ *
+ * After this point, any new FE attached to any new or existing
+ * mux will receive CHR_EVENT_OPENED notifications for the BE
+ * immediately.
+ */
+static int open_muxes(Chardev *chr)
+{
+    /* send OPENED to all already-attached FEs */
+    mux_chr_send_all_event(chr, CHR_EVENT_OPENED);
+    /*
+     * mark mux as OPENED so any new FEs will immediately receive
+     * OPENED event
+     */
+    qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+
+    return 0;
+}
+
 static void char_mux_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -357,6 +381,7 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_accept_input = mux_chr_accept_input;
     cc->chr_add_watch = mux_chr_add_watch;
     cc->chr_be_event = mux_chr_be_event;
+    cc->chr_machine_done = open_muxes;
 }
 
 static const TypeInfo char_mux_type_info = {
diff --git a/chardev/char.c b/chardev/char.c
index 01d979a1da..fda820863c 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -281,40 +281,31 @@ static const TypeInfo char_type_info = {
     .class_init = char_class_init,
 };
 
-/**
- * Called after processing of default and command-line-specified
- * chardevs to deliver CHR_EVENT_OPENED events to any FEs attached
- * to a mux chardev. This is done here to ensure that
- * output/prompts/banners are only displayed for the FE that has
- * focus when initial command-line processing/machine init is
- * completed.
- *
- * After this point, any new FE attached to any new or existing
- * mux will receive CHR_EVENT_OPENED notifications for the BE
- * immediately.
- */
-static int open_muxes(Object *child, void *opaque)
+static int chardev_machine_done_notify_one(Object *child, void *opaque)
 {
-    if (CHARDEV_IS_MUX(child)) {
-        /* send OPENED to all already-attached FEs */
-        mux_chr_send_all_event(CHARDEV(child), CHR_EVENT_OPENED);
-        /* mark mux as OPENED so any new FEs will immediately receive
-         * OPENED event
-         */
-        qemu_chr_be_event(CHARDEV(child), CHR_EVENT_OPENED);
+    Chardev *chr = (Chardev *)child;
+    ChardevClass *class = CHARDEV_GET_CLASS(chr);
+
+    if (class->chr_machine_done) {
+        return class->chr_machine_done(chr);
     }
 
     return 0;
 }
 
-static void muxes_realize_done(Notifier *notifier, void *unused)
+static void chardev_machine_done_hook(Notifier *notifier, void *unused)
 {
-    muxes_realized = true;
-    object_child_foreach(get_chardevs_root(), open_muxes, NULL);
+    int ret = object_child_foreach(get_chardevs_root(),
+                                   chardev_machine_done_notify_one, NULL);
+
+    if (ret) {
+        error_report("Failed to call chardev machine_done hooks");
+        exit(1);
+    }
 }
 
-static Notifier muxes_realize_notify = {
-    .notify = muxes_realize_done,
+static Notifier chardev_machine_done_notify = {
+    .notify = chardev_machine_done_hook,
 };
 
 static bool qemu_chr_is_busy(Chardev *s)
@@ -1118,7 +1109,7 @@ static void register_types(void)
      * as part of realize functions like serial_isa_realizefn when -nographic
      * is specified
      */
-    qemu_add_machine_init_done_notifier(&muxes_realize_notify);
+    qemu_add_machine_init_done_notifier(&chardev_machine_done_notify);
 }
 
 type_init(register_types);
diff --git a/include/chardev/char-mux.h b/include/chardev/char-mux.h
index 8928977897..1e13187767 100644
--- a/include/chardev/char-mux.h
+++ b/include/chardev/char-mux.h
@@ -27,8 +27,6 @@
 #include "chardev/char.h"
 #include "chardev/char-fe.h"
 
-extern bool muxes_realized;
-
 #define MAX_MUX 4
 #define MUX_BUFFER_SIZE 32 /* Must be a power of 2.  */
 #define MUX_BUFFER_MASK (MUX_BUFFER_SIZE - 1)
diff --git a/include/chardev/char.h b/include/chardev/char.h
index a381dc3df8..1cb1f4763f 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -247,6 +247,8 @@ typedef struct ChardevClass {
     void (*chr_set_echo)(Chardev *chr, bool echo);
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
     void (*chr_be_event)(Chardev *s, int event);
+    /* Return 0 if succeeded, 1 if failed */
+    int (*chr_machine_done)(Chardev *chr);
 } ChardevClass;
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
diff --git a/tests/test-char.c b/tests/test-char.c
index b358620911..e49af9e398 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -166,7 +166,6 @@ static void char_mux_test(void)
     FeHandler h1 = { 0, }, h2 = { 0, };
     CharBackend chr_be1, chr_be2;
 
-    muxes_realized = true; /* done after machine init */
     opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
                             1, &error_abort);
     qemu_opt_set(opts, "backend", "ringbuf", &error_abort);
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 5/9] chardev: introduce chr_machine_done hook

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:16PM +0800, Peter Xu wrote:
> Introduce ChardevClass.chr_machine_done() hook so that chardevs can run
> customized procedures after machine init.
> 
> There was an existing mux user already that did similar thing but used a
> raw machine done notifier.  Generalize it into a framework, and let the
> mux chardevs provide such a class-specific hook to achieve the same
> thing.  Then we can move the mux related code to the char-mux.c file.
> 
> Since at it, replace the mux_realized variable with the global
> machine_init_done varible.
> 
> This notifier framework will be further leverged by other type of
> chardevs soon.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-mux.c         | 33 +++++++++++++++++++++++++++++----
>  chardev/char.c             | 43 +++++++++++++++++--------------------------
>  include/chardev/char-mux.h |  2 --
>  include/chardev/char.h     |  2 ++
>  tests/test-char.c          |  1 -
>  5 files changed, 48 insertions(+), 33 deletions(-)

Reviewed-by: Daniel P. Berrangé <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 :|

[Qemu-devel] [PATCH v2 6/9] chardev: use chardev's gcontext for async connect

[Peter Xu]

Generalize the function to create the async QIO task connection.  Also,
fix the context pointer to use the chardev's gcontext.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index c22b3f330c..1ce5adad9a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -901,11 +901,22 @@ cleanup:
     object_unref(OBJECT(sioc));
 }
 
+static void tcp_chr_connect_async(Chardev *chr)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+    QIOChannelSocket *sioc;
+
+    sioc = qio_channel_socket_new();
+    tcp_chr_set_client_ioc_name(chr, sioc);
+    qio_channel_socket_connect_async(sioc, s->addr,
+                                     qemu_chr_socket_connected,
+                                     chr, NULL, chr->gcontext);
+}
+
 static gboolean socket_reconnect_timeout(gpointer opaque)
 {
     Chardev *chr = CHARDEV(opaque);
     SocketChardev *s = SOCKET_CHARDEV(opaque);
-    QIOChannelSocket *sioc;
 
     g_source_unref(s->reconnect_timer);
     s->reconnect_timer = NULL;
@@ -914,11 +925,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
         return false;
     }
 
-    sioc = qio_channel_socket_new();
-    tcp_chr_set_client_ioc_name(chr, sioc);
-    qio_channel_socket_connect_async(sioc, s->addr,
-                                     qemu_chr_socket_connected,
-                                     chr, NULL, NULL);
+    tcp_chr_connect_async(chr);
 
     return false;
 }
@@ -998,11 +1005,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     }
 
     if (s->reconnect_time) {
-        sioc = qio_channel_socket_new();
-        tcp_chr_set_client_ioc_name(chr, sioc);
-        qio_channel_socket_connect_async(sioc, s->addr,
-                                         qemu_chr_socket_connected,
-                                         chr, NULL, NULL);
+        tcp_chr_connect_async(chr);
     } else {
         if (s->is_listen) {
             char *name;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 6/9] chardev: use chardev's gcontext for async connect

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:17PM +0800, Peter Xu wrote:
> Generalize the function to create the async QIO task connection.  Also,
> fix the context pointer to use the chardev's gcontext.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <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 :|

[Qemu-devel] [PATCH v2 7/9] chardev: tcp: postpone async connection setup

[Peter Xu]

This patch allows the socket chardev async connection be setup with
non-default gcontext.  We do it by postponing the setup to machine done,
since until then we can know which context we should run the async
operation on.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1ce5adad9a..165612845a 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1004,9 +1004,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
         s->reconnect_time = reconnect;
     }
 
-    if (s->reconnect_time) {
-        tcp_chr_connect_async(chr);
-    } else {
+    /* If reconnect_time is set, will do that in chr_machine_done. */
+    if (!s->reconnect_time) {
         if (s->is_listen) {
             char *name;
             s->listener = qio_net_listener_new();
@@ -1138,6 +1137,17 @@ char_socket_get_connected(Object *obj, Error **errp)
     return s->connected;
 }
 
+static int tcp_chr_machine_done_hook(Chardev *chr)
+{
+    SocketChardev *s = SOCKET_CHARDEV(chr);
+
+    if (s->reconnect_time) {
+        tcp_chr_connect_async(chr);
+    }
+
+    return 0;
+}
+
 static void char_socket_class_init(ObjectClass *oc, void *data)
 {
     ChardevClass *cc = CHARDEV_CLASS(oc);
@@ -1153,6 +1163,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
     cc->chr_add_client = tcp_chr_add_client;
     cc->chr_add_watch = tcp_chr_add_watch;
     cc->chr_update_read_handler = tcp_chr_update_read_handler;
+    cc->chr_machine_done = tcp_chr_machine_done_hook;
 
     object_class_property_add(oc, "addr", "SocketAddress",
                               char_socket_get_addr, NULL,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 7/9] chardev: tcp: postpone async connection setup

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:18PM +0800, Peter Xu wrote:
> This patch allows the socket chardev async connection be setup with
> non-default gcontext.  We do it by postponing the setup to machine done,
> since until then we can know which context we should run the async
> operation on.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <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 :|

Re: [Qemu-devel] [PATCH v2 7/9] chardev: tcp: postpone async connection setup

[Marc-André Lureau]

Hi
On Tue, Mar 6, 2018 at 6:41 AM Peter Xu <peterx@redhat.com> wrote:
>
> This patch allows the socket chardev async connection be setup with
> non-default gcontext.  We do it by postponing the setup to machine done,
> since until then we can know which context we should run the async
> operation on.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1ce5adad9a..165612845a 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1004,9 +1004,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
>          s->reconnect_time = reconnect;
>      }
>
> -    if (s->reconnect_time) {
> -        tcp_chr_connect_async(chr);
> -    } else {
> +    /* If reconnect_time is set, will do that in chr_machine_done. */
> +    if (!s->reconnect_time) {
>          if (s->is_listen) {
>              char *name;
>              s->listener = qio_net_listener_new();
> @@ -1138,6 +1137,17 @@ char_socket_get_connected(Object *obj, Error **errp)
>      return s->connected;
>  }
>
> +static int tcp_chr_machine_done_hook(Chardev *chr)
> +{
> +    SocketChardev *s = SOCKET_CHARDEV(chr);
> +
> +    if (s->reconnect_time) {
> +        tcp_chr_connect_async(chr);
> +    }
> +
> +    return 0;
> +}

This patch broke /vhost-user/reconnect test, since it is using
reconnect chardev sockets under a test program, where no machine done
hook exist.

> +
>  static void char_socket_class_init(ObjectClass *oc, void *data)
>  {
>      ChardevClass *cc = CHARDEV_CLASS(oc);
> @@ -1153,6 +1163,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
>      cc->chr_add_client = tcp_chr_add_client;
>      cc->chr_add_watch = tcp_chr_add_watch;
>      cc->chr_update_read_handler = tcp_chr_update_read_handler;
> +    cc->chr_machine_done = tcp_chr_machine_done_hook;
>
>      object_class_property_add(oc, "addr", "SocketAddress",
>                                char_socket_get_addr, NULL,
> --
> 2.14.3
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 7/9] chardev: tcp: postpone async connection setup

[Marc-André Lureau]

Hi
On Thu, Aug 16, 2018 at 7:49 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
> On Tue, Mar 6, 2018 at 6:41 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > This patch allows the socket chardev async connection be setup with
> > non-default gcontext.  We do it by postponing the setup to machine done,
> > since until then we can know which context we should run the async
> > operation on.
> >
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 1ce5adad9a..165612845a 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1004,9 +1004,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
> >          s->reconnect_time = reconnect;
> >      }
> >
> > -    if (s->reconnect_time) {
> > -        tcp_chr_connect_async(chr);
> > -    } else {
> > +    /* If reconnect_time is set, will do that in chr_machine_done. */
> > +    if (!s->reconnect_time) {
> >          if (s->is_listen) {
> >              char *name;
> >              s->listener = qio_net_listener_new();
> > @@ -1138,6 +1137,17 @@ char_socket_get_connected(Object *obj, Error **errp)
> >      return s->connected;
> >  }
> >
> > +static int tcp_chr_machine_done_hook(Chardev *chr)
> > +{
> > +    SocketChardev *s = SOCKET_CHARDEV(chr);
> > +
> > +    if (s->reconnect_time) {
> > +        tcp_chr_connect_async(chr);
> > +    }
> > +
> > +    return 0;
> > +}
>
> This patch broke /vhost-user/reconnect test, since it is using
> reconnect chardev sockets under a test program, where no machine done
> hook exist.
>

What about chardev created after machine done?

> > +
> >  static void char_socket_class_init(ObjectClass *oc, void *data)
> >  {
> >      ChardevClass *cc = CHARDEV_CLASS(oc);
> > @@ -1153,6 +1163,7 @@ static void char_socket_class_init(ObjectClass *oc, void *data)
> >      cc->chr_add_client = tcp_chr_add_client;
> >      cc->chr_add_watch = tcp_chr_add_watch;
> >      cc->chr_update_read_handler = tcp_chr_update_read_handler;
> > +    cc->chr_machine_done = tcp_chr_machine_done_hook;
> >
> >      object_class_property_add(oc, "addr", "SocketAddress",
> >                                char_socket_get_addr, NULL,
> > --
> > 2.14.3
> >
> >
>
>
> --
> Marc-André Lureau



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 7/9] chardev: tcp: postpone async connection setup

[Peter Xu]

On Thu, Aug 16, 2018 at 08:27:40PM +0200, Marc-André Lureau wrote:
> Hi
> On Thu, Aug 16, 2018 at 7:49 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> > On Tue, Mar 6, 2018 at 6:41 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > This patch allows the socket chardev async connection be setup with
> > > non-default gcontext.  We do it by postponing the setup to machine done,
> > > since until then we can know which context we should run the async
> > > operation on.
> > >
> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  chardev/char-socket.c | 17 ++++++++++++++---
> > >  1 file changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 1ce5adad9a..165612845a 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -1004,9 +1004,8 @@ static void qmp_chardev_open_socket(Chardev *chr,
> > >          s->reconnect_time = reconnect;
> > >      }
> > >
> > > -    if (s->reconnect_time) {
> > > -        tcp_chr_connect_async(chr);
> > > -    } else {
> > > +    /* If reconnect_time is set, will do that in chr_machine_done. */
> > > +    if (!s->reconnect_time) {
> > >          if (s->is_listen) {
> > >              char *name;
> > >              s->listener = qio_net_listener_new();
> > > @@ -1138,6 +1137,17 @@ char_socket_get_connected(Object *obj, Error **errp)
> > >      return s->connected;
> > >  }
> > >
> > > +static int tcp_chr_machine_done_hook(Chardev *chr)
> > > +{
> > > +    SocketChardev *s = SOCKET_CHARDEV(chr);
> > > +
> > > +    if (s->reconnect_time) {
> > > +        tcp_chr_connect_async(chr);
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> >
> > This patch broke /vhost-user/reconnect test, since it is using
> > reconnect chardev sockets under a test program, where no machine done
> > hook exist.
> >
> 
> What about chardev created after machine done?

Hmm, so binding that with machine done might be problematic...

I'm thinking whether we can do that in ->chr_update_read_handler()
instead.

Btw, is there a short answer on why we disabled the reconnection test
in vhost-user-test?  Could I just verify the problem using:

QTEST_VHOST_USER_FIXME=1 QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 ./tests/vhost-user-test

?  Or any easy way to reproduce/verify the problem?

Regards,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 8/9] chardev: tcp: let TLS run on chardev context

[Peter Xu]

Now qio_channel_tls_handshake() is ready to receive the context.  Let
socket chardev use it, then the TLS handshake of chardev will always be
with the chardev's context.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 165612845a..bd40864f87 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -750,7 +750,7 @@ static void tcp_chr_tls_init(Chardev *chr)
                               tcp_chr_tls_handshake,
                               chr,
                               NULL,
-                              NULL);
+                              chr->gcontext);
 }
 
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 8/9] chardev: tcp: let TLS run on chardev context

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:19PM +0800, Peter Xu wrote:
> Now qio_channel_tls_handshake() is ready to receive the context.  Let
> socket chardev use it, then the TLS handshake of chardev will always be
> with the chardev's context.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <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 :|

[Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

TLS handshake may create background GSource tasks, while we won't know
the correct GMainContext until the whole chardev (including frontend)
inited.  Let's postpone the initial TLS handshake until machine done.

For dynamically created tcp chardev, we don't postpone that by checking
the init_machine_done variable.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-socket.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bd40864f87..997c70dd7d 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -31,6 +31,7 @@
 #include "qemu/option.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
+#include "sysemu/sysemu.h"
 
 #include "chardev/char-io.h"
 
@@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
     Error *err = NULL;
     gchar *name;
 
+    if (!machine_init_done) {
+        /* This will be postponed to machine_done notifier */
+        return;
+    }
+
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
@@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
         tcp_chr_connect_async(chr);
     }
 
+    if (s->tls_creds) {
+        tcp_chr_tls_init(chr);
+    }
+
     return 0;
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Daniel P. Berrangé]

On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
> TLS handshake may create background GSource tasks, while we won't know
> the correct GMainContext until the whole chardev (including frontend)
> inited.  Let's postpone the initial TLS handshake until machine done.
> 
> For dynamically created tcp chardev, we don't postpone that by checking
> the init_machine_done variable.

Not sure I see the need for this one - we've already postponed the
acceptance of a client in the patch 7.

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bd40864f87..997c70dd7d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -31,6 +31,7 @@
>  #include "qemu/option.h"
>  #include "qapi/error.h"
>  #include "qapi/clone-visitor.h"
> +#include "sysemu/sysemu.h"
>  
>  #include "chardev/char-io.h"
>  
> @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
>      Error *err = NULL;
>      gchar *name;
>  
> +    if (!machine_init_done) {
> +        /* This will be postponed to machine_done notifier */
> +        return;
> +    }
> +
>      if (s->is_listen) {
>          tioc = qio_channel_tls_new_server(
>              s->ioc, s->tls_creds,
> @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
>          tcp_chr_connect_async(chr);
>      }
>  
> +    if (s->tls_creds) {
> +        tcp_chr_tls_init(chr);
> +    }

This looks questionable - AFAICT, there's no guarantee we have any
client connection active when the machine dnoe hook runs. Only if
the chardev is set in client mode, and reconnect_time is *not* set,
but this seems to be run unconditionally.


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

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Daniel P. Berrangé]

On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
> > TLS handshake may create background GSource tasks, while we won't know
> > the correct GMainContext until the whole chardev (including frontend)
> > inited.  Let's postpone the initial TLS handshake until machine done.
> > 
> > For dynamically created tcp chardev, we don't postpone that by checking
> > the init_machine_done variable.
> 
> Not sure I see the need for this one - we've already postponed the
> acceptance of a client in the patch 7.

Opps, meant to remove this comment, in favour of the later comment - ignore
this bit.

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index bd40864f87..997c70dd7d 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -31,6 +31,7 @@
> >  #include "qemu/option.h"
> >  #include "qapi/error.h"
> >  #include "qapi/clone-visitor.h"
> > +#include "sysemu/sysemu.h"
> >  
> >  #include "chardev/char-io.h"
> >  
> > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> >      Error *err = NULL;
> >      gchar *name;
> >  
> > +    if (!machine_init_done) {
> > +        /* This will be postponed to machine_done notifier */
> > +        return;
> > +    }
> > +
> >      if (s->is_listen) {
> >          tioc = qio_channel_tls_new_server(
> >              s->ioc, s->tls_creds,
> > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> >          tcp_chr_connect_async(chr);
> >      }
> >  
> > +    if (s->tls_creds) {
> > +        tcp_chr_tls_init(chr);
> > +    }
> 
> This looks questionable - AFAICT, there's no guarantee we have any
> client connection active when the machine dnoe hook runs. Only if
> the chardev is set in client mode, and reconnect_time is *not* set,
> but this seems to be run unconditionally.
> 
> 
> 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 :|
> 

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

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Paolo Bonzini]

On 07/03/2018 13:40, Daniel P. Berrangé wrote:
> On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
>> On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
>>> TLS handshake may create background GSource tasks, while we won't know
>>> the correct GMainContext until the whole chardev (including frontend)
>>> inited.  Let's postpone the initial TLS handshake until machine done.
>>>
>>> For dynamically created tcp chardev, we don't postpone that by checking
>>> the init_machine_done variable.
>>
>> Not sure I see the need for this one - we've already postponed the
>> acceptance of a client in the patch 7.
> 
> Opps, meant to remove this comment, in favour of the later comment - ignore
> this bit.

Since time is ticking for soft freeze, I'll queue the series without
this patch.

Paolo

>>
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  chardev/char-socket.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>>> index bd40864f87..997c70dd7d 100644
>>> --- a/chardev/char-socket.c
>>> +++ b/chardev/char-socket.c
>>> @@ -31,6 +31,7 @@
>>>  #include "qemu/option.h"
>>>  #include "qapi/error.h"
>>>  #include "qapi/clone-visitor.h"
>>> +#include "sysemu/sysemu.h"
>>>  
>>>  #include "chardev/char-io.h"
>>>  
>>> @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
>>>      Error *err = NULL;
>>>      gchar *name;
>>>  
>>> +    if (!machine_init_done) {
>>> +        /* This will be postponed to machine_done notifier */
>>> +        return;
>>> +    }
>>> +
>>>      if (s->is_listen) {
>>>          tioc = qio_channel_tls_new_server(
>>>              s->ioc, s->tls_creds,
>>> @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
>>>          tcp_chr_connect_async(chr);
>>>      }
>>>  
>>> +    if (s->tls_creds) {
>>> +        tcp_chr_tls_init(chr);
>>> +    }
>>
>> This looks questionable - AFAICT, there's no guarantee we have any
>> client connection active when the machine dnoe hook runs. Only if
>> the chardev is set in client mode, and reconnect_time is *not* set,
>> but this seems to be run unconditionally.
>>
>>
>> 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 :|
>>
> 
> Regards,
> Daniel
> 

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

On Wed, Mar 07, 2018 at 04:06:53PM +0100, Paolo Bonzini wrote:
> On 07/03/2018 13:40, Daniel P. Berrangé wrote:
> > On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
> >> On Tue, Mar 06, 2018 at 01:33:20PM +0800, Peter Xu wrote:
> >>> TLS handshake may create background GSource tasks, while we won't know
> >>> the correct GMainContext until the whole chardev (including frontend)
> >>> inited.  Let's postpone the initial TLS handshake until machine done.
> >>>
> >>> For dynamically created tcp chardev, we don't postpone that by checking
> >>> the init_machine_done variable.
> >>
> >> Not sure I see the need for this one - we've already postponed the
> >> acceptance of a client in the patch 7.
> > 
> > Opps, meant to remove this comment, in favour of the later comment - ignore
> > this bit.
> 
> Since time is ticking for soft freeze, I'll queue the series without
> this patch.

Thanks Paolo.

Note that Dan's pull is still not merged yet, and this series will
need that one.

I'll try to see whether I can prepare a good version for the last TLS
patch before you start to test your next pull request.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:

[...]

> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index bd40864f87..997c70dd7d 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -31,6 +31,7 @@
> >  #include "qemu/option.h"
> >  #include "qapi/error.h"
> >  #include "qapi/clone-visitor.h"
> > +#include "sysemu/sysemu.h"
> >  
> >  #include "chardev/char-io.h"
> >  
> > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> >      Error *err = NULL;
> >      gchar *name;
> >  
> > +    if (!machine_init_done) {
> > +        /* This will be postponed to machine_done notifier */
> > +        return;
> > +    }
> > +
> >      if (s->is_listen) {
> >          tioc = qio_channel_tls_new_server(
> >              s->ioc, s->tls_creds,
> > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> >          tcp_chr_connect_async(chr);
> >      }
> >  
> > +    if (s->tls_creds) {
> > +        tcp_chr_tls_init(chr);
> > +    }
> 
> This looks questionable - AFAICT, there's no guarantee we have any
> client connection active when the machine dnoe hook runs. Only if
> the chardev is set in client mode, and reconnect_time is *not* set,
> but this seems to be run unconditionally.

You are right.  Thanks for spotting that.

Then how about this?  It's a bit ugly, but I think it should be safe:

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index bd40864f87..b4686fd23f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -31,6 +31,7 @@
 #include "qemu/option.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
+#include "sysemu/sysemu.h"

 #include "chardev/char-io.h"

@@ -51,6 +52,11 @@ typedef struct {
     QIONetListener *listener;
     GSource *hup_source;
     QCryptoTLSCreds *tls_creds;
+    /*
+     * This should only be used once - when we want to setup TLS for
+     * the session but we need to wait until machine init done.
+     */
+    bool tls_need_postponed_init;
     int connected;
     int max_size;
     int do_telnetopt;
@@ -791,7 +797,15 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
     }

     if (s->tls_creds) {
-        tcp_chr_tls_init(chr);
+        if (machine_init_done) {
+            tcp_chr_tls_init(chr);
+        } else {
+            /*
+             * Postpone to machine init done since we need the correct
+             * context to setup the TLS handshake.
+             */
+            s->tls_need_postponed_init = true;
+        }
     } else {
         if (s->do_telnetopt) {
             tcp_chr_telnet_init(chr);
@@ -1145,6 +1159,11 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
         tcp_chr_connect_async(chr);
     }

+    if (s->tls_need_postponed_init) {
+        assert(s->tls_creds);
+        tcp_chr_tls_init(chr);
+    }
+
     return 0;
 }

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Daniel P. Berrangé]

On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
> 
> [...]
> 
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index bd40864f87..997c70dd7d 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -31,6 +31,7 @@
> > >  #include "qemu/option.h"
> > >  #include "qapi/error.h"
> > >  #include "qapi/clone-visitor.h"
> > > +#include "sysemu/sysemu.h"
> > >  
> > >  #include "chardev/char-io.h"
> > >  
> > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > >      Error *err = NULL;
> > >      gchar *name;
> > >  
> > > +    if (!machine_init_done) {
> > > +        /* This will be postponed to machine_done notifier */
> > > +        return;
> > > +    }
> > > +
> > >      if (s->is_listen) {
> > >          tioc = qio_channel_tls_new_server(
> > >              s->ioc, s->tls_creds,
> > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > >          tcp_chr_connect_async(chr);
> > >      }
> > >  
> > > +    if (s->tls_creds) {
> > > +        tcp_chr_tls_init(chr);
> > > +    }
> > 
> > This looks questionable - AFAICT, there's no guarantee we have any
> > client connection active when the machine dnoe hook runs. Only if
> > the chardev is set in client mode, and reconnect_time is *not* set,
> > but this seems to be run unconditionally.
> 
> You are right.  Thanks for spotting that.
> 
> Then how about this?  It's a bit ugly, but I think it should be safe:

Is it perhaps not possible to just check if  's->ioc' is non-NULL
in the tcp_chr_machine_done_hook for your original patch ?

> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index bd40864f87..b4686fd23f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -31,6 +31,7 @@
>  #include "qemu/option.h"
>  #include "qapi/error.h"
>  #include "qapi/clone-visitor.h"
> +#include "sysemu/sysemu.h"
> 
>  #include "chardev/char-io.h"
> 
> @@ -51,6 +52,11 @@ typedef struct {
>      QIONetListener *listener;
>      GSource *hup_source;
>      QCryptoTLSCreds *tls_creds;
> +    /*
> +     * This should only be used once - when we want to setup TLS for
> +     * the session but we need to wait until machine init done.
> +     */
> +    bool tls_need_postponed_init;
>      int connected;
>      int max_size;
>      int do_telnetopt;
> @@ -791,7 +797,15 @@ static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc)
>      }
> 
>      if (s->tls_creds) {
> -        tcp_chr_tls_init(chr);
> +        if (machine_init_done) {
> +            tcp_chr_tls_init(chr);
> +        } else {
> +            /*
> +             * Postpone to machine init done since we need the correct
> +             * context to setup the TLS handshake.
> +             */
> +            s->tls_need_postponed_init = true;
> +        }
>      } else {
>          if (s->do_telnetopt) {
>              tcp_chr_telnet_init(chr);
> @@ -1145,6 +1159,11 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
>          tcp_chr_connect_async(chr);
>      }
> 
> +    if (s->tls_need_postponed_init) {
> +        assert(s->tls_creds);
> +        tcp_chr_tls_init(chr);
> +    }
> +
>      return 0;
>  }
> 
> Thanks,
> 
> -- 
> Peter Xu

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

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

On Thu, Mar 08, 2018 at 10:13:59AM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> > On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
> > 
> > [...]
> > 
> > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > index bd40864f87..997c70dd7d 100644
> > > > --- a/chardev/char-socket.c
> > > > +++ b/chardev/char-socket.c
> > > > @@ -31,6 +31,7 @@
> > > >  #include "qemu/option.h"
> > > >  #include "qapi/error.h"
> > > >  #include "qapi/clone-visitor.h"
> > > > +#include "sysemu/sysemu.h"
> > > >  
> > > >  #include "chardev/char-io.h"
> > > >  
> > > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > >      Error *err = NULL;
> > > >      gchar *name;
> > > >  
> > > > +    if (!machine_init_done) {
> > > > +        /* This will be postponed to machine_done notifier */
> > > > +        return;
> > > > +    }
> > > > +
> > > >      if (s->is_listen) {
> > > >          tioc = qio_channel_tls_new_server(
> > > >              s->ioc, s->tls_creds,
> > > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > > >          tcp_chr_connect_async(chr);
> > > >      }
> > > >  
> > > > +    if (s->tls_creds) {
> > > > +        tcp_chr_tls_init(chr);
> > > > +    }
> > > 
> > > This looks questionable - AFAICT, there's no guarantee we have any
> > > client connection active when the machine dnoe hook runs. Only if
> > > the chardev is set in client mode, and reconnect_time is *not* set,
> > > but this seems to be run unconditionally.
> > 
> > You are right.  Thanks for spotting that.
> > 
> > Then how about this?  It's a bit ugly, but I think it should be safe:
> 
> Is it perhaps not possible to just check if  's->ioc' is non-NULL
> in the tcp_chr_machine_done_hook for your original patch ?

In tcp_chr_new_client() I see that s->ioc will be set no matter what,
and that function can even be called without TLS enabled I think.
Does that mean only check against s->ioc would not be enough?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Daniel P. Berrangé]

On Thu, Mar 08, 2018 at 07:42:13PM +0800, Peter Xu wrote:
> On Thu, Mar 08, 2018 at 10:13:59AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> > > On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
> > > 
> > > [...]
> > > 
> > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > > index bd40864f87..997c70dd7d 100644
> > > > > --- a/chardev/char-socket.c
> > > > > +++ b/chardev/char-socket.c
> > > > > @@ -31,6 +31,7 @@
> > > > >  #include "qemu/option.h"
> > > > >  #include "qapi/error.h"
> > > > >  #include "qapi/clone-visitor.h"
> > > > > +#include "sysemu/sysemu.h"
> > > > >  
> > > > >  #include "chardev/char-io.h"
> > > > >  
> > > > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > > >      Error *err = NULL;
> > > > >      gchar *name;
> > > > >  
> > > > > +    if (!machine_init_done) {
> > > > > +        /* This will be postponed to machine_done notifier */
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > >      if (s->is_listen) {
> > > > >          tioc = qio_channel_tls_new_server(
> > > > >              s->ioc, s->tls_creds,
> > > > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > > > >          tcp_chr_connect_async(chr);
> > > > >      }
> > > > >  
> > > > > +    if (s->tls_creds) {
> > > > > +        tcp_chr_tls_init(chr);
> > > > > +    }
> > > > 
> > > > This looks questionable - AFAICT, there's no guarantee we have any
> > > > client connection active when the machine dnoe hook runs. Only if
> > > > the chardev is set in client mode, and reconnect_time is *not* set,
> > > > but this seems to be run unconditionally.
> > > 
> > > You are right.  Thanks for spotting that.
> > > 
> > > Then how about this?  It's a bit ugly, but I think it should be safe:
> > 
> > Is it perhaps not possible to just check if  's->ioc' is non-NULL
> > in the tcp_chr_machine_done_hook for your original patch ?
> 
> In tcp_chr_new_client() I see that s->ioc will be set no matter what,
> and that function can even be called without TLS enabled I think.
> Does that mean only check against s->ioc would not be enough?

I mean like this:

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index a1966aae51..19e3193817 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -723,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
     Error *err = NULL;
     gchar *name;
 
+    if (!machine_init_done) {
+        /* This will be postponed to machine_done notifier */
+        return;
+    }
+
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
@@ -1146,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
         tcp_chr_connect_async(chr);
     }
 
+    if (s->ioc && s->tls_creds) {
+        tcp_chr_tls_init(chr);
+    }
+
     return 0;
 }
 


s->ioc will only be != NULL, if some codepath in qemu_chr_parse_socket
ended up calling tcp_chr_new_client(). If that happened we will have
skipped setup of TLS, so calling tcp_chr_tls_init() based on
s->ioc && s->tls_creds feels right to me.

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

Re: [Qemu-devel] [PATCH v2 9/9] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

On Thu, Mar 08, 2018 at 01:31:43PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 08, 2018 at 07:42:13PM +0800, Peter Xu wrote:
> > On Thu, Mar 08, 2018 at 10:13:59AM +0000, Daniel P. Berrangé wrote:
> > > On Thu, Mar 08, 2018 at 11:44:09AM +0800, Peter Xu wrote:
> > > > On Wed, Mar 07, 2018 at 12:36:50PM +0000, Daniel P. Berrangé wrote:
> > > > 
> > > > [...]
> > > > 
> > > > > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > > > > index bd40864f87..997c70dd7d 100644
> > > > > > --- a/chardev/char-socket.c
> > > > > > +++ b/chardev/char-socket.c
> > > > > > @@ -31,6 +31,7 @@
> > > > > >  #include "qemu/option.h"
> > > > > >  #include "qapi/error.h"
> > > > > >  #include "qapi/clone-visitor.h"
> > > > > > +#include "sysemu/sysemu.h"
> > > > > >  
> > > > > >  #include "chardev/char-io.h"
> > > > > >  
> > > > > > @@ -722,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
> > > > > >      Error *err = NULL;
> > > > > >      gchar *name;
> > > > > >  
> > > > > > +    if (!machine_init_done) {
> > > > > > +        /* This will be postponed to machine_done notifier */
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > >      if (s->is_listen) {
> > > > > >          tioc = qio_channel_tls_new_server(
> > > > > >              s->ioc, s->tls_creds,
> > > > > > @@ -1145,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
> > > > > >          tcp_chr_connect_async(chr);
> > > > > >      }
> > > > > >  
> > > > > > +    if (s->tls_creds) {
> > > > > > +        tcp_chr_tls_init(chr);
> > > > > > +    }
> > > > > 
> > > > > This looks questionable - AFAICT, there's no guarantee we have any
> > > > > client connection active when the machine dnoe hook runs. Only if
> > > > > the chardev is set in client mode, and reconnect_time is *not* set,
> > > > > but this seems to be run unconditionally.
> > > > 
> > > > You are right.  Thanks for spotting that.
> > > > 
> > > > Then how about this?  It's a bit ugly, but I think it should be safe:
> > > 
> > > Is it perhaps not possible to just check if  's->ioc' is non-NULL
> > > in the tcp_chr_machine_done_hook for your original patch ?
> > 
> > In tcp_chr_new_client() I see that s->ioc will be set no matter what,
> > and that function can even be called without TLS enabled I think.
> > Does that mean only check against s->ioc would not be enough?
> 
> I mean like this:
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index a1966aae51..19e3193817 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -723,6 +723,11 @@ static void tcp_chr_tls_init(Chardev *chr)
>      Error *err = NULL;
>      gchar *name;
>  
> +    if (!machine_init_done) {
> +        /* This will be postponed to machine_done notifier */
> +        return;
> +    }
> +
>      if (s->is_listen) {
>          tioc = qio_channel_tls_new_server(
>              s->ioc, s->tls_creds,
> @@ -1146,6 +1151,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
>          tcp_chr_connect_async(chr);
>      }
>  
> +    if (s->ioc && s->tls_creds) {
> +        tcp_chr_tls_init(chr);
> +    }
> +
>      return 0;
>  }
>  
> 
> 
> s->ioc will only be != NULL, if some codepath in qemu_chr_parse_socket

I guess here you mean qmp_chardev_open_socket().

> ended up calling tcp_chr_new_client(). If that happened we will have
> skipped setup of TLS, so calling tcp_chr_tls_init() based on
> s->ioc && s->tls_creds feels right to me.

Yes, it looks right to me too. :)

I'll post a complete patch soon with your authorship.

Thanks!

-- 
Peter Xu

[Qemu-devel] [PATCH v2.1 9/9] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

From: "Daniel P. Berrange" <berrange@redhat.com>

TLS handshake may create background GSource tasks, while we won't know
the correct GMainContext until the whole chardev (including frontend)
inited.  Let's postpone the initial TLS handshake until machine done.

For dynamically created tcp chardev, we don't postpone that by checking
the init_machine_done variable.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
[peterx: add missing include line, do unit test]
Signed-off-by: Peter Xu <peterx@redhat.com>
---

Paolo: please feel free to review/queue this one if your pull test has
not yet started.  Thanks.

 chardev/char-socket.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index a1966aae51..f02bf118c9 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -32,6 +32,7 @@
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
+#include "sysemu/sysemu.h"
 
 #include "chardev/char-io.h"
 
@@ -723,6 +724,11 @@ static void tcp_chr_tls_init(Chardev *chr)
     Error *err = NULL;
     gchar *name;
 
+    if (!machine_init_done) {
+        /* This will be postponed to machine_done notifier */
+        return;
+    }
+
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
@@ -1146,6 +1152,10 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
         tcp_chr_connect_async(chr);
     }
 
+    if (s->ioc && s->tls_creds) {
+        tcp_chr_tls_init(chr);
+    }
+
     return 0;
 }
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 0/9] chardev: qio related non-default context support

[Stefan Hajnoczi]

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

On Tue, Mar 06, 2018 at 01:33:11PM +0800, Peter Xu wrote:
> Based-on: <20180305064324.9238-1-peterx@redhat.com>
> 
> This series is based on the QIO part:
>   [PATCH v3 0/6] qio: general non-default GMainContext support
> 
> v2:
> - fix the reported problem by patchew in patch 5
> - added some r-bs from Marc-Andre
> 
> Please review, thanks.
> 
> Peter Xu (9):
>   vl: export machine_init_done
>   chardev: fix leak in tcp_chr_telnet_init_io()
>   chardev: update net listener gcontext
>   chardev: allow telnet gsource to switch gcontext
>   chardev: introduce chr_machine_done hook
>   chardev: use chardev's gcontext for async connect
>   chardev: tcp: postpone async connection setup
>   chardev: tcp: let TLS run on chardev context
>   chardev: tcp: postpone TLS work until machine done
> 
>  chardev/char-mux.c         |  33 ++++++++--
>  chardev/char-socket.c      | 153 ++++++++++++++++++++++++++++++++++-----------
>  chardev/char.c             |  43 +++++--------
>  include/chardev/char-mux.h |   2 -
>  include/chardev/char.h     |   2 +
>  include/sysemu/sysemu.h    |   2 +
>  stubs/machine-init-done.c  |   2 +
>  tests/test-char.c          |   1 -
>  vl.c                       |   4 +-
>  9 files changed, 169 insertions(+), 73 deletions(-)

I'm not very familiar with chardev or qio, so I defer this to Daniel
Berrange:

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]