[Qemu-devel] [PATCH v2 00/15] qio: general non-default GMainContext support

[Qemu-devel] [PATCH v2 00/15] qio: general non-default GMainContext support

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

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

I'll still try to provide a changelog beside the big change mentioned:

v2:
- collect r-bs
- qio_channel_add_watch_full() should still return the same thing as
  the old one, and introduced qio_channel_add_watch_full() to return a
  GSource pointer. [Dan]
- Fix commit message on RDMA. It's using QIO, but still, I am not
  touching it.  [Dan]
- use qio_net_listener_set_client_func_full() directly, and avoid
  introducing new API. [Dan]

Please review.  Thanks.

Peter Xu (15):
  chardev: fix leak in tcp_chr_telnet_init_io()
  qio: rename qio_task_thread_result
  qio: introduce qio_channel_add_watch_{full|source}
  migration: let incoming side use thread context
  qio: refactor net listener source operations
  qio: store gsources for net listeners
  qio/chardev: update net listener gcontext
  chardev: allow telnet gsource to switch gcontext
  qio: non-default context for threaded qtask
  qio: non-default context for async conn
  qio: non-default context for TLS handshake
  chardev: introduce chr_machine_done hook
  char: use chardev's gcontext for async connect
  chardev: tcp: postpone async connection setup
  chardev: tcp: postpone TLS work until machine done

 chardev/char-mux.c             |  29 ++++++++++
 chardev/char-socket.c          | 124 ++++++++++++++++++++++++++++++++---------
 chardev/char.c                 |  43 ++++++--------
 include/chardev/char.h         |   2 +
 include/io/channel-socket.h    |   4 +-
 include/io/channel-tls.h       |  17 ++++++
 include/io/channel.h           |  44 +++++++++++++++
 include/io/net-listener.h      |  21 ++++++-
 include/io/task.h              |   6 +-
 io/channel-socket.c            |  12 ++--
 io/channel-tls.c               |  51 ++++++++++++-----
 io/channel.c                   |  40 +++++++++++--
 io/dns-resolver.c              |   3 +-
 io/net-listener.c              | 112 +++++++++++++++++++++----------------
 io/task.c                      |  22 +++++++-
 migration/exec.c               |   9 ++-
 migration/fd.c                 |   9 ++-
 migration/socket.c             |  13 +++--
 tests/test-io-channel-socket.c |   2 +-
 tests/test-io-task.c           |   2 +
 20 files changed, 415 insertions(+), 150 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 01/15] 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>
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 bdd6cff5f6..43a2cc2c1c 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

Re: [Qemu-devel] [PATCH v2 01/15] chardev: fix leak in tcp_chr_telnet_init_io()

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> Need to free TCPChardevTelnetInit when session established.
> 
> Since at it, switch to use G_SOURCE_* macros.
> 
> Reviewed-by: Daniel P. Berrange <berrange@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 bdd6cff5f6..43a2cc2c1c 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)
> 

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

Paolo

Re: [Qemu-devel] [PATCH v2 01/15] chardev: fix leak in tcp_chr_telnet_init_io()

[Peter Xu]

On Thu, Mar 01, 2018 at 06:39:34PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > Need to free TCPChardevTelnetInit when session established.
> > 
> > Since at it, switch to use G_SOURCE_* macros.
> > 
> > Reviewed-by: Daniel P. Berrange <berrange@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 bdd6cff5f6..43a2cc2c1c 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)
> > 
> 
> 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).

Sure, thanks.

I believe this is an equivalent "r-b" if I repost. :)

-- 
Peter Xu

[Qemu-devel] [PATCH v2 02/15] qio: rename qio_task_thread_result

[Peter Xu]

It is strange that it was called gio_task_thread_result.  Rename it to
follow the naming rule of the file.

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 io/task.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/task.c b/io/task.c
index 3ce556017c..1a0a1c7185 100644
--- a/io/task.c
+++ b/io/task.c
@@ -80,7 +80,7 @@ struct QIOTaskThreadData {
 };
 
 
-static gboolean gio_task_thread_result(gpointer opaque)
+static gboolean qio_task_thread_result(gpointer opaque)
 {
     struct QIOTaskThreadData *data = opaque;
 
@@ -110,7 +110,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
      * the worker results
      */
     trace_qio_task_thread_exit(data->task);
-    g_idle_add(gio_task_thread_result, data);
+    g_idle_add(qio_task_thread_result, data);
     return NULL;
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Peter Xu]

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 <peterx@redhat.com>
---
 include/io/channel.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 io/channel.c         | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 3995e243a3..e8cdadb0b0 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -648,6 +648,50 @@ guint qio_channel_add_watch(QIOChannel *ioc,
                             gpointer user_data,
                             GDestroyNotify notify);
 
