[Qemu-devel] [PATCH 05/19] tests/qapi-schema: Demonstrate insufficient 'if' checking

Markus Armbruster posted 19 patches 6 years, 1 month ago
Maintainers: Markus Armbruster <armbru@redhat.com>, Michael Roth <mdroth@linux.vnet.ibm.com>
[Qemu-devel] [PATCH 05/19] tests/qapi-schema: Demonstrate insufficient 'if' checking
Posted by Markus Armbruster 6 years, 1 month ago
Cover invalid 'if' in struct members, features, union and alternate
branches.  Four out of four are broken.  Mark FIXME.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile.include                        |  4 ++++
 .../alternate-branch-if-invalid.err           |  0
 .../alternate-branch-if-invalid.exit          |  1 +
 .../alternate-branch-if-invalid.json          |  4 ++++
 .../alternate-branch-if-invalid.out           | 16 +++++++++++++
 tests/qapi-schema/features-if-invalid.err     |  0
 tests/qapi-schema/features-if-invalid.exit    |  1 +
 tests/qapi-schema/features-if-invalid.json    |  5 ++++
 tests/qapi-schema/features-if-invalid.out     | 14 +++++++++++
 .../qapi-schema/struct-member-if-invalid.err  |  0
 .../qapi-schema/struct-member-if-invalid.exit |  1 +
 .../qapi-schema/struct-member-if-invalid.json |  4 ++++
 .../qapi-schema/struct-member-if-invalid.out  | 15 ++++++++++++
 tests/qapi-schema/union-branch-if-invalid.err |  0
 .../qapi-schema/union-branch-if-invalid.exit  |  1 +
 .../qapi-schema/union-branch-if-invalid.json  |  7 ++++++
 tests/qapi-schema/union-branch-if-invalid.out | 23 +++++++++++++++++++
 17 files changed, 96 insertions(+)
 create mode 100644 tests/qapi-schema/alternate-branch-if-invalid.err
 create mode 100644 tests/qapi-schema/alternate-branch-if-invalid.exit
 create mode 100644 tests/qapi-schema/alternate-branch-if-invalid.json
 create mode 100644 tests/qapi-schema/alternate-branch-if-invalid.out
 create mode 100644 tests/qapi-schema/features-if-invalid.err
 create mode 100644 tests/qapi-schema/features-if-invalid.exit
 create mode 100644 tests/qapi-schema/features-if-invalid.json
 create mode 100644 tests/qapi-schema/features-if-invalid.out
 create mode 100644 tests/qapi-schema/struct-member-if-invalid.err
 create mode 100644 tests/qapi-schema/struct-member-if-invalid.exit
 create mode 100644 tests/qapi-schema/struct-member-if-invalid.json
 create mode 100644 tests/qapi-schema/struct-member-if-invalid.out
 create mode 100644 tests/qapi-schema/union-branch-if-invalid.err
 create mode 100644 tests/qapi-schema/union-branch-if-invalid.exit
 create mode 100644 tests/qapi-schema/union-branch-if-invalid.json
 create mode 100644 tests/qapi-schema/union-branch-if-invalid.out

diff --git a/tests/Makefile.include b/tests/Makefile.include
index a11bde743e..c108a83076 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -295,6 +295,7 @@ check-qtest-generic-y += tests/test-hmp$(EXESUF)
 qapi-schema += alternate-any.json
 qapi-schema += alternate-array.json
 qapi-schema += alternate-base.json
+qapi-schema += alternate-branch-if-invalid.json
 qapi-schema += alternate-clash.json
 qapi-schema += alternate-conflict-dict.json
 qapi-schema += alternate-conflict-enum-bool.json
@@ -379,6 +380,7 @@ qapi-schema += event-member-invalid-dict.json
 qapi-schema += event-nest-struct.json
 qapi-schema += features-bad-type.json
 qapi-schema += features-duplicate-name.json
+qapi-schema += features-if-invalid.json
 qapi-schema += features-missing-name.json
 qapi-schema += features-name-bad-type.json
 qapi-schema += features-no-list.json
