Re: [Qemu-devel] [PATCH 0/3] Export machine type deprecation info through QMP

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Thu, May 09, 2019 at 10:31:46AM +0200, Markus Armbruster wrote:
>> We've wandered into the QAPI vs. QOM swamp.  Cc: Paolo.
>> 
>> Eduardo Habkost <address@hidden> writes:
>> 
>> > On Wed, May 08, 2019 at 11:16:50AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <address@hidden> writes:
>> >> 
>> >> > On Tue, May 07, 2019 at 07:07:04AM +0200, Markus Armbruster wrote:
>> >> >> Eduardo Habkost <address@hidden> writes:
>> >> >> 
>> >> >> > This series adds machine type deprecation information to the
>> >> >> > output of the `query-machines` QMP command.  With this, libvirt
>> >> >> > and management software will be able to show this information to
>> >> >> > users and/or suggest changes to VM configuration to avoid
>> >> >> > deprecated machine types.
>> >> >> 
>> >> >> This overlaps with something I want to try, namely using Kevin's
>> >> >> proposed QAPI feature flags for deprecation markings.  Let's compare 
>> >> >> the
>> >> >> two.
>> >> >> 
>> >> >> To mark something as deprecated with your patches, you add a
>> >> >> @support-status member somewhere, where "somewhere" is related to
>> >> >> "something" by "provides information on".
>> >> >> 
>> >> >> Example: MachineInfo (returned by query-machines) provides information
>> >> >> on possible values of -machine parameter type.  If -machine was
>> >> >> QAPIfied, it would provide information on possible values of a QAPI
>> >> >> object type's member.  The type might be anonymous.  The member should
>> >> >> be an enum (we currently use 'str' in MachineInfo).
>> >> >
>> >> > QAPIfying -machine, -cpu, and -device would be wonderful.
>> >> >
>> >> >> 
>> >> >> Example: say we want to deprecate block driver "vfat",
>> >> >> i.e. BlockdevDriver member @vfat.  Type BlockdevDriver is used in
>> >> >> multiple places; let's ignore all but BlockdevOptions.  We need to add
>> >> >> @support-status to something that provides information on
>> >> >> BlockdevDriver, or maybe on BlockdevOptions.  There is no ad hoc query
>> >> >> providing information on either of the two, because QAPI/QMP
>> >> >> introspection has been sufficient.  What now?
>> >> >> 
>> >> >> Can we add deprecation information to (general) QAPI/QMP introspection
>> >> >
>> >> > Yes, we can.  I think it's a good idea.  But:
>> >> >
>> >> >> instead of ad hoc queries?
>> >> >
>> >> > I'm not sure about the "instead of" part.  I don't want perfect
>> >> > to be the enemy of done, and I don't want QAPIfication of
>> >> > -machine to be a requirement to start reporting machine type
>> >> > deprecation information.
>> >> 
>> >> Valid point.  Still, I believe we should at least try to predict how the
>> >> pieces we create now would fit with the pieces we plan to create later
>> >> on.
>> >
>> > Sure.
>> >
>> >> 
>> >> Note that full QAPIfication of -machine isn't necessary to make QAPI
>> >> feature "deprecated" work for machine types.  Turning MachineInfo member
>> >> @name into an enum, so we can tack "deprecated" onto its values, would
>> >> suffice.
>> >> 
>> >> Such a QAPIfication of machine types is still hard: QOM types are
>> >> defined at compile time just like the QAPI schema, but their definition
>> >> is distributed, and collected into one place only at run time.  I
>> >> discussed this on slide 39 of my "QEMU interface introspection: From
>> >> hacks to solutions" talk (KVM Form 2015).  Just for device_add, but it's
>> >> just a special case of QOM.  Choices listed there:
>> >> 
>> >> * Collect drivers at compile time? Hard...
>> >> * Make QAPI schema dynamic? Hard...
>> >> * Forgo driver-specific arguments in schema?
>> >>   Defeats introspection...
>> >> 
>> >> I'd like to add to the last item:
>> >> 
>> >>   Provide QOM introspection on par with QAPI schema introspection
>> >> 
>> >> The QOM introspection we have (qom-list-types etc. is not on par.
>> >
>> > Agreed, but do we really want to do it?  We have been avoiding
>> > exposing QOM internals to the outside on purpose.  I believe
>> > there are at least two reasons for that:
>> >
>> > 1) Not every QOM type/property is supposed to be visible to the
>> >    outside
>> 
>> True.
>> 
>> However, the parts of QOM exposed via device_add and object-add are
>> definitely part of the stable external interface (unless explicitly
>> marked unstable).
>> 
>> >            (and nobody really knows what's the full set of
>> >    supported external QOM interfaces);
>> 
>> Also true.  And terrible.
>> 
>> > 2) QAPI is our preferred interface interface specification/introspection
>> >    mechanism.
>> 
>> When preferences and requirements collide, preferences tend to get run
>> over.
>> 
>> The QAPI schema is *declarative*: the schema declares QAPI objects and
>> properties.  We generate C from the schema, which we then compile and
>> link into QEMU.
>> 
>> QOM is by design *imperative*: we execute compiled C at QEMU run-time to
>> define QOM objects and properties.  Maximizes flexibility.  See also
>> Turing tarpit.
>> 
>> No matter how much we'd prefer to use QAPI to specify external
>> interfaces to QOM, we can't without making QAPI much more dynamic or QOM
>> much more static.  Either is hard.  Quite possibly infeasible.
>> 
>> We could try to extend QAPI/QMP introspection to somehow merge in
>> additional information at run-time[1].  Could be regarded as a limited
>> way to make QAPI more dynamic.  This is in the "vague idea, not sure
>> it's feasible" stage.
>> 
>> We could try to rearchitect QOM so that you can optionally specify QOM
>> stuff in the QAPI schema, then require that for QOM stuff that's part of
>> a stable external interface.  Same "vague idea" stage, feasibility even
>> more doubtful.
>> 
>> If I remember correctly, Paolo suggested to abandon the idea to have
>> query-qmp-schema cover device_add as infeasible.  Instead, cover it
>> separately.
>> 
>> I think this separate introspection mechanism should be as similar to
>> query-qmp-schema as practical.  It should cover not just device_add, but
>> also object-add, and whatever other external interface to QOM we deem
>> stable.
>
> I think we are in agreement here.  We don't need to make the new
> introspection mechanism part of query-qmp-schema, but I think it
> should use the same abstractions.

