qapi: Enforce naming rules

[PATCH 06/28] tests/qapi-schema: Tweak to demonstrate buggy member name check

Posted by Markus Armbruster 4 years, 10 months ago

Member name 'u' and names starting with 'has-' or 'has_' are reserved
for the generator.  check_type() enforces this, covered by tests
reserved-member-u and reserved-member-has.

These tests neglect to cover optional members, where the name starts
with '*'.  Tweak reserved-member-u to fix that.

This demonstrates the reserved member name check is broken for
optional members.  The next commit will fix it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/reserved-member-u.err  |  2 --
 tests/qapi-schema/reserved-member-u.json |  3 ++-
 tests/qapi-schema/reserved-member-u.out  | 14 ++++++++++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err
index 231d552494..e69de29bb2 100644
--- a/tests/qapi-schema/reserved-member-u.err
+++ b/tests/qapi-schema/reserved-member-u.err
@@ -1,2 +0,0 @@
-reserved-member-u.json: In struct 'Oops':
-reserved-member-u.json:7: 'data' member 'u' uses reserved name
diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json
index 1eaf0f301c..15005abb09 100644
--- a/tests/qapi-schema/reserved-member-u.json
+++ b/tests/qapi-schema/reserved-member-u.json
@@ -4,4 +4,5 @@
 # This is true even for non-unions, because it is possible to convert a
 # struct to flat union while remaining backwards compatible in QMP.
 # TODO - we could munge the member name to 'q_u' to avoid the collision
-{ 'struct': 'Oops', 'data': { 'u': 'str' } }
+# BUG: not rejected
+{ 'struct': 'Oops', 'data': { '*u': 'str' } }
diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out
index e69de29bb2..6a3705518b 100644
--- a/tests/qapi-schema/reserved-member-u.out
+++ b/tests/qapi-schema/reserved-member-u.out
@@ -0,0 +1,14 @@
+module ./builtin
+object q_empty
+enum QType
+    prefix QTYPE
+    member none
+    member qnull
+    member qnum
+    member qstring
+    member qdict
+    member qlist
+    member qbool
+module reserved-member-u.json
+object Oops
+    member u: str optional=True
-- 
2.26.3

Re: [PATCH 06/28] tests/qapi-schema: Tweak to demonstrate buggy member name check

Posted by John Snow 4 years, 10 months ago

On 3/23/21 5:40 AM, Markus Armbruster wrote:
> Member name 'u' and names starting with 'has-' or 'has_' are reserved
> for the generator.  check_type() enforces this, covered by tests
> reserved-member-u and reserved-member-has.
> 
> These tests neglect to cover optional members, where the name starts
> with '*'.  Tweak reserved-member-u to fix that.
> 
> This demonstrates the reserved member name check is broken for
> optional members.  The next commit will fix it.
> 

The test without an optional member goes away. Do we lose coverage? (Do 
we care?)

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/qapi-schema/reserved-member-u.err  |  2 --
>   tests/qapi-schema/reserved-member-u.json |  3 ++-
>   tests/qapi-schema/reserved-member-u.out  | 14 ++++++++++++++
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err
> index 231d552494..e69de29bb2 100644
> --- a/tests/qapi-schema/reserved-member-u.err
> +++ b/tests/qapi-schema/reserved-member-u.err
> @@ -1,2 +0,0 @@
> -reserved-member-u.json: In struct 'Oops':
> -reserved-member-u.json:7: 'data' member 'u' uses reserved name
> diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json
> index 1eaf0f301c..15005abb09 100644
> --- a/tests/qapi-schema/reserved-member-u.json
> +++ b/tests/qapi-schema/reserved-member-u.json
> @@ -4,4 +4,5 @@
>   # This is true even for non-unions, because it is possible to convert a
>   # struct to flat union while remaining backwards compatible in QMP.
>   # TODO - we could munge the member name to 'q_u' to avoid the collision
> -{ 'struct': 'Oops', 'data': { 'u': 'str' } }
> +# BUG: not rejected
> +{ 'struct': 'Oops', 'data': { '*u': 'str' } }
> diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out
> index e69de29bb2..6a3705518b 100644
> --- a/tests/qapi-schema/reserved-member-u.out
> +++ b/tests/qapi-schema/reserved-member-u.out
> @@ -0,0 +1,14 @@
> +module ./builtin
> +object q_empty
> +enum QType
> +    prefix QTYPE
> +    member none
> +    member qnull
> +    member qnum
> +    member qstring
> +    member qdict
> +    member qlist
> +    member qbool
> +module reserved-member-u.json
> +object Oops
> +    member u: str optional=True
>

Re: [PATCH 06/28] tests/qapi-schema: Tweak to demonstrate buggy member name check

Posted by Markus Armbruster 4 years, 10 months ago

John Snow <jsnow@redhat.com> writes:

> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>> Member name 'u' and names starting with 'has-' or 'has_' are reserved
>> for the generator.  check_type() enforces this, covered by tests
>> reserved-member-u and reserved-member-has.
>> These tests neglect to cover optional members, where the name starts
>> with '*'.  Tweak reserved-member-u to fix that.
>> This demonstrates the reserved member name check is broken for
>> optional members.  The next commit will fix it.
>> 
>
> The test without an optional member goes away. Do we lose coverage?
> (Do we care?)

Up to a point :)  We do try to cover all failure modes, just not in all
contexts.

The test is about this error:

         if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
             raise QAPISemError(info, "%s uses reserved name" % key_source)

Full matrix: (is "u", starts with "has_") x (optional, not optional).

Instead of covering all four cases, we cover two: non-optional "u"
(reserved-member-u) and non-optional "has-" (reserved-member-has).

The patch flips the former to optional.  The latter still covers
non-optional.

Good enough, I think.

Do you feel I should point to reserved-member-has in the commit message?

Re: [PATCH 06/28] tests/qapi-schema: Tweak to demonstrate buggy member name check

Posted by John Snow 4 years, 10 months ago

On 3/23/21 11:44 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>>> Member name 'u' and names starting with 'has-' or 'has_' are reserved
>>> for the generator.  check_type() enforces this, covered by tests
>>> reserved-member-u and reserved-member-has.
>>> These tests neglect to cover optional members, where the name starts
>>> with '*'.  Tweak reserved-member-u to fix that.
>>> This demonstrates the reserved member name check is broken for
>>> optional members.  The next commit will fix it.
>>>
>>
>> The test without an optional member goes away. Do we lose coverage?
>> (Do we care?)
> 
> Up to a point :)  We do try to cover all failure modes, just not in all
> contexts.
> 
> The test is about this error:
> 
>           if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
>               raise QAPISemError(info, "%s uses reserved name" % key_source)
> 
> Full matrix: (is "u", starts with "has_") x (optional, not optional).
> 
> Instead of covering all four cases, we cover two: non-optional "u"
> (reserved-member-u) and non-optional "has-" (reserved-member-has).
> 
> The patch flips the former to optional.  The latter still covers
> non-optional.
> 
> Good enough, I think.
> 

Relies a tiny bit on knowing these two reserved name checks are 
implemented in the same place. Doubt it'll matter practically. Coverage 
has increased overall.

> Do you feel I should point to reserved-member-has in the commit message?
> 

It'd be for my benefit, but you also already just explained it to me.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH 06/28] tests/qapi-schema: Tweak to demonstrate buggy member name check

Posted by Markus Armbruster 4 years, 10 months ago

John Snow <jsnow@redhat.com> writes:

> On 3/23/21 11:44 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 3/23/21 5:40 AM, Markus Armbruster wrote:
>>>> Member name 'u' and names starting with 'has-' or 'has_' are reserved
>>>> for the generator.  check_type() enforces this, covered by tests
>>>> reserved-member-u and reserved-member-has.
>>>> These tests neglect to cover optional members, where the name starts
>>>> with '*'.  Tweak reserved-member-u to fix that.
>>>> This demonstrates the reserved member name check is broken for
>>>> optional members.  The next commit will fix it.
>>>>
>>>
>>> The test without an optional member goes away. Do we lose coverage?
>>> (Do we care?)
>> Up to a point :)  We do try to cover all failure modes, just not in
>> all
>> contexts.
>> The test is about this error:
>>           if c_name(key, False) == 'u' or c_name(key,
>> False).startswith('has_'):
>>               raise QAPISemError(info, "%s uses reserved name" % key_source)
>> Full matrix: (is "u", starts with "has_") x (optional, not
>> optional).
>> Instead of covering all four cases, we cover two: non-optional "u"
>> (reserved-member-u) and non-optional "has-" (reserved-member-has).
>> The patch flips the former to optional.  The latter still covers
>> non-optional.
>> Good enough, I think.
>> 
>
> Relies a tiny bit on knowing these two reserved name checks are
> implemented in the same place. Doubt it'll matter
> practically. Coverage has increased overall.
>
>> Do you feel I should point to reserved-member-has in the commit message?
>> 
>
> It'd be for my benefit, but you also already just explained it to me.

Amending the second paragraph:

    These tests neglect to cover optional members, where the name starts
    with '*'.  Tweak reserved-member-u to fix that.  Test
    reserved-member-has still covers non-optional members.


> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!