Re: [PATCH v3 6/6] tests/qapi-schema: Test cases for aliases

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.09.2021 um 10:26 hat Markus Armbruster geschrieben:

[...]

>> >> We actually require much less: for QMP chardev-add, we need
>> >> 'data.addr.data.FOO' and nothing else, and for CLI -chardev, we need
>> >> 'FOO' and nothing else (I think).  The unneeded ones become accidental
>> >> parts of the external interface.  If experience is any guide, we'll have
>> >> plenty of opportunity to regret such accidents :)
>> >> 
>> >> Can we somehow restrict external interfaces to what we actually need?
>> >
>> > Not reasonably, I would say. Of course, you could try to cover all
>> > paths with aliases in the top level, but the top level really shouldn't
>> > have to know about the details of types deep inside some union variants.
>> >
>> > The solution for reducing the allowed options here is to work on
>> > introspection, mark 'data' deprecated everywhere and get rid of the
>> > simple union nonsense.
>> 
>> Accidental extension of QMP to enable QAPIfication elsewhere would be a
>> mistake.  Elsewhere right now: -chardev.
>>
>> The knee-jerk short-term solution for QMP is to ignore aliases there
>> completely.  Without introspection, they can't be used seriously anyway.
>
> I would say it's intentional enough. If we can flatten simple unions for
> the CLI, why not accept them in QMP, too? (And management tools will
> only be happier if they can use the same representation for QMP and
> CLI.) I hope that we can get introspection done for 6.2, but even if we
> can't, making the case already work shouldn't hurt anyone.
>
> Now you could argue that some aliases to be introduced for -chardev have
> no place in QMP because they have no practical use there. But isn't a
> consistent QAPI structure on all external interfaces more valuable than
> keeping the interface in QMP minimal, but inconsistent with the CLI?
>
> The problem I would generally see with accidental extension of QMP is
> that it may restrict future changes for no reason. But if we already
> get the restriction because we must stay compatible with the CLI, too,
> then this doesn't apply any more.

I agree consistency matters.  But what exactly should be consistent?

I believe we all agree that a CLI option's *JSON* argument should be
consistent with the corresponding QMP command.  This is easy: since both
JSON arguments are fed to the same QAPI machinery, consistency is the
default, and inconsistency would take extra effort.

But what about the dotted keys argument?

One point of view is the difference between the JSON and the dotted keys
argument should be concrete syntax only.  Fine print: there may be
arguments dotted keys can't express, but let's ignore that here.

Another point of view is that dotted keys arguments are to JSON
arguments what HMP is to QMP: a (hopefully) human-friendly layer on top,
where we have a certain degree of freedom to improve on the
machine-friendly interface for human use.

Let me try to explain why I believe the latter one makes more sense.

When QAPIfying existing CLI options, the new dotted keys option argument
must be backward compatible to the old option argument, and the QMP
command must also remain backward compatible.

If their abstract syntax is already perfectly consistent, we can simply
use qobject_input_visitor_new_str().  All we have to worry then is the
differences between dotted keys syntax and whatever it replaces (almost
always QemuOpts), which is of no concern to us right now.

What if their abstract syntax differs, as it does for -chardev and
QMP chardev-add?

One solution is to use the *union* of the two languages both for CLI and
QMP.  Complicates both.  I don't like this.  We can try to mitigate by
deprecating unwanted parts of the abstract syntax.  More on that below.

If we treat dotted keys as separate, human-friendly layer on top, we can
instead *translate* from one language to the other.  This is actually
what -chardev, HMP chardev-add, and all the other HMP commands wrapping
around QMP do today, or close to it.


Different tack.  I've been struggling with distilling my intuitions
about the proposed QAPI aliases feature into thought (one reason for me
taking so painfully long to reply).  I believe my difficulties are in
part due to a disconnect from use cases: there's a lot aliases could
enable us to do, and I keep getting lost in possibilities.  So let's try
to ground this by looking at QAPIfication of -chardev.  This way, we
discuss how to solve a specific problem in its entirety instead of
trying to come to a conclusion on a building block that may be useful
(but isn't sufficient) to solve an unclear set of problems (unclear to
*me*).

The equivalent QMP command is chardev-add.  The definition of its
argument is spread over several QAPI type definitions.  It boils down
to:

    id: str
    backend: ChardevBackend
        type: ChardevBackendKind        tag
        data: ChardevFile               when file
            *logfile: str
            *logappend: bool
            *in: str
            out: str
            *append: bool
        ...
        data: ChardevSocket             when socket
            *logfile: str
            *logappend: bool
            addr: SocketAddressLegacy
                type: SocketAddressType        tag
                data: InetSocketAddress        when inet
                    host: str
                    port: str
                    *numeric: bool
                    ...
                ...
            ...

This is old stuff; we'd use a lot less nesting today.

The argument of -chardev is flat, as output of -help shows:

    ...
    -chardev 
socket,id=id[,host=host],port=port[,to=to][,ipv4=on|off][,ipv6=on|off][,nodelay=on|off][,reconnect=seconds]
    ...
    -chardev file,id=id,path=path[,mux=on|off][,logfile=PATH][,logappend=on|off]
    ...

The historical reason for -chardev being flat is that QemuOpts supports
only flat out of the box.  Nested requires gymnastics of the kind we
generally perform only under duress.

There's also a reason for *keeping* it flat: nesting is awful in dotted
keys syntax.  With JSON, you simply throw another pair of braces on the
heap.  With dotted keys, you get to repeat long key prefixes over and
over.

Let me stitch the two interfaces together:

    -chardev file                       backend.type
             ,id=id,                    id
             path=path                  backend.data.out
                                        backend.data.in
                                        backend.data.append
             [,mux=on|off]
             [,logfile=PATH]            backend.data.logfile
             [,logappend=on|off]        backend.data.logappend
    ...
    -chardev socket                     backend.type
             ,id=id                     id
                                        backend.data.addr.type
             [,host=host]               backend.data.addr.data.host + default
             ,port=port                 backend.data.addr.data.port
                                        backend.data.addr.data.numeric
             [,to=to]                   backend.data.addr.data.to
             [,ipv4=on|off]             backend.data.addr.data.ipv4
             [,ipv6=on|off]             backend.data.addr.data.ipv6
                                        backend.data.addr.data.keep-alive
                                        backend.data.addr.data.mptcp
             [,nodelay=on|off]          backend.data.nodelay
             [,reconnect=seconds]       backend.data.reconnect
             [,server=on|off]           backend.data.server
             [,wait=on|off]             backend.data.wait
             [,telnet=on|off]           backend.data.telnet
                                        backend.data.tn3270
             [,websocket=on|off]        backend.data.websocket
             [,mux=on|off]
             [,logfile=PATH]            backend.data.logfile
             [,logappend=on|off]        backend.data.logappend
             [,tls-creds=ID]            backend.data.tls-creds
             [,tls-authz=ID]            backend.data.tls-authz

CLI parameter @mux has nothing in the right column.  The code looks like
it's syntactic sugar for something I don't understand.  Moving on.

A few things in the right column have nothing in the left column.  I can
see three possible cases:

* It's only missing in help.  Example: parameter append actually exists,
  and corresponds to backend.data.append

* -chardev can't do.  Example, maybe: backend.data.in.

* Magic.  Example: the value of backend.data.addr.type is derived from
  presence of path.

