diff mbox series

[26/28] qapi: Enforce struct member naming rules

Message ID 20210323094025.3569441-27-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
Struct members, including command arguments, event data, and union
inline base members, should use '-', not '_'.  Enforce this.  Fix the
fixable offenders (all in tests/), and add the remainder to pragma
member-name-exceptions.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt                    |  3 ++-
 qapi/pragma.json                                | 17 +++++++++++++++++
 qga/qapi-schema.json                            |  4 ++++
 scripts/qapi/expr.py                            |  5 +++--
 tests/qapi-schema/qapi-schema-test.json         |  8 ++++++--
 tests/qapi-schema/qapi-schema-test.out          |  4 ++--
 tests/qapi-schema/struct-member-name-clash.err  |  2 +-
 tests/qapi-schema/struct-member-name-clash.json |  1 +
 8 files changed, 36 insertions(+), 8 deletions(-)

Comments

Eric Blake March 23, 2021, 3:46 p.m. UTC | #1
On 3/23/21 4:40 AM, Markus Armbruster wrote:
> Struct members, including command arguments, event data, and union
> inline base members, should use '-', not '_'.  Enforce this.  Fix the
> fixable offenders (all in tests/), and add the remainder to pragma
> member-name-exceptions.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/qapi/pragma.json
> @@ -31,10 +31,27 @@
>      # Externally visible types whose member names may use uppercase
>      'member-name-exceptions': [     # visible in:
>          'ACPISlotType',             # query-acpi-ospm-status
> +        'AcpiTableOptions',         # -acpitable
> +        'BlkdebugSetStateOptions',  # blockdev-add, -blockdev
> +        'BlockDeviceInfo',          # query-block
> +        'BlockDeviceStats',         # query-blockstats
> +        'BlockDeviceTimedStats',    # query-blockstats
> +        'BlockIOThrottle',          # block_set_io_throttle
> +        'BlockInfo',                # query-block
>          'BlockdevVmdkAdapterType',  # blockdev-create (to match VMDK spec)
>          'BlockdevVmdkSubformat',    # blockdev-create (to match VMDK spec)
> +        'ColoCompareProperties',    # object_add, -object
> +        'FilterMirrorProperties',   # object_add, -object
> +        'FilterRedirectorProperties', # object_add, -object
> +        'FilterRewriterProperties', # object_add, -object
> +        'InputLinuxProperties',     # object_add, -object
> +        'NetdevTapOptions',         # netdev_add, query-netdev, -netdev
> +        'PciBusInfo',               # query-pci
> +        'PciDeviceInfo',            # query-pci
> +        'PciMemoryRegion',          # query-pci
>          'QapiErrorClass',           # QMP error replies
>          'UuidInfo',                 # query-uuid
> +        'VncClientInfo',            # query-vnc, query-vnc-servers, ...
>          'X86CPURegister32'          # qom-get of x86 CPU properties
>                                      # feature-words, filtered-features
>      ] } }

I was worried the list might be even longer.  And as before, we might
have future patches that want to add aliases and/or deprecate the old
spellings, as long as introspection can easily see new spellings.

At any rate, I'm in agreement with letting the computer flag new
instances instead of relying on me to notice during review.

> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 2b08b761c2..fb17eebde3 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -19,6 +19,10 @@
>  # Whitelists to permit QAPI rule violations; think twice before you

Did you want to fix this instance of the word 'Whitelists' somewhere in
the series?

>  # add to them!
>  { 'pragma': {
> +    # Types whose member names may use '_'
> +    'member-name-exceptions': [
> +        'GuestAgentInfo'
> +    ],
>      # Commands allowed to return a non-dictionary:
>      'command-returns-exceptions': [
>          'guest-file-open',
Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster March 23, 2021, 9:23 p.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 3/23/21 4:40 AM, Markus Armbruster wrote:
>> Struct members, including command arguments, event data, and union
>> inline base members, should use '-', not '_'.  Enforce this.  Fix the
>> fixable offenders (all in tests/), and add the remainder to pragma
>> member-name-exceptions.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/qapi/pragma.json
>> @@ -31,10 +31,27 @@
>>      # Externally visible types whose member names may use uppercase
>>      'member-name-exceptions': [     # visible in:
>>          'ACPISlotType',             # query-acpi-ospm-status
>> +        'AcpiTableOptions',         # -acpitable
>> +        'BlkdebugSetStateOptions',  # blockdev-add, -blockdev
>> +        'BlockDeviceInfo',          # query-block
>> +        'BlockDeviceStats',         # query-blockstats
>> +        'BlockDeviceTimedStats',    # query-blockstats
>> +        'BlockIOThrottle',          # block_set_io_throttle
>> +        'BlockInfo',                # query-block
>>          'BlockdevVmdkAdapterType',  # blockdev-create (to match VMDK spec)
>>          'BlockdevVmdkSubformat',    # blockdev-create (to match VMDK spec)
>> +        'ColoCompareProperties',    # object_add, -object
>> +        'FilterMirrorProperties',   # object_add, -object
>> +        'FilterRedirectorProperties', # object_add, -object
>> +        'FilterRewriterProperties', # object_add, -object
>> +        'InputLinuxProperties',     # object_add, -object
>> +        'NetdevTapOptions',         # netdev_add, query-netdev, -netdev
>> +        'PciBusInfo',               # query-pci
>> +        'PciDeviceInfo',            # query-pci
>> +        'PciMemoryRegion',          # query-pci
>>          'QapiErrorClass',           # QMP error replies
>>          'UuidInfo',                 # query-uuid
>> +        'VncClientInfo',            # query-vnc, query-vnc-servers, ...
>>          'X86CPURegister32'          # qom-get of x86 CPU properties
>>                                      # feature-words, filtered-features
>>      ] } }
>
> I was worried the list might be even longer.  And as before, we might
> have future patches that want to add aliases and/or deprecate the old
> spellings, as long as introspection can easily see new spellings.
>
> At any rate, I'm in agreement with letting the computer flag new
> instances instead of relying on me to notice during review.

Saves us review - fix up cycles.  Everybody wins.

>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index 2b08b761c2..fb17eebde3 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -19,6 +19,10 @@
>>  # Whitelists to permit QAPI rule violations; think twice before you
>
> Did you want to fix this instance of the word 'Whitelists' somewhere in
> the series?

There's more than just this one.  We can replace them on top if we care.

>>  # add to them!
>>  { 'pragma': {
>> +    # Types whose member names may use '_'
>> +    'member-name-exceptions': [
>> +        'GuestAgentInfo'
>> +    ],
>>      # Commands allowed to return a non-dictionary:
>>      'command-returns-exceptions': [
>>          'guest-file-open',
>
> 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 10e9a0f829..c1cb6f987d 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -168,7 +168,8 @@  Pragma 'command-returns-exceptions' takes a list of commands that may
 violate the rules on permitted return types.  Default is none.
 
 Pragma 'member-name-exceptions' takes a list of types whose member
-names may contain uppercase letters.  Default is none.
+names may contain uppercase letters, and '_' instead of '-'.  Default
+is none.
 
 
 === Enumeration types ===
diff --git a/qapi/pragma.json b/qapi/pragma.json
index 339f067943..f422a1a2ac 100644
--- a/qapi/pragma.json
+++ b/qapi/pragma.json
@@ -31,10 +31,27 @@ 
     # Externally visible types whose member names may use uppercase
     'member-name-exceptions': [     # visible in:
         'ACPISlotType',             # query-acpi-ospm-status
+        'AcpiTableOptions',         # -acpitable
+        'BlkdebugSetStateOptions',  # blockdev-add, -blockdev
+        'BlockDeviceInfo',          # query-block
+        'BlockDeviceStats',         # query-blockstats
+        'BlockDeviceTimedStats',    # query-blockstats
+        'BlockIOThrottle',          # block_set_io_throttle
+        'BlockInfo',                # query-block
         'BlockdevVmdkAdapterType',  # blockdev-create (to match VMDK spec)
         'BlockdevVmdkSubformat',    # blockdev-create (to match VMDK spec)
+        'ColoCompareProperties',    # object_add, -object
+        'FilterMirrorProperties',   # object_add, -object
+        'FilterRedirectorProperties', # object_add, -object
+        'FilterRewriterProperties', # object_add, -object
+        'InputLinuxProperties',     # object_add, -object
+        'NetdevTapOptions',         # netdev_add, query-netdev, -netdev
+        'PciBusInfo',               # query-pci
+        'PciDeviceInfo',            # query-pci
+        'PciMemoryRegion',          # query-pci
         'QapiErrorClass',           # QMP error replies
         'UuidInfo',                 # query-uuid
+        'VncClientInfo',            # query-vnc, query-vnc-servers, ...
         'X86CPURegister32'          # qom-get of x86 CPU properties
                                     # feature-words, filtered-features
     ] } }
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 2b08b761c2..fb17eebde3 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -19,6 +19,10 @@ 
 # Whitelists to permit QAPI rule violations; think twice before you
 # add to them!
 { 'pragma': {
+    # Types whose member names may use '_'
+    'member-name-exceptions': [
+        'GuestAgentInfo'
+    ],
     # Commands allowed to return a non-dictionary:
     'command-returns-exceptions': [
         'guest-file-open',
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index c0d4c164fe..17fe95fffe 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -184,7 +184,7 @@  def check_type(value, info, source,
         raise QAPISemError(info,
                            "%s should be an object or type name" % source)
 
-    permit_upper = allow_dict in info.pragma.member_name_exceptions
+    permissive = allow_dict in info.pragma.member_name_exceptions
 
     # value is a dictionary, check that each member is okay
     for (key, arg) in value.items():
@@ -192,7 +192,8 @@  def check_type(value, info, source,
         if key.startswith('*'):
             key = key[1:]
         check_name_lower(key, info, key_source,
-                         permit_upper, permit_underscore=True)
+                         permit_upper=permissive,
+                         permit_underscore=permissive)
         if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'):
             raise QAPISemError(info, "%s uses reserved name" % key_source)
         check_keys(arg, info, key_source, ['type'], ['if', 'features'])
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index e635db4a35..387678acbb 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -6,6 +6,10 @@ 
 
 # Whitelists to permit QAPI rule violations
 { 'pragma': {
+    # Types whose member names may use '_'
+    'member-name-exceptions': [
+        'UserDefA'
+    ],
     # Commands allowed to return a non-dictionary:
     'command-returns-exceptions': [
         'guest-get-time',
@@ -231,7 +235,7 @@ 
   'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
 
 { 'command': 'test-if-union-cmd',
-  'data': { 'union_cmd_arg': 'TestIfUnion' },
+  'data': { 'union-cmd-arg': 'TestIfUnion' },
   'if': 'defined(TEST_IF_UNION)' }
 
 { 'alternate': 'TestIfAlternate', 'data':
@@ -240,7 +244,7 @@ 
   'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
 
 { 'command': 'test-if-alternate-cmd',
-  'data': { 'alt_cmd_arg': 'TestIfAlternate' },
+  'data': { 'alt-cmd-arg': 'TestIfAlternate' },
   'if': 'defined(TEST_IF_ALT)' }
 
 { 'command': 'test-if-cmd',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3f1ea345fd..51efe5d7cd 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -320,7 +320,7 @@  object TestIfUnion
         if ['defined(TEST_IF_UNION_BAR)']
     if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
 object q_obj_test-if-union-cmd-arg
-    member union_cmd_arg: TestIfUnion optional=False
+    member union-cmd-arg: TestIfUnion optional=False
     if ['defined(TEST_IF_UNION)']
 command test-if-union-cmd q_obj_test-if-union-cmd-arg -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
@@ -332,7 +332,7 @@  alternate TestIfAlternate
         if ['defined(TEST_IF_ALT_BAR)']
     if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
 object q_obj_test-if-alternate-cmd-arg
-    member alt_cmd_arg: TestIfAlternate optional=False
+    member alt-cmd-arg: TestIfAlternate optional=False
     if ['defined(TEST_IF_ALT)']
 command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/struct-member-name-clash.err b/tests/qapi-schema/struct-member-name-clash.err
index 6ac042d59d..7e53a605d2 100644
--- a/tests/qapi-schema/struct-member-name-clash.err
+++ b/tests/qapi-schema/struct-member-name-clash.err
@@ -1,2 +1,2 @@ 
 struct-member-name-clash.json: In struct 'Oops':
-struct-member-name-clash.json:4: member 'a_b' collides with member 'a-b'
+struct-member-name-clash.json:5: member 'a_b' collides with member 'a-b'
diff --git a/tests/qapi-schema/struct-member-name-clash.json b/tests/qapi-schema/struct-member-name-clash.json
index 3fb69cc2ce..571acf05ce 100644
--- a/tests/qapi-schema/struct-member-name-clash.json
+++ b/tests/qapi-schema/struct-member-name-clash.json
@@ -1,4 +1,5 @@ 
 # C member name collision
 # Reject members that clash when mapped to C names (we would have two 'a_b'
 # members).
+{ 'pragma': { 'member-name-exceptions': [ 'Oops' ] } }
 { 'struct': 'Oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }