[PATCH for 6.1 0/1] Fix chardev frontend bug in HMP

[PATCH for 6.1 0/1] Fix chardev frontend bug in HMP

[Volker Rümelin]

Since commit 584af1f1d9 (ui/gtk: add a keyboard fifo to the VTE
consoles) a GTK VTE console chardev backend relies on the
connected chardev frontend to call qemu_chr_fe_accept_input()
whenever it can receive new characters. The HMP monitor doesn't
do this. It only schedules a call to qemu_chr_fe_accept_input()
after it handled a HMP command in monitor_command_cb().

To see the problem copy and paste the word help into the GTK VTE
monitor console. You will only see the letter h. Now press the
enter key several times. Each key press will add another letter
to the word help.

I think I need help with this patch. This is the first time I
had a closer look at the monitor code so it's quite possible my
patch is completely wrong.

Volker Rümelin (1):
   monitor/hmp: schedule qemu_chr_fe_accept_input() after read

  monitor/hmp.c              |  1 +
  monitor/monitor-internal.h |  1 +
  monitor/monitor.c          | 19 +++++++++++++++++--
  3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read

[Volker Rümelin]

Since commit 584af1f1d9 (ui/gtk: add a keyboard fifo to the VTE
consoles) a GTK VTE console chardev backend relies on the
connected chardev frontend to call qemu_chr_fe_accept_input()
whenever it can receive new characters. The HMP monitor doesn't
do this. It only schedules a call to qemu_chr_fe_accept_input()
after it handled a HMP command in monitor_command_cb().

This is a problem if you paste a few characters into the GTK
monitor console. Even entering a UTF-8 multibyte character leads
to the same problem.

Schedule a call to qemu_chr_fe_accept_input() after handling the
received bytes to fix the HMP monitor.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 monitor/hmp.c              |  1 +
 monitor/monitor-internal.h |  1 +
 monitor/monitor.c          | 19 +++++++++++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..470f56a71d 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1349,6 +1349,7 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
         for (i = 0; i < size; i++) {
             readline_handle_byte(mon->rs, buf[i]);
         }
+        monitor_accept_input(&mon->common);
     } else {
         if (size == 0 || buf[size - 1] != 0) {
             monitor_printf(&mon->common, "corrupted command\n");
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..af33c3c617 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -170,6 +170,7 @@ int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
                        bool use_io_thread);
 void monitor_data_destroy(Monitor *mon);
+void monitor_accept_input(Monitor *mon);
 int monitor_can_read(void *opaque);
 void monitor_list_append(Monitor *mon);
 void monitor_fdsets_cleanup(void);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 46a171bca6..8e3cf4ad98 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -519,13 +519,28 @@ int monitor_suspend(Monitor *mon)
     return 0;
 }
 
-static void monitor_accept_input(void *opaque)
+static void monitor_accept_input_bh(void *opaque)
 {
     Monitor *mon = opaque;
 
     qemu_chr_fe_accept_input(&mon->chr);
 }
 
+void monitor_accept_input(Monitor *mon)
+{
+    if (!qatomic_mb_read(&mon->suspend_cnt)) {
+        AioContext *ctx;
+
+        if (mon->use_io_thread) {
+            ctx = iothread_get_aio_context(mon_iothread);
+        } else {
+            ctx = qemu_get_aio_context();
+        }
+
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
+    }
+}
+
 void monitor_resume(Monitor *mon)
 {
     if (monitor_is_hmp_non_interactive(mon)) {
@@ -547,7 +562,7 @@ void monitor_resume(Monitor *mon)
             readline_show_prompt(hmp_mon->rs);
         }
 
-        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
     }
 
     trace_monitor_suspend(mon, -1);
-- 
2.26.2

Re: [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3570 bytes --]

Hi Volker

On Sat, Aug 7, 2021 at 11:32 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Since commit 584af1f1d9 (ui/gtk: add a keyboard fifo to the VTE
> consoles) a GTK VTE console chardev backend relies on the
> connected chardev frontend to call qemu_chr_fe_accept_input()
> whenever it can receive new characters. The HMP monitor doesn't
> do this. It only schedules a call to qemu_chr_fe_accept_input()
> after it handled a HMP command in monitor_command_cb().
>
> This is a problem if you paste a few characters into the GTK
> monitor console. Even entering a UTF-8 multibyte character leads
> to the same problem.
>
> Schedule a call to qemu_chr_fe_accept_input() after handling the
> received bytes to fix the HMP monitor.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

Wouldn't it work to make gd_vc_send_chars() write in a loop (until it
fails)?

If not, I think monitor/qmp.c should be adjusted too.

