Re: [PATCH 24/28] qapi: Enforce command naming rules

[Eric Blake]

On 3/23/21 4:40 AM, Markus Armbruster wrote:
> Command names should be lower-case.  Enforce this.  Fix the fixable
> offenders (all in tests/), and add the remainder to pragma
> command-name-exceptions.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/qapi/pragma.json
> @@ -4,6 +4,24 @@
>  # add to them!
>  { 'pragma': {
>      # Commands allowed to return a non-dictionary:
> +    'command-name-exceptions': [
> +        'add_client',
> +        'block_passwd',
> +        'block_resize',
> +        'block_set_io_throttle',
> +        'client_migrate_info',
> +        'device_add',
> +        'device_del',
> +        'expire_password',
> +        'migrate_cancel',
> +        'netdev_add',
> +        'netdev_del',
> +        'qmp_capabilities',
> +        'set_link',
> +        'set_password',
> +        'system_powerdown',
> +        'system_reset',
> +        'system_wakeup' ],

Outside the scope of this patch, do we have any intentions on adding
alias commands or deprecating old spellings in favor of new ones?

None of these have a capital letter...

qmp_capabilities is probably the hardest one to get rid of, since you
can't send any other commands until that one is complete (that is, you
can't introspect if a replacement exists; if we add a new spelling, all
you can do is try both spellings until one works, but that is extra
traffic).  The rest can be suitably probed via introspection.


> +++ b/tests/unit/test-qmp-cmds.c
> @@ -13,7 +13,7 @@
>  
>  static QmpCommandList qmp_commands;
>  
> -UserDefThree *qmp_TestCmdReturnDefThree(Error **errp)
> +UserDefThree *qmp_test_cmd_return_def_three(Error **errp)

...oh, we had a test command with capitals....

> +++ b/scripts/qapi/expr.py
> @@ -70,8 +70,9 @@ def check_defn_name_str(name, info, meta):
>      if meta == 'event':
>          check_name_upper(name, info, meta)
>      elif meta == 'command':
> -        check_name_lower(name, info, meta,
> -                         permit_upper=True, permit_underscore=True)
> +        check_name_lower(
> +            name, info, meta,
> +            permit_underscore=name in info.pragma.command_name_exceptions)

...and earlier in the series, I had asked why you wanted
permit_upper=True here.  So it is now obvious that it was just for the
tests and that you deferred fixing the tests until now.  If you don't
want to refactor the series, then it's at least worth a tweak to that
commit message to call it out.  At any rate, I'm glad to see the
permit_upper=True gone!

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

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