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

[Markus Armbruster]

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!

> diff --git a/tests/Makefile b/tests/Makefile
> index 5e01952..6fe34f7 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -203,8 +203,12 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>       double-data.json unknown-expr-key.json redefined-type.json \
>       redefined-command.json redefined-builtin.json redefined-event.json \
>       type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
> -     missing-colon.json missing-comma-list.json \
> -     missing-comma-object.json non-objects.json \
> +     data-array-empty.json data-array-unknown.json data-int.json \
> +     data-unknown.json data-member-unknown.json data-member-array.json \
> +     data-member-array-bad.json returns-array-bad.json returns-int.json \
> +     returns-unknown.json missing-colon.json missing-comma-list.json \
> +     missing-comma-object.json nested-struct-data.json \
> +     nested-struct-returns.json non-objects.json \
>       qapi-schema-test.json quoted-structural-chars.json \
>       trailing-comma-list.json trailing-comma-object.json \
>       unclosed-list.json unclosed-object.json unclosed-string.json \
> diff --git a/tests/qapi-schema/data-array-empty.err 
> b/tests/qapi-schema/data-array-empty.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-array-empty.exit 
> b/tests/qapi-schema/data-array-empty.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-array-empty.json 
> b/tests/qapi-schema/data-array-empty.json
> new file mode 100644
> index 0000000..41b6c1e
> --- /dev/null
> +++ 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'?

For what it's worth, docs/qmp/qmp-spec.txt specifies:

    2.3 Issuing Commands
    --------------------

    The format for command execution is:

    { "execute": json-string, "arguments": json-object, "id": json-value }

     Where,

    - The "execute" member identifies the command to be executed by the Server
    - The "arguments" member is used to pass any arguments required for the
      execution of the command, it is optional when no arguments are required
    - The "id" member is a transaction identification associated with the
      command execution, it is optional and will be part of the response if
      provided

The QAPI schema's 'data' is "arguments" on the wire.

Semantically, 'data' of a complex type / json-object "arguments" is an
ordered list of named parameters.  Makes obvious sense.

'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?

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.

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

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.

> diff --git a/tests/qapi-schema/data-array-empty.out 
> b/tests/qapi-schema/data-array-empty.out
> new file mode 100644
> index 0000000..67802be
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', [])])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/data-array-unknown.err 
> b/tests/qapi-schema/data-array-unknown.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-array-unknown.exit 
> b/tests/qapi-schema/data-array-unknown.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-unknown.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-array-unknown.json 
> b/tests/qapi-schema/data-array-unknown.json
> new file mode 100644
> index 0000000..434fb5f
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-unknown.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject an array for data if it does not contain a known 
> type
> +{ 'command': 'oops', 'data': [ 'NoSuchType' ] }
> diff --git a/tests/qapi-schema/data-array-unknown.out 
> b/tests/qapi-schema/data-array-unknown.out
> new file mode 100644
> index 0000000..c3bc05e
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-unknown.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', ['NoSuchType'])])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/data-int.err b/tests/qapi-schema/data-int.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-int.exit b/tests/qapi-schema/data-int.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-int.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-int.json b/tests/qapi-schema/data-int.json
> new file mode 100644
> index 0000000..37916e0
> --- /dev/null
> +++ b/tests/qapi-schema/data-int.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject commands where data is not an array or complex type
> +{ 'command': 'oops', 'data': 'int' }
> diff --git a/tests/qapi-schema/data-int.out b/tests/qapi-schema/data-int.out
> new file mode 100644
> index 0000000..e589a4f
> --- /dev/null
> +++ b/tests/qapi-schema/data-int.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', 'int')])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/data-member-array-bad.err 
> b/tests/qapi-schema/data-member-array-bad.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-member-array-bad.exit 
> b/tests/qapi-schema/data-member-array-bad.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-array-bad.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-member-array-bad.json 
> b/tests/qapi-schema/data-member-array-bad.json
> new file mode 100644
> index 0000000..c954af1
> --- /dev/null
> +++ 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'".

