[Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Gal Hammer]

The monitor's auto-completion feature stopped working when stdio is used
as an input and qemu was resumed after it was suspended (using ctrl-z).

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 qemu-char.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index ef84b53..786df33 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1113,12 +1113,22 @@ static int old_fd0_flags;
 static bool stdio_in_use;
 static bool stdio_allow_signal;
 
+static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo);
+
 static void term_exit(void)
 {
     tcsetattr (0, TCSANOW, &oldtty);
     fcntl(0, F_SETFL, old_fd0_flags);
 }
 
+static void term_stdio_handler(int sig)
+{
+    if (sig == SIGCONT) {
+        /* echo should be off after resume from suspend. */
+        qemu_chr_set_echo_stdio(NULL, false);
+    }
+}
+
 static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
 {
     struct termios tty;
@@ -1165,6 +1175,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
     tcgetattr(0, &oldtty);
     qemu_set_nonblock(0);
     atexit(term_exit);
+    signal(SIGCONT, term_stdio_handler);
 
     chr = qemu_chr_open_fd(0, 1);
     chr->chr_close = qemu_chr_close_stdio;
-- 
2.1.0

Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Marcel Apfelbaum]

On 01/05/2015 11:21 AM, Gal Hammer wrote:
> The monitor's auto-completion feature stopped working when stdio is used
> as an input and qemu was resumed after it was suspended (using ctrl-z).

Thanks Gal, it works now!

Tested-by: Marcel Apfelbaum <marcel@redhat.com>

>
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>   qemu-char.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index ef84b53..786df33 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1113,12 +1113,22 @@ static int old_fd0_flags;
>   static bool stdio_in_use;
>   static bool stdio_allow_signal;
>
> +static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo);
> +
>   static void term_exit(void)
>   {
>       tcsetattr (0, TCSANOW, &oldtty);
>       fcntl(0, F_SETFL, old_fd0_flags);
>   }
>
> +static void term_stdio_handler(int sig)
> +{
> +    if (sig == SIGCONT) {
> +        /* echo should be off after resume from suspend. */
> +        qemu_chr_set_echo_stdio(NULL, false);
> +    }
> +}
> +
>   static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
>   {
>       struct termios tty;
> @@ -1165,6 +1175,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>       tcgetattr(0, &oldtty);
>       qemu_set_nonblock(0);
>       atexit(term_exit);
> +    signal(SIGCONT, term_stdio_handler);
>
>       chr = qemu_chr_open_fd(0, 1);
>       chr->chr_close = qemu_chr_close_stdio;
>

Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Peter Maydell]

On 5 January 2015 at 09:21, Gal Hammer <ghammer@redhat.com> wrote:
> The monitor's auto-completion feature stopped working when stdio is used
> as an input and qemu was resumed after it was suspended (using ctrl-z).
>
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  qemu-char.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index ef84b53..786df33 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1113,12 +1113,22 @@ static int old_fd0_flags;
>  static bool stdio_in_use;
>  static bool stdio_allow_signal;
>
> +static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo);
> +
>  static void term_exit(void)
>  {
>      tcsetattr (0, TCSANOW, &oldtty);
>      fcntl(0, F_SETFL, old_fd0_flags);
>  }
>
> +static void term_stdio_handler(int sig)
> +{
> +    if (sig == SIGCONT) {

...why do we need this check? We don't register the function
for any other signals...

> +        /* echo should be off after resume from suspend. */
> +        qemu_chr_set_echo_stdio(NULL, false);

Should echo really be always off, even if the thing using the
char device had set it to on?

> +    }
> +}
> +
>  static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
>  {
>      struct termios tty;
> @@ -1165,6 +1175,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>      tcgetattr(0, &oldtty);
>      qemu_set_nonblock(0);
>      atexit(term_exit);
> +    signal(SIGCONT, term_stdio_handler);

This should probably be using sigaction() which is what we use
elsewhere for signal handler registration.

-- PMM

Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Gal Hammer]

On 06/01/2015 15:49, Peter Maydell wrote:
> On 5 January 2015 at 09:21, Gal Hammer <ghammer@redhat.com> wrote:
>> The monitor's auto-completion feature stopped working when stdio is used
>> as an input and qemu was resumed after it was suspended (using ctrl-z).
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>   qemu-char.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index ef84b53..786df33 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -1113,12 +1113,22 @@ static int old_fd0_flags;
>>   static bool stdio_in_use;
>>   static bool stdio_allow_signal;
>>
>> +static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo);
>> +
>>   static void term_exit(void)
>>   {
>>       tcsetattr (0, TCSANOW, &oldtty);
>>       fcntl(0, F_SETFL, old_fd0_flags);
>>   }
>>
>> +static void term_stdio_handler(int sig)
>> +{
>> +    if (sig == SIGCONT) {
>
> ...why do we need this check? We don't register the function
> for any other signals...

It's a caution and not a must. It can be removed if you think it is 
redundant.

>
>> +        /* echo should be off after resume from suspend. */
>> +        qemu_chr_set_echo_stdio(NULL, false);
>
> Should echo really be always off, even if the thing using the
> char device had set it to on?

That's what the function qemu_chr_open_stdio() do, always set the stdio 
char device echo to off. I didn't change the current behavior I just 
restore it.

As I understood from my tests, the auto-complete feature doesn't work if 
the echo is enabled because pressing the tab key prints a tab char.

>> +    }
>> +}
>> +
>>   static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
>>   {
>>       struct termios tty;
>> @@ -1165,6 +1175,7 @@ static CharDriverState *qemu_chr_open_stdio(ChardevStdio *opts)
>>       tcgetattr(0, &oldtty);
>>       qemu_set_nonblock(0);
>>       atexit(term_exit);
>> +    signal(SIGCONT, term_stdio_handler);
>
> This should probably be using sigaction() which is what we use
> elsewhere for signal handler registration.

signal() is used in the code. Although I can switch to sigaction() and 
earn few more LOC ;-).

