[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests |
Date: |
Thu, 25 Sep 2014 18:12:40 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
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!
- Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types, (continued)
[Qemu-devel] [PATCH v4 19/19] qapi: Drop support for inline subtypes, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 06/19] qapi: Better error messages for bad enums, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 18/19] qapi: Drop inline subtype in query-pci, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 15/19] qapi: Merge UserDefTwo and UserDefNested in tests, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass, Eric Blake, 2014/09/19