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

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

[Marc-André Lureau]

Hi,

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

v3:
 - replaced an error_report() with an assert()
 - simplify a mon->use_io_thread condition, removing needless QMP check
 - modify/add some code comments
 - update commit messages

v2 bis:
 - update comments/commit messages
 - add Peter r-b

v2: after Peter review
 - patch 2: fix resuming with oob=off
 - patch 4: keep MUX case explicit, improve commit message

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              | 71 ++++++++++++++++++++++++++----------------
 3 files changed, 59 insertions(+), 26 deletions(-)

-- 
2.20.0.rc1

[Qemu-devel] [PATCH v3 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>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index d39390c2f2..d531e8ccc9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4453,16 +4453,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);
@@ -4549,7 +4539,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);
@@ -4601,7 +4591,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.20.0.rc1

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

[Marc-André Lureau]

Chardev backends may not handle safely IO events from concurrent
threads (they are not thread-safe in general, only the write path is
since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). Better to
wake up the chardev from the monitor IO thread if it's being used as
the chardev context.

Unify code paths by using a BH in all cases.

Drop the now redundant aio_notify() call.

Simplify the condition, based on mon->use_io_thread (only QMP so far).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index d531e8ccc9..79afe99079 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4297,7 +4297,7 @@ int monitor_suspend(Monitor *mon)
 
     atomic_inc(&mon->suspend_cnt);
 
-    if (monitor_is_qmp(mon) && mon->use_io_thread) {
+    if (mon->use_io_thread) {
         /*
          * Kick I/O thread to make sure this takes effect.  It'll be
          * evaluated again in prepare() of the watch object.
@@ -4309,6 +4309,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)) {
@@ -4316,20 +4323,22 @@ void monitor_resume(Monitor *mon)
     }
 
     if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
-        if (monitor_is_qmp(mon)) {
-            /*
-             * For QMP monitors that are running in the I/O thread,
-             * let's kick the thread in case it's sleeping.
-             */
-            if (mon->use_io_thread) {
-                aio_notify(iothread_get_aio_context(mon_iothread));
-            }
+        AioContext *ctx;
+
+        if (mon->use_io_thread) {
+            ctx = iothread_get_aio_context(mon_iothread);
         } else {
+            ctx = qemu_get_aio_context();
+        }
+
+        if (!monitor_is_qmp(mon)) {
             assert(mon->rs);
             readline_show_prompt(mon->rs);
         }
-        qemu_chr_fe_accept_input(&mon->chr);
+
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
     }
+
     trace_monitor_suspend(mon, -1);
 }
 
-- 
2.20.0.rc1

[Qemu-devel] [PATCH v3 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 capability. Otherwise, print an
error (arguably, it may assert instead).

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 152dde5327..123b566cba 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) {
+        g_assert_not_reached();
     }
 }
 
@@ -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.20.0.rc1

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

[Marc-André Lureau]

Not all backends are able to switch gcontext. Those backends cannot
drive a OOB monitor (the monitor would then be blocking on main
thread).

For example, ringbuf, spice, or more esoteric input chardevs like
braille or MUX.

We currently forbid MUX because not all frontends are ready to run
outside main loop. Extend to add a context-switching feature check.

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

diff --git a/monitor.c b/monitor.c
index 79afe99079..25cf4223e8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
     bool use_oob = flags & MONITOR_USE_OOB;
 
     if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {
+        if (CHARDEV_IS_MUX(chr) ||
+            !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.20.0.rc1

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

[Marc-André Lureau]

monitor_cleanup() is one of the last things main() calls before it
returns.  In the following patch, monitor_cleanup() will release the
monitor_lock during flushing. There may be pending commands to insert
new monitors, which would modify the mon_list during iteration, and
the clean-up could thus miss those new insertions.

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 | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 25cf4223e8..f0256bdec5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -263,10 +263,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;
@@ -4538,8 +4539,21 @@ 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);
+    /*
+     * This prevents inserting new monitors during monitor_cleanup().
+     * A cleaner solution would involve the main thread telling other
+     * threads to terminate, waiting for their termination.
+     */
+    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)
@@ -4635,6 +4649,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.20.0.rc1

