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

[Gal Hammer <ghammer@redhat.com>]

----- 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.