| Message ID | 20201023101222.250147-2-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-storage-daemon: QAPIfy --chardev | expand |
On Fri, Oct 23, 2020 at 2:14 PM Kevin Wolf <kwolf@redhat.com> wrote: > 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 > opposite of the old behaviour (which is an even older documentation > bug). > > 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> > Reviewed-by: Marc-André Lureau <marcandre.lureau@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 > ## > 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 > -- > 2.28.0 > > >
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>
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 ## 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
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 opposite of the old behaviour (which is an even older documentation bug). 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(-)