[Qemu-devel] [PATCH 0/6] monitor: misc fixes

[Qemu-devel] [PATCH 0/6] monitor: misc fixes

[Marc-André Lureau]

Hi,

Here is a small series of fixes for the monitor, mostly related to
threading issues.

Marc-André Lureau (6):
  monitor: inline ambiguous helper functions
  monitor: accept chardev input from iothread
  char: add a QEMU_CHAR_FEATURE_GCONTEXT flag
  monitor: check if chardev can switch gcontext for OOB
  monitor: prevent inserting new monitors after cleanup
  monitor: avoid potential dead-lock when cleaning up

 include/chardev/char.h |  3 +++
 chardev/char.c         | 11 ++++++++++
 monitor.c              | 47 ++++++++++++++++++++++++++----------------
 3 files changed, 43 insertions(+), 18 deletions(-)

-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH 1/6] monitor: inline ambiguous helper functions

[Marc-André Lureau]

The function were not named with "mon_iothread", or following the AIO
vs GMainContext distinction. Inline them instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index b9258a7438..ab60c9f87e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4441,16 +4441,6 @@ static void sortcmdlist(void)
     qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
 }
 
-static GMainContext *monitor_get_io_context(void)
-{
-    return iothread_get_g_main_context(mon_iothread);
-}
-
-static AioContext *monitor_get_aio_context(void)
-{
-    return iothread_get_aio_context(mon_iothread);
-}
-
 static void monitor_iothread_init(void)
 {
     mon_iothread = iothread_create("mon_iothread", &error_abort);
@@ -4538,7 +4528,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     GMainContext *context;
 
     assert(mon->use_io_thread);
-    context = monitor_get_io_context();
+    context = iothread_get_g_main_context(mon_iothread);
     assert(context);
     qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                              monitor_qmp_event, NULL, mon, context, true);
@@ -4590,7 +4580,7 @@ void monitor_init(Chardev *chr, int flags)
              * since chardev might be running in the monitor I/O
              * thread.  Schedule a bottom half.
              */
-            aio_bh_schedule_oneshot(monitor_get_aio_context(),
+            aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
                                     monitor_qmp_setup_handlers_bh, mon);
             /* The bottom half will add @mon to @mon_list */
             return;
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH 2/6] monitor: accept chardev input from iothread

[Marc-André Lureau]

Chardev backends may not handle safely IO events from concurrent
threads. Better to wake up the chardev from the monitor IO thread if
it's being used as the chardev context.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index ab60c9f87e..a25514490a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4297,6 +4297,13 @@ int monitor_suspend(Monitor *mon)
     return 0;
 }
 
+static void monitor_accept_input(void *opaque)
+{
+    Monitor *mon = opaque;
+
+    qemu_chr_fe_accept_input(&mon->chr);
+}
+
 void monitor_resume(Monitor *mon)
 {
     if (monitor_is_hmp_non_interactive(mon)) {
@@ -4310,13 +4317,14 @@ void monitor_resume(Monitor *mon)
              * let's kick the thread in case it's sleeping.
              */
             if (mon->use_io_thread) {
-                aio_notify(iothread_get_aio_context(mon_iothread));
+                aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
+                                        monitor_accept_input, mon);
             }
         } else {
             assert(mon->rs);
             readline_show_prompt(mon->rs);
+            monitor_accept_input(mon);
         }
-        qemu_chr_fe_accept_input(&mon->chr);
     }
     trace_monitor_suspend(mon, -1);
 }
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Marc-André Lureau]

The feature should be set if the chardev is able to switch
GMainContext. Callers that want to put a chardev in a different thread
context can/should check this capabilities.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char.h |  3 +++
 chardev/char.c         | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 7becd8c80c..014566c3de 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -47,6 +47,9 @@ typedef enum {
     QEMU_CHAR_FEATURE_FD_PASS,
     /* Whether replay or record mode is enabled */
     QEMU_CHAR_FEATURE_REPLAY,
+    /* Whether the gcontext can be changed after calling
+     * qemu_chr_be_update_read_handlers() */
+    QEMU_CHAR_FEATURE_GCONTEXT,
 
     QEMU_CHAR_FEATURE_LAST,
 } ChardevFeature;
