Eric Blake <[email protected]> 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 <[email protected]> >> --- > >> +++ 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 <[email protected]> Thanks!
