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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.10.2021 um 09:44 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> [...]
>> 
>> > What I had in mind was using the schema to generate the necessary code,
>> > using the generic keyval parser everywhere, and just providing a hook
>> > where the QDict could be modified after the keyval parser and before the
>> > visitor. Most command line options would not have any explicit code for
>> > parsing input, only the declarative schema and the final option handler
>> > getting a QAPI type (which could actually be the corresponding QMP
>> > command handler in the common case).
>> 
>> A walk to the bakery made me see a problem with transforming the qdict
>> between keyval parsing and the visitor: error reporting.  On closer
>> investigation, the problem exists even with just aliases.
>
> I already commented on part of this on IRC, but let me reply here as
> well.
>
> On general remark is that I would make the same distinction between
> aliases for compatibility and usability that I mentioned elsewhere in
> this thread.
>
> In the case of compatibility, it's already a concession that we still
> accept the option - suboptimal error messages for incorrect command
> lines aren't a major concern for me there. Users are supposed to move to
> the new syntax anyway.

Well, these aren't "incorrect", merely deprecated.  Bad UX is still bad
there, but at least it'll "fix" itself when the deprecated part goes
away.

> On the other hand, aliases we employ for usability are cases where we
> don't expect humans to move to something else. I think this is mostly
> for flattening structures. Here error messages should be good.
>
>> All experiments performed with your complete QAPIfication at
>> https://repo.or.cz/qemu/kevin.git qapi-alias-chardev-v4.
>> 
>> 
>> Example: flattening leads to suboptimal error
>> 
>>     $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,ipv4=om
>>     qemu-system-x86_64: -chardev udp,id=chr0,port=12345,ipv4=om: Parameter 
>> 'backend.data.remote.data.ipv4' expects 'on' or 'off'
>> 
>> We're using "alternate" notation, but the error message barks back in
>> "standard" notation.  It comes from the visitor.  While less than
>> pleasant, it's still understandable, because the "standard" key ends
>> with the "alternate" key.
>
> This is not a fundamental problem with aliases. The right name for the
> option is unambiguous and known to the visitor: It's the name that the
> user specified.

This is "1. The visitor reports errors as if aliases didn't exist"
below.

> With manual QDict modification it becomes a more fundamental problem
> because the visitor can't know the original name any more.

This is "2. The visitor reports errors as if manual transformation
didn't exist" below.

I agree 1 is easier to fix than 2.

>> Example: renaming leads to confusing error
>> 
>> So far, we rename only members of type 'str', where the visitor can't
>> fail.  Just for illustrating the problem, I'm adding one where it can:
>> 
>>     diff --git a/qapi/char.json b/qapi/char.json
>>     index 0e39840d4f..b436d83cbe 100644
>>     --- a/qapi/char.json
>>     +++ b/qapi/char.json
>>     @@ -398,7 +398,8 @@
>>      ##
>>      { 'struct': 'ChardevRingbuf',
>>        'data': { '*size': 'int' },
>>     -  'base': 'ChardevCommon' }
>>     +  'base': 'ChardevCommon',
>>     +  'aliases': [ { 'name': 'capacity', 'source': [ 'size' ] } ] }
>> 
>>      ##
>>      # @ChardevQemuVDAgent:
>> 
>> With this patch:
>> 
>>     $ qemu-system-x86_64 -chardev ringbuf,id=chr0,capacity=lots
>>     qemu-system-x86_64: -chardev ringbuf,id=chr0,capacity=lots: Parameter 
>> 'backend.data.size' expects integer
>> 
>> The error message fails to connect to the offending key=value.
>
> Same problem as above. The error message should use the key that the
> user actually specified.

Yes.

>> Example: manual transformation leads to confusion #1
>> 
>> Starting point:
>> 
>>     $ qemu-system-x86_64 -chardev udp,id=chr0,port=12345,localaddr=localhost
>> 
>> Works.  host defaults to localhost, localport defaults to 0, same as in
>> git master.
>> 
>> Now "forget" to specify @port:
>> 
>>     $ qemu-system-x86_64 -chardev udp,id=chr0,localaddr=localhost
>>     qemu-system-x86_64: -chardev udp,id=chr0,localaddr=localhost: Parameter 
>> 'backend.data.remote.data.port' is missing
>> 
>> Again, the visitor's error message uses "standard" notation.
>
> The output isn't wrong, it's just more verbose than necessary.
>
> Getting this one shortened is a bit harder because the right name is
> ambiguous, the user didn't specify anything we can just print back.
>
> Possibly in CLI context, making use of any wildcard aliases would be a
> reasonable strategy. This would reduce this one to just 'port'.

Which of the possible keys we use in the error message boils down to
picking a preferred key.

>> We obediently do what the error message tells us to do:
>> 
>>     $ qemu-system-x86_64 -chardev 
>> udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345
>>     qemu-system-x86_64: -chardev 
>> udp,id=chr0,localaddr=localhost,backend.data.remote.data.port=12345: 
>> Parameters 'backend.*' used inconsistently
>> 
>> Mission accomplished: confusion :)
>
> This one already fails before aliases do their work. The reason is that
> the default key for -chardev is 'backend', and QMP and CLI use the same
> name 'backend' for two completely different things.

Right.  I was confused (and the mission was truly accomplished).

