qio: general non-default GMainContext support

Need to free TCPChardevTelnetInit when session established.

Since at it, switch to use G_SOURCE_* macros.

Reviewed-by Daniel P. Berrange <[email protected]>

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

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

chardev/char-socket.c +2 -2

index bdd6cff5f6..43a2cc2c1c 100644
@@ -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);

Quoted 40 lines · Peter Xu

Need to free TCPChardevTelnetInit when session established.

Since at it, switch to use G_SOURCE_* macros.

Reviewed-by Daniel P. Berrange <[email protected]>

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

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

chardev/char-socket.c +7 -3

index bdd6cff5f6..43a2cc2c1c 100644
@@ -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;
 }
 

chardev/char-socket.c static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc, · line 613

Quoted 2 lines

chardev/char-socket.c +0 -0

 static void tcp_chr_telnet_init(Chardev *chr)

Queued, thanks (but it's okay if you post it again in v3, because I'm not sure I'll be able to send a pull request tomorrow).

Firstly, introduce an internal qio_channel_add_watch_full(), which enhances qio_channel_add_watch() that context can be specified.

Then add a new API wrapper qio_channel_add_watch_source() to return a GSource pointer rather than a tag ID.

Note that the _source() call will keep a reference of GSource so that callers need to unref them explicitly when finished using the GSource.

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

---
 include/io/channel.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 io/channel.c         | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 78 insertions(+), 6 deletions(-)

+ * qio_channel_add_watch_source:
+ * @ioc: the channel object
+ * @condition: the I/O condition to monitor
+ * @func: callback to invoke when the source becomes ready
+ * @user_data: opaque data to pass to @func
+ * @notify: callback to free @user_data
+ * @context: gcontext to bind the source to
+ *
+ * Similar as qio_channel_add_watch(), but allows to specify context
+ * to run the watch source, meanwhile return the GSource object
+ * instead of tag ID, with the GSource referenced already.
+ *
+ * Note: callers is responsible to unref the source when not needed.
+ *
+ * Returns: the source pointer
+ */

Quoted 17 lines · Peter Xu

+ * qio_channel_add_watch_source:
+ * @ioc: the channel object
+ * @condition: the I/O condition to monitor
+ * @func: callback to invoke when the source becomes ready
+ * @user_data: opaque data to pass to @func
+ * @notify: callback to free @user_data
+ * @context: gcontext to bind the source to
+ *
+ * Similar as qio_channel_add_watch(), but allows to specify context
+ * to run the watch source, meanwhile return the GSource object
+ * instead of tag ID, with the GSource referenced already.
+ *
+ * Note: callers is responsible to unref the source when not needed.
+ *
+ * Returns: the source pointer
+ */

include/io/channel.h guint qio_channel_add_watch(QIOChannel *ioc, · line 694

Quoted 6 lines

include/io/channel.h +6 -0

+GSource *qio_channel_add_watch_source(QIOChannel *ioc,
+                                      GIOCondition condition,
+                                      QIOChannelFunc func,
+                                      gpointer user_data,
+                                      GDestroyNotify notify,
+                                      GMainContext *context);

Just a small thing, this is a bit inconsistent with the rest of the GSource API, where the g_source_attach is usually left to the caller when a function returns GSource *.

You might therefore name it instead qio_channel_create_watch, for consistency with g_io_{add,create}_watch, and remove the "context" argument.

Quoted 29 lines · Paolo Bonzini

Quoted 17 lines · Peter Xu

+ * qio_channel_add_watch_source:
+ * @ioc: the channel object
+ * @condition: the I/O condition to monitor
+ * @func: callback to invoke when the source becomes ready
+ * @user_data: opaque data to pass to @func
+ * @notify: callback to free @user_data
+ * @context: gcontext to bind the source to
+ *
+ * Similar as qio_channel_add_watch(), but allows to specify context
+ * to run the watch source, meanwhile return the GSource object
+ * instead of tag ID, with the GSource referenced already.
+ *
+ * Note: callers is responsible to unref the source when not needed.
+ *
+ * Returns: the source pointer
+ */

include/io/channel.h guint qio_channel_add_watch(QIOChannel *ioc, · line 694

Quoted 6 lines

include/io/channel.h +6 -0

+GSource *qio_channel_add_watch_source(QIOChannel *ioc,
+                                      GIOCondition condition,
+                                      QIOChannelFunc func,
+                                      gpointer user_data,
+                                      GDestroyNotify notify,
+                                      GMainContext *context);

Just a small thing, this is a bit inconsistent with the rest of the GSource API, where the g_source_attach is usually left to the caller when a function returns GSource *.

Quoted 2 lines

You might therefore name it instead qio_channel_create_watch, for consistency with g_io_{add,create}_watch, and remove the "context" argument.

Looks like there is already a qio_channel_create_watch() (io/channel.c).

How about qio_channel_create_watch_attached()? Or... anything better?

Quoted 17 lines · Peter Xu

+ * qio_channel_add_watch_source:
+ * @ioc: the channel object
+ * @condition: the I/O condition to monitor
+ * @func: callback to invoke when the source becomes ready
+ * @user_data: opaque data to pass to @func
+ * @notify: callback to free @user_data
+ * @context: gcontext to bind the source to
+ *
+ * Similar as qio_channel_add_watch(), but allows to specify context
+ * to run the watch source, meanwhile return the GSource object
+ * instead of tag ID, with the GSource referenced already.
+ *
+ * Note: callers is responsible to unref the source when not needed.
+ *
+ * Returns: the source pointer
+ */

include/io/channel.h guint qio_channel_add_watch(QIOChannel *ioc, · line 694

Quoted 6 lines

include/io/channel.h +6 -0

+GSource *qio_channel_add_watch_source(QIOChannel *ioc,
+                                      GIOCondition condition,
+                                      QIOChannelFunc func,
+                                      gpointer user_data,
+                                      GDestroyNotify notify,
+                                      GMainContext *context);

Just a small thing, this is a bit inconsistent with the rest of the GSource API, where the g_source_attach is usually left to the caller when a function returns GSource *.

Quoted 3 lines

Just a small thing, this is a bit inconsistent with the rest of the GSource API, where the g_source_attach is usually left to the caller when a function returns GSource *.

The APIs which return a GSource in glib typically don't even set the callback function - we already cover that scenario with the qio_channel_create_watch APIs.

GLib doesn't typically have APIs which return a GSource after the mix of creating the watch, setting callback & attaching to context.

They all just return the watch ID value.

So I think this proposal is ok as it is as there's no real precedence.

Alternatively we could simply do without this API entirely. It is trivial enough for the code that needs a GSource to get iuse the normal qio_channel_add_watch|watch_full APIs, and then lookup the GSource themselves - only one extra line of code in the callers.

The old incoming migration is running in main thread and default gcontext. With the new qio_channel_add_watch_full() we can now let it run in the thread's own gcontext (if there is one).

Currently this patch does nothing alone. But when any of the incoming migration is run in another iothread (e.g., the upcoming migrate-recover command), this patch will bind the incoming logic to the iothread instead of the main thread (which may already get page faulted and hanged).

RDMA is not considered for now since it's not even using the QIO watch framework at all.

CC Juan Quintela <[email protected]>

CC Dr. David Alan Gilbert <[email protected]>

CC Laurent Vivier <[email protected]>

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

---
 migration/exec.c   |  9 ++++-----
 migration/fd.c     |  9 ++++-----
 migration/socket.c | 10 +++++-----
 3 files changed, 13 insertions(+), 15 deletions(-)

Quoted 17 lines · Peter Xu

The old incoming migration is running in main thread and default gcontext. With the new qio_channel_add_watch_full() we can now let it run in the thread's own gcontext (if there is one).

Currently this patch does nothing alone. But when any of the incoming migration is run in another iothread (e.g., the upcoming migrate-recover command), this patch will bind the incoming logic to the iothread instead of the main thread (which may already get page faulted and hanged).

RDMA is not considered for now since it's not even using the QIO watch framework at all.

CC Juan Quintela <[email protected]>

CC Dr. David Alan Gilbert <[email protected]>

CC Laurent Vivier <[email protected]>

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

Quoted 5 lines

---
 migration/exec.c   |  9 ++++-----
 migration/fd.c     |  9 ++++-----
 migration/socket.c | 10 +++++-----
 3 files changed, 13 insertions(+), 15 deletions(-)

This should probably just be in a separate series, since it does nothing on its own, and nothing following in this series touches migration at all.

Three functions are abstracted from the old code:

- qio_net_listener_source_add(): create one source for listener
- qio_net_listener_sources_clear(): unset existing net lister sources
- qio_net_listener_sources_update(): setup all sources for listener

Use them where possible.

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

---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

Quoted 8 lines · Peter Xu

Three functions are abstracted from the old code:

- qio_net_listener_source_add(): create one source for listener
- qio_net_listener_sources_clear(): unset existing net lister sources
- qio_net_listener_sources_update(): setup all sources for listener

Use them where possible.

Quoted 4 lines

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

---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

This patch can be dropped since nothing else in the series now depends on it.

Quoted 14 lines · Daniel P. Berrangé

Quoted 12 lines · Peter Xu

Three functions are abstracted from the old code:

- qio_net_listener_source_add(): create one source for listener
- qio_net_listener_sources_clear(): unset existing net lister sources
- qio_net_listener_sources_update(): setup all sources for listener

Use them where possible.

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

---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

Quoted 2 lines

This patch can be dropped since nothing else in the series now depends on it.

Do you think it's still acceptable even as a cleanup? Thanks,

Quoted 14 lines · Daniel P. Berrangé

Quoted 12 lines · Peter Xu

Three functions are abstracted from the old code:

- qio_net_listener_source_add(): create one source for listener
- qio_net_listener_sources_clear(): unset existing net lister sources
- qio_net_listener_sources_update(): setup all sources for listener

Use them where possible.

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

---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

Quoted 2 lines

This patch can be dropped since nothing else in the series now depends on it.

Do you think it's still acceptable even as a cleanup? Thanks,

Quoted 15 lines · Peter Xu

Quoted 14 lines · Daniel P. Berrangé

Quoted 12 lines · Peter Xu

Three functions are abstracted from the old code:

- qio_net_listener_source_add(): create one source for listener
- qio_net_listener_sources_clear(): unset existing net lister sources
- qio_net_listener_sources_update(): setup all sources for listener

Use them where possible.

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

---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

Quoted 4 lines

Quoted 2 lines

This patch can be dropped since nothing else in the series now depends on it.

Do you think it's still acceptable even as a cleanup? Thanks,

I don't think it really adds value over what's there already.

Originally we were storing the GSources tag IDs. That'll be not enough if we are going to support non-default gcontext for QIO code. Switch to GSources without changing anything real. Now we still always pass in NULL, which means the default gcontext.

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

---
 include/io/net-listener.h | 21 ++++++++++++++--
 io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 25 deletions(-)

Originally we were storing the GSources tag IDs. That'll be not enough if we are going to support non-default gcontext for QIO code. Switch to GSources without changing anything real. Now we still always pass in NULL, which means the default gcontext.

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

---
 include/io/net-listener.h | 21 ++++++++++++++--
 io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 25 deletions(-)

include/io/net-listener.h +1 -1

index 56d6da7a76..566be283b3 100644
@@ -53,7 +53,7 @@ struct QIONetListener {
 
     char *name;
     QIOChannelSocket **sioc;
-    gulong *io_tag;
+    GSource **io_source;
     size_t nsioc;
 
     bool connected;

Quoted 27 lines · Peter Xu

Originally we were storing the GSources tag IDs. That'll be not enough if we are going to support non-default gcontext for QIO code. Switch to GSources without changing anything real. Now we still always pass in NULL, which means the default gcontext.

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

---
 include/io/net-listener.h | 21 ++++++++++++++--
 io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 25 deletions(-)

include/io/net-listener.h +1 -1

index 56d6da7a76..566be283b3 100644
@@ -53,7 +53,7 @@ struct QIONetListener {
 
     char *name;
     QIOChannelSocket **sioc;
-    gulong *io_tag;
+    GSource **io_source;
     size_t nsioc;
 
     bool connected;

@@ -120,17 +120,34 @@ void qio_net_listener_add(QIONetListener *listener,
                           QIOChannelSocket *sioc);
 

include/io/net-listener.h void qio_net_listener_add(QIONetListener *listener, · line 128

Quoted 8 lines

include/io/net-listener.h +2 -1

 /**
- * qio_net_listener_set_client_func:
+ * qio_net_listener_set_client_func_full:
  * @listener: the network listener object
  * @func: the callback function
  * @data: opaque data to pass to @func
  * @notify: callback to free @data
+ * @context: the context that the sources will be bound to

Please add a note like "if %NULL, the default context will be used".

Quoted 27 lines · Peter Xu

Originally we were storing the GSources tag IDs. That'll be not enough if we are going to support non-default gcontext for QIO code. Switch to GSources without changing anything real. Now we still always pass in NULL, which means the default gcontext.

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

---
 include/io/net-listener.h | 21 ++++++++++++++--
 io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 25 deletions(-)

include/io/net-listener.h +1 -1

index 56d6da7a76..566be283b3 100644
@@ -53,7 +53,7 @@ struct QIONetListener {
 
     char *name;
     QIOChannelSocket **sioc;
-    gulong *io_tag;
+    GSource **io_source;
     size_t nsioc;
 
     bool connected;

@@ -120,17 +120,34 @@ void qio_net_listener_add(QIONetListener *listener,
                           QIOChannelSocket *sioc);
 

include/io/net-listener.h void qio_net_listener_add(QIONetListener *listener, · line 128

Quoted 8 lines

include/io/net-listener.h +2 -1

 /**
- * qio_net_listener_set_client_func:
+ * qio_net_listener_set_client_func_full:
  * @listener: the network listener object
  * @func: the callback function
  * @data: opaque data to pass to @func
  * @notify: callback to free @data
+ * @context: the context that the sources will be bound to

Please add a note like "if %NULL, the default context will be used".

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.

Since at it, generalize a tcp_chr_net_listener_setup() helper function and clean up the old code a bit.

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

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

chardev/char-socket.c +1 -0

index 43a2cc2c1c..5cd20cc932 100644
@@ -410,6 +410,19 @@ static void update_disconnected_filename(SocketChardev *s)
                                          s->is_listen, s->is_telnet);
 }
 
+/* Set enable=true to start net listeners, false to stop them. */

chardev/char-socket.c +15 -6

+}
+
 /* NB may be called even if tcp_chr_connect has not been
  * reached, due to TLS or telnet initialization failure,
  * so can *not* assume s->connected == true

@@ -422,8 +435,7 @@ 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);
+        tcp_chr_net_listener_setup(s, true);
     }
     update_disconnected_filename(s);
     if (emit_close) {

@@ -559,6 +571,15 @@ 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.
+         */
+        tcp_chr_net_listener_setup(s, true);
+    }
+
     if (!s->connected) {
         return;
     }

@@ -742,7 +763,7 @@ 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);
+        tcp_chr_net_listener_setup(s, false);
     }
 
     if (s->tls_creds) {

@@ -823,7 +844,7 @@ 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);
+        tcp_chr_net_listener_setup(s, false);
         object_unref(OBJECT(s->listener));
     }
     if (s->tls_creds) {

@@ -979,8 +1000,7 @@ 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);
+                tcp_chr_net_listener_setup(s, true);
             }
         } else if (qemu_chr_wait_connected(chr, errp) < 0) {
             goto error;

Signature · 1 line

2.14.3

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

Since at it, generalize a tcp_chr_net_listener_setup() helper function and clean up the old code a bit.

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

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

chardev/char-socket.c +1 -0

index 43a2cc2c1c..5cd20cc932 100644
@@ -410,6 +410,19 @@ static void update_disconnected_filename(SocketChardev *s)
                                          s->is_listen, s->is_telnet);
 }
 