+/**
+ * qio_channel_add_watch_full:
+ * @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: the context to run the watch source
+ *
+ * Similar as qio_channel_add_watch(), but allows to specify context
+ * to run the watch source.
+ *
+ * Returns: the source ID
+ */
+guint qio_channel_add_watch_full(QIOChannel *ioc,
+                                 GIOCondition condition,
+                                 QIOChannelFunc func,
+                                 gpointer user_data,
+                                 GDestroyNotify notify,
+                                 GMainContext *context);
+
+/**
+ * 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
+ */
+GSource *qio_channel_add_watch_source(QIOChannel *ioc,
+                                      GIOCondition condition,
+                                      QIOChannelFunc func,
+                                      gpointer user_data,
+                                      GDestroyNotify notify,
+                                      GMainContext *context);
 
 /**
  * qio_channel_attach_aio_context:
diff --git a/io/channel.c b/io/channel.c
index ec4b86de7c..8dd0684f5d 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -299,11 +299,12 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
     klass->io_set_aio_fd_handler(ioc, ctx, io_read, io_write, opaque);
 }
 
-guint qio_channel_add_watch(QIOChannel *ioc,
-                            GIOCondition condition,
-                            QIOChannelFunc func,
-                            gpointer user_data,
-                            GDestroyNotify notify)
+guint qio_channel_add_watch_full(QIOChannel *ioc,
+                                 GIOCondition condition,
+                                 QIOChannelFunc func,
+                                 gpointer user_data,
+                                 GDestroyNotify notify,
+                                 GMainContext *context)
 {
     GSource *source;
     guint id;
@@ -312,12 +313,39 @@ guint qio_channel_add_watch(QIOChannel *ioc,
 
     g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
 
-    id = g_source_attach(source, NULL);
+    id = g_source_attach(source, context);
     g_source_unref(source);
 
     return id;
 }
 
+guint qio_channel_add_watch(QIOChannel *ioc,
+                            GIOCondition condition,
+                            QIOChannelFunc func,
+                            gpointer user_data,
+                            GDestroyNotify notify)
+{
+    return qio_channel_add_watch_full(ioc, condition, func,
+                                      user_data, notify, NULL);
+}
+
+GSource *qio_channel_add_watch_source(QIOChannel *ioc,
+                                      GIOCondition condition,
+                                      QIOChannelFunc func,
+                                      gpointer user_data,
+                                      GDestroyNotify notify,
+                                      GMainContext *context)
+{
+    GSource *source;
+    guint id;
+
+    id = qio_channel_add_watch_full(ioc, condition, func,
+                                    user_data, notify, context);
+    source = g_main_context_find_source_by_id(context, id);
+    g_source_ref(source);
+    return source;
+}
+
 
 int qio_channel_shutdown(QIOChannel *ioc,
                          QIOChannelShutdown how,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:26PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  include/io/channel.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  io/channel.c         | 40 ++++++++++++++++++++++++++++++++++------
>  2 files changed, 78 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 :|

Re: [Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> + * 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
> + */
> +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.

(Not making it perfectly the same API is okay, for example in practice
all callers would use g_source_set_callback so it's okay IMO to add the
three arguments func/user_data/notify.  However, inconsistency on
g_source_add is more subtle).

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Peter Xu]

On Thu, Mar 01, 2018 at 06:13:06PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > + * 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
> > + */
> > +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.

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

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

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Paolo Bonzini]

On 02/03/2018 04:54, Peter Xu wrote:
> On Thu, Mar 01, 2018 at 06:13:06PM +0100, Paolo Bonzini wrote:
>> On 01/03/2018 09:44, Peter Xu wrote:
>>> + * 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
>>> + */
>>> +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.
> 
> Looks like there is already a qio_channel_create_watch() (io/channel.c).
> 
> How about qio_channel_create_watch_attached()?  Or... anything better?

I would add the g_source_set_callback call (and arguments) to
qio_channel_create_watch and leave the g_source_attach to the caller.

Paolo

Re: [Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 06:13:06PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > + * 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
> > + */
> > +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 *.

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.

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 03/15] qio: introduce qio_channel_add_watch_{full|source}

[Paolo Bonzini]

On 02/03/2018 16:44, Daniel P. Berrangé wrote:
>> 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.

True, on the other hand it's annoying for qio_channel_create_watch
because of the function pointer case.  So I suggested a mix of creating
the watch and setting the callback.

Having a function that takes a GMainContext and returns a tag is fine, too.

Paolo

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

[Qemu-devel] [PATCH v2 04/15] migration: let incoming side use thread context

[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 <quintela@redhat.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/exec.c   |  9 ++++-----
 migration/fd.c     |  9 ++++-----
 migration/socket.c | 10 +++++-----
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 0bc5a427dd..9d0f82f1f0 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -65,9 +65,8 @@ void exec_start_incoming_migration(const char *command, Error **errp)
     }
 
     qio_channel_set_name(ioc, "migration-exec-incoming");
-    qio_channel_add_watch(ioc,
-                          G_IO_IN,
-                          exec_accept_incoming_migration,
-                          NULL,
-                          NULL);
+    qio_channel_add_watch_full(ioc, G_IO_IN,
+                               exec_accept_incoming_migration,
+                               NULL, NULL,
+                               g_main_context_get_thread_default());
 }
diff --git a/migration/fd.c b/migration/fd.c
index cd06182d1e..9a380bbbc4 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -66,9 +66,8 @@ void fd_start_incoming_migration(const char *infd, Error **errp)
     }
 
     qio_channel_set_name(QIO_CHANNEL(ioc), "migration-fd-incoming");
-    qio_channel_add_watch(ioc,
-                          G_IO_IN,
-                          fd_accept_incoming_migration,
-                          NULL,
-                          NULL);
+    qio_channel_add_watch_full(ioc, G_IO_IN,
+                               fd_accept_incoming_migration,
+                               NULL, NULL,
+                               g_main_context_get_thread_default());
 }
diff --git a/migration/socket.c b/migration/socket.c
index e090097077..60d732535c 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -173,11 +173,11 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
         return;
     }
 
-    qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
-                          G_IO_IN,
-                          socket_accept_incoming_migration,
-                          listen_ioc,
-                          (GDestroyNotify)object_unref);
+    qio_channel_add_watch_full(QIO_CHANNEL(listen_ioc), G_IO_IN,
+                               socket_accept_incoming_migration,
+                               listen_ioc,
+                               (GDestroyNotify)object_unref,
+                               g_main_context_get_thread_default());
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 04/15] migration: let incoming side use thread context

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:27PM +0800, Peter Xu wrote:
> 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 <quintela@redhat.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Laurent Vivier <lvivier@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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.


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 04/15] migration: let incoming side use thread context

[Peter Xu]

On Thu, Mar 01, 2018 at 04:03:44PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:27PM +0800, Peter Xu wrote:
> > 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 <quintela@redhat.com>
> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > CC: Laurent Vivier <lvivier@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  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.

It was trying to solve all problems related to QIO+context, and
migration is just one user of it.  But sure I can postpone this patch
to the postcopy recovery series.

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 05/15] qio: refactor net listener source operations

[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 <peterx@redhat.com>
---
 io/net-listener.c | 82 +++++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/io/net-listener.c b/io/net-listener.c
index de38dfae99..3e9ac51b0e 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -106,6 +106,39 @@ int qio_net_listener_open_sync(QIONetListener *listener,
     }
 }
 