[Qemu-devel] [PATCH v3 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.

monitor_lock protects mon_list, monitor_qapi_event_state and
monitor_destroyed. monitor_flush() and monitor_data_destroy() don't
access any of those variables.

monitor_cleanup()'s loop is safe because it uses
QTAILQ_FOREACH_SAFE(), and no further monitor can be added after
calling monitor_cleanup() thanks to monitor_destroyed check in
monitor_list_append().

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

diff --git a/monitor.c b/monitor.c
index f0256bdec5..936c040b2d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4652,8 +4652,11 @@ void monitor_cleanup(void)
     monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
+        /* Permit QAPI event emission from character frontend release */
+        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.20.0.rc1

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Chardev backends may not handle safely IO events from concurrent
> threads (they are not thread-safe in general, only the write path is

Suggest "may not handle I/O events from concurrent threads safely".

> since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). Better to
> wake up the chardev from the monitor IO thread if it's being used as
> the chardev context.
>
> Unify code paths by using a BH in all cases.
>
> Drop the now redundant aio_notify() call.
>
> Simplify the condition, based on mon->use_io_thread (only QMP so far).

Suggest

  Clean up control flow not to rely on mon->use_io_thread implying
  monitor_is_qmp(mon).

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Happy to improve the commit message in my tree.

Preferably with commit message improvements:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> 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 capability. Otherwise, print an
> error (arguably, it may assert instead).

Really?  I think you do assert instead.  Hmm, misunderstanding; this
isn't about what the patch does, it's about what callers should do.
Perhaps:

  QEMU_CHAR_FEATURE_GCONTEXT declares the character device can switch
  GMainContext.

  Assert we don't switch context when the character device doesn't
  provide this feature.  Character device users must not violate this
  restriction.  In particular, user configurations that violate them
  must be rejected.

This leads me to my next question: is it currently possible to violate
this restriction and trip the new assertion?

>
> 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 152dde5327..123b566cba 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) {
> +        g_assert_not_reached();
>      }

Hmm.

cc->chr_update_read_handler is true iff QEMU_CHAR_FEATURE_GCONTEXT.
Therefore, the assertion really asserts "context may be non-null only
when we have QEMU_CHAR_FEATURE_GCONTEXT".  Let's make that clearer:

       assert(qemu_chr_has_feature(s, QEMU_CHAR_FEATURE_GCONTEXT)
              || !context);
       s->gcontext = context;
       if (cc->chr_update_read_handler) {
           cc->chr_update_read_handler(s);
       }

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

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

[Marc-André Lureau]

On Wed, Dec 5, 2018 at 12:21 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Chardev backends may not handle safely IO events from concurrent
> > threads (they are not thread-safe in general, only the write path is
>
> Suggest "may not handle I/O events from concurrent threads safely".
>
> > since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). Better to
> > wake up the chardev from the monitor IO thread if it's being used as
> > the chardev context.
> >
> > Unify code paths by using a BH in all cases.
> >
> > Drop the now redundant aio_notify() call.
> >
> > Simplify the condition, based on mon->use_io_thread (only QMP so far).
>
> Suggest
>
>   Clean up control flow not to rely on mon->use_io_thread implying
>   monitor_is_qmp(mon).
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Happy to improve the commit message in my tree.
>
> Preferably with commit message improvements:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>


ack, thanks


-- 
Marc-André Lureau

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Not all backends are able to switch gcontext. Those backends cannot
> drive a OOB monitor (the monitor would then be blocking on main
> thread).
>
> For example, ringbuf, spice, or more esoteric input chardevs like
> braille or MUX.
>
> We currently forbid MUX because not all frontends are ready to run
> outside main loop. Extend to add a context-switching feature check.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 79afe99079..25cf4223e8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
>      bool use_oob = flags & MONITOR_USE_OOB;
>  
>      if (use_oob) {
> -        if (CHARDEV_IS_MUX(chr)) {
> +        if (CHARDEV_IS_MUX(chr) ||
> +            !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) {

Aha, this answers my question on the previous patch: yes, it is possible
to trip the new assertion.

Are there any ways other than this one?

We could squash the two patches.  But I figure you kept the previous
patch separate on purpose.  That's okay, but it should mention the
assertion can be tripped, and the next patch (this one) will fix it.

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> monitor_cleanup() is one of the last things main() calls before it
> returns.  In the following patch, monitor_cleanup() will release the
> monitor_lock during flushing. There may be pending commands to insert
> new monitors, which would modify the mon_list during iteration, and
> the clean-up could thus miss those new insertions.
>
> 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 | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 25cf4223e8..f0256bdec5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -263,10 +263,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;
> @@ -4538,8 +4539,21 @@ 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);
> +    /*
> +     * This prevents inserting new monitors during monitor_cleanup().
> +     * A cleaner solution would involve the main thread telling other
> +     * threads to terminate, waiting for their termination.

Let's add this sentence to the commit message as well.

> +     */
> +    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)
> @@ -4635,6 +4649,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);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> 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.
>
> monitor_lock protects mon_list, monitor_qapi_event_state and
> monitor_destroyed. monitor_flush() and monitor_data_destroy() don't
> access any of those variables.
>
> monitor_cleanup()'s loop is safe because it uses
> QTAILQ_FOREACH_SAFE(), and no further monitor can be added after
> calling monitor_cleanup() thanks to monitor_destroyed check in
> monitor_list_append().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index f0256bdec5..936c040b2d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4652,8 +4652,11 @@ void monitor_cleanup(void)
>      monitor_destroyed = true;
>      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>          QTAILQ_REMOVE(&mon_list, mon, entry);
> +        /* Permit QAPI event emission from character frontend release */
> +        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);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Marc-André Lureau]

Hi

On Wed, Dec 5, 2018 at 12:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Not all backends are able to switch gcontext. Those backends cannot
> > drive a OOB monitor (the monitor would then be blocking on main
> > thread).
> >
> > For example, ringbuf, spice, or more esoteric input chardevs like
> > braille or MUX.
> >
> > We currently forbid MUX because not all frontends are ready to run
> > outside main loop. Extend to add a context-switching feature check.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 79afe99079..25cf4223e8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
> >      bool use_oob = flags & MONITOR_USE_OOB;
> >
> >      if (use_oob) {
> > -        if (CHARDEV_IS_MUX(chr)) {
> > +        if (CHARDEV_IS_MUX(chr) ||
> > +            !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) {
>
> Aha, this answers my question on the previous patch: yes, it is possible
> to trip the new assertion.
>
> Are there any ways other than this one?

Good question, if there are, we have latent bugs if switching gcontext
on a non-capable chardev.

Doing a quick review of qemu_chr_fe_set_handlers() for context != NULL
reveals net/colo-compare.c:

I think we should have the capability check added to find_and_check_chardev().

I don't see other candidates. Considering the problem is pre-existing,
I can either update the patch or add a follow-up patch.

>
> We could squash the two patches.  But I figure you kept the previous
> patch separate on purpose.  That's okay, but it should mention the
> assertion can be tripped, and the next patch (this one) will fix it.
>


-- 
Marc-André Lureau

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Dec 5, 2018 at 12:43 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Not all backends are able to switch gcontext. Those backends cannot
>> > drive a OOB monitor (the monitor would then be blocking on main
>> > thread).
>> >
>> > For example, ringbuf, spice, or more esoteric input chardevs like
>> > braille or MUX.
>> >
>> > We currently forbid MUX because not all frontends are ready to run
>> > outside main loop. Extend to add a context-switching feature check.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  monitor.c | 6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index 79afe99079..25cf4223e8 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
>> >      bool use_oob = flags & MONITOR_USE_OOB;
>> >
>> >      if (use_oob) {
>> > -        if (CHARDEV_IS_MUX(chr)) {
>> > +        if (CHARDEV_IS_MUX(chr) ||
>> > +            !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) {
>>
>> Aha, this answers my question on the previous patch: yes, it is possible
>> to trip the new assertion.
>>
>> Are there any ways other than this one?
>
> Good question, if there are, we have latent bugs if switching gcontext
> on a non-capable chardev.
>
> Doing a quick review of qemu_chr_fe_set_handlers() for context != NULL
> reveals net/colo-compare.c:
>
> I think we should have the capability check added to find_and_check_chardev().
>
> I don't see other candidates. Considering the problem is pre-existing,
> I can either update the patch or add a follow-up patch.

If the pre-existing bug is just as bad as an assertion failure, then I'm
fine with turning it into an assertion failure, with a brief explanation
in the commit message.

>> We could squash the two patches.  But I figure you kept the previous
>> patch separate on purpose.  That's okay, but it should mention the
>> assertion can be tripped, and the next patch (this one) will fix it.

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

[Markus Armbruster]

Looks ready except for a few commit message tweaks and the proposed
assertion tweak in 3/6.