Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/25/2014 01:34 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Demonstrate that the qapi generator silently parses confusing
>>> types, which may cause other errors later on. Later patches
>>> will update the expected results as the generator is made stricter.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>> ---
>>>  tests/Makefile                               | 8 ++++++--
>>>  tests/qapi-schema/data-array-empty.err       | 0
>>>  tests/qapi-schema/data-array-empty.exit      | 1 +
>>>  tests/qapi-schema/data-array-empty.json      | 2 ++
>> [Twelve new tests...]
>>>  create mode 100644 tests/qapi-schema/returns-unknown.err
>>>  create mode 100644 tests/qapi-schema/returns-unknown.exit
>>>  create mode 100644 tests/qapi-schema/returns-unknown.json
>>>  create mode 100644 tests/qapi-schema/returns-unknown.out
>> 
>> Holy moly!
>
> Yeah, this series cleans up a lot of cruft, which means a lot of testing.

Very much appreciated.

>>> +++ b/tests/qapi-schema/data-array-empty.json
>>> @@ -0,0 +1,2 @@
>>> +# FIXME: we should reject an array for data if it does not contain
>>> a known type
>>> +{ 'command': 'oops', 'data': [ ] }
>> 
>> Do we want to permit anything but a complex type for 'data'?
>
> Oh, good question.  Probably not (I just tested, and nothing already
> does that).  I'll tighten it in v5 (mostly doc changes, plus a one-liner
> in 13/19 to remove allow_array=True when calling check_type for a
> command data member).

Yes, please.

>> For what it's worth, docs/qmp/qmp-spec.txt specifies:
>
> Ooh, I probably ought to skim that file when making my doc improvements
> on the front end of the series.
>
>> 'data' of list type / json-array "arguments" is an ordered list of
>> unnamed parameters.  Makes sense, but it isn't how QMP works.  Or C for
>> that matter.  Do we really want to support this in QAPI?
>
> No existing command takes a non-dict for "arguments", and the generator
> probably chokes on it.

Let's stick to dict arguments.

>> If yes, then 'data': [] means the same thing as 'data': {} or no 'data':
>> no arguments.
>> 
>> Aside: discussion of list types in qapi-code-gen.txt is spread over the
>> places that use them.  I feel giving them their own section on the same
>> level as complex types etc. would make sense.
>
> Good idea, will do in v5.
>
>> 
>> 'data' of built-in or enumeration type / json-number or json-string
>> "arguments" could be regarded as a single unnamed parameter.  Even if we
>> want unnamed parameters, the question remains whether we want two
>> syntactic forms for a single unnamed parameter, boxed in a [ ] and
>> unboxed.  I doubt it.
>
> No. We don't (patch 13/19 already forbids them, with no violators
> found).  It's not extensible (well, maybe it is by having some way to
> mark a dict so that at most one of its keys is the default key to be
> implied when used in a non-dict form, and all other keys being optional,
> but that's ugly).

Agreed.

>> One kind of types left to discuss: unions.  I figure the semantics of a
>> simple or flat union type are obvious enough, so we can discuss whether
>> we want them.  Anonymous unions are a different matter, because they
>> boil down to a single parameter that need not be json-object.
>
> Oooh, I didn't even consider anon unions.  We absolutely need to support
> { 'command': 'foo', 'data': 'FlatUnion' } (blockdev-add, anyone), but
> you are probably right that we don't want to support { 'command': 'foo',
> 'data': 'AnonUnion'}, because it would allow "arguments" to be a
> non-dictionary (unless the AnonUnion has only a dict branch, but then
> why make it a union?).  I'll have to make qapi.py be smarter about
> regular vs. anon unions - it might be easier by using an actual
> different keyword for anon unions, because they are so different in
> nature.  (Generated code will be the same, just a tweak to the qapi
> representation and to qapi.py).  I'll play with that for v5.

Okay :)

>>> +++ b/tests/qapi-schema/data-member-array-bad.json
>>> @@ -0,0 +1,2 @@
>>> +# FIXME: we should reject data if it does not contain a valid array type
>>> +{ 'command': 'oops', 'data': { 'member': [ { 'nested': 'str' } ] } }
>> 
>> I'm probably just suffering from temporary denseness here... why is this
>> example problematic?  To me, it looks like a single argument 'member' of
>> type "array of complex type with a single member 'nested' of
>> builtin-type 'str'".
>
> The generator does not have a way to produce a List of an unnamed type.
>  All lists are of named types (or rather, every creation of a named type
> generates code for both that type and its list counterpart).  Maybe we
> eventually want to support an array of an anonymous type, but the
> generator doesn't handle it now.  So it was easier to forbid it when
> writing 13/19.

I see.  We already accepted restricting nested structs (see series
subject), and this is just one instance.

>>> +# FIXME: we should reject an array return that is not a single type
>>> +{ 'command': 'oops', 'returns': [ 'str', 'str' ] }
>> 
>> Do we want to permit anything but a complex type for 'returns'?
>
> Sadly, yes.  We have existing commands that do just that.  I already
> documented that new commands should avoid it (it's not extensible).

If we care, we can whitelist the existing offenders, and reject new
offenders.

>> The QAPI schema's 'returns' becomes "return" on the wire.  We suck.
>
> We could search-and-replace the schema, but why bother.  Yeah, the
> discrepancy is a bit annoying; on the other hand, it makes it easy to
> tell schema apart from on-the-wire samples, at least for commands that
> return something :)
>
>> 
>> qmp-spec.txt is *wrong*!  We actually use json-array in addition to
>> json-object.
>
> Yep, added to my list of doc improvements for v5.
>
>
>>> +++ b/tests/qapi-schema/returns-unknown.out
>>> @@ -0,0 +1,3 @@
>>> +[OrderedDict([('command', 'oops'), ('returns', 'NoSuchType')])]
>>> +[]
>>> +[]
>> 
>> scripts/qapi* is a sick joke when it comes to semantic analysis.
>
> That gets a lot better in 13/19 :)

Will review as soon as I can!