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

[Kevin Wolf <kwolf@redhat.com>]

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.

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.

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

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

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

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

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.

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.

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

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.

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

Kevin