[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] qom: Implement qom-get HMP command
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [RFC PATCH] qom: Implement qom-get HMP command |
Date: |
Wed, 20 May 2020 10:59:20 +0100 |
User-agent: |
Mutt/1.13.4 (2020-02-15) |
* Markus Armbruster (address@hidden) wrote:
> "Dr. David Alan Gilbert" <address@hidden> writes:
>
> > * Markus Armbruster (address@hidden) wrote:
> >> Cédric Le Goater <address@hidden> writes:
> >>
> >> > From: "Dr. David Alan Gilbert" <address@hidden>
> >> >
> >> > Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> >> > to strings.
> >> >
> >> > Inspired-by: Paolo Bonzini <address@hidden>
> >> > Signed-off-by: Andreas Färber <address@hidden>
> >> >
> >> > Slight fix for bit-rot:
> >> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> >> > [clg: updates for QEMU 5.0 ]
> >> > Signed-off-by: Cédric Le Goater <address@hidden>
> >> > ---
> >> >
> >> > I would like to restart the discussion on qom-get command to understand
> >> > what was the conclusion and see if things have changed since.
> >>
> >> I've since learned more about QOM. I believe we should do HMP qom-get,
> >> but not quite like this patch does. Let me explain.
> >>
> >> A QOM property has ->get() and ->set() methods to expose the property
> >> via the Visitor interface.
> >>
> >> ->get() works with an output visitor. With the QObject output visitor,
> >> you can get the property value as a QObject. QMP qom-get uses this.
> >> With the string output visitor, you can get it as a string. Your
> >> proposed HMP qom-get uses this.
> >>
> >> ->set() works with an input visitor. With the QObject input visitor,
> >> you can set the property value from a QObject. QMP qom-set uses this.
> >> With the string input visitor, you can set it from a string. HMP
> >> qom-set uses this. With the options visitor, you can set it from a
> >> QemuOpts. CLI -object uses this.
> >>
> >> The Visitor interface supports arbitrary QAPI types. The ->get() and
> >> ->set() methods can use them all.
> >>
> >> Some visitors, howerver, carry restrictions:
> >>
> >> * The string output visitor does not implement support for visiting
> >> * QAPI structs, alternates, null, or arbitrary QTypes. It also
> >> * requires a non-null list argument to visit_start_list().
> >>
> >> * The string input visitor does not implement support for visiting
> >> * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
> >> * of integers (except type "size") are supported.
> >>
> >> * The Opts input visitor does not implement support for visiting QAPI
> >> * alternates, numbers (other than integers), null, or arbitrary
> >> * QTypes. It also requires a non-null list argument to
> >> * visit_start_list().
> >>
> >> If you try to qom-set a property whose ->set() uses something the string
> >> input visitor doesn't support, QEMU crashes. Same for -object, and your
> >> proposed qom-get.
> >
> > Crashing would be bad.
> >
> >> I'm not aware of such a ->set(), but this is a death trap all the same.
> >>
> >> The obvious way to disarm it is removing the restrictions.
> >>
> >> One small step we took towards that goal is visible in the comments
> >> above: support for flat lists of integers. The code for that will make
> >> your eyes bleed. It's been a thorn in my side ever since I inherited
> >> QAPI. Perhaps it could be done better. But there's a reason why it
> >> should not be done at all: it's language design.
> >>
> >> The QObject visitors translate between QAPI types and our internal
> >> representation of JSON. The JSON parser and formatter complete the
> >> translation to and from JSON. Sensible enough.
> >>
> >> The string visitors translate between QAPI and ... well, a barely
> >> documented language of our own "design". Tolerable when the language it
> >> limited to numbers, booleans and strings. Foolish when it grows into
> >> something isomorphic to JSON.
> >>
> >> This is a dead end.
> >>
> >> Can we still implement a safe and tolerably useful HMP qom-get and
> >> qom-set? I think we can. Remember the basic rule of HMP command
> >> implementation: wrap around the QMP command. So let's try that.
> >>
> >> hmp_qom_get() calls qmp_qom_get() and formats the resulting QObject with
> >> qobject_to_json_pretty().
> >
> > Why don't we *just* do this?
> > OK, we wont fix the qom-set or the CLI, but if we just get hmp_qom_get
> > to call QMP, format it into json and then dump the json to the user,
> > then we get a usable, if not pretty, qom-get command, without having to
> > solve all these hard problems to move it forward?
>
> Count me in favour. I'd like to see the matching change to HMP qom-set,
> though.
It turns out I actually did do a JSON version, as the follow up to the
version Cédric reposted; but then that got lost in people not liking
JSON; https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01041.html
Having just resurrected that code, the only difference from my patch
then and what I just wrote was that instead of doing the object
resolution and stuff, I just call the qmp function.
It's still JSON output and I don't think the arguments from 4 years ago
moved forward. I'll post it soon.
Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK