----- Original Message -----
> From: "Peter Maydell" <peter.mayd...@linaro.org>
> To: "Gal Hammer" <gham...@redhat.com>
> Cc: "Paolo Bonzini" <pbonz...@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 <gham...@redhat.com> wrote:
> > On 06/01/2015 15:49, Peter Maydell wrote:
> >>
> >> On 5 January 2015 at 09:21, Gal Hammer <gham...@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.
