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

[Peter Maydell <peter.maydell@linaro.org>]

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