+/* Set enable=true to start net listeners, false to stop them. */

chardev/char-socket.c static void update_disconnected_filename(SocketChardev *s) · line 423

Quoted 10 lines

chardev/char-socket.c +10 -0

+static void tcp_chr_net_listener_setup(SocketChardev *s, bool enable)
+{
+    Chardev *chr = CHARDEV(s);
+
+    /* Net listeners' context will follow the Chardev's. */
+    qio_net_listener_set_client_func_full(s->listener,
+                                          enable ? tcp_chr_accept : NULL,
+                                          enable ? chr : NULL,
+                                          NULL,
+                                          chr->gcontext);

I don't think this helper method is really a benefit. In fact I think it makes understanding the code harder, because when you see tcp_chr_net_listener_setup(s, true), you've no idea what 'true' means without going to finding the impl of tcp_chr_net_listener_setup().

Just leave the direct calls to qio_net_listener_set_client_func_full as they are IMHO.

Frankly speaking I was a bit confused when I started to read chardev/qio codes with so many hooks, e.g., when I saw:

     qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
                                      chr, NULL);

I totally have no idea on what happened. I need to go deeper into the net listener code to know that, hmm, it's setting up something to accept connections!

If I can have something like:

    tcp_chr_net_listener_setup(s, true);

It may be easier for me to understand that there's something either registered for the listening ports, and I don't need to care about which function will be called when accept happened. Basically it "hides" some logic inside, that's IMHO where functions/macros help.

