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

[Eric Blake]

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.

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

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

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

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


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


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


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

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

signature.asc
Description: OpenPGP digital signature