I agree we're basically in agreement :)

Covering dynamically defined external interfaces (such as device_add and
object-add) in query-qmp-schema would certainly be nice, but it's not a
hard requirement.  If we can't, then having separate introspection
commands share abstractions as much as practical is desirable.

>> Should there be more stable external interfaces that can't be defined in
>> the QAPI schema, we might want to cover them in this second
>> introspection mechanism, too.
>
> Agreed.
>
>> 
>> >> Back to exposing machine type deprecation.
>> >> 
>> >> I'm doubtful your proposed solution can be applied widely.  It relies on
>> >> adding @support-status to something that provides information on
>> >> whatever is deprecated.  The initial use is with a something that is an
>> >> ad hoc query, namely query-machines.  To use it, the management
>> >> application needs to understand what query-machines' @support-status
>> >> applies to.  Certainly feasible.  But I fear every use will be a special
>> >> case.  Furthermore, a suitable ad hoc query need not exist.  What then?
>> >> Create suitable ad hoc queries just for communicating deprecation?
>> >> 
>> >> Instead, I'd like us to think about a more genral solution.  Or perhaps
>> >> two: one for properly QAPIfied stuff, and one for QOM.
>> >
>> > Should we really spend our time designing a second solution, if
>> > we could build this on top of QAPI abstractions?  Making the QAPI
>> > schema dynamic might be hard, but reinventing QAPI and
>> > maintaining the two systems in parallel seems harder.
>> 
>> I'd love to see a practical way forward that keeps QAPI/QMP
>> introspection the only game in town.
>> 
>> >> >> Kevin's proposed QAPI feature flags[*] extend the QAPI language so that
>> >> >> struct types can optionally have a list of feature flags, which are
>> >> >> strings.  Struct types suffice for his immediate needs.  I'd like to 
>> >> >> use
>> >> >> feature flags to mark deprecation by tacking a "deprecated" feature 
>> >> >> onto
>> >> >> whatever is deprecated.  This obviously needs feature support for
>> >> >> everything we want to be able to deprecate: commands, and events, as
>> >> >> well as members of enum and object types.
>> >> >> 
>> >> >> Example: to deprecate block driver "vfat", add feature "deprecated" to
>> >> >> BlockdevDriver member @vfat.
>> >> >> 
>> >> >> Unlike your patches, this does not require finding a "somewhere" that
>> >> >> provides information on "something".  You simply tack "deprecated" 
>> >> >> right
>> >> >> onto "something".
>> >> >> 
>> >> >> Your patches provide more information, however: human-readable 
>> >> >> messages.
>> >> >
>> >> > It also includes a machine-friendly suggested alternative (which
>> >> > I think is even more important that the human-readable message).
>> >> 
>> >> I agree we should point to a preferred replacement whenever we deprecate
>> >> something.
>> >> 
>> >> We have to do it in documentation.  And we generally do, in
>> >> qemu-deprecated.texi.
>> >> 
>> >> How useful would doing it in QMP as well be?  Depends on what management
>> >> applications can do with the additional information.
>> >
>> > I expect it to be useful for things that have obvious
>> > replacements, like old machine type or CPU model versions.
>> 
>> I doubt a management application should apply suggested replacements
>> automatically, and I doubt libvirt would.  Not even when QEMU developers
>> deem them "obvious".
>> 
>> >> > We could extend QAPI introspection to return that if necessary,
>> >> > right?
>> >> 
>> >> I'm confident we can come up with *something*.  It might kill the neat
>> >> and simple "use QAPI features to communicate deprecation" idea, though.
>> >
>> > If something is important enough to be communicated through
>> > stderr, it's important enough to be communicated through QMP.
>> 
>> Mostly.  Differences are due to the different consumers.
>> 
>> stderr is primarily for human users.  We print stuff useful to human
>> users.
>
> We have users that don't have access to stderr.  They might have
> access to log files, but log files are pretty bad user
> interfaces.  If it's important for some set of human users, apps
> using libvirt or QMP need access to that information so they can
> show it to their human users too.

