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

[Eric Blake]

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"}}

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.

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

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature