John Snow <js...@redhat.com> writes:
> 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!)
I added it myself. I guess it made me feel safer back then :)
>> Fixes: 9fb081e0b98409556d023c7193eeb68947cd1211
>> Signed-off-by: Markus Armbruster <arm...@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.
Correct.
>> 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.
Right again.
> Reviewed-by: John Snow <js...@redhat.com>
Thanks!
> (assuming that this was tested and didn't break something /else/ I
> haven't considered.)
Fortunately, tests/qapi-schema is reasonably comprehensive.
