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

[Markus Armbruster]

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.

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.


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.


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.

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


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.


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!

I'm afraid I need another round of thinking on how to best drag legacy
syntax along when we QAPIfy.

[...]