Quoted 14 lines · Peter Xu

Frankly speaking I was a bit confused when I started to read chardev/qio codes with so many hooks, e.g., when I saw:

     qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
                                      chr, NULL);

I totally have no idea on what happened. I need to go deeper into the net listener code to know that, hmm, it's setting up something to accept connections!

If I can have something like:

    tcp_chr_net_listener_setup(s, true);

Quoted 4 lines

It may be easier for me to understand that there's something either registered for the listening ports, and I don't need to care about which function will be called when accept happened. Basically it "hides" some logic inside, that's IMHO where functions/macros help.

I tend to agree with Daniel, but I probably would be convinced if you had a pair of functions tcp_chr_{start,stop}_listen instead of a bool argument.

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 in SocketChardev.telnet_source so that we can also do dynamic context switch when update read handlers.

Quoted 7 lines · 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 in SocketChardev.telnet_source so that we can also do dynamic context switch when update read handlers.

I don't see why we would ever want to dynamically switch the GMainContext in use while in middle of reading the telnet greeting.

Quoted 6 lines · Daniel P. Berrangé

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

Quoted 4 lines

Quoted 2 lines

Meanwhile, caching the gsource in SocketChardev.telnet_source so that we can also do dynamic context switch when update read handlers.

I don't see why we would ever want to dynamically switch the GMainContext in use while in middle of reading the telnet greeting.

