Re: [Qemu-devel] [PATCH v2] Show values and description when using "qom-list"

[Perez Blanco, Ricardo (Nokia - BE/Antwerp)]

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilb...@redhat.com]
> Sent: Friday, June 29, 2018 2:17 PM
> To: Markus Armbruster <arm...@redhat.com>
> Cc: Perez Blanco, Ricardo (Nokia - BE/Antwerp)
> <ricardo.perez_bla...@nokia.com>; Andreas Färber <afaer...@suse.de>;
> Dongli Zhang <dongli.zh...@oracle.com>; qemu-devel <qemu-
> de...@nongnu.org>
> Subject: Re: [Qemu-devel] [PATCH v2] Show values and description when
> using "qom-list"
> 
> * Markus Armbruster (arm...@redhat.com) wrote:
> > "Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> >
> > > * Markus Armbruster (arm...@redhat.com) wrote:
> > >> Markus Armbruster <arm...@redhat.com> writes:
> > >>
> > >> > Andreas Färber <afaer...@suse.de> writes:
> > >> >
> > >> >> Am 08.06.2018 um 11:41 schrieb Dr. David Alan Gilbert:
> > >> >>> * Andreas Färber (afaer...@suse.de) wrote:
> > >> >>>> Am 01.06.2018 um 17:39 schrieb Ricardo Perez Blanco:
> > >> >>>>> For debugging purposes it is very useful to:
> > >> >>>>>  - See the description of the field. This information is already
> filled
> > >> >>>>>    in but not shown in "qom-list" command.
> > >> >>>>
> > >> >>>> No objection on this part.
> > >> >>>>
> > >> >>>>>  - Display value of the field.
> > >> >>>>
> > >> >>>> That is by definition the qom-get operation, not qom-list.
> > >> >>>> Just like the ls command does not show file contents, there's
> > >> >>>> cat etc. for that. For debugging purposes we had a qom-tree
> > >> >>>> (?) command that would combine both.
> > >> >>>
> > >> >>> I'm not too bothered about distinguishing between the two
> > >> >>> commands; but it would be nice
> > >> >
> > >> > When an HMP and QMP both have a command with the same name,
> they
> > >> > should do the same.
> > >> >
> > >> > HMP may add convenience features that aren't wanted in QMP, but I
> > >> > feel extending an operation to list objects to also show their
> > >> > contents goes beyond that.  If we want an HMP command that does
> > >> > both, it should be named differently.  Perhaps that might even be
> > >> > more appropriate for HMP than low-level commands qom-list and
> > >> > qom-get, but I leave that to the HMP maintainer to decide.
> > >> >
> > >> >>>                      - one reason I'm not too bothered is
> > >> >>> because we've failed to get a qom-get in multiple years of trying.
> > >> >
> > >> > We clearly haven't tried hard enough.
> > >> >
> > >> > If we can figure out how to show values in qom-list, surely we
> > >> > can figure out how to show them in qom-get.
> > >> >
> > >> >>>>       There might be unmerged patches on qemu-devel related to
> > >> >>>> display of certain data types.
> > >> >>>
> > >> >>> Which ones?
> > >> >>
> > >> >> My original qom-info series needed StringOutputVisitor changes
> > >> >> for enums (test case: rtc) that did not get accepted immediately
> > >> >> and thus some part of HMP qom-info/qom-get got stuck due to
> > >> >> risking assertions for qom-info / otherwise; QMP was not affected
> IIRC.
> > >> >
> > >> > Here's the last try I can find:
> > >> > [PATCH v2] qom: Implement qom-get HMP command
> > >> > Message-Id:
> > >> > <1473157086-12062-1-git-send-email-dgilb...@redhat.com>
> > >> > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01041.ht
> > >> > ml
> > >>
> > >> Stalled on output format and consistency with qom-set.  I wrote
> > >> back then "We can take qom-get as is and improve its output later."
> > >> I'd like to encourage you to dust it off.  Perfect's the enemy of good.
> > >>
> > >> Wanted improvements include:
> > >>
> > >> * Prettier output format.  I'd suggest creating a keyval variant of the
> > >>   output visitor.
> > >
> > > I'm not too bothered about pretty at first, as long as we don't stop
> > > anyway of making it pretty later; especially if non-compound types
> > > work well.
> >
> > We've waited so long for "someone" to post a solution that satisfies
> > all wants.  We should take a solution that is useful and can grow.
> >
> > >> * Make qom-set input format consistent by switching to the matching
> > >>   input visitor.
> > >>
> > >> > Its v1 tries a different approach:
> > >> > [PATCH 0/2] qom-get [for 2.8]
> > >> > Message-Id:
> > >> > <1472117833-10236-1-git-send-email-dgilb...@redhat.com>
> > >> > Unfortunately the mailing list archive doesn't show the full
> > >> > thread, so you get to follow three links:
> > >> > https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg03815.ht
> > >> > ml
> > >> > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04261.ht
> > >> > ml
> > >> > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg04267.ht
> > >> > ml
> > >>
> > >> This one stalled on string visitor limitations.  You didn't feel
> > >> like addressing them just to get qom-get working.  Understandable.
> > >
> > > Have there been any changes in the last 2 years that have helped?
> >
> > There's been progress, but there are gaps left.
> >
> > Back then, we had the choice of string input/output visitors, and the
> > options visitor, all (severely) restricted to certain kinds of data.
> >
> > Now we have the qobject keyval input visitor, which is almost general.
> > There is no matching output visitor, yet, simply because we haven't
> > had the need.
> 
> That's potentially a big help; is:
>    a) Does keyval output here make sense (It seems reasonable, perhaps
>       with some wrapping etc)
>    b) Is this the code in util/keyval.c + qapi/qobject-input-visitor.c ?
>    c) What's the right way to build the output - is it to use the
>       existing qobject_output_visitor and add a qobject_to_keyval - or
>       is it a job for a new visitor?
> 
> Dave
> 
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Hi guys,

This discussion is out of my field of expertise. I am still a newcomer in 
qemu's project : D

I developed the improvement in qom-list and not in qom-get as it was easier and 
much more straight forward to me. Moreover, for debugging purposes, I found 
easier to see all values in one command and not to have to execute several 
times qom-get.

Nonetheless, I see this is a controversial topic (and I fully understand why it 
is) so I let up to you to make the final decision of accepting it or not.

Regarding code improvements/bugs, there are still a couple of points to be 
addressed: 
-@Erik blake pointed an error in the "since version" is not correct
- I recall another colleague pointing that I should merge again with mainstream.

If these are the only remarks, I will deliver a v3 patch with these and it is 
up to you to decide to make official or not.

Kr,

Ricardo