| Message ID | 20180731104258.2459-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: accept input on resume | expand |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > A chardev may stop trying to write if the associated can_read() > callback returned 0. This happens when the monitor is suspended. > The frontend is supposed to call qemu_chr_fe_accept_input() when it is > ready to accept data again. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Impact? Is this to be considered for 3.0? > --- > monitor.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/monitor.c b/monitor.c > index d580c5a79c..e1a14e02cf 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -4412,6 +4412,7 @@ void monitor_resume(Monitor *mon) > assert(mon->rs); > readline_show_prompt(mon->rs); > } > + qemu_chr_fe_accept_input(&mon->chr); > } > trace_monitor_suspend(mon, -1); > }
Hi On Tue, Jul 31, 2018 at 1:30 PM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> A chardev may stop trying to write if the associated can_read() >> callback returned 0. This happens when the monitor is suspended. >> The frontend is supposed to call qemu_chr_fe_accept_input() when it is >> ready to accept data again. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Impact? I have observed the issue with a spice port, all pending commands aren't flushed. Most chardev don't use the accept_input() callback, and instead poll regularly, like char-socket.c:tcp_chr_read_poll() > > Is this to be considered for 3.0? This is not a regression, afaik, and doesn't impact common monitor chardev. So it could be delayed. But it's also a small fix for Spice chardev that shouldn't create regression, so it should be fine for 3.0. thanks > >> --- >> monitor.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/monitor.c b/monitor.c >> index d580c5a79c..e1a14e02cf 100644 >> --- a/monitor.c >> +++ b/monitor.c >> @@ -4412,6 +4412,7 @@ void monitor_resume(Monitor *mon) >> assert(mon->rs); >> readline_show_prompt(mon->rs); >> } >> + qemu_chr_fe_accept_input(&mon->chr); >> } >> trace_monitor_suspend(mon, -1); >> } >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Tue, Jul 31, 2018 at 1:30 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >> >>> A chardev may stop trying to write if the associated can_read() >>> callback returned 0. This happens when the monitor is suspended. >>> The frontend is supposed to call qemu_chr_fe_accept_input() when it is >>> ready to accept data again. >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> Impact? > > I have observed the issue with a spice port, all pending commands > aren't flushed. Most chardev don't use the accept_input() callback, > and instead poll regularly, like char-socket.c:tcp_chr_read_poll() The ones that do use it are braille, mux, msmouse, spice (abstract), spicevmc, spiceport, wctablet. A description of the impact should be worked into the commit message. >> Is this to be considered for 3.0? > > This is not a regression, afaik, and doesn't impact common monitor > chardev. So it could be delayed. But it's also a small fix for Spice > chardev that shouldn't create regression, so it should be fine for > 3.0. If the callbacks we now call all work fine, we should be good. But that's a chardev question, and I'm the guy for the monitor questions. I think the decision needs to be made by maintainers of the chardev subsystem. Preferably with an improved commit message: Acked-by: Markus Armbruster <armbru@redhat.com>
diff --git a/monitor.c b/monitor.c index d580c5a79c..e1a14e02cf 100644 --- a/monitor.c +++ b/monitor.c @@ -4412,6 +4412,7 @@ void monitor_resume(Monitor *mon) assert(mon->rs); readline_show_prompt(mon->rs); } + qemu_chr_fe_accept_input(&mon->chr); } trace_monitor_suspend(mon, -1); }
A chardev may stop trying to write if the associated can_read() callback returned 0. This happens when the monitor is suspended. The frontend is supposed to call qemu_chr_fe_accept_input() when it is ready to accept data again. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- monitor.c | 1 + 1 file changed, 1 insertion(+)