diff --git a/chardev/char.c b/chardev/char.c
index e115166995..fa1b74e0d9 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
     s->gcontext = context;
     if (cc->chr_update_read_handler) {
         cc->chr_update_read_handler(s);
+    } else if (s->gcontext) {
+        error_report("switching context isn't supported by this chardev");
     }
 }
 
@@ -240,6 +242,15 @@ static void char_init(Object *obj)
 
     chr->logfd = -1;
     qemu_mutex_init(&chr->chr_write_lock);
+
+    /*
+     * Assume if chr_update_read_handler is implemented it will
+     * take the updated gcontext into account.
+     */
+    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
+        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
+    }
+
 }
 
 static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH 4/6] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Note: this patch will conflict with Peter "[PATCH v9 3/6] monitor:
remove "x-oob", turn oob on by default", but can be trivially updated.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index a25514490a..c175cf6f0d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4550,9 +4550,10 @@ void monitor_init(Chardev *chr, int flags)
     bool use_oob = flags & MONITOR_USE_OOB;
 
     if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {
+        if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
             error_report("Monitor out-of-band is not supported with "
-                         "MUX typed chardev backend");
+                         "%s typed chardev backend",
+                         object_get_typename(OBJECT(chr)));
             exit(1);
         }
         if (use_readline) {
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH 5/6] monitor: prevent inserting new monitors after cleanup

[Marc-André Lureau]

Add a monitor_destroyed global to check if monitor_cleanup() has been
already called. In this case, don't insert the new monitor in the
list, but free it instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index c175cf6f0d..90b7867ed4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -262,10 +262,11 @@ typedef struct QMPRequest QMPRequest;
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
-/* Protects mon_list, monitor_qapi_event_state.  */
+/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
 static QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
+static bool monitor_destroyed;
 
 /* Protects mon_fdsets */
 static QemuMutex mon_fdsets_lock;
@@ -4526,8 +4527,16 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
 static void monitor_list_append(Monitor *mon)
 {
     qemu_mutex_lock(&monitor_lock);
-    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+    if (!monitor_destroyed) {
+        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+        mon = NULL;
+    }
     qemu_mutex_unlock(&monitor_lock);
+
+    if (mon) {
+        monitor_data_destroy(mon);
+        g_free(mon);
+    }
 }
 
 static void monitor_qmp_setup_handlers_bh(void *opaque)
@@ -4620,6 +4629,7 @@ void monitor_cleanup(void)
 
     /* Flush output buffers and destroy monitors */
     qemu_mutex_lock(&monitor_lock);
+    monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
         monitor_flush(mon);
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH 6/6] monitor: avoid potential dead-lock when cleaning up

[Marc-André Lureau]

When a monitor is connected to a Spice chardev, the monitor cleanup
can dead-lock:

 #0  0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0
 #1  0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0
 #2  0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 <monitor_lock>, file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66
 #3  0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645
 #4  0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 <error_abort>) at qapi/qapi-events-ui.c:149
 #5  0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235
 #6  0x00007f434560a6bb in reds_handle_channel_event (reds=<optimized out>, event=3, info=0x556ddad1b590) at reds.c:316
 #7  0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197
 #8  0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197
 #9  0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414
 #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388
 #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347
 #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0
 #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341
 #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241
 #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340
 #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280
 #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233
 #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786
 #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683
 #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660

Because spice code tries to emit a "disconnected" signal on the
monitors. Fix this dead-lock by releasing the monitor lock for
flush/destroy.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/monitor.c b/monitor.c
index 90b7867ed4..cb80f39ada 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4632,8 +4632,10 @@ void monitor_cleanup(void)
     monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
+        qemu_mutex_unlock(&monitor_lock);
         monitor_flush(mon);
         monitor_data_destroy(mon);
