Eric Blake <ebl...@redhat.com> writes: > On 06/16/2016 07:40 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> We finally have all the required pieces for doing a type-safe >>> representation of netdev_add as a flat union, where the >>> discriminator 'type' now selects which additional members may >>> appear in the "arguments" JSON object sent over QMP, while >>> making no changes to the set of previously-valid QMP commands >>> that would work, and without breaking command line parsing. >> >> Isn't it amazing that you pulled this off without a compatibility break? > > No command line compatibility break, but in testing, I _did_ notice a > potential QMP break [it's hard to argue whether it is a break, given > that it was previously undocumented - I don't know if any QMP clients > were actually relying on loose behavior]: > > Pre-patch: > {'execute':'netdev_add', > 'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}} > {"return": {}} > {'execute':'netdev_add', > 'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}} > {"return": {}} > > Post-patch: > {'execute':'netdev_add', > 'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}} > {"return": {}} > {'execute':'netdev_add', > 'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}} > {"error": {"class": "GenericError", "desc": "Invalid parameter type > for 'hubid', expected: integer"}}
You're right. My brain keeps suppressing this ugly case. > I'm half-tempted to claim that we should update the QMP spec to say that > our parser is ALWAYS loose (anywhere a built-in scalar type is listed in > introspection, whether bool or integer, the parser will always accept an > equivalent string on input - but output will always be the named type), > and then relax qmp-input-visitor accordingly. In fact, danpb has > already proposed patches that allow "parse-string-as-int" as intentional > behavior, although under the guise of a new visitor rather than tweaking > qmp-input-visitor - so it just becomes a question of do we do it in > limited situations, or always. "Be liberal in what you accept" comes to > mind. I'm afraid we may indeed need a lose mode for backward compatibility of QMP commands that used to take a detour through the not-really-typed parts of the command line code (netdev_add now, device_add later). Nevertheless, I'm reluctant to adopt it wholesale. With a few decades of being "liberal in what you accept" under our belts, it doesn't sound like good advice anymore. All too often we've ended up with differently liberal implementations (both on the accepting and the sending side) to the point where nobody really knows what crap you have to accept for interoperability. Instead, I'd prefer a way for a QMP client to opt into a type-correct version of the command. Easiest way to do it is to add a new one, and deprecate the old one. > And as a followon thought: if we DO update the QMP spec to state that we > always accept a string in place of an integer, then we also have the > luxury of stating that accepting a string "inf" for a QAPI 'number' is > valid (even though strict JSON will not let us pass a bare-word inf) - > and that hits back on my proposal of whether we want to accept bare-word > inf on input as an extension, and whether outputting a string "inf" when > we specified a QAPI type of 'number' would be acceptable (since we would > be canonicalizing input "2" into output 2, going the other direction and > canonicalizing input inf into output "inf" is a bit easier to justify). I've come to the conclusion that the "can't express non-finite numbers in QMP" problem is basically irrelevant now. Solving it wouldn't really accomplish much, as we'd nevertheless want to avoid non-finite numbers for compatibility with older clients. There are very few places that can conceivably emit them anyway. Besides, if you don't like JSON as a transport encoding, then switching to one you like better makes a whole lot more sense to me than using it in unnatural ways like "if the string contents looks like a number (my definition of 'number', not JSON's), then it can also be a number, and whether it is depends on the context." > But given that it is soft freeze time, I guess we need to be > conservative at what changes we want to support at this phase of > development. Yup.