+static guint qio_net_listener_source_add(QIONetListener *listener,
+                                         QIOChannelSocket *sioc)
+{
+    return qio_channel_add_watch(QIO_CHANNEL(sioc), G_IO_IN,
+                                 qio_net_listener_channel_func,
+                                 listener, (GDestroyNotify)object_unref);
+}
+
+static void qio_net_listener_sources_clear(QIONetListener *listener)
+{
+    size_t i;
+
+    for (i = 0; i < listener->nsioc; i++) {
+        if (listener->io_tag[i]) {
+            g_source_remove(listener->io_tag[i]);
+            listener->io_tag[i] = 0;
+        }
+    }
+}
+
+static void qio_net_listener_sources_update(QIONetListener *listener)
+{
+    size_t i;
+
+    if (listener->io_func != NULL) {
+        for (i = 0; i < listener->nsioc; i++) {
+            assert(listener->io_tag[i] == 0);
+            object_ref(OBJECT(listener));
+            listener->io_tag[i] = qio_net_listener_source_add(
+                listener, listener->sioc[i]);
+        }
+    }
+}
 
 void qio_net_listener_add(QIONetListener *listener,
                           QIOChannelSocket *sioc)
@@ -127,10 +160,8 @@ void qio_net_listener_add(QIONetListener *listener,
 
     if (listener->io_func != NULL) {
         object_ref(OBJECT(listener));
-        listener->io_tag[listener->nsioc] = qio_channel_add_watch(
-            QIO_CHANNEL(listener->sioc[listener->nsioc]), G_IO_IN,
-            qio_net_listener_channel_func,
-            listener, (GDestroyNotify)object_unref);
+        listener->io_tag[listener->nsioc] = qio_net_listener_source_add(
+            listener, listener->sioc[listener->nsioc]);
     }
 
     listener->nsioc++;
@@ -142,8 +173,6 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
                                       gpointer data,
                                       GDestroyNotify notify)
 {
-    size_t i;
-
     if (listener->io_notify) {
         listener->io_notify(listener->io_data);
     }
@@ -151,22 +180,8 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
     listener->io_data = data;
     listener->io_notify = notify;
 
-    for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
-        }
-    }
-
-    if (listener->io_func != NULL) {
-        for (i = 0; i < listener->nsioc; i++) {
-            object_ref(OBJECT(listener));
-            listener->io_tag[i] = qio_channel_add_watch(
-                QIO_CHANNEL(listener->sioc[i]), G_IO_IN,
-                qio_net_listener_channel_func,
-                listener, (GDestroyNotify)object_unref);
-        }
-    }
+    qio_net_listener_sources_clear(listener);
+    qio_net_listener_sources_update(listener);
 }
 
 
@@ -210,12 +225,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
     };
     size_t i;
 
-    for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
-        }
-    }
+    qio_net_listener_sources_clear(listener);
 
     sources = g_new0(GSource *, listener->nsioc);
     for (i = 0; i < listener->nsioc; i++) {
@@ -238,15 +248,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
     g_main_loop_unref(loop);
     g_main_context_unref(ctxt);
 
-    if (listener->io_func != NULL) {
-        for (i = 0; i < listener->nsioc; i++) {
-            object_ref(OBJECT(listener));
-            listener->io_tag[i] = qio_channel_add_watch(
-                QIO_CHANNEL(listener->sioc[i]), G_IO_IN,
-                qio_net_listener_channel_func,
-                listener, (GDestroyNotify)object_unref);
-        }
-    }
+    qio_net_listener_sources_update(listener);
 
     return data.sioc;
 }
@@ -259,11 +261,9 @@ void qio_net_listener_disconnect(QIONetListener *listener)
         return;
     }
 
+    qio_net_listener_sources_clear(listener);
+
     for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
-        }
         qio_channel_close(QIO_CHANNEL(listener->sioc[i]), NULL);
     }
     listener->connected = false;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 05/15] qio: refactor net listener source operations

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:28PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  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.


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 05/15] qio: refactor net listener source operations

[Peter Xu]

On Thu, Mar 01, 2018 at 10:47:17AM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:28PM +0800, Peter Xu wrote:
> > 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 <peterx@redhat.com>
> > ---
> >  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.

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

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 05/15] qio: refactor net listener source operations

[Peter Xu]

On Fri, Mar 02, 2018 at 11:58:52AM +0800, Peter Xu wrote:
> On Thu, Mar 01, 2018 at 10:47:17AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Mar 01, 2018 at 04:44:28PM +0800, Peter Xu wrote:
> > > 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 <peterx@redhat.com>
> > > ---
> > >  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.
> 
> Do you think it's still acceptable even as a cleanup?  Thanks,

Ah, and patch 6 actually depends on it (currently).  For sure I can do
some rebase work to drop current one, but IMHO I would prefer to keep
both there.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 05/15] qio: refactor net listener source operations

[Daniel P. Berrangé]

On Fri, Mar 02, 2018 at 11:58:52AM +0800, Peter Xu wrote:
> On Thu, Mar 01, 2018 at 10:47:17AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Mar 01, 2018 at 04:44:28PM +0800, Peter Xu wrote:
> > > 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 <peterx@redhat.com>
> > > ---
> > >  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.
> 
> 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.

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 05/15] qio: refactor net listener source operations

[Peter Xu]

On Fri, Mar 02, 2018 at 10:51:01AM +0000, Daniel P. Berrangé wrote:
> On Fri, Mar 02, 2018 at 11:58:52AM +0800, Peter Xu wrote:
> > On Thu, Mar 01, 2018 at 10:47:17AM +0000, Daniel P. Berrangé wrote:
> > > On Thu, Mar 01, 2018 at 04:44:28PM +0800, Peter Xu wrote:
> > > > 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 <peterx@redhat.com>
> > > > ---
> > > >  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.
> > 
> > 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.

I'll drop it.  Thanks.

-- 
Peter Xu

[Qemu-devel] [PATCH v2 06/15] qio: store gsources for net listeners

