diff mbox series

[PULL,16/16] vl: Deprecate -mon pretty=... for HMP monitors

Message ID 20190617184903.19436-17-armbru@redhat.com
State New
Headers show
Series [PULL,01/16] monitor: Fix return type of monitor_fdset_dup_fd_find | expand

Commit Message

Markus Armbruster June 17, 2019, 6:49 p.m. UTC 
From: Kevin Wolf <kwolf@redhat.com>

The -mon pretty=on|off switch of the -mon option applies only to QMP
monitors. It's silently ignored for HMP. Deprecate this combination so
that we can make it an error in future versions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20190613153405.24769-16-kwolf@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
[Commit message tweaked]
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-deprecated.texi |  6 ++++++
 vl.c                 | 10 +++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Daniel P. Berrangé June 18, 2019, 9:01 a.m. UTC | #1 
On Mon, Jun 17, 2019 at 08:49:03PM +0200, Markus Armbruster wrote:
> From: Kevin Wolf <kwolf@redhat.com>
> 
> The -mon pretty=on|off switch of the -mon option applies only to QMP
> monitors. It's silently ignored for HMP. Deprecate this combination so
> that we can make it an error in future versions.

No objection to merging this PR as is, but how about we extend the
deprecation to QMP too ?

I was responsible for adding this option back in 2010 and I don't
think I've used it since 2012 when I added pretty printing support
to scripts/qmp/qmp-shell. I struggle to imagine good reasons for
using QMP directly with pretty printing, as opposed to doing it
via qmp-shell or another wrapper tool.


Regards,
Daniel
Kevin Wolf June 18, 2019, 10:34 a.m. UTC | #2 
Am 18.06.2019 um 11:01 hat Daniel P. Berrangé geschrieben:
> On Mon, Jun 17, 2019 at 08:49:03PM +0200, Markus Armbruster wrote:
> > From: Kevin Wolf <kwolf@redhat.com>
> > 
> > The -mon pretty=on|off switch of the -mon option applies only to QMP
> > monitors. It's silently ignored for HMP. Deprecate this combination so
> > that we can make it an error in future versions.
> 
> No objection to merging this PR as is, but how about we extend the
> deprecation to QMP too ?
> 
> I was responsible for adding this option back in 2010 and I don't
> think I've used it since 2012 when I added pretty printing support
> to scripts/qmp/qmp-shell. I struggle to imagine good reasons for
> using QMP directly with pretty printing, as opposed to doing it
> via qmp-shell or another wrapper tool.

qemu-iotests uses it. It doesn't only make the output (and espeically
diffs on failure) much more readable, but in fact also avoids very long
lines in the refernce output that used to break patch emails when we
didn't use pretty printing yet.

So let's keep it for QMP, please.

Kevin
Markus Armbruster June 19, 2019, 6:42 a.m. UTC | #3 
Kevin Wolf <kwolf@redhat.com> writes:

> Am 18.06.2019 um 11:01 hat Daniel P. Berrangé geschrieben:
>> On Mon, Jun 17, 2019 at 08:49:03PM +0200, Markus Armbruster wrote:
>> > From: Kevin Wolf <kwolf@redhat.com>
>> > 
>> > The -mon pretty=on|off switch of the -mon option applies only to QMP
>> > monitors. It's silently ignored for HMP. Deprecate this combination so
>> > that we can make it an error in future versions.
>> 
>> No objection to merging this PR as is, but how about we extend the
>> deprecation to QMP too ?
>> 
>> I was responsible for adding this option back in 2010 and I don't
>> think I've used it since 2012 when I added pretty printing support
>> to scripts/qmp/qmp-shell. I struggle to imagine good reasons for
>> using QMP directly with pretty printing, as opposed to doing it
>> via qmp-shell or another wrapper tool.
>
> qemu-iotests uses it. It doesn't only make the output (and espeically
> diffs on failure) much more readable, but in fact also avoids very long
> lines in the refernce output that used to break patch emails when we
> didn't use pretty printing yet.
>
> So let's keep it for QMP, please.

Perhaps we can get rid of it if we find a suitable filter.

Hmm, Python comes with one: "python -m json.tool".  It expects just one
expression, and fails if anything follows:

    $ printf '{"execute": "qmp_capabilities"}\n{"execute": "query-version"}\n' | socat UNIX:/work/armbru/images/test-qmp STDIO | python3 -m json.tool
    Extra data: line 2 column 1 (char 134)

To pretty print a sequence of expressions, you have to wrap a loop
around it:

    $ printf '{"execute": "qmp_capabilities"}\n{"execute": "query-version"}\n' | socat UNIX:/work/armbru/images/test-qmp STDIO | { while read line; do echo "$line" | python3 -m json.tool; done; }
    {
        "QMP": {
            "version": {
                "qemu": {
                    "micro": 50,
                    "minor": 0,
                    "major": 4
                },
                "package": "v4.0.0-1467-g6385dd6613"
            },
            "capabilities": [
                "oob"
            ]
        }
    }
    {
        "return": {}
    }
    {
        "return": {
            "qemu": {
                "micro": 50,
                "minor": 0,
                "major": 4
            },
            "package": "v4.0.0-1467-g6385dd6613"
        }
    }

I figure we'd want to loop in Python instead of shell.

My point is: pretty-printing is trivial in Python.  The case for
maintaining C code to do it seems weak.
Kevin Wolf June 19, 2019, 9:18 a.m. UTC | #4 
Am 19.06.2019 um 08:42 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 18.06.2019 um 11:01 hat Daniel P. Berrangé geschrieben:
> >> On Mon, Jun 17, 2019 at 08:49:03PM +0200, Markus Armbruster wrote:
> >> > From: Kevin Wolf <kwolf@redhat.com>
> >> > 
> >> > The -mon pretty=on|off switch of the -mon option applies only to QMP
> >> > monitors. It's silently ignored for HMP. Deprecate this combination so
> >> > that we can make it an error in future versions.
> >> 
> >> No objection to merging this PR as is, but how about we extend the
> >> deprecation to QMP too ?
> >> 
> >> I was responsible for adding this option back in 2010 and I don't
> >> think I've used it since 2012 when I added pretty printing support
> >> to scripts/qmp/qmp-shell. I struggle to imagine good reasons for
> >> using QMP directly with pretty printing, as opposed to doing it
> >> via qmp-shell or another wrapper tool.
> >
> > qemu-iotests uses it. It doesn't only make the output (and espeically
> > diffs on failure) much more readable, but in fact also avoids very long
> > lines in the refernce output that used to break patch emails when we
> > didn't use pretty printing yet.
> >
> > So let's keep it for QMP, please.
> 
> Perhaps we can get rid of it if we find a suitable filter.
> 
> Hmm, Python comes with one: "python -m json.tool".  It expects just one
> expression, and fails if anything follows:
> 
>     $ printf '{"execute": "qmp_capabilities"}\n{"execute": "query-version"}\n' | socat UNIX:/work/armbru/images/test-qmp STDIO | python3 -m json.tool
>     Extra data: line 2 column 1 (char 134)
> 
> To pretty print a sequence of expressions, you have to wrap a loop
> around it:
> 
>     $ printf '{"execute": "qmp_capabilities"}\n{"execute": "query-version"}\n' | socat UNIX:/work/armbru/images/test-qmp STDIO | { while read line; do echo "$line" | python3 -m json.tool; done; }

Yes, it's doable. It's not a very nice command line, but it does its
job.

What do we win by removing pretty printing from qemu? We invest some
work doing the patches, reviewing them, etc. and save the whole
complexity of adding a few newlines and spaces to an already existing
string buffer in a single place (qjson.c).

In exchange, we have to add the above overlong command line to every
qemu-iotests case for which pretty printed QMP is useful, and lose the
ability to just do -qmp-pretty stdio manually (which I do every now and
then) instead of having to dig up the above line in some script to copy
it.

It doesn't look like a net win to me.

> I figure we'd want to loop in Python instead of shell.
> 
> My point is: pretty-printing is trivial in Python.  The case for
> maintaining C code to do it seems weak.

The pretty printing is fairly trivial in C, too, when you already
generate JSON. The code seems to have been last touched in 2014, and
before that in 2010 when it was introduced. The maintenance burden
doesn't seem to be that bad.

Removing features that have users can be justified sometimes, but it
does need a justification, in my opinion.