+        qemu_mutex_lock(&monitor_lock);
         g_free(mon);
     }
     qemu_mutex_unlock(&monitor_lock);
-- 
2.19.0.271.gfe8321ec05

Re: [Qemu-devel] [PATCH 2/6] monitor: accept chardev input from iothread

[Peter Xu]

On Tue, Oct 09, 2018 at 05:12:47PM +0400, Marc-André Lureau wrote:
> Chardev backends may not handle safely IO events from concurrent
> threads. Better to wake up the chardev from the monitor IO thread if
> it's being used as the chardev context.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ab60c9f87e..a25514490a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4297,6 +4297,13 @@ int monitor_suspend(Monitor *mon)
>      return 0;
>  }
>  
> +static void monitor_accept_input(void *opaque)
> +{
> +    Monitor *mon = opaque;
> +
> +    qemu_chr_fe_accept_input(&mon->chr);
> +}
> +
>  void monitor_resume(Monitor *mon)
>  {
>      if (monitor_is_hmp_non_interactive(mon)) {
> @@ -4310,13 +4317,14 @@ void monitor_resume(Monitor *mon)
>               * let's kick the thread in case it's sleeping.
>               */
>              if (mon->use_io_thread) {
> -                aio_notify(iothread_get_aio_context(mon_iothread));
> +                aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +                                        monitor_accept_input, mon);

Just to make sure: this will definitely cover the previous
aio_notify(), am I right?

Imho some comment would always be nice here because QMPs with
use_io_thread=true seems special anyway.

>              }
>          } else {
>              assert(mon->rs);
>              readline_show_prompt(mon->rs);
> +            monitor_accept_input(mon);
>          }
> -        qemu_chr_fe_accept_input(&mon->chr);

How about the QMP monitors with oob=off?  Will it miss the call?

I would consider caching the aio context into Monitor struct when
monitor init, then call aio_bh_schedule_oneshot() always with the
per-monitor aio cache.  This could unify the code paths too so we keep
the oob special path as less as possible.

>      }
>      trace_monitor_suspend(mon, -1);
>  }
> -- 
> 2.19.0.271.gfe8321ec05
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Peter Xu]

On Tue, Oct 09, 2018 at 05:12:48PM +0400, Marc-André Lureau wrote:
> The feature should be set if the chardev is able to switch
> GMainContext. Callers that want to put a chardev in a different thread
> context can/should check this capabilities.

IIRC we've had some discussion about whether we should allow to
dynamically switch context for chardevs, and a (temporarily)
conclusion is that we'd prefer to forbidden it for simplicity
(although it's still allowed in current master).  Does this patch mean
that we'd still want to allow that for some future scenarios?

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char.h |  3 +++
>  chardev/char.c         | 11 +++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 7becd8c80c..014566c3de 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -47,6 +47,9 @@ typedef enum {
>      QEMU_CHAR_FEATURE_FD_PASS,
>      /* Whether replay or record mode is enabled */
>      QEMU_CHAR_FEATURE_REPLAY,
> +    /* Whether the gcontext can be changed after calling
> +     * qemu_chr_be_update_read_handlers() */
> +    QEMU_CHAR_FEATURE_GCONTEXT,
>  
>      QEMU_CHAR_FEATURE_LAST,
>  } ChardevFeature;
> diff --git a/chardev/char.c b/chardev/char.c
> index e115166995..fa1b74e0d9 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
>      s->gcontext = context;
>      if (cc->chr_update_read_handler) {
>          cc->chr_update_read_handler(s);
> +    } else if (s->gcontext) {
> +        error_report("switching context isn't supported by this chardev");
>      }
>  }
>  
> @@ -240,6 +242,15 @@ static void char_init(Object *obj)
>  
>      chr->logfd = -1;
>      qemu_mutex_init(&chr->chr_write_lock);
> +
> +    /*
> +     * Assume if chr_update_read_handler is implemented it will
> +     * take the updated gcontext into account.
> +     */
> +    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
> +    }
> +
>  }
>  
>  static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
> -- 
> 2.19.0.271.gfe8321ec05
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/6] monitor: check if chardev can switch gcontext for OOB

