[PATCH 0/4] monitor/hmp: cleanup monitor_event() and suspend_cnt

[PATCH 0/4] monitor/hmp: cleanup monitor_event() and suspend_cnt

[Paolo Bonzini]

monitor_event() is not using monitor_suspend() and monitor_resume()
even though that is the desired outcome of moving out of and back into
the monitor with Ctrl-a c.

While cleaning this up, make the locking of several fields in struct
Monitor less esoteric, i.e. protect them just with mon_lock.  This gets
rid of one of the last two cases where qatomic_mb_read()/qatomic_mb_set()
pair is used to achieve a semblance of sequential consistency.

Paolo

Paolo Bonzini (4):
  monitor: use QEMU_LOCK_GUARD a bit more
  monitor: allow calling monitor_resume under mon_lock
  monitor: add more *_locked() functions
  monitor: do not use mb_read/mb_set for suspend_cnt

 include/monitor/monitor.h     |  3 +++
 monitor/hmp.c                 | 41 ++++++++++++++++-------------------
 monitor/monitor-internal.h    |  3 +--
 monitor/monitor.c             | 39 ++++++++++++++++++---------------
 tests/qemu-iotests/051.out    |  4 ++--
 tests/qemu-iotests/051.pc.out | 20 ++++++++---------
 6 files changed, 57 insertions(+), 53 deletions(-)

-- 
2.40.1

[PATCH 1/4] monitor: use QEMU_LOCK_GUARD a bit more

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 602535696c59..4b11bca2a21d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -161,10 +161,9 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
 {
     Monitor *mon = opaque;
 
-    qemu_mutex_lock(&mon->mon_lock);
+    QEMU_LOCK_GUARD(&mon->mon_lock);
     mon->out_watch = 0;
     monitor_flush_locked(mon);
-    qemu_mutex_unlock(&mon->mon_lock);
     return FALSE;
 }
 
@@ -203,9 +202,8 @@ static void monitor_flush_locked(Monitor *mon)
 
 void monitor_flush(Monitor *mon)
 {
-    qemu_mutex_lock(&mon->mon_lock);
+    QEMU_LOCK_GUARD(&mon->mon_lock);
     monitor_flush_locked(mon);
-    qemu_mutex_unlock(&mon->mon_lock);
 }
 
 /* flush at every end of line */
-- 
2.40.1

[PATCH 2/4] monitor: allow calling monitor_resume under mon_lock

[Paolo Bonzini]

Move monitor_resume()'s call to readline_show_prompt() outside the
potentially locked section.  Reuse the existing monitor_accept_input()
bottom half for this purpose.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/monitor.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 4b11bca2a21d..7080d2da8ec6 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -567,6 +567,12 @@ static void monitor_accept_input(void *opaque)
 {
     Monitor *mon = opaque;
 
+    if (!monitor_is_qmp(mon)) {
+        MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
+        assert(hmp_mon->rs);
+        readline_show_prompt(hmp_mon->rs);
+    }
+
     qemu_chr_fe_accept_input(&mon->chr);
 }
 
@@ -585,12 +591,6 @@ void monitor_resume(Monitor *mon)
             ctx = qemu_get_aio_context();
         }
 
-        if (!monitor_is_qmp(mon)) {
-            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
-            assert(hmp_mon->rs);
-            readline_show_prompt(hmp_mon->rs);
-        }
-
         aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
     }
 
-- 
2.40.1

[PATCH 3/4] monitor: add more *_locked() functions

[Paolo Bonzini]

Allow flushing and printing to the monitor while mon->mon_lock is
held.  This will help cleaning up the locking of mon->mux_out and
mon->suspend_cnt.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/monitor/monitor.h |  3 +++
 monitor/monitor.c         | 14 ++++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 033390f69917..965f5d545003 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -40,6 +40,9 @@ void monitor_flush(Monitor *mon);
 int monitor_set_cpu(Monitor *mon, int cpu_index);
 int monitor_get_cpu_index(Monitor *mon);
 
+int monitor_puts_locked(Monitor *mon, const char *str);
+void monitor_flush_locked(Monitor *mon);
+
 void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp);
 
 void monitor_read_command(MonitorHMP *mon, int show_prompt);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 7080d2da8ec6..20e33e28d20d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -154,8 +154,6 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
     return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
 }
 
-static void monitor_flush_locked(Monitor *mon);
-
 static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
                                   void *opaque)
 {
@@ -168,7 +166,7 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
 }
 
 /* Caller must hold mon->mon_lock */
-static void monitor_flush_locked(Monitor *mon)
+void monitor_flush_locked(Monitor *mon)
 {
     int rc;
     size_t len;
@@ -207,12 +205,11 @@ void monitor_flush(Monitor *mon)
 }
 
 /* flush at every end of line */