[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 <peterx@redhat.com>
---
 include/io/net-listener.h | 21 ++++++++++++++--
 io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
 2 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index 56d6da7a76..566be283b3 100644
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -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);
 
 /**
- * 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
  *
  * Register @func to be invoked whenever a new client
  * connects to the listener. @func will be invoked
  * passing in the QIOChannelSocket instance for the
  * client.
  */
+void qio_net_listener_set_client_func_full(QIONetListener *listener,
+                                           QIONetListenerClientFunc func,
+                                           gpointer data,
+                                           GDestroyNotify notify,
+                                           GMainContext *context);
+
+/**
+ * qio_net_listener_set_client_func:
+ * @listener: the network listener object
+ * @func: the callback function
+ * @data: opaque data to pass to @func
+ * @notify: callback to free @data
+ *
+ * Wrapper of qio_net_listener_set_client_func_full(), only that the
+ * sources will always be bound to default main context.
+ */
 void qio_net_listener_set_client_func(QIONetListener *listener,
                                       QIONetListenerClientFunc func,
                                       gpointer data,
diff --git a/io/net-listener.c b/io/net-listener.c
index 3e9ac51b0e..14fd47db6c 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -106,12 +106,15 @@ int qio_net_listener_open_sync(QIONetListener *listener,
     }
 }
 
-static guint qio_net_listener_source_add(QIONetListener *listener,
-                                         QIOChannelSocket *sioc)
+static GSource *qio_net_listener_source_add(QIONetListener *listener,
+                                            QIOChannelSocket *sioc,
+                                            GMainContext *context)
 {
-    return qio_channel_add_watch(QIO_CHANNEL(sioc), G_IO_IN,
-                                 qio_net_listener_channel_func,
-                                 listener, (GDestroyNotify)object_unref);
+    return qio_channel_add_watch_source(QIO_CHANNEL(sioc), G_IO_IN,
+                                        qio_net_listener_channel_func,
+                                        listener,
+                                        (GDestroyNotify)object_unref,
+                                        context);
 }
 
 static void qio_net_listener_sources_clear(QIONetListener *listener)
@@ -119,23 +122,25 @@ static void qio_net_listener_sources_clear(QIONetListener *listener)
     size_t i;
 
     for (i = 0; i < listener->nsioc; i++) {
-        if (listener->io_tag[i]) {
-            g_source_remove(listener->io_tag[i]);
-            listener->io_tag[i] = 0;
+        if (listener->io_source[i]) {
+            g_source_destroy(listener->io_source[i]);
+            g_source_unref(listener->io_source[i]);
+            listener->io_source[i] = NULL;
         }
     }
 }
 
-static void qio_net_listener_sources_update(QIONetListener *listener)
+static void qio_net_listener_sources_update(QIONetListener *listener,
+                                            GMainContext *context)
 {
     size_t i;
 
     if (listener->io_func != NULL) {
         for (i = 0; i < listener->nsioc; i++) {
-            assert(listener->io_tag[i] == 0);
+            assert(listener->io_source[i] == NULL);
             object_ref(OBJECT(listener));
-            listener->io_tag[i] = qio_net_listener_source_add(
-                listener, listener->sioc[i]);
+            listener->io_source[i] = qio_net_listener_source_add(
+                listener, listener->sioc[i], context);
         }
     }
 }
@@ -151,27 +156,30 @@ void qio_net_listener_add(QIONetListener *listener,
 
     listener->sioc = g_renew(QIOChannelSocket *, listener->sioc,
                              listener->nsioc + 1);
-    listener->io_tag = g_renew(gulong, listener->io_tag, listener->nsioc + 1);
+    listener->io_source = g_renew(typeof(listener->io_source[0]),
+                                  listener->io_source,
+                                  listener->nsioc + 1);
     listener->sioc[listener->nsioc] = sioc;
-    listener->io_tag[listener->nsioc] = 0;
+    listener->io_source[listener->nsioc] = NULL;
 
     object_ref(OBJECT(sioc));
     listener->connected = true;
 
     if (listener->io_func != NULL) {
         object_ref(OBJECT(listener));
-        listener->io_tag[listener->nsioc] = qio_net_listener_source_add(
-            listener, listener->sioc[listener->nsioc]);
+        listener->io_source[listener->nsioc] = qio_net_listener_source_add(
+            listener, listener->sioc[listener->nsioc], NULL);
     }
 
     listener->nsioc++;
 }
 
 
