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

[Kevin Wolf]

Am 17.09.2021 um 10:26 hat Markus Armbruster geschrieben:
> >> When exactly are collisions an error?  How exactly are non-erroneous
> >> collisions resolved?  qapi-code-gen.rst needs to answer this.
> >
> > The strategy implemented in this version is: Collisions are generally an
> > error, except for wildcard aliases conflicting with a non-wildcard-alias
> > value. In this case the wildcard alias is ignored and the value is
> > assumed to belong elsewhere.
> 
> Kinds of collisions:
> 
>                 member          ordinary alias  wildcard alias
> member          error[1]        error[2]        member wins[4]
> ordinary alias                  error[3]        ordinary wins[4]
> wildcard alias                                  ???[5]
> 
> [1] Test case duplicate-key demonstrates.
> 
> [2] Test case alias-name-conflict demonstrates.
> 
> [3] No test case, manual testing results in "alias 'a' collides with
>     alias 'a'".
> 
> [4] Please confirm I got this right.
> 
> [5] No test case, manual testing results in no error.  What's the
>     intended behavior?

[5] is going to become "more local wins". In v3 it's "runtime error if
both are specified, except sometimes".

Both [4] and [5] are runtime errors if two values are specified and
there aren't two consumers for them.

> >> 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.

> 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?

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. And just having a mapping of JSON to the
command line is why it should be there in dotted key syntax, too.

> 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.

> 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.

> > I think I've come to the conclusion that it's not easy enough to
> > explain. As long as the parsing order should remain an implementation
> > detail that schema authors shouldn't rely on, it's not possible at all.
> >
> > It's a pity because the code would have been simpler and it would
> > probably have worked for the cases we're interested in. But it doesn't
> > work in all hypothetical cases and we can't document the conditions for
> > that without making people rely on the parsing order.
> 
> The order in which the visitors visit members is not really specified.
> The examples in qapi-code-gen.rst show it's in source order, and the
> order is quite unlikely to change.  So, making it official is not out of
> the question.
> 
> However, depending on member order for anything feels iffy.  I'd like to
> be able to shuffle them around without breaking anything.
> 
> Thoughts?

No, I agree. As I said it's not simple enough to explain, so I'll just
have the deleted code back and add a bit to improve error reporting. Not
a big deal for me - though it might be one for your review...

Kevin