Maybe because the remote client hangs in the middle of the telnet greeting? The user of the Chardev can't know that the initial handshake hasn't been done yet.

Quoted 6 lines · Daniel P. Berrangé

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

Quoted 4 lines

Quoted 2 lines

Meanwhile, caching the gsource in SocketChardev.telnet_source so that we can also do dynamic context switch when update read handlers.

I don't see why we would ever want to dynamically switch the GMainContext in use while in middle of reading the telnet greeting.

Maybe because the remote client hangs in the middle of the telnet greeting? The user of the Chardev can't know that the initial handshake hasn't been done yet.

qio_task_run_in_thread() allows main thread to run blocking operations in the background. However it has an assumption on that it's always working with the default context. This patch tries to allow the threaded QIO task framework to run with non-default gcontext.

Currently no functional change so far, so the QIOTasks are still always running on main context.

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

---
 include/io/task.h    |  6 ++++--
 io/channel-socket.c  |  9 ++++++---
 io/dns-resolver.c    |  3 ++-
 io/task.c            | 20 ++++++++++++++++++--
 tests/test-io-task.c |  2 ++
 5 files changed, 32 insertions(+), 8 deletions(-)

+ * @context: the context to run the complete hook

* Run a task in a background thread. When @worker
* returns it will call qio_task_complete() in
- * the main event thread context.
+ * the event thread context that provided. */
void qio_task_run_in_thread(QIOTask *task,
QIOTaskWorker worker, gpointer opaque,

include/io/task.h +2 -1

-                            GDestroyNotify destroy);
+                            GDestroyNotify destroy,
+                            GMainContext *context);