> We could rename the default key into 'x-backend' and make it behave the
> same as 'backend', then the keyval parser would only fail when you
> explicitly wrote '-chardev backend=udp,...' and the problem is more
> obvious.

Technically a compatibility break, but we can hope that the longhand
form we change isn't used.

> By the way, we have a very similar issue with '-drive file=...', if we
> ever want to parse that one with the keyval parser. It can be both a
> string for the filename or an object for the configuration of the 'file'
> child that many block drivers have.

Should I be running for the hills?

>> Example: manual transformation leads to confusion #2
>> 
>> Confusion is possible even without tricking the user into mixing
>> "standard" and "alternate" explicitly:
>> 
>>     $ qemu-system-x86_64 -chardev udp,id=chr0,backend.data.remote.type=udp
>>     qemu-system-x86_64: -chardev udp,id=chr0,backend.data.remote.type=udp: 
>> Parameters 'backend.*' used inconsistently
>> 
>> Here, the user tried to stick to "standard", but forgot to specify a
>> required member.  The transformation machinery then "helpfully"
>> transformed nothing into something, which made the visitor throw up.
>
> Not the visitor, but the keyval parser. Same problem as above.
>
>> Clear error reporting is a critical part of a human-friendly interface.
>> We have two separate problems with it:
>> 
>> 1. The visitor reports errors as if aliases didn't exist
>> 
>>    Fixing this is "merely" a matter of tracing back alias applications.
>>    More complexity...
>> 
>> 2. The visitor reports errors as if manual transformation didn't exist
>> 
>>    Manual transformation can distort the users input beyond recognition.
>>    The visitor reports errors for the transformed input.
>> 
>>    To fix this one, we'd have to augment the parse tree so it points
>>    back at the actual user input, and then make the manual
>>    transformations preserve that.  Uff!
>
> Manual transformations are hard to write in a way that they give perfect
> results. As long as they are only used for legacy syntax and we expect
> users to move to a new way to spell things, this might be acceptable for
> a transition period until we remove the old syntax.

Valid point.

> In other cases, the easiest way is probably to duplicate even more of
> the schema and manually make sure that the visitor will accept the input
> before we transform it.
>
> The best way to avoid this is providing tools in QAPI that make manual
> transformations unnecessary.

Reducing the amount of handwritten code is good, as long as the price is
reasonable.  Complex code fetches a higher price.

I think there are a couple of ways to skin this cat.

0. Common to all ways: specify "standard" in the QAPI schema.  This
   specifies both the "standard" wire format, its parse tree
   (represented as QObject), and the "standard" C interface (C types,
   basically).

   Generic parsers parse into the parse tree.  The appropriate input
   visitor validates and converts to C types.

1. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), validate,
   then transform for the "standard" C interface.  Parsing and
   validating can fail, the transformation can't.

   Drawbacks:

   * We duplicate validation, which is a standing invitation for bugs.

   * Longwinded, handwritten transformation code.  Less vulnerable to
     bugs than validation code, I believe.

2. Parse "alternate" by any means (QemuOpts, keyval, ad hoc), transform
   to "standard" parse tree.

   Drawbacks:

   * Bad error messages from input visitor.

   * The handwritten transformation code is more longwinded than with 1.

3. Specify "alternate" in the QAPI schema.  Map "alternate" C interface
   to the "standard" one.

   Drawbacks:

   * QAPI schema code duplication

   * Longwinded, handwritten mapping code.

   This is what we did with SocketAddressLegacy.

4. Extend the QAPI schema language to let us specify non-standard wire
   format(s).  Use that to get the "alternate" wire format we need.

   Drawbacks:

   * QAPI schema language and generator become more complex.

   * Input visitor(s) become more complex.

   * Unless we accept "alternate" everywhere, we need a way to limit it
     to where it's actually needed.  More complexity.

   The concrete proposal on the table is aliases.  They are not a
   complete solution.  To solve the whole problem, we combine them with
   2.  We accept 4's drawbacks for a (sizable) reduction of 2's.
   Additionally, we accept "ghost" aliases.

5. Extend the QAPI schema language to let us specify "alternate" wire
   formats for "standard" types

   This is like 3, except the schema code is mostly referenced instead
   duplicated, and the mapping code is generated.  Something like
   "derive type T' from T with these members moved / renamed".

   Drawbacks

   * QAPI schema language and generator become more complex.

   * Unlike 4, we don't have working code.

   Like 4, this will likely solve only part of the problem.

Which one is the least bad?

If this was just about dragging deprecated interfaces to the end of
their grace period, I'd say "whatever is the least work", and that's
almost certainly whatever we have now, possibly hacked up to use the
appropriate internal interface.

Unfortunately, it is also about providing a friendlier interface to
humans "forever".

With 4 or 5, we invest into infrastructure once, then maintain it
forever, to make solving the problem easier and the result easier to
maintain.

Whether the investment will pay off depends on how big and bad the
problem is.  Hard to say.  One of the reasons we're looking at -chardev
now is that we believe it's the worst of the bunch.  But how big is the
bunch, and how bad are the others in there?

>> I'm afraid I need another round of thinking on how to best drag legacy
>> syntax along when we QAPIfy.
>
> Let me know when you've come to any conclusions (or more things to
> discuss).

Is 3 too awful to contemplate?