@@ -453,6 +455,7 @@ qapi-schema += string-code-point-127.json
 qapi-schema += struct-base-clash-deep.json
 qapi-schema += struct-base-clash.json
 qapi-schema += struct-data-invalid.json
+qapi-schema += struct-member-if-invalid.json
 qapi-schema += struct-member-invalid-dict.json
 qapi-schema += struct-member-invalid.json
 qapi-schema += trailing-comma-list.json
@@ -464,6 +467,7 @@ qapi-schema += unclosed-string.json
 qapi-schema += union-base-empty.json
 qapi-schema += union-base-no-discriminator.json
 qapi-schema += union-branch-case.json
+qapi-schema += union-branch-if-invalid.json
 qapi-schema += union-branch-invalid-dict.json
 qapi-schema += union-clash-branches.json
 qapi-schema += union-empty.json
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.err b/tests/qapi-schema/alternate-branch-if-invalid.err
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.exit b/tests/qapi-schema/alternate-branch-if-invalid.exit
new file mode 100644
index 0000000000..573541ac97
--- /dev/null
+++ b/tests/qapi-schema/alternate-branch-if-invalid.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.json b/tests/qapi-schema/alternate-branch-if-invalid.json
new file mode 100644
index 0000000000..6497f53475
--- /dev/null
+++ b/tests/qapi-schema/alternate-branch-if-invalid.json
@@ -0,0 +1,4 @@
+# Cover alternative with invalid 'if'
+# FIXME not rejected, would generate '#if  \n'
+{ 'alternate': 'Alt',
+  'data': { 'branch': { 'type': 'int', 'if': ' ' } } }
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.out b/tests/qapi-schema/alternate-branch-if-invalid.out
new file mode 100644
index 0000000000..89305d7f21
--- /dev/null
+++ b/tests/qapi-schema/alternate-branch-if-invalid.out
@@ -0,0 +1,16 @@
+module None
+object q_empty
+enum QType
+    prefix QTYPE
+    member none
+    member qnull
+    member qnum
+    member qstring
+    member qdict
+    member qlist
+    member qbool
+module alternate-branch-if-invalid.json
+alternate Alt
+    tag type
+    case branch: int
+        if [' ']
diff --git a/tests/qapi-schema/features-if-invalid.err b/tests/qapi-schema/features-if-invalid.err
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/features-if-invalid.exit b/tests/qapi-schema/features-if-invalid.exit
new file mode 100644
index 0000000000..573541ac97
--- /dev/null
+++ b/tests/qapi-schema/features-if-invalid.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/features-if-invalid.json b/tests/qapi-schema/features-if-invalid.json
new file mode 100644
index 0000000000..e6a524196d
--- /dev/null
+++ b/tests/qapi-schema/features-if-invalid.json
@@ -0,0 +1,5 @@
+# Cover feature with invalid 'if'
+# FIXME not rejected, misinterpreded as unconditional
+{ 'struct': 'Stru',
+  'data': {},
+  'features': [{'name': 'f', 'if': null }] }
diff --git a/tests/qapi-schema/features-if-invalid.out b/tests/qapi-schema/features-if-invalid.out
new file mode 100644
index 0000000000..9c2637baa3
--- /dev/null
+++ b/tests/qapi-schema/features-if-invalid.out
@@ -0,0 +1,14 @@
+module None
+object q_empty
+enum QType
+    prefix QTYPE
+    member none
+    member qnull
+    member qnum
+    member qstring
+    member qdict
+    member qlist
+    member qbool
+module features-if-invalid.json
+object Stru
+    feature f
diff --git a/tests/qapi-schema/struct-member-if-invalid.err b/tests/qapi-schema/struct-member-if-invalid.err
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/struct-member-if-invalid.exit b/tests/qapi-schema/struct-member-if-invalid.exit
new file mode 100644
index 0000000000..573541ac97
--- /dev/null
+++ b/tests/qapi-schema/struct-member-if-invalid.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/struct-member-if-invalid.json b/tests/qapi-schema/struct-member-if-invalid.json
new file mode 100644
index 0000000000..73987e04fc
--- /dev/null
+++ b/tests/qapi-schema/struct-member-if-invalid.json
@@ -0,0 +1,4 @@
+# Cover member with invalid 'if'
+# FIXME not rejected, would generate '#if True\n'
+{ 'struct': 'Stru',
+  'data': { 'member': { 'type': 'int', 'if': true } } }
diff --git a/tests/qapi-schema/struct-member-if-invalid.out b/tests/qapi-schema/struct-member-if-invalid.out
new file mode 100644
index 0000000000..8fbb97985c
--- /dev/null
+++ b/tests/qapi-schema/struct-member-if-invalid.out
@@ -0,0 +1,15 @@
+module None
+object q_empty
+enum QType
+    prefix QTYPE
+    member none
+    member qnull
+    member qnum
+    member qstring
+    member qdict
+    member qlist
+    member qbool
+module struct-member-if-invalid.json
+object Stru
+    member member: int optional=False
+        if [True]
diff --git a/tests/qapi-schema/union-branch-if-invalid.err b/tests/qapi-schema/union-branch-if-invalid.err
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/union-branch-if-invalid.exit b/tests/qapi-schema/union-branch-if-invalid.exit
new file mode 100644
index 0000000000..573541ac97
--- /dev/null
+++ b/tests/qapi-schema/union-branch-if-invalid.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/union-branch-if-invalid.json b/tests/qapi-schema/union-branch-if-invalid.json
new file mode 100644
index 0000000000..859b63b610
--- /dev/null
+++ b/tests/qapi-schema/union-branch-if-invalid.json
@@ -0,0 +1,7 @@
+# Cover branch with invalid 'if'
+# FIXME not rejected, would generate '#if \n'
+{ 'enum': 'Branches', 'data': ['branch1'] }
+{ 'struct': 'Stru', 'data': { 'member': 'str' } }
+{ 'union': 'Uni',
+  'base': { 'tag': 'Branches' }, 'discriminator': 'tag',
+  'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } }
diff --git a/tests/qapi-schema/union-branch-if-invalid.out b/tests/qapi-schema/union-branch-if-invalid.out
new file mode 100644
index 0000000000..2ed43218af
--- /dev/null
+++ b/tests/qapi-schema/union-branch-if-invalid.out
@@ -0,0 +1,23 @@
+module None
+object q_empty
+enum QType
+    prefix QTYPE
+    member none
+    member qnull
+    member qnum
+    member qstring
+    member qdict
+    member qlist
+    member qbool
+module union-branch-if-invalid.json
+enum Branches
+    member branch1
+object Stru
+    member member: str optional=False
+object q_obj_Uni-base
+    member tag: Branches optional=False
+object Uni
+    base q_obj_Uni-base
+    tag tag
+    case branch1: Stru
+        if ['']
-- 
2.21.0


Re: [Qemu-devel] [PATCH 05/19] tests/qapi-schema: Demonstrate insufficient 'if' checking
Posted by Eric Blake 6 years, 1 month ago
On 9/14/19 10:34 AM, Markus Armbruster wrote:
> Cover invalid 'if' in struct members, features, union and alternate
> branches.  Four out of four are broken.  Mark FIXME.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Embarrassing. But the fact that you're pointing them out presumably
means that you fix it later in the series ;)