We have worked on qio_task_run_in_thread() already. Further, let qio_channel_socket_connect_async() pass that context to it.

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

---
 chardev/char-socket.c          | 4 ++--
 include/io/channel-socket.h    | 4 +++-
 io/channel-socket.c            | 5 +++--
 migration/socket.c             | 3 ++-
 tests/test-io-channel-socket.c | 2 +-
 5 files changed, 11 insertions(+), 7 deletions(-)

include/io/channel-socket.h +1 -0

index 53801f6042..90f7227397 100644
@@ -101,6 +101,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
  * @callback: the function to invoke on completion
  * @opaque: user data to pass to @callback
  * @destroy: the function to free @opaque
+ * @context: the context to run the async task
  *
  * Attempt to connect to the address @addr. This method
  * will run in the background so the caller will regain

Quoted 27 lines · Peter Xu

We have worked on qio_task_run_in_thread() already. Further, let qio_channel_socket_connect_async() pass that context to it.

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

---
 chardev/char-socket.c          | 4 ++--
 include/io/channel-socket.h    | 4 +++-
 io/channel-socket.c            | 5 +++--
 migration/socket.c             | 3 ++-
 tests/test-io-channel-socket.c | 2 +-
 5 files changed, 11 insertions(+), 7 deletions(-)

include/io/channel-socket.h +1 -0

index 53801f6042..90f7227397 100644
@@ -101,6 +101,7 @@ int qio_channel_socket_connect_sync(QIOChannelSocket *ioc,
  * @callback: the function to invoke on completion
  * @opaque: user data to pass to @callback
  * @destroy: the function to free @opaque
+ * @context: the context to run the async task
  *
  * Attempt to connect to the address @addr. This method
  * will run in the background so the caller will regain

@@ -113,7 +114,8 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
                                       SocketAddress *addr,
                                       QIOTaskFunc callback,

include/io/channel-socket.h void qio_channel_socket_connect_async(QIOChannelSocket *ioc, · line 118

Quoted 4 lines

include/io/channel-socket.h +2 -1

                                       gpointer opaque,
-                                      GDestroyNotify destroy);
+                                      GDestroyNotify destroy,
+                                      GMainContext *context);

If you're going to add a GMainContext() to connect_async, then please also do it for listen_async and dgram_async at the same time, so we remain consistent in API design.

qio_channel_tls_handshake_full() is introduced to allow the TLS to be run on a non-default context. Still, no functional change.

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

---
 include/io/channel-tls.h | 17 ++++++++++++++++
 io/channel-tls.c         | 51 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 54 insertions(+), 14 deletions(-)

static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,

io/channel-tls.c +2 -1

-                                           QIOTask *task)
+                                           QIOTask *task,
+                                           GMainContext *context)
 {
     Error *err = NULL;
     QCryptoTLSSessionHandshakeStatus status;

io/channel-tls.c +1 -1

@@ -191,20 +203,23 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
                                              GIOCondition condition,
                                              gpointer user_data)
 {
-    QIOTask *task = user_data;
+    QIOChannelTLSData *data = user_data;

Quoted 18 lines · Peter Xu

qio_channel_tls_handshake_full() is introduced to allow the TLS to be run on a non-default context. Still, no functional change.

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

---
 include/io/channel-tls.h | 17 ++++++++++++++++
 io/channel-tls.c         | 51 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 54 insertions(+), 14 deletions(-)

static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,

io/channel-tls.c +2 -1

-                                           QIOTask *task)
+                                           QIOTask *task,
+                                           GMainContext *context)
 {
     Error *err = NULL;
     QCryptoTLSSessionHandshakeStatus status;

io/channel-tls.c static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc, · line 183

Quoted 7 lines

io/channel-tls.c +4 -0

@@ -171,6 +177,11 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         qio_task_complete(task);
     } else {
         GIOCondition condition;
+        QIOChannelTLSData *data = g_new0(typeof(*data), 1);
+
+        data->task = task;
+        data->context = context;

The 'context' reference is only valid for as long as the caller exists. So you need to acquire a reference on 'context' here....

Quoted 11 lines

io/channel-tls.c +3 -1

@@ -191,20 +203,23 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
                                              GIOCondition condition,
                                              gpointer user_data)
 {
-    QIOTask *task = user_data;
+    QIOChannelTLSData *data = user_data;
+    QIOTask *task = data->task;
+    GMainContext *context = data->context;
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(
         qio_task_get_source(task));
 

io/channel-tls.c static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,

Quoted 5 lines

io/channel-tls.c +2 -2

-    qio_channel_tls_handshake_task(
-       tioc, task);
 
+    g_free(data);
+    qio_channel_tls_handshake_task(tioc, task, context);

And release the reference on context here.

include/io/channel-tls.h +7 -0

+/**
+ * qio_channel_tls_handshake_full:
+ * @ioc: the TLS channel object
+ * @func: the callback to invoke when completed
+ * @opaque: opaque data to pass to @func
+ * @destroy: optional callback to free @opaque
+ * @context: the context that TLS handshake will run with

Quoted 12 lines · Peter Xu

include/io/channel-tls.h +12 -0

+/**
+ * qio_channel_tls_handshake_full:
+ * @ioc: the TLS channel object
+ * @func: the callback to invoke when completed
+ * @opaque: opaque data to pass to @func
+ * @destroy: optional callback to free @opaque
+ * @context: the context that TLS handshake will run with
+ *
+ * Similar to qio_channel_tls_handshake(), but allows the task to be
+ * run on a specific context.
+ */
+void qio_channel_tls_handshake_full(QIOChannelTLS *ioc,

include/io/channel-tls.h void qio_channel_tls_handshake(QIOChannelTLS *ioc, · line 147

Quoted 5 lines

include/io/channel-tls.h +5 -0

+                                    QIOTaskFunc func,
+                                    gpointer opaque,
+                                    GDestroyNotify destroy,
+                                    GMainContext *context);
+

You're not consistent in introducing "_full" functions. I would add the argument directly to the qio_channel_tls_handshake() function.

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.

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

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

---
 chardev/char-mux.c     | 29 +++++++++++++++++++++++++++++
 chardev/char.c         | 43 +++++++++++++++++--------------------------
 include/chardev/char.h |  2 ++
 3 files changed, 48 insertions(+), 26 deletions(-)

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

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

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

chardev/char-socket.c +13 -6

index bd90680f5c..cd9db123f2 100644
@@ -894,11 +894,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;

@@ -907,11 +918,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;
 }

@@ -991,11 +998,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
     }
 

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.

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

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

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

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

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

I don't like this as it is special casing behaviour wrt GMainContext only the the case where the chardev is configured as a client with non-blocking connect. So any code that uses chardevs and wants to set a different GMainContext may or may not work, depending on whether the user gave the ',wait' option to the chardev. I'm struggling to see why this is really needed at all.

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.

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

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

chardev/char-socket.c +13 -3

index cd9db123f2..2b355fc7a8 100644
@@ -997,9 +997,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();

@@ -1128,6 +1127,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);

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.

If we dynamically add tcp chardev, it won't be affected since we have a new tcp_chr_machine_done flag to know whether we should postpone it or not.

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

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

chardev/char-socket.c +14 -0

index 2b355fc7a8..13aeca0b27 100644
@@ -72,6 +72,8 @@ typedef struct {
 static gboolean socket_reconnect_timeout(gpointer opaque);
 static void tcp_chr_telnet_init(Chardev *chr);
 
+static bool tcp_chr_machine_done;
+
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
     if (s->reconnect_timer) {

@@ -719,6 +721,11 @@ static void tcp_chr_tls_init(Chardev *chr)
     Error *err = NULL;
     gchar *name;
 
+    if (!tcp_chr_machine_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,

@@ -1131,10 +1138,17 @@ static int tcp_chr_machine_done_hook(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
+    /* Set it multiple times won't hurt */
+    tcp_chr_machine_done = true;
+
     if (s->reconnect_time) {
         tcp_chr_connect_async(chr);
     }
 
+    if (s->tls_creds) {
+        tcp_chr_tls_init(chr);
+    }
+
     return 0;
 }
 

Signature · 1 line

2.14.3

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

If we dynamically add tcp chardev, it won't be affected since we have a new tcp_chr_machine_done flag to know whether we should postpone it or not.

Quoted 4 lines

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

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

I don't like this patch either for the same reasons as previous patch - its creating different behaviour depending on whether the 'wait' flag happens to have been set in -chardev.

chardev/char-socket.c +2 -0

+static bool tcp_chr_machine_done;
+
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
     if (s->reconnect_timer) {

Quoted 6 lines · Peter Xu

chardev/char-socket.c +2 -0

+static bool tcp_chr_machine_done;
+
 static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
 {
     if (s->reconnect_timer) {

chardev/char-socket.c static void tcp_chr_tls_init(Chardev *chr) · line 728

Quoted 8 lines

chardev/char-socket.c +5 -0

@@ -719,6 +721,11 @@ static void tcp_chr_tls_init(Chardev *chr)
     Error *err = NULL;
     gchar *name;
 
+    if (!tcp_chr_machine_done) {
+        /* This will be postponed to machine_done notifier */
+        return;
+    }
+

Can you instead add a global machine_init_done bool to vl.c and include/sysemu/sysemu.h (and make it always true in stubs/machine-init-done.c)?

Then muxes_realized can go away too.

This is another preparation work for monitor OOB seires.

V1: http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg06972.html

V2 rewrote the bottom half of the code. The first 8 patches are mostly the same, but I rewrote the last patches to solve both TLS and reconnect use cases by introducing a machine_done hook for chardevs in general. So if I copy the problems:

- migration
- incoming side: still always running on main context, while we need to be able to run some command in OOB thread [1]
- tcp chardev (non-tcp chardevs should all support non-NULL context now)
- server listening mode: QIO net listener used [2]
- TELNET session: an isolated GSource used (tcp_chr_telnet_init) [3]
- when "reconnect=N" is used, QIO threaded task is used [4]
- TLS session: QIO tls handshake is used (tcp_chr_tls_init) [5]

Problem [1-3] are still fixed in the old way, but [4-5] now are fixed by using the new machine_done notifier.

Quoted 18 lines · Peter Xu

This is another preparation work for monitor OOB seires.

V1: http://lists.nongnu.org/archive/html/qemu-devel/2018-02/msg06972.html

V2 rewrote the bottom half of the code. The first 8 patches are mostly the same, but I rewrote the last patches to solve both TLS and reconnect use cases by introducing a machine_done hook for chardevs in general. So if I copy the problems:

- migration
- incoming side: still always running on main context, while we need to be able to run some command in OOB thread [1]
- tcp chardev (non-tcp chardevs should all support non-NULL context now)
- server listening mode: QIO net listener used [2]
- TELNET session: an isolated GSource used (tcp_chr_telnet_init) [3]
- when "reconnect=N" is used, QIO threaded task is used [4]
- TLS session: QIO tls handshake is used (tcp_chr_tls_init) [5]

Quoted 2 lines

Problem [1-3] are still fixed in the old way, but [4-5] now are fixed by using the new machine_done notifier.

The QIO code changes all look good to me know, aside from minor comments. I really dislike all of the chardev stuff though. I think it makes the chardev code even harder to follow & rationalize behaviour of.

If you post a v3 series contaning just the qio/ directory changes, I'd queue those patches, while we discuss chardev stuff more.

I struggle to suggest better approach, because its any missing context of how the changes are going to be used, presumably by patch series yet to be posted.