Markus Armbruster <arm...@redhat.com> writes:
> Kevin Wolf <kw...@redhat.com> writes:
>
>> Am 11.10.2021 um 09:44 hat Markus Armbruster geschrieben:
>>> Kevin Wolf <kw...@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.
I got one more:
6. Stir more sugar into the input visitor we use with keyval_parse():
- Recognze unique suffix of full key. Example: "ipv6" as
abbreviation of "backend.data.remote.data.ipv4".
- Default union type tag when variant members of exactly one branch
are present. Example: when we got "backend.data.addr.data.host",
"backend.data.addr.type" defaults to "inet".
Beware, this is even hairier than it may look. For instance, we want
to expand "host=playground.redhat.com,port=12345" into
backend.type=socket
backend.data.addr.type=inet
backend.data.addr.data.host=playground.redhat.com
backend.data.addr.data.port=12345
and not into
backend.type=udp
backend.data.remote.type=inet
backend.data.remote.data.host=playground.redhat.com
backend.data.remote.data.port=12345
I'm afraid this idea also solves 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?
