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

[Markus Armbruster]

John Snow <jsnow@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 <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.

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 <jsnow@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.