Re: [PATCH 07/28] qapi: Fix to reject optional members with reserved names

[John Snow]

On 3/23/21 5:40 AM, Markus Armbruster wrote:
> check_type() fails to reject optional members with reserved names,
> because it neglects to strip off the leading '*'.  Fix that.
>
> The stripping in check_name_str() is now useless.  Drop.
>
> Also drop the "no leading '*'" assertion, because valid_name.match()
> ensures it can't fail.

(Yep, I noticed that, but assumed that it made someone feel safe, so I 
left it!)

> Fixes: 9fb081e0b98409556d023c7193eeb68947cd1211
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/expr.py                     |  9 ++++-----
>   tests/qapi-schema/reserved-member-u.err  |  2 ++
>   tests/qapi-schema/reserved-member-u.json |  1 -
>   tests/qapi-schema/reserved-member-u.out  | 14 --------------
>   4 files changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 2fcaaa2497..cf09fa9fd3 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -34,12 +34,10 @@ def check_name_is_str(name, info, source):
>   
>   
>   def check_name_str(name, info, source,
> -                   allow_optional=False, enum_member=False,
> +                   enum_member=False,

I guess we now assume here (in this function) that '*' is /never/ allowed.

>                      permit_upper=False):
>       membername = name
>   
> -    if allow_optional and name.startswith('*'):
> -        membername = name[1:]
>       # Enum members can start with a digit, because the generated C
>       # code always prefixes it with the enum name
>       if enum_member and membername[0].isdigit():
> @@ -52,7 +50,6 @@ def check_name_str(name, info, source,
>       if not permit_upper and name.lower() != name:
>           raise QAPISemError(
>               info, "%s uses uppercase in name" % source)
> -    assert not membername.startswith('*')
>   
>   
>   def check_defn_name_str(name, info, meta):
> @@ -171,8 +168,10 @@ def check_type(value, info, source,
>       # value is a dictionary, check that each member is okay
>       for (key, arg) in value.items():
>           key_source = "%s member '%s'" % (source, key)
> +        if key.startswith('*'):
> +            key = key[1:]

And we'll strip it out up here instead...

>           check_name_str(key, info, key_source,
> -                       allow_optional=True, permit_upper=permit_upper)
> +                       permit_upper=permit_upper)

Which makes that check the same, but

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

This check now behaves differently, fixing the bug.


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

(assuming that this was tested and didn't break something /else/ I 
haven't considered.)

>           check_keys(arg, info, key_source, ['type'], ['if', 'features'])
> diff --git a/tests/qapi-schema/reserved-member-u.err 
> b/tests/qapi-schema/reserved-member-u.err
> index e69de29bb2..b58e599a00 100644
> --- a/tests/qapi-schema/reserved-member-u.err
> +++ b/tests/qapi-schema/reserved-member-u.err
> @@ -0,0 +1,2 @@
> +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 15005abb09..2bfb8f59b6 100644
> --- a/tests/qapi-schema/reserved-member-u.json
> +++ b/tests/qapi-schema/reserved-member-u.json
> @@ -4,5 +4,4 @@
>   # 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
> -# 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 6a3705518b..e69de29bb2 100644
> --- a/tests/qapi-schema/reserved-member-u.out
> +++ b/tests/qapi-schema/reserved-member-u.out
> @@ -1,14 +0,0 @@
> -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