> diff --git a/tests/qapi-schema/data-member-array-bad.out 
> b/tests/qapi-schema/data-member-array-bad.out
> new file mode 100644
> index 0000000..0e00c41
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-array-bad.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', OrderedDict([('member', 
> [OrderedDict([('nested', 'str')])])]))])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/data-member-array.err 
> b/tests/qapi-schema/data-member-array.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-member-array.exit 
> b/tests/qapi-schema/data-member-array.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-array.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-member-array.json 
> b/tests/qapi-schema/data-member-array.json
> new file mode 100644
> index 0000000..7cce276
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-array.json
> @@ -0,0 +1,4 @@
> +# valid array members
> +{ 'enum': 'abc', 'data': [ 'a', 'b', 'c' ] }
> +{ 'type': 'def', 'data': { 'array': [ 'abc' ] } }
> +{ 'command': 'okay', 'data': { 'member1': [ 'int' ], 'member2': [ 'def' ] } }
> diff --git a/tests/qapi-schema/data-member-array.out 
> b/tests/qapi-schema/data-member-array.out
> new file mode 100644
> index 0000000..8287120
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-array.out
> @@ -0,0 +1,5 @@
> +[OrderedDict([('enum', 'abc'), ('data', ['a', 'b', 'c'])]),
> + OrderedDict([('type', 'def'), ('data', OrderedDict([('array', ['abc'])]))]),
> + OrderedDict([('command', 'okay'), ('data', OrderedDict([('member1', 
> ['int']), ('member2', ['def'])]))])]
> +[{'enum_name': 'abc', 'enum_values': ['a', 'b', 'c']}]
> +[OrderedDict([('type', 'def'), ('data', OrderedDict([('array', ['abc'])]))])]
> diff --git a/tests/qapi-schema/data-member-unknown.err 
> b/tests/qapi-schema/data-member-unknown.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-member-unknown.exit 
> b/tests/qapi-schema/data-member-unknown.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-unknown.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-member-unknown.json 
> b/tests/qapi-schema/data-member-unknown.json
> new file mode 100644
> index 0000000..40e6252
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-unknown.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject data if it does not contain a known type
> +{ 'command': 'oops', 'data': { 'member': 'NoSuchType' } }
> diff --git a/tests/qapi-schema/data-member-unknown.out 
> b/tests/qapi-schema/data-member-unknown.out
> new file mode 100644
> index 0000000..36252a5
> --- /dev/null
> +++ b/tests/qapi-schema/data-member-unknown.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', OrderedDict([('member', 
> 'NoSuchType')]))])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/data-unknown.err 
> b/tests/qapi-schema/data-unknown.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-unknown.exit 
> b/tests/qapi-schema/data-unknown.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-unknown.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-unknown.json 
> b/tests/qapi-schema/data-unknown.json
> new file mode 100644
> index 0000000..776bd34
> --- /dev/null
> +++ b/tests/qapi-schema/data-unknown.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject data if it does not contain a known type
> +{ 'command': 'oops', 'data': 'NoSuchType' }
> diff --git a/tests/qapi-schema/data-unknown.out 
> b/tests/qapi-schema/data-unknown.out
> new file mode 100644
> index 0000000..2c60726
> --- /dev/null
> +++ b/tests/qapi-schema/data-unknown.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', 'NoSuchType')])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/nested-struct-data.err 
> b/tests/qapi-schema/nested-struct-data.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/nested-struct-data.exit 
> b/tests/qapi-schema/nested-struct-data.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/nested-struct-data.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/nested-struct-data.json 
> b/tests/qapi-schema/nested-struct-data.json
> new file mode 100644
> index 0000000..0247c8c
> --- /dev/null
> +++ b/tests/qapi-schema/nested-struct-data.json
> @@ -0,0 +1,4 @@
> +# FIXME: inline subtypes collide with our desired future use of defaults
> +{ 'command': 'foo',
> +  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' },
> +  'returns': {} }
> diff --git a/tests/qapi-schema/nested-struct-data.out 
> b/tests/qapi-schema/nested-struct-data.out
> new file mode 100644
> index 0000000..999cbb8
> --- /dev/null
> +++ b/tests/qapi-schema/nested-struct-data.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'foo'), ('data', OrderedDict([('a', 
> OrderedDict([('string', 'str'), ('integer', 'int')])), ('b', 'str')])), 
> ('returns', OrderedDict())])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/nested-struct-returns.err 
> b/tests/qapi-schema/nested-struct-returns.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/nested-struct-returns.exit 
> b/tests/qapi-schema/nested-struct-returns.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/nested-struct-returns.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/nested-struct-returns.json 
> b/tests/qapi-schema/nested-struct-returns.json
> new file mode 100644
> index 0000000..5a46840
> --- /dev/null
> +++ b/tests/qapi-schema/nested-struct-returns.json
> @@ -0,0 +1,3 @@
> +# FIXME: inline subtypes collide with our desired future use of defaults
> +{ 'command': 'foo',
> +  'returns': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }
> diff --git a/tests/qapi-schema/nested-struct-returns.out 
> b/tests/qapi-schema/nested-struct-returns.out
> new file mode 100644
> index 0000000..c53d23b
> --- /dev/null
> +++ b/tests/qapi-schema/nested-struct-returns.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'foo'), ('returns', OrderedDict([('a', 
> OrderedDict([('string', 'str'), ('integer', 'int')])), ('b', 'str')]))])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/returns-array-bad.err 
> b/tests/qapi-schema/returns-array-bad.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/returns-array-bad.exit 
> b/tests/qapi-schema/returns-array-bad.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/returns-array-bad.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/returns-array-bad.json 
> b/tests/qapi-schema/returns-array-bad.json
> new file mode 100644
> index 0000000..14882c1
> --- /dev/null
> +++ b/tests/qapi-schema/returns-array-bad.json
> @@ -0,0 +1,2 @@
> +# 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'?

