diff mbox series

[24/28] qapi: Enforce command naming rules

Message ID 20210323094025.3569441-25-armbru@redhat.com
State New
Headers show
Series qapi: Enforce naming rules | expand

Commit Message

Markus Armbruster March 23, 2021, 9:40 a.m. UTC 
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>
---
 docs/devel/qapi-code-gen.txt            |  8 ++++++--
 qapi/pragma.json                        | 18 +++++++++++++++++
 tests/unit/test-qmp-cmds.c              | 10 +++++-----
 scripts/qapi/expr.py                    |  5 +++--
 scripts/qapi/parser.py                  |  3 +++
 scripts/qapi/source.py                  |  2 ++
 tests/qapi-schema/qapi-schema-test.json | 21 +++++++++++---------
 tests/qapi-schema/qapi-schema-test.out  | 26 ++++++++++++-------------
 8 files changed, 62 insertions(+), 31 deletions(-)

Comments

Eric Blake March 23, 2021, 3:23 p.m. UTC | #1 
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>
Markus Armbruster March 23, 2021, 9:19 p.m. UTC | #2 
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!
diff mbox series

Patch

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 23e9823019..10e9a0f829 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -149,6 +149,7 @@  prevent incomplete include files.
 Syntax:
     PRAGMA = { 'pragma': {
                    '*doc-required': BOOL,
+                   '*command-name-exceptions': [ STRING, ... ],
                    '*command-returns-exceptions': [ STRING, ... ],
                    '*member-name-exceptions': [ STRING, ... ] } }
 
@@ -160,6 +161,9 @@  pragma to different values in parts of the schema doesn't work.
 Pragma 'doc-required' takes a boolean value.  If true, documentation
 is required.  Default is false.
 
+Pragma 'command-name-exceptions' takes a list of commands whose names
+may contain '_' instead of '-'.  Default is none.
+
 Pragma 'command-returns-exceptions' takes a list of commands that may
 violate the rules on permitted return types.  Default is none.
 
@@ -756,8 +760,8 @@  Any name (command, event, type, member, or enum value) beginning with
 "x-" is marked experimental, and may be withdrawn or changed
 incompatibly in a future release.
 
-Pragma 'member-name-exceptions' lets you violate the rules on use of
-upper and lower case.  Use for new code is strongly discouraged.
+Pragmas 'command-name-exceptions' and 'member-name-exceptions' let you
+violate naming rules.  Use for new code is strongly discouraged.
 
 
 === Downstream extensions ===
diff --git a/qapi/pragma.json b/qapi/pragma.json
index 4c47c802d1..339f067943 100644
--- a/qapi/pragma.json
+++ 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' ],
     'command-returns-exceptions': [
         'human-monitor-command',
         'qom-get',
diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
index 99973dde7c..1b0b7d99df 100644
--- a/tests/unit/test-qmp-cmds.c
+++ 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)
 {
     return NULL;
 }
@@ -199,7 +199,7 @@  static void test_dispatch_cmd(void)
 
     ret = qobject_to(QDict,
                      do_qmp_dispatch(false,
-                                     "{ 'execute': 'user_def_cmd' }"));
+                                     "{ 'execute': 'user-def-cmd' }"));
     assert(ret && qdict_size(ret) == 0);
     qobject_unref(ret);
 }
@@ -220,11 +220,11 @@  static void test_dispatch_cmd_failure(void)
 {
     /* missing arguments */
     do_qmp_dispatch_error(false, ERROR_CLASS_GENERIC_ERROR,
-                          "{ 'execute': 'user_def_cmd2' }");
+                          "{ 'execute': 'user-def-cmd2' }");
 
     /* extra arguments */
     do_qmp_dispatch_error(false, ERROR_CLASS_GENERIC_ERROR,
-                          "{ 'execute': 'user_def_cmd',"
+                          "{ 'execute': 'user-def-cmd',"
                           " 'arguments': { 'a': 66 } }");
 }
 
@@ -248,7 +248,7 @@  static void test_dispatch_cmd_io(void)
     int64_t val;
 
     ret = qobject_to(QDict, do_qmp_dispatch(false,
-        "{ 'execute': 'user_def_cmd2', 'arguments': {"
+        "{ 'execute': 'user-def-cmd2', 'arguments': {"
         " 'ud1a': { 'integer': 42, 'string': 'hello' },"
         " 'ud1b': { 'integer': 422, 'string': 'hello2' } } }"));
 
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 01a994412d..c0d4c164fe 100644
--- a/scripts/qapi/expr.py
+++ 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)
     else:
         check_name_camel(name, info, meta)
     if name.endswith('Kind') or name.endswith('List'):
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index c16b8b6995..58267c3db9 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -132,6 +132,9 @@  def _pragma(self, name, value, info):
                 raise QAPISemError(info,
                                    "pragma 'doc-required' must be boolean")
             info.pragma.doc_required = value
+        elif name == 'command-name-exceptions':
+            self._check_pragma_list_of_str(name, value, info)
+            info.pragma.command_name_exceptions = value
         elif name == 'command-returns-exceptions':
             self._check_pragma_list_of_str(name, value, info)
             info.pragma.command_returns_exceptions = value
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d8f9ec377f..03b6ede082 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -21,6 +21,8 @@  class QAPISchemaPragma:
     def __init__(self) -> None:
         # Are documentation comments required?
         self.doc_required = False
+        # Commands whose names may use '_'
+        self.command_name_exceptions: List[str] = []
         # Commands allowed to return a non-dictionary
         self.command_returns_exceptions: List[str] = []
         # Types whose member names may violate case conventions
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 16b03bf308..e635db4a35 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -31,7 +31,7 @@ 
   'base': { 'type': 'EnumOne' }, 'discriminator': 'type',
   'data': { } }
 
-{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
+{ 'command': 'user-def-cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
 
 # for testing override of default naming heuristic
 { 'enum': 'QEnumTwo',
@@ -141,9 +141,9 @@ 
 { 'include': 'include/sub-module.json' }
 
 # testing commands
-{ 'command': 'user_def_cmd', 'data': {} }
-{ 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
-{ 'command': 'user_def_cmd2',
+{ 'command': 'user-def-cmd', 'data': {} }
+{ 'command': 'user-def-cmd1', 'data': {'ud1a': 'UserDefOne'} }
+{ 'command': 'user-def-cmd2',
   'data': {'ud1a': {'type': 'UserDefOne'}, '*ud1b': 'UserDefOne'},
   'returns': 'UserDefTwo' }
 
@@ -230,7 +230,8 @@ 
     'union_bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
   'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
 
-{ 'command': 'TestIfUnionCmd', 'data': { 'union_cmd_arg': 'TestIfUnion' },
+{ 'command': 'test-if-union-cmd',
+  'data': { 'union_cmd_arg': 'TestIfUnion' },
   'if': 'defined(TEST_IF_UNION)' }
 
 { 'alternate': 'TestIfAlternate', 'data':
@@ -238,16 +239,18 @@ 
     'bar': { 'type': 'TestStruct', 'if': 'defined(TEST_IF_ALT_BAR)'} },
   'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
 
-{ 'command': 'TestIfAlternateCmd', 'data': { 'alt_cmd_arg': 'TestIfAlternate' },
+{ 'command': 'test-if-alternate-cmd',
+  'data': { 'alt_cmd_arg': 'TestIfAlternate' },
   'if': 'defined(TEST_IF_ALT)' }
 
-{ 'command': 'TestIfCmd', 'data':
-  { 'foo': 'TestIfStruct',
+{ 'command': 'test-if-cmd',
+  'data': {
+    'foo': 'TestIfStruct',
     'bar': { 'type': 'TestIfEnum', 'if': 'defined(TEST_IF_CMD_BAR)' } },
   'returns': 'UserDefThree',
   'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
 
-{ 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' }
+{ 'command': 'test-cmd-return-def-three', 'returns': 'UserDefThree' }
 
 { 'event': 'TEST_IF_EVENT', 'data':
   { 'foo': 'TestIfStruct',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 882d0e7c56..3f1ea345fd 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -32,7 +32,7 @@  object Union
     case value2: q_empty
     case value3: q_empty
     case value4: q_empty
-command user_def_cmd0 Empty2 -> Empty2
+command user-def-cmd0 Empty2 -> Empty2
     gen=True success_response=True boxed=False oob=False preconfig=False
 enum QEnumTwo
     prefix QENUM_TWO
@@ -190,16 +190,16 @@  object UserDefListUnion
     case any: q_obj_anyList-wrapper
     case user: q_obj_StatusList-wrapper
 include include/sub-module.json
-command user_def_cmd None -> None
+command user-def-cmd None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
-object q_obj_user_def_cmd1-arg
+object q_obj_user-def-cmd1-arg
     member ud1a: UserDefOne optional=False
-command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
+command user-def-cmd1 q_obj_user-def-cmd1-arg -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
-object q_obj_user_def_cmd2-arg
+object q_obj_user-def-cmd2-arg
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
-command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
+command user-def-cmd2 q_obj_user-def-cmd2-arg -> UserDefTwo
     gen=True success_response=True boxed=False oob=False preconfig=False
 command cmd-success-response None -> None
     gen=True success_response=False boxed=False oob=False preconfig=False
@@ -319,10 +319,10 @@  object TestIfUnion
     case union_bar: q_obj_str-wrapper
         if ['defined(TEST_IF_UNION_BAR)']
     if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
-object q_obj_TestIfUnionCmd-arg
+object q_obj_test-if-union-cmd-arg
     member union_cmd_arg: TestIfUnion optional=False
     if ['defined(TEST_IF_UNION)']
-command TestIfUnionCmd q_obj_TestIfUnionCmd-arg -> None
+command test-if-union-cmd q_obj_test-if-union-cmd-arg -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
     if ['defined(TEST_IF_UNION)']
 alternate TestIfAlternate
@@ -331,21 +331,21 @@  alternate TestIfAlternate
     case bar: TestStruct
         if ['defined(TEST_IF_ALT_BAR)']
     if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
-object q_obj_TestIfAlternateCmd-arg
+object q_obj_test-if-alternate-cmd-arg
     member alt_cmd_arg: TestIfAlternate optional=False
     if ['defined(TEST_IF_ALT)']
-command TestIfAlternateCmd q_obj_TestIfAlternateCmd-arg -> None
+command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
     if ['defined(TEST_IF_ALT)']
-object q_obj_TestIfCmd-arg
+object q_obj_test-if-cmd-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnum optional=False
         if ['defined(TEST_IF_CMD_BAR)']
     if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
-command TestIfCmd q_obj_TestIfCmd-arg -> UserDefThree
+command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
     if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
-command TestCmdReturnDefThree None -> UserDefThree
+command test-cmd-return-def-three None -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
 array TestIfEnumList TestIfEnum
     if ['defined(TEST_IF_ENUM)']