Command line means stderr.

I'm afraid our command line is awkward both for machines and for humans,
albeit for different reasons.

For humans doing simple things, the command line is okay.  But beyond
that, it gets forbiddingly unwieldy[2].

Machines are fine with that kind of unwieldy, but would prefer something
with more structure, both on input (talking to QEMU) and even more so on
output (QEMU talking back).

Ideally, we'd support machines do their work in (structured) QMP,
resorting to the command line only to set up a QMP monitor.  We're not
anywhere close to this.

As long as management applications use the command line in not-trivial
ways, they have to deal with configuration errors reported via stderr.

>> QMP is primarily for machines, secondarily for the humans building these
>> machines.  We send stuff useful to the machines themselves, and stuff
>> the machines can use to be more useful for their users (which may be
>> machines or humans).  We can also send stuff to help the humans building
>> the machines.
>>
>> In any case, the information we provide is limited by the cost to
>> provide it.
>
> Absolutely.
>
>> 
>> > Is that enough reason to provide something more complex?
>> 
>> We need to consider cost / benefit.
>> 
>> On benefit, I'd like to know what libvirt would do with the additional
>> information beyond logging it.
>
> I'd say it should provide it to apps, otherwise this won't be
> more useful than the existing log files.

A management application simply showing its user whatever error QEMU
reports or hint it provides is bound to be confusing: since QEMU talks
in QEMU terms, its errors and hints generally need translation to make
sense at higher layers.  Translation involves recognizing specific
messages, which means it's limited to special cases (and painfully
brittle).

The farther you propagate QEMU's messages up the stack, the less sense
they'll likely make.

Management applications logging QEMU's messages is useful anyway, mainly
because it's better than nothing.

I doubt logging them some more further up the stack would be all that
useful, but I might be wrong.

Discussed further elsewhere in this thread.

>> Is the additional information you propose to provide static or dynamic?
>> 
>> By "static", I mean each occurence of a feature in the QAPI schema is
>> tied to one fixed instance of "additional information".
>
> I don't think I understand this description of "static".  I
> expect the data to be fixed at build time, but I expect it to be
> different in downstream distributions of QEMU.

Let me try differently.

QAPI features as currently envisaged convey one bit of information:
there / not there.  The information is fixed at build time.  It is tied
to a specific QAPI entity (command, object type, enumeration value,
...).

My question is about the difference between this and what you have in
mind.  Specifically, is the difference only the amount of information
(one bit vs. a pair of string literals), or is there more?

"More" includes string values that can vary at run time or between
different uses of the QAPI entity in the schema.

>> > Do we need QAPI features to be just strings?  Can't they be a
>> > more complex type, like a QAPI alternate?
>> 
>> Adds complexity.
>> 
>> We currently imagine QAPI features enum-like, i.e. a list of strings,
>> optionally with conditions.  The conditions are evaluated at QAPI
>> generation time, and not visible in introspection.
>> 
>> This is probably the stupidest solution that could possibly work.  The
>> structure of features is trivial.
>> 
>> More expressive solutions include:
>> 
>> * List of 'any'.  Feels like a bad idea, because it's completely
>>   unstructured.
>
> Agreed.
>
>> 
>> * List of some 'QapiFeatures' object type.  Lets us expose the variable
>>   structure of features in introspection.
>> 
>> * List of some 'QapiFeatures' alternate type.  Like the previous, but
>>   permits coding simple feature flags as strings.
>> 
>> All of the more expressive solutions I listed treat the additional
>> information as dynamic.  Overly general in case the information is
>> actually always static.
>
> The static vs. dynamic distinction is getting me confused.  Why
> are the more expressive solutions more dynamic than "list of
> strings"?  Can you give examples?