-int monitor_puts(Monitor *mon, const char *str)
+int monitor_puts_locked(Monitor *mon, const char *str)
 {
     int i;
     char c;
 
-    qemu_mutex_lock(&mon->mon_lock);
     for (i = 0; str[i]; i++) {
         c = str[i];
         if (c == '\n') {
@@ -223,11 +220,16 @@ int monitor_puts(Monitor *mon, const char *str)
             monitor_flush_locked(mon);
         }
     }
-    qemu_mutex_unlock(&mon->mon_lock);
 
     return i;
 }
 
+int monitor_puts(Monitor *mon, const char *str)
+{
+    QEMU_LOCK_GUARD(&mon->mon_lock);
+    return monitor_puts_locked(mon, str);
+}
+
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
     char *buf;
-- 
2.40.1

[PATCH 4/4] monitor: do not use mb_read/mb_set for suspend_cnt

[Paolo Bonzini]

Clean up monitor_event to just use monitor_suspend/monitor_resume,
using mon->mux_out to protect against incorrect nesting (especially
on startup).

The only remaining case of reading suspend_cnt is in the can_read
callback, which is just advisory and can use qatomic_read.

As an extra benefit, mux_out is now simply protected by mon_lock.
Also, moving the prompt to after the event loop has started removes
it from the output in some error cases, where QEMU does not actually
start successfully.  It is not a full fix and it would be nice to
also remove the monitor heading, but this is already a small (though
unintentional) improvement.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor/hmp.c                 | 41 ++++++++++++++++-------------------
 monitor/monitor-internal.h    |  3 +--
 monitor/monitor.c             |  9 ++++++--
 tests/qemu-iotests/051.out    |  4 ++--
 tests/qemu-iotests/051.pc.out | 20 ++++++++---------
 5 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index 5cab56d355c8..69c1b7e98abb 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1401,45 +1401,42 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
 static void monitor_event(void *opaque, QEMUChrEvent event)
 {
     Monitor *mon = opaque;
-    MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
 
     switch (event) {
     case CHR_EVENT_MUX_IN:
         qemu_mutex_lock(&mon->mon_lock);
-        mon->mux_out = 0;
-        qemu_mutex_unlock(&mon->mon_lock);
-        if (mon->reset_seen) {
-            readline_restart(hmp_mon->rs);
+        if (mon->mux_out) {
+            mon->mux_out = 0;
             monitor_resume(mon);
-            monitor_flush(mon);
-        } else {
-            qatomic_mb_set(&mon->suspend_cnt, 0);
         }
+        qemu_mutex_unlock(&mon->mon_lock);
         break;
 
     case CHR_EVENT_MUX_OUT:
-        if (mon->reset_seen) {
-            if (qatomic_mb_read(&mon->suspend_cnt) == 0) {
-                monitor_printf(mon, "\n");
-            }
-            monitor_flush(mon);
-            monitor_suspend(mon);
-        } else {
-            qatomic_inc(&mon->suspend_cnt);
-        }
         qemu_mutex_lock(&mon->mon_lock);
-        mon->mux_out = 1;
+        if (!mon->mux_out) {
+            if (mon->reset_seen && !mon->suspend_cnt) {
+                monitor_puts_locked(mon, "\n");
+            } else {
+                monitor_flush_locked(mon);
+            }
+            monitor_suspend(mon);
+            mon->mux_out = 1;
+        }
         qemu_mutex_unlock(&mon->mon_lock);
         break;
 
     case CHR_EVENT_OPENED:
         monitor_printf(mon, "QEMU %s monitor - type 'help' for more "
                        "information\n", QEMU_VERSION);
-        if (!mon->mux_out) {
-            readline_restart(hmp_mon->rs);
-            readline_show_prompt(hmp_mon->rs);
-        }
+        qemu_mutex_lock(&mon->mon_lock);
         mon->reset_seen = 1;
+        if (!mon->mux_out) {
+            /* Suspend-resume forces the prompt to be printed.  */
+            monitor_suspend(mon);
+            monitor_resume(mon);
+        }
+        qemu_mutex_unlock(&mon->mon_lock);
         mon_refcount++;
         break;
 
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 53e3808054c7..61c9b6916db3 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -94,7 +94,6 @@ typedef struct HMPCommand {
 
 struct Monitor {
     CharBackend chr;
-    int reset_seen;
     int suspend_cnt;            /* Needs to be accessed atomically */
     bool is_qmp;
     bool skip_flush;
@@ -115,8 +114,8 @@ struct Monitor {
     QLIST_HEAD(, mon_fd_t) fds;
     GString *outbuf;
     guint out_watch;
-    /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
     int mux_out;
+    int reset_seen;
 };
 
 struct MonitorHMP {
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 20e33e28d20d..15f97538ef2b 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -569,10 +569,15 @@ static void monitor_accept_input(void *opaque)
 {
     Monitor *mon = opaque;
 
-    if (!monitor_is_qmp(mon)) {
+    qemu_mutex_lock(&mon->mon_lock);
+    if (!monitor_is_qmp(mon) && mon->reset_seen) {
         MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
         assert(hmp_mon->rs);
+        readline_restart(hmp_mon->rs);
+        qemu_mutex_unlock(&mon->mon_lock);
         readline_show_prompt(hmp_mon->rs);
+    } else {
+        qemu_mutex_unlock(&mon->mon_lock);
     }
 
     qemu_chr_fe_accept_input(&mon->chr);
@@ -603,7 +608,7 @@ int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return !qatomic_mb_read(&mon->suspend_cnt);
+    return !qatomic_read(&mon->suspend_cnt);
 }
 
 void monitor_list_append(Monitor *mon)
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index e5ddb03bda1d..d46215640506 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -74,7 +74,7 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'fo
 
 Testing: -device virtio-scsi -device scsi-hd
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device scsi-hd: drive property not set
+QEMU_PROG: -device scsi-hd: drive property not set
 
 
 === Overriding backing file ===
@@ -134,7 +134,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=virtio
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
+QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
 
 
 === Attach to node in non-default iothread ===
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index bade1ff3b929..4d4af5a486df 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -74,7 +74,7 @@ QEMU_PROG: -drive file=TEST_DIR/t.qcow2,node-name=foo#12: Invalid node-name: 'fo
 
 Testing: -device virtio-scsi -device scsi-hd
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device scsi-hd: drive property not set
+QEMU_PROG: -device scsi-hd: drive property not set
 
 
 === Overriding backing file ===
@@ -142,11 +142,11 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=ide
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: Device needs media, but drive is empty
+QEMU_PROG: Device needs media, but drive is empty
 
 Testing: -drive if=virtio
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
+QEMU_PROG: -drive if=virtio: Device needs media, but drive is empty
 
 Testing: -drive if=none,id=disk -device ide-cd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -158,22 +158,22 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive if=none,id=disk -device ide-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-hd,drive=disk: Device needs media, but drive is empty
+QEMU_PROG: -device ide-hd,drive=disk: Device needs media, but drive is empty
 
 Testing: -drive if=none,id=disk -device lsi53c895a -device scsi-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
+QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty
 
 
 === Attach to node in non-default iothread ===
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device ide-hd,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend
+QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend
+QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device lsi53c895a,id=lsi0 -device scsi-hd,bus=lsi0.0,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -185,7 +185,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend
+QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -204,7 +204,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: Block node is read-only
+QEMU_PROG: Block node is read-only
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on
 QEMU X.Y.Z monitor - type 'help' for more information
@@ -220,7 +220,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device ide-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) QEMU_PROG: -device ide-hd,drive=disk: Block node is read-only
+QEMU_PROG: -device ide-hd,drive=disk: Block node is read-only
 
 Testing: -drive file=TEST_DIR/t.qcow2,if=none,id=disk,readonly=on -device lsi53c895a -device scsi-hd,drive=disk
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.40.1

Re: [PATCH 1/4] monitor: use QEMU_LOCK_GUARD a bit more

[Richard Henderson]

On 5/18/23 03:18, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   monitor/monitor.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/4] monitor: add more *_locked() functions

[Richard Henderson]

On 5/18/23 03:18, Paolo Bonzini wrote:
> Allow flushing and printing to the monitor while mon->mon_lock is
> held.  This will help cleaning up the locking of mon->mux_out and
> mon->suspend_cnt.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   include/monitor/monitor.h |  3 +++
>   monitor/monitor.c         | 14 ++++++++------
>   2 files changed, 11 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/4] monitor: use QEMU_LOCK_GUARD a bit more

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  monitor/monitor.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 602535696c59..4b11bca2a21d 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -161,10 +161,9 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
>  {
>      Monitor *mon = opaque;
>  
> -    qemu_mutex_lock(&mon->mon_lock);
> +    QEMU_LOCK_GUARD(&mon->mon_lock);
>      mon->out_watch = 0;
>      monitor_flush_locked(mon);
> -    qemu_mutex_unlock(&mon->mon_lock);
>      return FALSE;
>  }
>  
> @@ -203,9 +202,8 @@ static void monitor_flush_locked(Monitor *mon)
>  
>  void monitor_flush(Monitor *mon)
>  {
> -    qemu_mutex_lock(&mon->mon_lock);
> +    QEMU_LOCK_GUARD(&mon->mon_lock);
>      monitor_flush_locked(mon);
> -    qemu_mutex_unlock(&mon->mon_lock);
>  }
>  
>  /* flush at every end of line */

I wouldn't bother to make this change when the protected region is this
short and doesn't branch, but since you did, why not take it.

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

Re: [PATCH 2/4] monitor: allow calling monitor_resume under mon_lock

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Move monitor_resume()'s call to readline_show_prompt() outside the
> potentially locked section.  Reuse the existing monitor_accept_input()
> bottom half for this purpose.

This describes what the patch changes, but not why we want the change.
Could you elaborate a bit?

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  monitor/monitor.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 4b11bca2a21d..7080d2da8ec6 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -567,6 +567,12 @@ static void monitor_accept_input(void *opaque)
>  {
>      Monitor *mon = opaque;
>  
> +    if (!monitor_is_qmp(mon)) {
> +        MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
> +        assert(hmp_mon->rs);
> +        readline_show_prompt(hmp_mon->rs);
> +    }
> +
>      qemu_chr_fe_accept_input(&mon->chr);
>  }
>  
> @@ -585,12 +591,6 @@ void monitor_resume(Monitor *mon)
>              ctx = qemu_get_aio_context();
>          }
>  
> -        if (!monitor_is_qmp(mon)) {
> -            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
> -            assert(hmp_mon->rs);
> -            readline_show_prompt(hmp_mon->rs);
> -        }
> -
>          aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
>      }

Re: [PATCH 3/4] monitor: add more *_locked() functions

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Allow flushing and printing to the monitor while mon->mon_lock is
> held.  This will help cleaning up the locking of mon->mux_out and
> mon->suspend_cnt.

Actually, the patch adds just monitor_puts_locked().
monitor_flush_locked() it merely gives external linkage.  Suggest

  monitor: More *_locked() functions

  Being able to print to the monitor and flush while mon->mon_lock is
  held will help cleaning up the locking of mon->mux_out and
  mon->suspend_cnt in the next commit.  Give monitor_flush_locked()
  external linkage, and factor monitor_puts_locked() out of
  monitor_puts().
  
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/monitor/monitor.h |  3 +++
>  monitor/monitor.c         | 14 ++++++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 033390f69917..965f5d545003 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -40,6 +40,9 @@ void monitor_flush(Monitor *mon);
>  int monitor_set_cpu(Monitor *mon, int cpu_index);
>  int monitor_get_cpu_index(Monitor *mon);
>  
> +int monitor_puts_locked(Monitor *mon, const char *str);
> +void monitor_flush_locked(Monitor *mon);
> +
>  void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp);
>  
>  void monitor_read_command(MonitorHMP *mon, int show_prompt);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 7080d2da8ec6..20e33e28d20d 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -154,8 +154,6 @@ static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
>      return !monitor_uses_readline(container_of(mon, MonitorHMP, common));
>  }
>  
> -static void monitor_flush_locked(Monitor *mon);
> -
>  static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
>                                    void *opaque)
>  {
> @@ -168,7 +166,7 @@ static gboolean monitor_unblocked(void *do_not_use, GIOCondition cond,
>  }
>  
>  /* Caller must hold mon->mon_lock */
> -static void monitor_flush_locked(Monitor *mon)
> +void monitor_flush_locked(Monitor *mon)
>  {
>      int rc;
>      size_t len;
> @@ -207,12 +205,11 @@ void monitor_flush(Monitor *mon)
>  }
>  
>  /* flush at every end of line */
> -int monitor_puts(Monitor *mon, const char *str)
> +int monitor_puts_locked(Monitor *mon, const char *str)
>  {
>      int i;
>      char c;
>  
> -    qemu_mutex_lock(&mon->mon_lock);
>      for (i = 0; str[i]; i++) {
>          c = str[i];
>          if (c == '\n') {
> @@ -223,11 +220,16 @@ int monitor_puts(Monitor *mon, const char *str)
>              monitor_flush_locked(mon);
>          }
>      }
> -    qemu_mutex_unlock(&mon->mon_lock);
>  
>      return i;
>  }
>  
> +int monitor_puts(Monitor *mon, const char *str)
> +{
> +    QEMU_LOCK_GUARD(&mon->mon_lock);
> +    return monitor_puts_locked(mon, str);
> +}
> +
>  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
>  {
>      char *buf;

Prefereably with a clarified commit message
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 0/4] monitor/hmp: cleanup monitor_event() and suspend_cnt

[Markus Armbruster]

Copying the HMP maintainer for another pair of eyes.

Re: [PATCH 0/4] monitor/hmp: cleanup monitor_event() and suspend_cnt

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Copying the HMP maintainer for another pair of eyes.

Crossed Paolo's PR, so nevermind.