Kevin
Daniel P. Berrangé June 19, 2019, 9:20 a.m. UTC | #5 
On Wed, Jun 19, 2019 at 11:18:52AM +0200, Kevin Wolf wrote:
> Am 19.06.2019 um 08:42 hat Markus Armbruster geschrieben:
> > Kevin Wolf <kwolf@redhat.com> writes:
> > 
> > > Am 18.06.2019 um 11:01 hat Daniel P. Berrangé geschrieben:
> > >> On Mon, Jun 17, 2019 at 08:49:03PM +0200, Markus Armbruster wrote:
> > >> > From: Kevin Wolf <kwolf@redhat.com>
> > >> > 
> > >> > The -mon pretty=on|off switch of the -mon option applies only to QMP
> > >> > monitors. It's silently ignored for HMP. Deprecate this combination so
> > >> > that we can make it an error in future versions.
> > >> 
> > >> No objection to merging this PR as is, but how about we extend the
> > >> deprecation to QMP too ?
> > >> 
> > >> I was responsible for adding this option back in 2010 and I don't
> > >> think I've used it since 2012 when I added pretty printing support
> > >> to scripts/qmp/qmp-shell. I struggle to imagine good reasons for
> > >> using QMP directly with pretty printing, as opposed to doing it
> > >> via qmp-shell or another wrapper tool.
> > >
> > > qemu-iotests uses it. It doesn't only make the output (and espeically
> > > diffs on failure) much more readable, but in fact also avoids very long
> > > lines in the refernce output that used to break patch emails when we
> > > didn't use pretty printing yet.
> > >
> > > So let's keep it for QMP, please.
> > 
> > Perhaps we can get rid of it if we find a suitable filter.
> > 
> > Hmm, Python comes with one: "python -m json.tool".  It expects just one
> > expression, and fails if anything follows:
> > 
> >     $ printf '{"execute": "qmp_capabilities"}\n{"execute": "query-version"}\n' | socat UNIX:/work/armbru/images/test-qmp STDIO | python3 -m json.tool
> >     Extra data: line 2 column 1 (char 134)
> > 
> > To pretty print a sequence of expressions, you have to wrap a loop
> > around it:
> > 
> >     $ printf '{"execute": "qmp_capabilities"}\n{"execute": "query-version"}\n' | socat UNIX:/work/armbru/images/test-qmp STDIO | { while read line; do echo "$line" | python3 -m json.tool; done; }
> 
> Yes, it's doable. It's not a very nice command line, but it does its
> job.
> 
> What do we win by removing pretty printing from qemu? We invest some
> work doing the patches, reviewing them, etc. and save the whole
> complexity of adding a few newlines and spaces to an already existing
> string buffer in a single place (qjson.c).
> 
> In exchange, we have to add the above overlong command line to every
> qemu-iotests case for which pretty printed QMP is useful, and lose the
> ability to just do -qmp-pretty stdio manually (which I do every now and
> then) instead of having to dig up the above line in some script to copy
> it.
> 
> It doesn't look like a net win to me.
> 
> > I figure we'd want to loop in Python instead of shell.
> > 
> > My point is: pretty-printing is trivial in Python.  The case for
> > maintaining C code to do it seems weak.
> 
> The pretty printing is fairly trivial in C, too, when you already
> generate JSON. The code seems to have been last touched in 2014, and
> before that in 2010 when it was introduced. The maintenance burden
> doesn't seem to be that bad.
> 
> Removing features that have users can be justified sometimes, but it
> does need a justification, in my opinion.

I'm fine with keeping the print-printing in QEMU given that you illustrated
an existing usage of it I wasn't aware of & its not difficult code to
maintain.

Regards,
Daniel
diff mbox series

Patch

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 50292d820b..df04f2840b 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -72,6 +72,12 @@  backend settings instead of environment variables.  To ease migration to
 the new format, the ``-audiodev-help'' option can be used to convert
 the current values of the environment variables to ``-audiodev'' options.
 
+@subsection -mon ...,control=readline,pretty=on|off (since 4.1)
+
+The @code{pretty=on|off} switch has no effect for HMP monitors, but is
+silently ignored. Using the switch with HMP monitors will become an
+error in the future.
+
 @subsection -realtime (since 4.1)
 
 The @code{-realtime mlock=on|off} argument has been replaced by the
diff --git a/vl.c b/vl.c
index 32daa434eb..99a56b5556 100644
--- a/vl.c
+++ b/vl.c
@@ -2317,6 +2317,10 @@  static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
         return -1;
     }
 
+    if (!qmp && qemu_opt_get(opts, "pretty")) {
+        warn_report("'pretty' is deprecated for HMP monitors, it has no effect "
+                    "and will be removed in future versions");
+    }
     if (qemu_opt_get_bool(opts, "pretty", 0)) {
         pretty = true;
     }
@@ -2362,7 +2366,11 @@  static void monitor_parse(const char *optarg, const char *mode, bool pretty)
     opts = qemu_opts_create(qemu_find_opts("mon"), label, 1, &error_fatal);
     qemu_opt_set(opts, "mode", mode, &error_abort);
     qemu_opt_set(opts, "chardev", label, &error_abort);
-    qemu_opt_set_bool(opts, "pretty", pretty, &error_abort);
+    if (!strcmp(mode, "control")) {
+        qemu_opt_set_bool(opts, "pretty", pretty, &error_abort);
+    } else {
+        assert(pretty == false);
+    }
     monitor_device_index++;
 }