For what it's worth, docs/qmp/qmp-spec.txt specifies:

    2.4.1 success
    -------------

    The format of a success response is:

    { "return": json-object, "id": json-value }

     Where,

    - The "return" member contains the command returned data, which is defined
      in a per-command basis or an empty json-object if the command does not
      return data
    - The "id" member contains the transaction identification associated
      with the command execution if issued by the Client

The QAPI schema's 'returns' becomes "return" on the wire.  We suck.

qmp-spec.txt is *wrong*!  We actually use json-array in addition to
json-object.

Similar argument on types wanted as for 'data' / "arguments" above.  I
think we should permit exactly the same QAPI types, plus lists.

> diff --git a/tests/qapi-schema/returns-array-bad.out 
> b/tests/qapi-schema/returns-array-bad.out
> new file mode 100644
> index 0000000..eccad55
> --- /dev/null
> +++ b/tests/qapi-schema/returns-array-bad.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('returns', ['str', 'str'])])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/returns-int.err 
> b/tests/qapi-schema/returns-int.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/returns-int.exit 
> b/tests/qapi-schema/returns-int.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/returns-int.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/returns-int.json 
> b/tests/qapi-schema/returns-int.json
> new file mode 100644
> index 0000000..7888fb1
> --- /dev/null
> +++ b/tests/qapi-schema/returns-int.json
> @@ -0,0 +1,2 @@
> +# It is okay (although not extensible) to return a non-dictionary
> +{ 'command': 'okay', 'returns': 'int' }
> diff --git a/tests/qapi-schema/returns-int.out 
> b/tests/qapi-schema/returns-int.out
> new file mode 100644
> index 0000000..36b00a9
> --- /dev/null
> +++ b/tests/qapi-schema/returns-int.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'okay'), ('returns', 'int')])]
> +[]
> +[]
> diff --git a/tests/qapi-schema/returns-unknown.err 
> b/tests/qapi-schema/returns-unknown.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/returns-unknown.exit 
> b/tests/qapi-schema/returns-unknown.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/returns-unknown.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/returns-unknown.json 
> b/tests/qapi-schema/returns-unknown.json
> new file mode 100644
> index 0000000..61f20eb
> --- /dev/null
> +++ b/tests/qapi-schema/returns-unknown.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject returns if it does not contain a known type
> +{ 'command': 'oops', 'returns': 'NoSuchType' }
> diff --git a/tests/qapi-schema/returns-unknown.out 
> b/tests/qapi-schema/returns-unknown.out
> new file mode 100644
> index 0000000..a482c83
> --- /dev/null
> +++ 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.