[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 16/16] vl: Deprecate -mon pretty=... for HMP moni
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PULL 16/16] vl: Deprecate -mon pretty=... for HMP monitors |
Date: |
Wed, 19 Jun 2019 10:20:09 +0100 |
User-agent: |
Mutt/1.11.4 (2019-03-13) |
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 <address@hidden> 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 <address@hidden>
> > >> >
> > >> > 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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
- [Qemu-devel] [PULL 06/16] monitor: Remove Monitor.cmd_table indirection, (continued)
- [Qemu-devel] [PULL 06/16] monitor: Remove Monitor.cmd_table indirection, Markus Armbruster, 2019/06/17
- [Qemu-devel] [PULL 02/16] monitor: Remove unused password prompting fields, Markus Armbruster, 2019/06/17
- [Qemu-devel] [PULL 11/16] monitor: Split out monitor/qmp.c, Markus Armbruster, 2019/06/17
- [Qemu-devel] [PULL 15/16] monitor: Replace monitor_init() with monitor_init_{hmp, qmp}(), Markus Armbruster, 2019/06/17
- [Qemu-devel] [PULL 05/16] monitor: Create MonitorHMP with readline state, Markus Armbruster, 2019/06/17
- [Qemu-devel] [PULL 16/16] vl: Deprecate -mon pretty=... for HMP monitors, Markus Armbruster, 2019/06/17
[Qemu-devel] [PULL 07/16] monitor: Rename HMP command type and tables, Markus Armbruster, 2019/06/17
[Qemu-devel] [PULL 13/16] monitor: Split out monitor/monitor.c, Markus Armbruster, 2019/06/17
[Qemu-devel] [PULL 12/16] monitor: Split out monitor/hmp.c, Markus Armbruster, 2019/06/17
[Qemu-devel] [PULL 10/16] monitor: Create monitor-internal.h with common definitions, Markus Armbruster, 2019/06/17
Re: [Qemu-devel] [PULL 00/16] Monitor patches for 2019-06-17, Peter Maydell, 2019/06/17
Re: [Qemu-devel] [PULL 00/16] Monitor patches for 2019-06-17, no-reply, 2019/06/17
Re: [Qemu-devel] [PULL 00/16] Monitor patches for 2019-06-17, no-reply, 2019/06/17