Both -chardev and QMP chardev-add use the same helpers (there are minor
differences that don't matter here).  The former basically translates
its flat argument into the latter's arguments.  HMP chardev-add is just
like -chardev.

The quickest way to QAPIfy existing -chardev is probably the one we
chose for -object: if the argument is JSON, use the new, QAPI-based
interface, else use the old interface.

Sugar is only available with the old QemuOpts argument.

Feature-parity with QMP is only available with the new JSON argument.

This kicks the "retire QemuOpts" can down the road.  The hope is to
eventually weaken the compatibility promise for the non-JSON argument
(like we did for HMP), then switch it to dotted keys.  Whether we ditch
the translation from flat to nested then is an open question.

A different problem would be adding a new -chardev somewhere.  Not
having to drag all the compatibility baggage along would be nice.  On
the other hand, dotted keys without flattening would result in an awful
human interface.  Flattening by hand like the existing code does is
unappealing.  We could use a bit of help from the QAPI generator.

Perhaps we hate QMP chardev-add's nesting enough to flatten it to
something like

    id: str
    type: ChardevBackendKind        tag
    # ChardevFile                   when file
        *logfile: str
        *logappend: bool
        *in: str
        out: str
        *append: bool
    ...
    # ChardevSocket                 when socket
    *logfile: str
    *logappend: bool
    addr: SocketAddress
        type: SocketAddressType         tag
        # InetSocketAddress             when inet
        host: str
        port: str
        *numeric: bool

To avoid a compatibility break, we'd actually have to keep the old
structure around, too.  Deprecate, drop after grace period expires.


Now the question that matters for this series: how can QAPI aliases help
us with the things discussed above?

1. Aliases can help with flattening input wire formats.  Missing parts:
introspection, feature deprecated for aliases.

Note:

* Input: aliases don't affect output.

* Wire format: aliases don't affect the generated C types.

* Aliases can only be "flatter" than the thing they alias.  The members
  remain nested until drop the old interface.

* Feature deprecated for aliases: straightforward, I think.

* Introspection: requires a schema extension, which means users of
  introspection will need an update to actually see aliases.

  Corollary: until all of the users that matter are updated, we can't
  deprecate members, only aliases.  Unfortunately, members is what we
  actually *want* to deprecate here.

  Why do we need a schema extension?  We can't simply expose aliases as
  if they were members, because that would look as if you had to specify
  both (which is actually an error).

  When we drop the old interface, the old, deprecated members go away,
  and the aliases morph into members.  Well-behaved users of
  introspection should deal with the morphing.  Still, there's a risk.

As much as I hate excessive nesting, I'm not sure flattening old
interfaces such as QMP chardev-add is worth the trouble.  More on that
below, in reply to your arguments.

Flattening could reduce the difference to the current, non-QAPIfied
-chardev, though, which could help with QAPIfying it.  How exactly
remains unclear, at least to me, especially for sugar and magic.

2. Aliases can help with adding flatter options to existing input wire
formats.  This is 1. less the hope to get rid of the nested option some
day.  Missing parts: introspection.  Old introspection users simply
can't see the flat option, which is fine.

Is adding a flat option to old interfaces such as QMP chardev-add worth
the trouble?  If you have to check whether the flat option is available
before you can use it, and else fall back to the nested option, always
using the nested option is simpler.

I figure the most likely user of these aliases would be a QAPIfied
-chardev.  That use is legitimate.  But if it remains the only one, all
the extra work to expose them in the external interface in a consumable
way (the "missing parts" above) is wasted.

3. Aliases might help with creating flatter variations of old input wire
formats.  This is 2. plus a way to limit each interface to one option.
Missing parts: a way to limit, introspection.

Note:

* "A way to limit" is too vague to be useful.  Needs design work.
  Stupidest solution that could possibly work: hard-code two options,
  with and without aliases.

* Introspection is needed only for machine-friendly interfaces.
  Implementing it might be expensive.

If we use the flat variation just internally, say for -chardev's dotted
keys argument, then introspection is not needed.  We'd use "with
aliases" just for translating -chardev's dotted keys argument.

We could also use the flat variation to create alternate, more modern
interfaces with much less code duplication in the schema, and much less
boring translation code between the differently nested duplicates.

Example: we have SocketAddress for use by "modern" interfaces, and
SocketAddressLegacy strictly for existing ones.  We have hand-written
code to convert SocketAddressLegacy to SocketAddress.  Not terrible for
a simple type like SocketAddress, but imagine doing the same for
flattening ChardevBackend.

>> Of course, we eventually want to use them for evolving QMP, e.g. to
>> flatten simple unions.  The knee-jerk solution sets up another obstacle.
>> 
>> The issue also exists in -chardev with a JSON argument.  We can apply
>> the knee-jerk solution to any JSON-based interface, not just to QMP.
>> 
>> The issue also exists in -chardev with a dotted keys argument.  There,
>> we definitely need the outermost alias (e.g. "host") for compatibility,
>> and we may want the member ("data.addr.data.host") for symmetry with
>> JSON.  I can't see an argument for exposing the intermediate aliases as
>> dotted keys, though.
>> 
>> I find the argument "for symmetry with JSON" quite weak.  But exposing
>> the member seems unlikely to create problems later on.
>
> Well, my simple argument is: It's hard to get rid of them, so why bother
> with extra complexity to get rid of them?

It's either that, or writing documentation explaining the now many ways
to do the same thing.

> But I think there is a better argument, and "symmetry with JSON"
> actually covers support for the intermediate aliases, too:
>
> The alias that flattens SocketAddressLegacy isn't an alias for the
> command chardev-add, it's an alias for the type. If you have code that
> formats JSON for SocketAddressLegacy, then you should be able to use it
> everywhere where a SocketAddressLegacy is required.
>
> So if your code to format JSON for SocketAddressLegacy uses the alias to
> provide a flat representation, but the caller producing ChardevBackend
> doesn't flatten the union yet, then that should be fine. And if you have
> code for flat ChardevBackend, but your common SocketAddressLegacy code
> still produces the nesting, then that should be fine, too.
>
> Essentially partial use of aliases in JSON is a feature to allow libvirt
> adopting changes incrementally.

This is about evolving existing JSON-based interfaces (QMP, basically)
compatibly.

To be able to use aliases for this, we need to supply missing parts
(detailed above), and they are anything but trivial.  Users of
introspection need updates, too.

I hate excessive nesting, I really do.  I'd love to see excessively
nested existing interfaces cleaned up.  Sadly, the paths towards that
goal I can see feel too costly just for cleanliness.

>                                 And just having a mapping of JSON to the
> command line is why it should be there in dotted key syntax, too.

We don't have a mapping from HMP to QMP, only the other way round.

I believe we should make CLI with dotted keys like HMP (not least
because it already is for many complex options).  Then we don't have a
mapping from QMP to CLI with dotted keys.  We do have a (trivial!)
mapping to CLI with JSON.

>> You argue that "the top level really shouldn't have to know about the
>> details of types deep inside some union variants."  That's a valid
>> argument about the QAPI schema language's support for abstraction.  But
>> the QAPI schema language is means, while the external interfaces are
>> ends.  They come first.  A nicer schema language is certainly desirable,
>> but the niceties shouldn't leak crap into the external interfaces.
>> 
>> Let me work through an example to ground this.  Consider chardev-add /
>> -chardev.  Structure of chardev-add's argument:
>> 
>>     id: str
>>     backend:
>>         type: enum ChardevBackendKind
>>         data: one of the following, selected by the value of @type:
>>         for socket:
>>             addr:
>>                 type: enum SocketAddressType
>>                 data: one of the following, selected by the value of @type:
>>                   for inet:
>>                   host: str
>>                   ...
>>           ...
>> 
>> In contrast, -chardev's argument is flat.  To QAPIfy it, we could use
>> aliases from the root into the object nest:
>> 
>>     from type to backend.type
>>     from host to backend.data.addr.data.host
>>     ...
>> 
>> We'd certainly design chardev-add's argument differently today, namely
>> with much less nesting.  Say we want to evolve to this structure:
>> 
>>     id: str
>>     type: enum ChardevBackendKind
>>     one of the following, selected by the value of @type:
>>     for socket:
>>     addr:
>>         type: enum SocketAddressType
>>         one of the following, selected by the value of @type:
>>         for inet:
>>         host: str
>>         ...
>>     ...
>> 
>> We obviously need to keep the old structure around for compatibility.
>> For that, we could use a *different* set of aliases:
>> 
>>     from type to backend.type
>>     from addr.host to backend.data.addr.data.host
>>     ...
>> 
>> What's the plan for supporting different uses wanting different aliases?
>> Throw all the aliases together and shake?  Then one interface's
>> requirements will contaminate other interfaces with unwanted aliases.
>> Getting one interface right is hard enough, having to reason about *all*
>> QAPI-based interfaces would set us up for failure.  And what if we run
>> into contradictory requirements?
>
> Are there legitimate reasons for exposing the same QAPI type in
> different ways on different interfaces? This sounds like a bad idea to
> me. If it's the same thing, it should look the same.
>
> The biggest reason for QAPIfying things is to unify interfaces instead
> of having different parsers everywhere. Intentionally accepting some
> keys only in QMP and others only in the CLI seems to go against this
> goal.

Valid argument.  CLI with JSON argument should match QMP.  Even when
that means both are excessively nested.

CLI with dotted keys is a different matter, in my opinion.

>> Could we instead tag aliases somehow, pass a tag to the visitor, and
>> have it ignore aliases with a different tag?
>> 
>> Reminds me of the problem of generating multiple QMPs from a single
>> schema, e.g. one for qemu-system-FOO, and another one for
>> qemu-storage-daemon.  Inchoate idea: use tags for that somehow.  But I
>> digress.
>> 
>> I'm actually tempted to try how far we can get with just one level of
>> aliases, i.e. aliases that can only resolve to a member, not to another
>> alias.  I'd expect the code to become quite a bit simpler.
>
> The visitor code would become slightly simpler, but the schema would
> become much less maintainable. If someone adds a new field to, say,
> InetSocketAddress, review would have to catch this and request that a
> new alias be added to ChardevOptions. I don't think this is a realistic
> option.

Shouldn't a wildcard alias in ChardevOptions take care of that?

[...]

I apologize for this wall of text.  It's a desparate attempt to cut
through the complexity and my confusion, and make sense of the actual
problems we're trying to solve.

So, what problems exactly are we trying to solve?