> +++ b/tests/qapi-schema/features-if-invalid.json
> @@ -0,0 +1,5 @@
> +# Cover feature with invalid 'if'
> +# FIXME not rejected, misinterpreded as unconditional

misinterpreted

With the typo fix,

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/tests/qapi-schema/struct-member-if-invalid.json
> @@ -0,0 +1,4 @@
> +# Cover member with invalid 'if'
> +# FIXME not rejected, would generate '#if True\n'

Which might actually compile, depending on what else is present in
various headers!  But certainly not what was intended.

> +++ b/tests/qapi-schema/union-branch-if-invalid.json
> @@ -0,0 +1,7 @@
> +# Cover branch with invalid 'if'
> +# FIXME not rejected, would generate '#if \n'
> +{ 'enum': 'Branches', 'data': ['branch1'] }
> +{ 'struct': 'Stru', 'data': { 'member': 'str' } }
> +{ 'union': 'Uni',
> +  'base': { 'tag': 'Branches' }, 'discriminator': 'tag',
> +  'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } }

So you're pointing out a difference between an empty string and a string
not containing a C macro name (possibly because later patches will give
them different error messages).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 05/19] tests/qapi-schema: Demonstrate insufficient 'if' checking
Posted by Markus Armbruster 6 years, 1 month ago
Eric Blake <eblake@redhat.com> writes:

> On 9/14/19 10:34 AM, Markus Armbruster wrote:
>> Cover invalid 'if' in struct members, features, union and alternate
>> branches.  Four out of four are broken.  Mark FIXME.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Embarrassing. But the fact that you're pointing them out presumably
> means that you fix it later in the series ;)

Yes:
[PATCH 09/19] qapi: Remove null from schema language
[PATCH 12/19] qapi: Fix missing 'if' checks in struct, union, alternate 'data'

>> +++ b/tests/qapi-schema/features-if-invalid.json
>> @@ -0,0 +1,5 @@
>> +# Cover feature with invalid 'if'
>> +# FIXME not rejected, misinterpreded as unconditional
>
> misinterpreted
>
> With the typo fix,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

>> +++ b/tests/qapi-schema/struct-member-if-invalid.json
>> @@ -0,0 +1,4 @@
>> +# Cover member with invalid 'if'
>> +# FIXME not rejected, would generate '#if True\n'
>
> Which might actually compile, depending on what else is present in
> various headers!  But certainly not what was intended.
>
>> +++ b/tests/qapi-schema/union-branch-if-invalid.json
>> @@ -0,0 +1,7 @@
>> +# Cover branch with invalid 'if'
>> +# FIXME not rejected, would generate '#if \n'
>> +{ 'enum': 'Branches', 'data': ['branch1'] }
>> +{ 'struct': 'Stru', 'data': { 'member': 'str' } }
>> +{ 'union': 'Uni',
>> +  'base': { 'tag': 'Branches' }, 'discriminator': 'tag',
>> +  'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } }
>
> So you're pointing out a difference between an empty string and a string
> not containing a C macro name (possibly because later patches will give
> them different error messages).

Not sure I got this comment.

Re: [Qemu-devel] [PATCH 05/19] tests/qapi-schema: Demonstrate insufficient 'if' checking
Posted by Eric Blake 6 years, 1 month ago
On 9/23/19 6:55 AM, Markus Armbruster wrote:

>>> +++ b/tests/qapi-schema/union-branch-if-invalid.json
>>> @@ -0,0 +1,7 @@
>>> +# Cover branch with invalid 'if'
>>> +# FIXME not rejected, would generate '#if \n'
>>> +{ 'enum': 'Branches', 'data': ['branch1'] }
>>> +{ 'struct': 'Stru', 'data': { 'member': 'str' } }
>>> +{ 'union': 'Uni',
>>> +  'base': { 'tag': 'Branches' }, 'discriminator': 'tag',
>>> +  'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } }
>>
>> So you're pointing out a difference between an empty string and a string
>> not containing a C macro name (possibly because later patches will give
>> them different error messages).
> 
> Not sure I got this comment.

I was comparing:

> +++ b/tests/qapi-schema/union-branch-if-invalid.json
> +  'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } }

with:

> +++ b/tests/qapi-schema/alternate-branch-if-invalid.json
> +  'data': { 'branch': { 'type': 'int', 'if': ' ' } } }

Both of which produce invalid expansions, but because of the difference
between empty string vs. all-whitespace might be fixed differently in
later patches.  At any rate, nothing to change in those tests.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org