chardev: qio related non-default context support

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 <[email protected]>

Reviewed-by Marc-André Lureau <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 include/sysemu/sysemu.h   | 2 ++
 stubs/machine-init-done.c | 2 ++
 vl.c                      | 4 ++--
 3 files changed, 6 insertions(+), 2 deletions(-)

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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

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 <[email protected]>

---
 chardev/char-socket.c | 67 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 16 deletions(-)

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 <[email protected]>

---
 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(-)

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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

chardev/char-socket.c +2 -3

index 1ce5adad9a..165612845a 100644
@@ -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;
 }
 

chardev/char-socket.c +2 -1

+
 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

Hi

Quoted 32 lines · 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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

chardev/char-socket.c +2 -3

index 1ce5adad9a..165612845a 100644
@@ -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;
 }
 

chardev/char-socket.c char_socket_get_connected(Object *obj, Error **errp) · line 1149

Quoted 10 lines

chardev/char-socket.c +10 -0

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

Quoted 13 lines

chardev/char-socket.c +2 -1

+
 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,
--

Quoted 3 lines

2.14.3

--
Marc-André Lureau

Hi

Quoted 35 lines · Marc-André Lureau

Hi

Quoted 32 lines · 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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 chardev/char-socket.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

chardev/char-socket.c +2 -3

index 1ce5adad9a..165612845a 100644
@@ -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;
 }
 

Quoted 15 lines

chardev/char-socket.c char_socket_get_connected(Object *obj, Error **errp) · line 1149

Quoted 10 lines

chardev/char-socket.c +10 -0

+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?

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 <[email protected]>

---
 chardev/char-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 <[email protected]>

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

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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,

Quoted 6 lines · 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.

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

Quoted 32 lines

Signed-off-by Peter Xu <[email protected]>

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

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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,

chardev/char-socket.c static int tcp_chr_machine_done_hook(Chardev *chr) · line 1156

Quoted 6 lines

chardev/char-socket.c +3 -0

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

Signature · 3 lines

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

Quoted 5 lines · Daniel P. Berrangé

Quoted 4 lines · 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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 45 lines

Quoted 32 lines

Signed-off-by Peter Xu <[email protected]>

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

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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,

chardev/char-socket.c static int tcp_chr_machine_done_hook(Chardev *chr) · line 1156

Quoted 6 lines

chardev/char-socket.c +3 -0

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

Quoted 7 lines

Regards,
Daniel

Signature · 3 lines

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

Quoted 9 lines · Daniel P. Berrangé

Quoted 8 lines · Daniel P. Berrangé

Quoted 6 lines · 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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 28 lines

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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);
     }
 

chardev/char-socket.c static int tcp_chr_machine_done_hook(Chardev *chr) · line 1156

Quoted 3 lines

chardev/char-socket.c +3 -0

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

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

[...]

Quoted 28 lines

Quoted 28 lines

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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);
     }
 

Quoted 8 lines

chardev/char-socket.c static int tcp_chr_new_client(Chardev *chr, QIOChannelSocket *sioc) · line 808

Quoted 3 lines

chardev/char-socket.c +3 -0

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

chardev/char-socket.c +20 -1

index bd40864f87..b4686fd23f 100644
@@ -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,

Signature · 1 line

Peter Xu

Quoted 41 lines · Peter Xu

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

[...]

Quoted 36 lines

Quoted 25 lines

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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,

chardev/char-socket.c static int tcp_chr_machine_done_hook(Chardev *chr) · line 1156

Quoted 6 lines

chardev/char-socket.c +3 -0

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

Quoted 3 lines

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 ?

Quoted 44 lines · Daniel P. Berrangé

Quoted 43 lines · Peter Xu

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

[...]

Quoted 28 lines

Quoted 28 lines

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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);
     }
 

Quoted 8 lines

chardev/char-socket.c static int tcp_chr_machine_done_hook(Chardev *chr) · line 1156

Quoted 3 lines

chardev/char-socket.c +3 -0

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

Quoted 4 lines

Quoted 1 line

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?

Quoted 45 lines · Peter Xu

Quoted 44 lines · Daniel P. Berrangé

Quoted 43 lines · Peter Xu

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

[...]

Quoted 28 lines

Quoted 28 lines

chardev/char-socket.c +6 -0

index bd40864f87..997c70dd7d 100644
@@ -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);
     }
 

Quoted 8 lines

chardev/char-socket.c static int tcp_chr_machine_done_hook(Chardev *chr) · line 1156

Quoted 3 lines

chardev/char-socket.c +3 -0

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

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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:

chardev/char-socket.c +9 -0

index a1966aae51..19e3193817 100644
@@ -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.

Based-on: <[email protected]>

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(-)