---
>  monitor/hmp.c              |  1 +
>  monitor/monitor-internal.h |  1 +
>  monitor/monitor.c          | 19 +++++++++++++++++--
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index d50c3124e1..470f56a71d 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1349,6 +1349,7 @@ static void monitor_read(void *opaque, const uint8_t
> *buf, int size)
>          for (i = 0; i < size; i++) {
>              readline_handle_byte(mon->rs, buf[i]);
>          }
> +        monitor_accept_input(&mon->common);
>      } else {
>          if (size == 0 || buf[size - 1] != 0) {
>              monitor_printf(&mon->common, "corrupted command\n");
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 9c3a09cb01..af33c3c617 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -170,6 +170,7 @@ int monitor_puts(Monitor *mon, const char *str);
>  void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>                         bool use_io_thread);
>  void monitor_data_destroy(Monitor *mon);
> +void monitor_accept_input(Monitor *mon);
>  int monitor_can_read(void *opaque);
>  void monitor_list_append(Monitor *mon);
>  void monitor_fdsets_cleanup(void);
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 46a171bca6..8e3cf4ad98 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -519,13 +519,28 @@ int monitor_suspend(Monitor *mon)
>      return 0;
>  }
>
> -static void monitor_accept_input(void *opaque)
> +static void monitor_accept_input_bh(void *opaque)
>  {
>      Monitor *mon = opaque;
>
>      qemu_chr_fe_accept_input(&mon->chr);
>  }
>
> +void monitor_accept_input(Monitor *mon)
> +{
> +    if (!qatomic_mb_read(&mon->suspend_cnt)) {
> +        AioContext *ctx;
> +
> +        if (mon->use_io_thread) {
> +            ctx = iothread_get_aio_context(mon_iothread);
> +        } else {
> +            ctx = qemu_get_aio_context();
> +        }
> +
> +        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
> +    }
> +}
> +
>  void monitor_resume(Monitor *mon)
>  {
>      if (monitor_is_hmp_non_interactive(mon)) {
> @@ -547,7 +562,7 @@ void monitor_resume(Monitor *mon)
>              readline_show_prompt(hmp_mon->rs);
>          }
>
> -        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
> +        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
>      }
>
>      trace_monitor_suspend(mon, -1);
> --
> 2.26.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4591 bytes --]

Re: [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read

[Volker Rümelin]

>     Since commit 584af1f1d9 (ui/gtk: add a keyboard fifo to the VTE
>     consoles) a GTK VTE console chardev backend relies on the
>     connected chardev frontend to call qemu_chr_fe_accept_input()
>     whenever it can receive new characters. The HMP monitor doesn't
>     do this. It only schedules a call to qemu_chr_fe_accept_input()
>     after it handled a HMP command in monitor_command_cb().
>
>     This is a problem if you paste a few characters into the GTK
>     monitor console. Even entering a UTF-8 multibyte character leads
>     to the same problem.
>
>     Schedule a call to qemu_chr_fe_accept_input() after handling the
>     received bytes to fix the HMP monitor.
>
>     Signed-off-by: Volker Rümelin <vr_qemu@t-online.de
>     <mailto:vr_qemu@t-online.de>>
>
>
> Wouldn't it work to make gd_vc_send_chars() write in a loop (until it 
> fails)?

Hi Marc-André,

yes that works. I found similar code in the udp_chr_flush_buffer() 
function in chardev/char-udp.c. I will send a new patch within the next 
few hours.

With best regards,
Volker

>
> If not, I think monitor/qmp.c should be adjusted too.
>
>     ---
>      monitor/hmp.c              |  1 +
>      monitor/monitor-internal.h |  1 +
>      monitor/monitor.c          | 19 +++++++++++++++++--
>      3 files changed, 19 insertions(+), 2 deletions(-)
>
>     diff --git a/monitor/hmp.c b/monitor/hmp.c
>     index d50c3124e1..470f56a71d 100644
>     --- a/monitor/hmp.c
>     +++ b/monitor/hmp.c
>     @@ -1349,6 +1349,7 @@ static void monitor_read(void *opaque, const
>     uint8_t *buf, int size)
>              for (i = 0; i < size; i++) {
>                  readline_handle_byte(mon->rs, buf[i]);
>              }
>     +        monitor_accept_input(&mon->common);
>          } else {
>              if (size == 0 || buf[size - 1] != 0) {
>                  monitor_printf(&mon->common, "corrupted command\n");
>     diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>     index 9c3a09cb01..af33c3c617 100644
>     --- a/monitor/monitor-internal.h
>     +++ b/monitor/monitor-internal.h
>     @@ -170,6 +170,7 @@ int monitor_puts(Monitor *mon, const char *str);
>      void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
>                             bool use_io_thread);
>      void monitor_data_destroy(Monitor *mon);
>     +void monitor_accept_input(Monitor *mon);
>      int monitor_can_read(void *opaque);
>      void monitor_list_append(Monitor *mon);
>      void monitor_fdsets_cleanup(void);
>     diff --git a/monitor/monitor.c b/monitor/monitor.c
>     index 46a171bca6..8e3cf4ad98 100644
>     --- a/monitor/monitor.c
>     +++ b/monitor/monitor.c
>     @@ -519,13 +519,28 @@ int monitor_suspend(Monitor *mon)
>          return 0;
>      }
>
>     -static void monitor_accept_input(void *opaque)
>     +static void monitor_accept_input_bh(void *opaque)
>      {
>          Monitor *mon = opaque;
>
>          qemu_chr_fe_accept_input(&mon->chr);
>      }
>
>     +void monitor_accept_input(Monitor *mon)
>     +{
>     +    if (!qatomic_mb_read(&mon->suspend_cnt)) {
>     +        AioContext *ctx;
>     +
>     +        if (mon->use_io_thread) {
>     +            ctx = iothread_get_aio_context(mon_iothread);
>     +        } else {
>     +            ctx = qemu_get_aio_context();
>     +        }
>     +
>     +        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
>     +    }
>     +}
>     +
>      void monitor_resume(Monitor *mon)
>      {
>          if (monitor_is_hmp_non_interactive(mon)) {
>     @@ -547,7 +562,7 @@ void monitor_resume(Monitor *mon)
>                  readline_show_prompt(hmp_mon->rs);
>              }
>
>     -        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
>     +        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
>          }
>
>          trace_monitor_suspend(mon, -1);
>     -- 
>     2.26.2
>
>
>
>
> -- 
> Marc-André Lureau