Re: [PATCH 1/6] char/stdio: Fix QMP default for 'signal'

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Commit 02c4bdf1 tried to make signal=on the default for stdio chardevs
> except for '-serial mon:stdio', but it forgot about QMP and accidentally
> switched the QMP default from true (except for -nographic) to false
> (always). The documentation was kept unchanged and still describes the

The QMP documentation, actually.  CLI documentation was updated.

> opposite of the old behaviour (which is an even older documentation
> bug).

Hardly surprising, given how vague the documentation was (and is).  See
below.

> Fix all of this by making signal=true the default in ChardevStdio and
> documenting it as such.
>
> Fixes: 02c4bdf1d2ca8c02a9bae16398f260b5c08d08bf
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/char.json       | 3 +--
>  chardev/char-stdio.c | 4 +---
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/qapi/char.json b/qapi/char.json
> index b4d66ec90b..43486d1daa 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -321,8 +321,7 @@
>  # Configuration info for stdio chardevs.
>  #
>  # @signal: Allow signals (such as SIGINT triggered by ^C)
> -#          be delivered to qemu.  Default: true in -nographic mode,
> -#          false otherwise.
> +#          be delivered to qemu.  Default: true.
>  #
>  # Since: 1.5
>  ##

What does it mean to "allow signals to be delivered"?

To understand, I had to check what the QEMU code does with it (@signal
off clears termios c_lflag ISIG), and I had to look up what that means
(tcsetattr(3): "When any of the characters INTR, QUIT, SUSP, or DSUSP
are received, generate the corresponding signal").

Stealing that sentence verbatim from tcsetattr(3) would be a marked
improvement over what we have, in my opinion.

Nothing wrong with this patch, of course, but if you need to respin for
some other reason, consider improving the comment.

> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
> index 82eaebc1db..403da308c9 100644
> --- a/chardev/char-stdio.c
> +++ b/chardev/char-stdio.c
> @@ -112,9 +112,7 @@ static void qemu_chr_open_stdio(Chardev *chr,
>  
>      qemu_chr_open_fd(chr, 0, 1);
>  
> -    if (opts->has_signal) {
> -        stdio_allow_signal = opts->signal;
> -    }
> +    stdio_allow_signal = !opts->has_signal || opts->signal;
>      qemu_chr_set_echo_stdio(chr, false);
>  }
>  #endif

Preferably with s/documentation/QMP documentation/ in the commit
message:

Reviewed-by: Markus Armbruster <armbru@redhat.com>