>
> -- PMM
>

     Gal.

Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Peter Maydell]

On 6 January 2015 at 14:30, Gal Hammer <ghammer@redhat.com> wrote:
> On 06/01/2015 15:49, Peter Maydell wrote:
>>
>> On 5 January 2015 at 09:21, Gal Hammer <ghammer@redhat.com> wrote:
>>>
>>> The monitor's auto-completion feature stopped working when stdio is used
>>> as an input and qemu was resumed after it was suspended (using ctrl-z).

>>> +        /* echo should be off after resume from suspend. */
>>> +        qemu_chr_set_echo_stdio(NULL, false);
>>
>>
>> Should echo really be always off, even if the thing using the
>> char device had set it to on?
>
>
> That's what the function qemu_chr_open_stdio() do, always set the stdio char
> device echo to off. I didn't change the current behavior I just restore it.

But qemu_chr_open_stdio also registers this function as the
chr_set_echo pointer in the CharDriverState, so if the
user of the CharDriverState calls qemu_chr_fe_set_echo(chr, true)
then we'll set the echo to on. And then if we ^Z and resume, your
patch will end up setting the echo to off, which seems wrong.

> As I understood from my tests, the auto-complete feature doesn't work if the
> echo is enabled because pressing the tab key prints a tab char.

Right, if the echo was disabled we want to make sure it is disabled.
But if the echo wasn't disabled we don't want to disable it.

>>> +    }
>>> +}
>>> +
>>>   static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
>>>   {
>>>       struct termios tty;
>>> @@ -1165,6 +1175,7 @@ static CharDriverState
>>> *qemu_chr_open_stdio(ChardevStdio *opts)
>>>       tcgetattr(0, &oldtty);
>>>       qemu_set_nonblock(0);
>>>       atexit(term_exit);
>>> +    signal(SIGCONT, term_stdio_handler);
>>
>>
>> This should probably be using sigaction() which is what we use
>> elsewhere for signal handler registration.
>
>
> signal() is used in the code.

Only when we're setting a signal to SIG_IGN, I think. signal()'s
semantics aren't portable, which is why we use sigaction().
[See the Linux signal(2) manpage for some discussion of this.]

thanks
-- PMM

Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.

[Gal Hammer]

----- Original Message -----
> From: "Peter Maydell" <peter.maydell@linaro.org>
> To: "Gal Hammer" <ghammer@redhat.com>
> Cc: "Paolo Bonzini" <pbonzini@redhat.com>, "QEMU Developers" <qemu-devel@nongnu.org>
> Sent: Tuesday, January 6, 2015 4:36:19 PM
> Subject: Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from	suspend.
> 
> On 6 January 2015 at 14:30, Gal Hammer <ghammer@redhat.com> wrote:
> > On 06/01/2015 15:49, Peter Maydell wrote:
> >>
> >> On 5 January 2015 at 09:21, Gal Hammer <ghammer@redhat.com> wrote:
> >>>
> >>> The monitor's auto-completion feature stopped working when stdio is used
> >>> as an input and qemu was resumed after it was suspended (using ctrl-z).
> 
> >>> +        /* echo should be off after resume from suspend. */
> >>> +        qemu_chr_set_echo_stdio(NULL, false);
> >>
> >>
> >> Should echo really be always off, even if the thing using the
> >> char device had set it to on?
> >
> >
> > That's what the function qemu_chr_open_stdio() do, always set the stdio
> > char
> > device echo to off. I didn't change the current behavior I just restore it.
> 
> But qemu_chr_open_stdio also registers this function as the
> chr_set_echo pointer in the CharDriverState, so if the
> user of the CharDriverState calls qemu_chr_fe_set_echo(chr, true)
> then we'll set the echo to on. And then if we ^Z and resume, your
> patch will end up setting the echo to off, which seems wrong.

I've missed the fact that qemu_chr_fe_set_echo(chr, true) is called if the monitor is in qmp mode. I'll post a new version of this patch.
 
> > As I understood from my tests, the auto-complete feature doesn't work if
> > the
> > echo is enabled because pressing the tab key prints a tab char.
> 
> Right, if the echo was disabled we want to make sure it is disabled.
> But if the echo wasn't disabled we don't want to disable it.
> 
> >>> +    }
> >>> +}
> >>> +
> >>>   static void qemu_chr_set_echo_stdio(CharDriverState *chr, bool echo)
> >>>   {
> >>>       struct termios tty;
> >>> @@ -1165,6 +1175,7 @@ static CharDriverState
> >>> *qemu_chr_open_stdio(ChardevStdio *opts)
> >>>       tcgetattr(0, &oldtty);
> >>>       qemu_set_nonblock(0);
> >>>       atexit(term_exit);
> >>> +    signal(SIGCONT, term_stdio_handler);
> >>
> >>
> >> This should probably be using sigaction() which is what we use
> >> elsewhere for signal handler registration.
> >
> >
> > signal() is used in the code.
> 
> Only when we're setting a signal to SIG_IGN, I think. signal()'s
> semantics aren't portable, which is why we use sigaction().
> [See the Linux signal(2) manpage for some discussion of this.]

No problem. I'll use sigaction() and won't check the signal identifier in the handler function.

> thanks
> -- PMM
> 
> 

    Gal.