-void qio_net_listener_set_client_func(QIONetListener *listener,
-                                      QIONetListenerClientFunc func,
-                                      gpointer data,
-                                      GDestroyNotify notify)
+void qio_net_listener_set_client_func_full(QIONetListener *listener,
+                                           QIONetListenerClientFunc func,
+                                           gpointer data,
+                                           GDestroyNotify notify,
+                                           GMainContext *context)
 {
     if (listener->io_notify) {
         listener->io_notify(listener->io_data);
@@ -181,9 +189,17 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
     listener->io_notify = notify;
 
     qio_net_listener_sources_clear(listener);
-    qio_net_listener_sources_update(listener);
+    qio_net_listener_sources_update(listener, context);
 }
 
+void qio_net_listener_set_client_func(QIONetListener *listener,
+                                      QIONetListenerClientFunc func,
+                                      gpointer data,
+                                      GDestroyNotify notify)
+{
+    qio_net_listener_set_client_func_full(listener, func, data,
+                                          notify, NULL);
+}
 
 struct QIONetListenerClientWaitData {
     QIOChannelSocket *sioc;
@@ -248,7 +264,7 @@ QIOChannelSocket *qio_net_listener_wait_client(QIONetListener *listener)
     g_main_loop_unref(loop);
     g_main_context_unref(ctxt);
 
-    qio_net_listener_sources_update(listener);
+    qio_net_listener_sources_update(listener, NULL);
 
     return data.sioc;
 }
@@ -285,7 +301,7 @@ static void qio_net_listener_finalize(Object *obj)
     for (i = 0; i < listener->nsioc; i++) {
         object_unref(OBJECT(listener->sioc[i]));
     }
-    g_free(listener->io_tag);
+    g_free(listener->io_source);
     g_free(listener->sioc);
     g_free(listener->name);
 }
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 06/15] qio: store gsources for net listeners

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:29PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  include/io/net-listener.h | 21 ++++++++++++++--
>  io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
>  2 files changed, 58 insertions(+), 25 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 06/15] qio: store gsources for net listeners

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  include/io/net-listener.h | 21 ++++++++++++++--
>  io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
>  2 files changed, 58 insertions(+), 25 deletions(-)
> 
> diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> index 56d6da7a76..566be283b3 100644
> --- a/include/io/net-listener.h
> +++ b/include/io/net-listener.h
> @@ -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);
>  
>  /**
> - * 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".

Paolo

Re: [Qemu-devel] [PATCH v2 06/15] qio: store gsources for net listeners

[Peter Xu]

On Thu, Mar 01, 2018 at 06:12:57PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > 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 <peterx@redhat.com>
> > ---
> >  include/io/net-listener.h | 21 ++++++++++++++--
> >  io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
> >  2 files changed, 58 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > index 56d6da7a76..566be283b3 100644
> > --- a/include/io/net-listener.h
> > +++ b/include/io/net-listener.h
> > @@ -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);
> >  
> >  /**
> > - * 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".

Will do. Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 06/15] qio: store gsources for net listeners

[Peter Xu]

On Thu, Mar 01, 2018 at 06:12:57PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > 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 <peterx@redhat.com>
> > ---
> >  include/io/net-listener.h | 21 ++++++++++++++--
> >  io/net-listener.c         | 62 +++++++++++++++++++++++++++++------------------
> >  2 files changed, 58 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > index 56d6da7a76..566be283b3 100644
> > --- a/include/io/net-listener.h
> > +++ b/include/io/net-listener.h
> > @@ -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);
> >  
> >  /**
> > - * 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".

Fixed this one too (and for both patches, I kept Dan's r-b).  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 07/15] qio/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.

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 <peterx@redhat.com>
---
 chardev/char-socket.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 43a2cc2c1c..5cd20cc932 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -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. */
+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);
+}
+
 /* 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;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcontext

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:30PM +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.
> 
> 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 <peterx@redhat.com>
> ---
>  chardev/char-socket.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 43a2cc2c1c..5cd20cc932 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -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. */
> +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.

> +}
> +
>  /* 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;
> -- 
> 2.14.3
> 

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 07/15] qio/chardev: update net listener gcontext

[Peter Xu]

On Thu, Mar 01, 2018 at 03:43:31PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:30PM +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.
> > 
> > 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 <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c | 32 ++++++++++++++++++++++++++------
> >  1 file changed, 26 insertions(+), 6 deletions(-)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 43a2cc2c1c..5cd20cc932 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -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. */
> > +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.

(Here the naming of function is discussible for sure, along with how
 to define the parameters)

I think it may be a flavor issue.  In that case, I'm always fine with
either way. I assume the previous cleanup patch 5 is similarly a
flavor issue too, so I'll follow your final judgement on what you
would prefer.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcontext

[Paolo Bonzini]

On 02/03/2018 05:26, Peter Xu wrote:
> 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.

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.

Paolo

Re: [Qemu-devel] [PATCH v2 07/15] qio/chardev: update net listener gcontext

[Peter Xu]

On Fri, Mar 02, 2018 at 12:17:30PM +0100, Paolo Bonzini wrote:
> On 02/03/2018 05:26, Peter Xu wrote:
> > 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.
> 
> 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.

Thanks Paolo for your suggestion.

Let me just keep the code untouched so that we can be more focused on
the topic of the series.  I'll try to avoid unecessary clean ups
there.  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 08/15] 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 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 | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 5cd20cc932..6c3f1de013 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -59,6 +59,7 @@ typedef struct {
     bool is_listen;
     bool is_telnet;
     bool is_tn3270;
+    GSource *telnet_source;
 
     GSource *reconnect_timer;
     int64_t reconnect_time;
@@ -69,6 +70,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)
 {
@@ -567,6 +569,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);
@@ -580,6 +591,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
         tcp_chr_net_listener_setup(s, true);
     }
 
+    if (s->telnet_source) {
+        tcp_chr_telnet_destroy(s);
+        tcp_chr_telnet_init(CHARDEV(s));
+    }
+
     if (!s->connected) {
         return;
     }
@@ -604,6 +620,7 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
                                        gpointer user_data)
 {
     TCPChardevTelnetInit *init = user_data;
+    SocketChardev *s = SOCKET_CHARDEV(init->chr);
     ssize_t ret;
 
     ret = qio_channel_write(ioc, init->buf, init->buflen, NULL);
@@ -628,6 +645,8 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
 
 end:
     g_free(init);
+    g_source_unref(s->telnet_source);
+    s->telnet_source = NULL;
     return G_SOURCE_REMOVE;
 }
 
@@ -667,10 +686,10 @@ 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);
+    s->telnet_source = qio_channel_add_watch_source(s->ioc, G_IO_OUT,
+                                                    tcp_chr_telnet_init_io,
+                                                    init, NULL,
+                                                    chr->gcontext);
 }
 
 
@@ -843,6 +862,7 @@ 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);
     if (s->listener) {
         tcp_chr_net_listener_setup(s, false);
         object_unref(OBJECT(s->listener));
-- 
2.14.3

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

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:31PM +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 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.

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 08/15] chardev: allow telnet gsource to switch gcontext

[Paolo Bonzini]

On 01/03/2018 16:46, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:31PM +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 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.

Paolo

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

[Peter Xu]

On Thu, Mar 01, 2018 at 06:16:53PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 16:46, Daniel P. Berrangé wrote:
> > On Thu, Mar 01, 2018 at 04:44:31PM +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 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.

Ah, this reminded me that I should better cache the
TCPChardevTelnetInit struct, otherwise when context changes we'll
possibly restart a telnet handshake.

Actually if only considering the monitor-OOB series (which is the only
one now who may change the context) it's not really necessary, since
the context will only be changed once when init monitors. But it's
easy to even achieve a higher goal to support real dynamic switch for
telnet connections.  So I think I'll try that in my next post.

Thanks,

