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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

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

Another option is to accept '-' instead of '_' in QMP command and
argument names.

The harder problem is QMP output.

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

Thanks!