[Peter Xu]

On Tue, Oct 09, 2018 at 05:12:49PM +0400, Marc-André Lureau wrote:
> Note: this patch will conflict with Peter "[PATCH v9 3/6] monitor:
> remove "x-oob", turn oob on by default", but can be trivially updated.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index a25514490a..c175cf6f0d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4550,9 +4550,10 @@ void monitor_init(Chardev *chr, int flags)
>      bool use_oob = flags & MONITOR_USE_OOB;
>  
>      if (use_oob) {
> -        if (CHARDEV_IS_MUX(chr)) {
> +        if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
>              error_report("Monitor out-of-band is not supported with "
> -                         "MUX typed chardev backend");
> +                         "%s typed chardev backend",
> +                         object_get_typename(OBJECT(chr)));

This seems a bit confusing to me at the first glance since we forbid
mux because not all frontends are ready to run outside main loop (and
now we have mon_iothread so it'll be odd too to run anything
non-monitor on that too...), rather than whether the backend can
dynamically switch its context.

I'm not sure, but do you mean you want to disable oob for backends
like spice or braille?  I just noticed that it seems even legal if we
pipe a qmp monitor with a windows mouse...

I believe in all cases the commit message can be enhanced on
explaining "why" of this patch. :)

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/6] monitor: accept chardev input from iothread

[Marc-André Lureau]

Hi

On Wed, Oct 10, 2018 at 7:45 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 09, 2018 at 05:12:47PM +0400, Marc-André Lureau wrote:
> > Chardev backends may not handle safely IO events from concurrent
> > threads. Better to wake up the chardev from the monitor IO thread if
> > it's being used as the chardev context.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index ab60c9f87e..a25514490a 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4297,6 +4297,13 @@ int monitor_suspend(Monitor *mon)
> >      return 0;
> >  }
> >
> > +static void monitor_accept_input(void *opaque)
> > +{
> > +    Monitor *mon = opaque;
> > +
> > +    qemu_chr_fe_accept_input(&mon->chr);
> > +}
> > +
> >  void monitor_resume(Monitor *mon)
> >  {
> >      if (monitor_is_hmp_non_interactive(mon)) {
> > @@ -4310,13 +4317,14 @@ void monitor_resume(Monitor *mon)
> >               * let's kick the thread in case it's sleeping.
> >               */
> >              if (mon->use_io_thread) {
> > -                aio_notify(iothread_get_aio_context(mon_iothread));
> > +                aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> > +                                        monitor_accept_input, mon);
>
> Just to make sure: this will definitely cover the previous
> aio_notify(), am I right?
>
> Imho some comment would always be nice here because QMPs with
> use_io_thread=true seems special anyway.
>
> >              }
> >          } else {
> >              assert(mon->rs);
> >              readline_show_prompt(mon->rs);
> > +            monitor_accept_input(mon);
> >          }
> > -        qemu_chr_fe_accept_input(&mon->chr);
>
> How about the QMP monitors with oob=off?  Will it miss the call?

good catch

>
> I would consider caching the aio context into Monitor struct when
> monitor init, then call aio_bh_schedule_oneshot() always with the
> per-monitor aio cache.  This could unify the code paths too so we keep
> the oob special path as less as possible.

Saving the aio context isn't really necessary. However, I'll unify the
code paths in v2.

thanks

>
> >      }
> >      trace_monitor_suspend(mon, -1);
> >  }
> > --
> > 2.19.0.271.gfe8321ec05
> >
>
> Regards,
>
> --
> Peter Xu
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Marc-André Lureau]

Hi