-- 
Peter Xu

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

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 06:16:53PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 16:46, Daniel P. Berrangé wrote:
> > On Thu, Mar 01, 2018 at 04:44:31PM +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 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.

NB, the chardev will emit the CHR_EVENT_OPENED event once the connection
is successfully established, which includes completion of the telnet
and/or TLS handshake.

I'm just not seeing what code in QEMU would actually trigger a decision
to change the GMainContext, in between the accept() of the socket, and
the CHR_EVENT_OPENED, since we don't tell anyone that the accept() has
actually taken place - they first they'll know of a new client being
connected is when the CHR_EVENT_OPENED is emitted.

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 09/15] qio: non-default context for threaded qtask

[Peter Xu]

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 <peterx@redhat.com>
---
 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(-)

diff --git a/include/io/task.h b/include/io/task.h
index 6021f51336..9dbe3758d7 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -227,15 +227,17 @@ QIOTask *qio_task_new(Object *source,
  * @worker: the function to invoke in a thread
  * @opaque: opaque data to pass to @worker
  * @destroy: function to free @opaque
+ * @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,
-                            GDestroyNotify destroy);
+                            GDestroyNotify destroy,
+                            GMainContext *context);
 
 /**
  * qio_task_complete:
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 563e297357..4224ce323a 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -187,7 +187,8 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
     qio_task_run_in_thread(task,
                            qio_channel_socket_connect_worker,
                            addrCopy,
-                           (GDestroyNotify)qapi_free_SocketAddress);
+                           (GDestroyNotify)qapi_free_SocketAddress,
+                           NULL);
 }
 
 
@@ -245,7 +246,8 @@ void qio_channel_socket_listen_async(QIOChannelSocket *ioc,
     qio_task_run_in_thread(task,
                            qio_channel_socket_listen_worker,
                            addrCopy,
-                           (GDestroyNotify)qapi_free_SocketAddress);
+                           (GDestroyNotify)qapi_free_SocketAddress,
+                           NULL);
 }
 
 
@@ -321,7 +323,8 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc,
     qio_task_run_in_thread(task,
                            qio_channel_socket_dgram_worker,
                            data,
-                           qio_channel_socket_dgram_worker_free);
+                           qio_channel_socket_dgram_worker_free,
+                           NULL);
 }
 
 
diff --git a/io/dns-resolver.c b/io/dns-resolver.c
index c072d121c3..75c2ca9c4a 100644
--- a/io/dns-resolver.c
+++ b/io/dns-resolver.c
@@ -233,7 +233,8 @@ void qio_dns_resolver_lookup_async(QIODNSResolver *resolver,
     qio_task_run_in_thread(task,
                            qio_dns_resolver_lookup_worker,
                            data,
-                           qio_dns_resolver_lookup_data_free);
+                           qio_dns_resolver_lookup_data_free,
+                           NULL);
 }
 
 
diff --git a/io/task.c b/io/task.c
index 1a0a1c7185..2886a2c1bc 100644
--- a/io/task.c
+++ b/io/task.c
@@ -77,6 +77,7 @@ struct QIOTaskThreadData {
     QIOTaskWorker worker;
     gpointer opaque;
     GDestroyNotify destroy;
+    GMainContext *context;
 };
 
 
@@ -91,6 +92,10 @@ static gboolean qio_task_thread_result(gpointer opaque)
         data->destroy(data->opaque);
     }
 
+    if (data->context) {
+        g_main_context_unref(data->context);
+    }
+
     g_free(data);
 
     return FALSE;
@@ -100,6 +105,7 @@ static gboolean qio_task_thread_result(gpointer opaque)
 static gpointer qio_task_thread_worker(gpointer opaque)
 {
     struct QIOTaskThreadData *data = opaque;
+    GSource *idle;
 
     trace_qio_task_thread_run(data->task);
     data->worker(data->task, data->opaque);
@@ -110,7 +116,11 @@ static gpointer qio_task_thread_worker(gpointer opaque)
      * the worker results
      */
     trace_qio_task_thread_exit(data->task);
-    g_idle_add(qio_task_thread_result, data);
+
+    idle = g_idle_source_new();
+    g_source_set_callback(idle, qio_task_thread_result, data, NULL);
+    g_source_attach(idle, data->context);
+
     return NULL;
 }
 
@@ -118,15 +128,21 @@ static gpointer qio_task_thread_worker(gpointer opaque)
 void qio_task_run_in_thread(QIOTask *task,
                             QIOTaskWorker worker,
                             gpointer opaque,
-                            GDestroyNotify destroy)
+                            GDestroyNotify destroy,
+                            GMainContext *context)
 {
     struct QIOTaskThreadData *data = g_new0(struct QIOTaskThreadData, 1);
     QemuThread thread;
 
+    if (context) {
+        g_main_context_ref(context);
+    }
+
     data->task = task;
     data->worker = worker;
     data->opaque = opaque;
     data->destroy = destroy;
+    data->context = context;
 
     trace_qio_task_thread_start(task, worker, opaque);
     qemu_thread_create(&thread,
diff --git a/tests/test-io-task.c b/tests/test-io-task.c
index 141aa2c55d..bac1bb4e7a 100644
--- a/tests/test-io-task.c
+++ b/tests/test-io-task.c
@@ -187,6 +187,7 @@ static void test_task_thread_complete(void)
     qio_task_run_in_thread(task,
                            test_task_thread_worker,
                            &data,
+                           NULL,
                            NULL);
 
     g_main_loop_run(data.loop);
@@ -228,6 +229,7 @@ static void test_task_thread_failure(void)
     qio_task_run_in_thread(task,
                            test_task_thread_worker,
                            &data,
+                           NULL,
                            NULL);
 
     g_main_loop_run(data.loop);
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 09/15] qio: non-default context for threaded qtask

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:32PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  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(-)
> 

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 09/15] qio: non-default context for threaded qtask

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> + * @context: the context to run the complete hook

Please note the behavior for NULL context here, too.

Paolo

>   * 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,
> -                            GDestroyNotify destroy);
> +                            GDestroyNotify destroy,
> +                            GMainContext *context);

[Qemu-devel] [PATCH v2 10/15] qio: non-default context for async conn

[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 <peterx@redhat.com>
---
 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(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 6c3f1de013..bd90680f5c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -911,7 +911,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
     tcp_chr_set_client_ioc_name(chr, sioc);
     qio_channel_socket_connect_async(sioc, s->addr,
                                      qemu_chr_socket_connected,
-                                     chr, NULL);
+                                     chr, NULL, NULL);
 
     return false;
 }
@@ -995,7 +995,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
         tcp_chr_set_client_ioc_name(chr, sioc);
         qio_channel_socket_connect_async(sioc, s->addr,
                                          qemu_chr_socket_connected,
-                                         chr, NULL);
+                                         chr, NULL, NULL);
     } else {
         if (s->is_listen) {
             char *name;
diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
index 53801f6042..90f7227397 100644
--- a/include/io/channel-socket.h
+++ b/include/io/channel-socket.h
@@ -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,
                                       gpointer opaque,
-                                      GDestroyNotify destroy);
+                                      GDestroyNotify destroy,
+                                      GMainContext *context);
 
 
 /**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 4224ce323a..a843e49939 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -173,7 +173,8 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
                                       SocketAddress *addr,
                                       QIOTaskFunc callback,
                                       gpointer opaque,
-                                      GDestroyNotify destroy)
+                                      GDestroyNotify destroy,
+                                      GMainContext *context)
 {
     QIOTask *task = qio_task_new(
         OBJECT(ioc), callback, opaque, destroy);
@@ -188,7 +189,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc,
                            qio_channel_socket_connect_worker,
                            addrCopy,
                            (GDestroyNotify)qapi_free_SocketAddress,
-                           NULL);
+                           context);
 }
 
 
diff --git a/migration/socket.c b/migration/socket.c
index 60d732535c..91071b06ef 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -103,7 +103,8 @@ static void socket_start_outgoing_migration(MigrationState *s,
                                      saddr,
                                      socket_outgoing_migration,
                                      data,
-                                     socket_connect_data_free);
+                                     socket_connect_data_free,
+                                     NULL);
     qapi_free_SocketAddress(saddr);
 }
 
diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index d357cd2a8e..a67635b9b8 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -200,7 +200,7 @@ static void test_io_channel_setup_async(SocketAddress *listen_addr,
 
     qio_channel_socket_connect_async(
         QIO_CHANNEL_SOCKET(*src), connect_addr,
-        test_io_channel_complete, &data, NULL);
+        test_io_channel_complete, &data, NULL, NULL);
 
     g_main_loop_run(data.loop);
     g_main_context_iteration(g_main_context_default(), FALSE);
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 10/15] qio: non-default context for async conn

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:33PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  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(-)

> diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> index 53801f6042..90f7227397 100644
> --- a/include/io/channel-socket.h
> +++ b/include/io/channel-socket.h
> @@ -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,
>                                        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.


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 10/15] qio: non-default context for async conn

[Peter Xu]

On Thu, Mar 01, 2018 at 03:48:44PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:33PM +0800, Peter Xu wrote:
> > 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 <peterx@redhat.com>
> > ---
> >  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(-)
> 
> > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > index 53801f6042..90f7227397 100644
> > --- a/include/io/channel-socket.h
> > +++ b/include/io/channel-socket.h
> > @@ -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,
> >                                        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.

Sure.  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 11/15] qio: non-default context for TLS handshake

[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 <peterx@redhat.com>
---
 include/io/channel-tls.h | 17 ++++++++++++++++
 io/channel-tls.c         | 51 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h
index d157eb10e8..d6f5271088 100644
--- a/include/io/channel-tls.h
+++ b/include/io/channel-tls.h
@@ -128,6 +128,23 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc,
                                gpointer opaque,
                                GDestroyNotify destroy);
 
+/**
+ * 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,
+                                    QIOTaskFunc func,
+                                    gpointer opaque,
+                                    GDestroyNotify destroy,
+                                    GMainContext *context);
+
 /**
  * qio_channel_tls_get_session:
  * @ioc: the TLS channel object
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 6182702dab..4fe03d9c6c 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -140,13 +140,19 @@ qio_channel_tls_new_client(QIOChannel *master,
     return NULL;
 }
 
+struct QIOChannelTLSData {
+    QIOTask *task;
+    GMainContext *context;
+};
+typedef struct QIOChannelTLSData QIOChannelTLSData;
 
 static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
                                              GIOCondition condition,
                                              gpointer user_data);
 
 static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
-                                           QIOTask *task)
+                                           QIOTask *task,
+                                           GMainContext *context)
 {
     Error *err = NULL;
     QCryptoTLSSessionHandshakeStatus status;
@@ -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;
+
         if (status == QCRYPTO_TLS_HANDSHAKE_SENDING) {
             condition = G_IO_OUT;
         } else {
@@ -178,11 +189,12 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         }
 
         trace_qio_channel_tls_handshake_pending(ioc, status);
-        qio_channel_add_watch(ioc->master,
-                              condition,
-                              qio_channel_tls_handshake_io,
-                              task,
-                              NULL);
+        qio_channel_add_watch_full(ioc->master,
+                                   condition,
+                                   qio_channel_tls_handshake_io,
+                                   data,
+                                   NULL,
+                                   context);
     }
 }
 
@@ -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));
 
-    qio_channel_tls_handshake_task(
-       tioc, task);
+    g_free(data);
+    qio_channel_tls_handshake_task(tioc, task, context);
 
     return FALSE;
 }
 
-void qio_channel_tls_handshake(QIOChannelTLS *ioc,
-                               QIOTaskFunc func,
-                               gpointer opaque,
-                               GDestroyNotify destroy)
+void qio_channel_tls_handshake_full(QIOChannelTLS *ioc,
+                                    QIOTaskFunc func,
+                                    gpointer opaque,
+                                    GDestroyNotify destroy,
+                                    GMainContext *context)
 {
     QIOTask *task;
 
@@ -212,7 +227,15 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc,
                         func, opaque, destroy);
 
     trace_qio_channel_tls_handshake_start(ioc);
-    qio_channel_tls_handshake_task(ioc, task);
+    qio_channel_tls_handshake_task(ioc, task, context);
+}
+
+void qio_channel_tls_handshake(QIOChannelTLS *ioc,
+                               QIOTaskFunc func,
+                               gpointer opaque,
+                               GDestroyNotify destroy)
+{
+    qio_channel_tls_handshake_full(ioc, func, opaque, destroy, NULL);
 }
 
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 11/15] qio: non-default context for TLS handshake

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:34PM +0800, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>  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,
> -                                           QIOTask *task)
> +                                           QIOTask *task,
> +                                           GMainContext *context)
>  {
>      Error *err = NULL;
>      QCryptoTLSSessionHandshakeStatus status;
> @@ -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....


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


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 11/15] qio: non-default context for TLS handshake

[Peter Xu]

On Thu, Mar 01, 2018 at 03:50:01PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:34PM +0800, Peter Xu wrote:
> > 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 <peterx@redhat.com>
> > ---
> >  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,
> > -                                           QIOTask *task)
> > +                                           QIOTask *task,
> > +                                           GMainContext *context)
> >  {
> >      Error *err = NULL;
> >      QCryptoTLSSessionHandshakeStatus status;
> > @@ -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....
> 
> 
> > @@ -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));
> >  
> > -    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.

Yeah, fixed both.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 11/15] qio: non-default context for TLS handshake

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> +/**
> + * 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,
> +                                    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.

Paolo

Re: [Qemu-devel] [PATCH v2 11/15] qio: non-default context for TLS handshake

[Peter Xu]

On Thu, Mar 01, 2018 at 06:22:40PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > +/**
> > + * 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,
> > +                                    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.

Will take your advise.  Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v2 12/15] 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.

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     | 29 +++++++++++++++++++++++++++++
 chardev/char.c         | 43 +++++++++++++++++--------------------------
 include/chardev/char.h |  2 ++
 3 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index d48e78103a..1379a9f254 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -347,6 +347,34 @@ 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)
+{
+    /* Set it multiple times won't hurt */
+    muxes_realized = true;
+
+    /* 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 +385,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.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,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 12/15] chardev: introduce chr_machine_done hook

[Paolo Bonzini]

On 01/03/2018 09:44, 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.
> 
> 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     | 29 +++++++++++++++++++++++++++++
>  chardev/char.c         | 43 +++++++++++++++++--------------------------
>  include/chardev/char.h |  2 ++
>  3 files changed, 48 insertions(+), 26 deletions(-)

Modulo the note on muxes_realized (see patch 15/15),

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH v2 13/15] char: 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.

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 bd90680f5c..cd9db123f2 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -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,
     }
 
     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 13/15] char: use chardev's gcontext for async connect

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> 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 <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 bd90680f5c..cd9db123f2 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -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,
>      }
>  
>      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;
> 


Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH v2 14/15] 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.

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 cd9db123f2..2b355fc7a8 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -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);
@@ -1143,6 +1153,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 14/15] chardev: tcp: postpone async connection setup

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:37PM +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.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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.

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 14/15] chardev: tcp: postpone async connection setup

[Peter Xu]

On Thu, Mar 01, 2018 at 04:01:38PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:37PM +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.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  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.

Not sure whether I fully got your point, but IMHO when "wait" is there
we should be perfectly fine too if with all the TLS/TELNET patches in
the this series.  And for sure this patch only solves the problem when
"wait" is specified.

Or say, with this series, all configuration of chardev _should_ work
with non-default context now.  If not, then I must have missed
something else (which may be possible), then I would be very glad that
anyone can give me a hint on where.  Thanks,

-- 
Peter Xu

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

[Paolo Bonzini]

On 01/03/2018 09:44, 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.
> 
> 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 cd9db123f2..2b355fc7a8 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -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);
> @@ -1143,6 +1153,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,
> 


Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

[Qemu-devel] [PATCH v2 15/15] 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.

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 <peterx@redhat.com>
---
 chardev/char-socket.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 2b355fc7a8..13aeca0b27 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -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;
 }
 
-- 
2.14.3

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

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:38PM +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.
> 
> 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 <peterx@redhat.com>
> ---
>  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.

> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 2b355fc7a8..13aeca0b27 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -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;
>  }
>  
> -- 
> 2.14.3
> 

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 15/15] chardev: tcp: postpone TLS work until machine done

[Peter Xu]

On Thu, Mar 01, 2018 at 04:03:04PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:38PM +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.
> > 
> > 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 <peterx@redhat.com>
> > ---
> >  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.

IMHO it's because the socket chardev is indeed complicated... If you
see qmp_chardev_open_socket(), that's where most of the complexity
lies in.  And as I explained, each of the patch, or group of patches,
were only trying to solve a single problem.

Though I admit this patch has brought a little bit more complexity,
though in the short term I don't see a better solution. And, if you
consider the existing MUX machine done hook, then it's merely using
the same way to do it but even cleaned it up a bit...

Please let me know if you have any suggestion that I can do it in a
better way.  Thanks!

-- 
Peter Xu

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

[Paolo Bonzini]

On 01/03/2018 09:44, Peter Xu wrote:
> +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;
> +    }
> +

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.

Thanks,

Paolo

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

[Peter Xu]

On Thu, Mar 01, 2018 at 06:37:47PM +0100, Paolo Bonzini wrote:
> On 01/03/2018 09:44, Peter Xu wrote:
> > +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;
> > +    }
> > +
> 
> 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.

Sure!  I'll add a new patch for it.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 00/15] qio: general non-default GMainContext support

[Daniel P. Berrangé]

On Thu, Mar 01, 2018 at 04:44:23PM +0800, Peter Xu wrote:
> 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.

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. 


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 00/15] qio: general non-default GMainContext support

[Peter Xu]

On Thu, Mar 01, 2018 at 04:07:06PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:23PM +0800, Peter Xu wrote:
> > 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.
> 
> 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. 

Yeah I think I'll split the series into two.

Thank you and Paolo for the quick review comments!

-- 
Peter Xu