[Qemu-devel] [PATCH] monitor: accept input on resume

[Qemu-devel] [PATCH] monitor: accept input on resume

[Marc-André Lureau]

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(+)

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);
 }
-- 
2.18.0.321.gffc6fa0e39

Re: [Qemu-devel] [PATCH] monitor: accept input on resume

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH] monitor: accept input on resume

[Marc-André Lureau]

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

Re: [Qemu-devel] [PATCH] monitor: accept input on resume

[Markus Armbruster]

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>