On Wed, Oct 10, 2018 at 7:54 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 09, 2018 at 05:12:48PM +0400, Marc-André Lureau wrote:
> > The feature should be set if the chardev is able to switch
> > GMainContext. Callers that want to put a chardev in a different thread
> > context can/should check this capabilities.
>
> IIRC we've had some discussion about whether we should allow to
> dynamically switch context for chardevs, and a (temporarily)
> conclusion is that we'd prefer to forbidden it for simplicity
> (although it's still allowed in current master).  Does this patch mean
> that we'd still want to allow that for some future scenarios?

Currently, the API let you change context dynamically, although doing
it safely is left for the caller to handle. And it's not guarantee to
work with all backends: some chardev don't simply allow you to use a
different gcontext, this is what this flag should be about.

The flag may become obsolete if we change the API to restrict setting
context at creation time. For now, I think we should consider this
patch and the following one.

thanks

>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/chardev/char.h |  3 +++
> >  chardev/char.c         | 11 +++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> > index 7becd8c80c..014566c3de 100644
> > --- a/include/chardev/char.h
> > +++ b/include/chardev/char.h
> > @@ -47,6 +47,9 @@ typedef enum {
> >      QEMU_CHAR_FEATURE_FD_PASS,
> >      /* Whether replay or record mode is enabled */
> >      QEMU_CHAR_FEATURE_REPLAY,
> > +    /* Whether the gcontext can be changed after calling
> > +     * qemu_chr_be_update_read_handlers() */
> > +    QEMU_CHAR_FEATURE_GCONTEXT,
> >
> >      QEMU_CHAR_FEATURE_LAST,
> >  } ChardevFeature;
> > diff --git a/chardev/char.c b/chardev/char.c
> > index e115166995..fa1b74e0d9 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
> >      s->gcontext = context;
> >      if (cc->chr_update_read_handler) {
> >          cc->chr_update_read_handler(s);
> > +    } else if (s->gcontext) {
> > +        error_report("switching context isn't supported by this chardev");
> >      }
> >  }
> >
> > @@ -240,6 +242,15 @@ static void char_init(Object *obj)
> >
> >      chr->logfd = -1;
> >      qemu_mutex_init(&chr->chr_write_lock);
> > +
> > +    /*
> > +     * Assume if chr_update_read_handler is implemented it will
> > +     * take the updated gcontext into account.
> > +     */
> > +    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
> > +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
> > +    }
> > +
> >  }
> >
> >  static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
> > --
> > 2.19.0.271.gfe8321ec05
> >
>
> Regards,
>
> --
> Peter Xu
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 4/6] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Hi

On Wed, Oct 10, 2018 at 8:38 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 09, 2018 at 05:12:49PM +0400, Marc-André Lureau wrote:
> > Note: this patch will conflict with Peter "[PATCH v9 3/6] monitor:
> > remove "x-oob", turn oob on by default", but can be trivially updated.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index a25514490a..c175cf6f0d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4550,9 +4550,10 @@ void monitor_init(Chardev *chr, int flags)
> >      bool use_oob = flags & MONITOR_USE_OOB;
> >
> >      if (use_oob) {
> > -        if (CHARDEV_IS_MUX(chr)) {
> > +        if (!qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> >              error_report("Monitor out-of-band is not supported with "
> > -                         "MUX typed chardev backend");
> > +                         "%s typed chardev backend",
> > +                         object_get_typename(OBJECT(chr)));
>
> This seems a bit confusing to me at the first glance since we forbid
> mux because not all frontends are ready to run outside main loop (and
> now we have mon_iothread so it'll be odd too to run anything
> non-monitor on that too...), rather than whether the backend can
> dynamically switch its context.
>
> I'm not sure, but do you mean you want to disable oob for backends
> like spice or braille?  I just noticed that it seems even legal if we
> pipe a qmp monitor with a windows mouse...

yes, basically any chardev that doesn't handle the gcontext argument.

They match those that don't implement chr_update_read_handler at this point.

> I believe in all cases the commit message can be enhanced on
> explaining "why" of this patch. :)

ok, I'll keep the explicit mux case, even if it overlaps with gcontext
feature, and update commit message.

thanks

>
> Regards,
>
> --
> Peter Xu
>


-- 
Marc-André Lureau