Insufficiently precise thinking leads to vague and confusing prose.  Let
me try again.

The scenario I had in mind is having only the (static) structure of the
additional information in query-qmp-schema, and the actual (possibly
dynamic) information elsewhere, say in query-machines, or a new member
of the QMP success response.

> Also, why do we want to place all info inside the same "features"
> attribute instead of just adding new fields to SchemaInfoObject?
>
> i.e. why are these options:
>
>   { "members" [ ... ],
>     "features": [ "dynamic-read-only", "deprecated" ] }
>
>   { "members" [ ... ],
>     "features": [ "dynamic-read-only",
>                   { "deprecated": true, "hint": "FOO" } ] }
>
> better than these options:
>
>   { "members" [ ... ],
>     "features": [ "dynamic-read-only" ],
>     "deprecated": true }
>
>   { "members" [ ... ],
>     "features": [ "dynamic-read-only" ],
>     "deprecation-info": { "deprecated": true, "hint": "FOO" } }

I wouldn't claim "better".  I merely observe it takes its own QAPI
language extension, unlike the 'neat and simple "use QAPI features to
communicate deprecation" idea'.

>> >> >> Food for thought :)
>> >> >> 
>> >> >> 
>> >> >> [*] Hiding in
>> >> >> Subject: [PATCH 0/4] file-posix: Add dynamic-auto-read-only QAPI 
>> >> >> feature
>> >> >> Date: Mon,  8 Apr 2019 16:35:39 +0200
>> >> >> Message-Id: <address@hidden>
>> 
>> 
>> [1] Special case of "merge additional information into QAPI/QMP
>> introspection at run-time, relegated to a footnote, because I think it's
>> of mostly historical interest now.
>> 
>> Qdev was designed to be declarative, but the declarations always got
>> collected only at run-time.  It has since been rebased onto imperative
>> QOM.  Since you can mix declarative qdev and imperative QOM, the
>> combination is effectively imperative.
>> 
>> We could collect the declarative parts of qdev at compile-time somehow,
>> and fold them into QAPI introspection.  That's the "Collect drives at
>> compile time" item from the slide I quoted.  I think this idea has
>> become pretty much worthless, since it would cover device_add only in
>> part, and object-add not at all.

[2] Example quoted from my "Towards a more expressive and introspectable
QEMU command line" talk at KVM Forum 2017 (predating -blockdev, which
spices up things even more):
/usr/bin/qemu-system-x86_64 -machine accel=kvm -name boxes-unknown -S -machine 
pc-i440fx-1.6,accel=kvm,usb=off -cpu Penryn -m 3115 -realtime mlock=off -smp 
4,sockets=1,cores=4,threads=1 -uuid 8bd53789-adab-484f-8c53-a6df9d5f1dbf 
-no-user-config -nodefaults -chardev 
socket,id=charmonitor,path=/home/guillaume/.config/libvirt/qemu/lib/boxes-unknown.monitor,server,nowait
 -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew 
-global kvm-pit.lost_tick_policy=discard -no-shutdown -global 
PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device 
ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x5.0x7 -device 
ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x5 
-device 
ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x5.0x1-device 
ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x5.0x2 -device 
virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 -device 
usb-ccid,id=ccid0 -drive 
file=/home/guillaume/.local/share/gnome-boxes/images/boxes-unknown,if=none,id=drive-ide0-0-0,format=qcow2,cache=none
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 
-drive if=none,id=drive-ide0-1-0,readonly=on,format=raw -device 
ide-cd,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -netdev 
tap,fd=23,id=hostnet0 -device 
rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:db:56:54,bus=pci.0,addr=0x3 
-chardev spicevmc,id=charsmartcard0,name=smartcard -device 
ccid-card-passthru,chardev=charsmartcard0,id=smartcard0,bus=ccid0.0 -chardev 
pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev 
spicevmc,id=charchannel0,name=vdagent -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
 -device usb-tablet,id=input0 -spice 
port=5901,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on
 -device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vgamem_mb=16,bus=pci.0,addr=0x2
 -device AC97,id=sound0,bus=pci.0,addr=0x4 -chardev 
spicevmc,id=charredir0,name=usbredir -device 
usb-redir,chardev=charredir0,id=redir0 -chardev 
spicevmc,id=charredir1,name=usbredir -device 
usb-redir,chardev=charredir1,id=redir1 -chardev 
spicevmc,id=charredir2,name=usbredir -device 
usb-redir,chardev=charredir2,id=redir2 -chardev 
spicevmc,id=charredir3,name=usbredir -device 
usb-redir,chardev=charredir3,id=redir3 -incoming fd:20 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 -msg timestamp=on