On 09/25/2014 01:34 AM, Markus Armbruster wrote: > Eric Blake 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 >> --- >> 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