* [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.
@ 2015-01-05 9:21 Gal Hammer
2015-01-06 13:33 ` Marcel Apfelbaum
2015-01-06 13:49 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Gal Hammer @ 2015-01-05 9:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Gal Hammer, Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.
2015-01-05 9:21 [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend Gal Hammer
@ 2015-01-06 13:33 ` Marcel Apfelbaum
2015-01-06 13:49 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Marcel Apfelbaum @ 2015-01-06 13:33 UTC (permalink / raw)
To: Gal Hammer, qemu-devel; +Cc: Paolo Bonzini
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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.
2015-01-05 9:21 [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend Gal Hammer
2015-01-06 13:33 ` Marcel Apfelbaum
@ 2015-01-06 13:49 ` Peter Maydell
2015-01-06 14:30 ` Gal Hammer
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-01-06 13:49 UTC (permalink / raw)
To: Gal Hammer; +Cc: Paolo Bonzini, QEMU Developers
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.
2015-01-06 13:49 ` Peter Maydell
@ 2015-01-06 14:30 ` Gal Hammer
2015-01-06 14:36 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Gal Hammer @ 2015-01-06 14:30 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.
2015-01-06 14:30 ` Gal Hammer
@ 2015-01-06 14:36 ` Peter Maydell
2015-01-06 18:45 ` Gal Hammer
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2015-01-06 14:36 UTC (permalink / raw)
To: Gal Hammer; +Cc: Paolo Bonzini, QEMU Developers
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend.
2015-01-06 14:36 ` Peter Maydell
@ 2015-01-06 18:45 ` Gal Hammer
0 siblings, 0 replies; 6+ messages in thread
From: Gal Hammer @ 2015-01-06 18:45 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers
----- 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-06 18:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-05 9:21 [Qemu-devel] [PATCH] char: disable stdio echo on resume from suspend Gal Hammer
2015-01-06 13:33 ` Marcel Apfelbaum
2015-01-06 13:49 ` Peter Maydell
2015-01-06 14:30 ` Gal Hammer
2015-01-06 14:36 ` Peter Maydell
2015-01-06 18:45 ` Gal Hammer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.