* [PATCH for 6.1 0/1] Fix chardev frontend bug in HMP @ 2021-08-07 19:27 Volker Rümelin 2021-08-07 19:29 ` [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read Volker Rümelin 0 siblings, 1 reply; 4+ messages in thread From: Volker Rümelin @ 2021-08-07 19:27 UTC (permalink / raw) To: Dr. David Alan Gilbert, Markus Armbruster; +Cc: Gerd Hoffmann, qemu-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read 2021-08-07 19:27 [PATCH for 6.1 0/1] Fix chardev frontend bug in HMP Volker Rümelin @ 2021-08-07 19:29 ` Volker Rümelin 2021-08-09 9:57 ` Marc-André Lureau 0 siblings, 1 reply; 4+ messages in thread From: Volker Rümelin @ 2021-08-07 19:29 UTC (permalink / raw) To: Dr. David Alan Gilbert, Markus Armbruster; +Cc: Gerd Hoffmann, qemu-devel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read 2021-08-07 19:29 ` [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read Volker Rümelin @ 2021-08-09 9:57 ` Marc-André Lureau 2021-08-10 5:18 ` Volker Rümelin 0 siblings, 1 reply; 4+ messages in thread From: Marc-André Lureau @ 2021-08-09 9:57 UTC (permalink / raw) To: Volker Rümelin Cc: QEMU, Gerd Hoffmann, Dr. David Alan Gilbert, Markus Armbruster [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read 2021-08-09 9:57 ` Marc-André Lureau @ 2021-08-10 5:18 ` Volker Rümelin 0 siblings, 0 replies; 4+ messages in thread From: Volker Rümelin @ 2021-08-10 5:18 UTC (permalink / raw) To: Marc-André Lureau Cc: QEMU, Gerd Hoffmann, Dr. David Alan Gilbert, Markus Armbruster > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-10 5:20 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-08-07 19:27 [PATCH for 6.1 0/1] Fix chardev frontend bug in HMP Volker Rümelin 2021-08-07 19:29 ` [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read Volker Rümelin 2021-08-09 9:57 ` Marc-André Lureau 2021-08-10 5:18 ` Volker Rümelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).