Re: [Qemu-devel] [PATCH v7 13/15] net: Complete qapi-fication of netdev_add

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/16/2016 07:40 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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.