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

[Eric Blake]

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, so I won't withhold R-b, but it
may be worth a tweak to the commit message.


>  
>  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?

Otherwise the patch makes sense.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org