Re: [PATCH 10/28] qapi: Rework name checking in preparation of stricter checking

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 3/23/21 4:40 AM, Markus Armbruster wrote:
>> Naming rules differ for the various kinds of names.  To prepare
>> enforcing them, define functions to check them: check_name_upper(),
>> check_name_lower(), and check_name_camel().  For now, these merely
>> wrap around check_name_str(), but that will change shortly.  Replace
>> the other uses of check_name_str() by appropriate uses of the
>> wrappers.  No change in behavior just yet.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++-------------
>>  1 file changed, 36 insertions(+), 15 deletions(-)
>> 
>
>> +++ b/scripts/qapi/expr.py
>> @@ -21,11 +21,12 @@
>>  from .error import QAPISemError
>>  
>>  
>> -# Names must be letters, numbers, -, and _.  They must start with letter,
>> -# except for downstream extensions which must start with __RFQDN_.
>> -# Dots are only valid in the downstream extension prefix.
>> -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?'
>> -                        '[a-zA-Z][a-zA-Z0-9_-]*$')
>
> I'm assuming python concatenates r'' with '' in the obvious manner...
>
>> +# Names consist of letters, digits, -, and _, starting with a letter.
>> +# An experimental name is prefixed with x-.  A name of a downstream
>> +# extension is prefixed with __RFQDN_.  The latter prefix goes first.
>> +valid_name = re.compile(r'(__[a-z0-9.-]+_)?'
>> +                        r'(x-)?'
>> +                        r'([a-z][a-z0-9_-]*)$', re.IGNORECASE)
>
> ...but like your explicit use of r'' r''.
>
> Splitting out special handling of r'(x-)?' does not change behavior, but
> is not otherwise mentioned in your commit message.  I suspect you did it
> to make it easier to permit x-EVENT_NAME in later patches where upper is
> handled differently from lower or camel,

Yes.

>                                          so I won't withhold R-b, but it
> may be worth a tweak to the commit message.

Probably.  I'm failing at coming up with a concise text that isn't
confusing.

>>  def check_defn_name_str(name, info, meta):
>> -    check_name_str(name, info, meta, permit_upper=True)
>> +    if meta == 'event':
>> +        check_name_upper(name, info, meta)
>> +    elif meta == 'command':
>> +        check_name_lower(name, info, meta, permit_upper=True)
>
> Why do commands need to permit upper?  I guess just downstream FQDN
> extensions?

This is just so that the patch doesn't change behavior.  PATCH 24 will
flip it to False.

> Otherwise the patch makes sense.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!