From 7552cbe1d0aacf2fd5b7f69cb65534891e9e23d3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:39:58 +0100 Subject: [PATCH 01/29] qapi/pragma: Tidy up after removal of deprecated commands Commit cbde7be900 "migrate: remove QMP/HMP commands for speed, downtime and cache size" neglected to remove query-migrate-cache-size from pragma returns-whitelist. Commit 8af54b9172 "machine: remove 'query-cpus' QMP command" neglected to remove CpuInfo & friends from pragma name-case-exceptions. Remove these now. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-2-armbru@redhat.com> Reviewed-by: John Snow --- qapi/pragma.json | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/qapi/pragma.json b/qapi/pragma.json index cffae27666..7f158e183d 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -7,18 +7,14 @@ 'returns-whitelist': [ 'human-monitor-command', 'qom-get', - 'query-migrate-cache-size', 'query-tpm-models', 'query-tpm-types', 'ringbuf-read' ], 'name-case-whitelist': [ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status - 'CpuInfoMIPS', # PC, visible through query-cpu - 'CpuInfoTricore', # PC, visible through query-cpu 'BlockdevVmdkSubformat', # all members, to match VMDK spec spellings 'BlockdevVmdkAdapterType', # legacyESX, to match VMDK spec spellings 'QapiErrorClass', # all members, visible through errors 'UuidInfo', # UUID, visible through query-uuid - 'X86CPURegister32', # all members, visible indirectly through qom-get - 'CpuInfo' # CPU, visible through query-cpu + 'X86CPURegister32' # all members, visible indirectly through qom-get ] } } From 00d16f239f3a1ba0b1ea09dc0852386a25a144bc Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:39:59 +0100 Subject: [PATCH 02/29] tests/qapi-schema: Drop redundant flat-union-inline test flat-union-inline.json covers longhand branch definition with an invalid type value. It's redundant: longhand branch definition is covered by flat-union-inline-invalid-dict.json, and invalid type value is covered by nested-struct-data.json. Drop the test. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-3-armbru@redhat.com> Reviewed-by: John Snow --- tests/qapi-schema/flat-union-inline.err | 2 -- tests/qapi-schema/flat-union-inline.json | 11 ----------- tests/qapi-schema/flat-union-inline.out | 0 tests/qapi-schema/meson.build | 1 - 4 files changed, 14 deletions(-) delete mode 100644 tests/qapi-schema/flat-union-inline.err delete mode 100644 tests/qapi-schema/flat-union-inline.json delete mode 100644 tests/qapi-schema/flat-union-inline.out diff --git a/tests/qapi-schema/flat-union-inline.err b/tests/qapi-schema/flat-union-inline.err deleted file mode 100644 index 538283f5db..0000000000 --- a/tests/qapi-schema/flat-union-inline.err +++ /dev/null @@ -1,2 +0,0 @@ -flat-union-inline.json: In union 'TestUnion': -flat-union-inline.json:7: 'data' member 'value1' should be a type name diff --git a/tests/qapi-schema/flat-union-inline.json b/tests/qapi-schema/flat-union-inline.json deleted file mode 100644 index a9b3ce3f0d..0000000000 --- a/tests/qapi-schema/flat-union-inline.json +++ /dev/null @@ -1,11 +0,0 @@ -# we require branches to be a struct name -# TODO: should we allow anonymous inline branch types? -{ 'enum': 'TestEnum', - 'data': [ 'value1', 'value2' ] } -{ 'struct': 'Base', - 'data': { 'enum1': 'TestEnum', 'kind': 'str' } } -{ 'union': 'TestUnion', - 'base': 'Base', - 'discriminator': 'enum1', - 'data': { 'value1': { 'type': {} }, - 'value2': { 'integer': 'int' } } } diff --git a/tests/qapi-schema/flat-union-inline.out b/tests/qapi-schema/flat-union-inline.out deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index 304ef939bd..d5fa035507 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -111,7 +111,6 @@ schemas = [ 'flat-union-clash-member.json', 'flat-union-discriminator-bad-name.json', 'flat-union-empty.json', - 'flat-union-inline.json', 'flat-union-inline-invalid-dict.json', 'flat-union-int-branch.json', 'flat-union-invalid-branch-key.json', From 5bd18d98cd88e2df1e1e274546a06ebe7fdd5eec Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:00 +0100 Subject: [PATCH 03/29] tests/qapi-schema: Rework comments on longhand member definitions A few old comments talk about "desired future use of defaults" and "anonymous inline branch types". Kind of misleading since commit 87adbbffd4 "qapi: add a dictionary form for TYPE" added longhand member definitions. Talk about that instead. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-4-armbru@redhat.com> Reviewed-by: Eric Blake Reviewed-by: John Snow --- tests/qapi-schema/event-member-invalid-dict.err | 2 +- tests/qapi-schema/event-member-invalid-dict.json | 2 ++ tests/qapi-schema/flat-union-inline-invalid-dict.json | 4 ++-- tests/qapi-schema/nested-struct-data-invalid-dict.err | 2 +- tests/qapi-schema/nested-struct-data-invalid-dict.json | 3 ++- tests/qapi-schema/nested-struct-data.json | 2 +- tests/qapi-schema/struct-member-invalid-dict.err | 2 +- tests/qapi-schema/struct-member-invalid-dict.json | 3 ++- 8 files changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/qapi-schema/event-member-invalid-dict.err b/tests/qapi-schema/event-member-invalid-dict.err index c7a6a24305..82f8989344 100644 --- a/tests/qapi-schema/event-member-invalid-dict.err +++ b/tests/qapi-schema/event-member-invalid-dict.err @@ -1,2 +1,2 @@ event-member-invalid-dict.json: In event 'EVENT_A': -event-member-invalid-dict.json:1: 'data' member 'a' misses key 'type' +event-member-invalid-dict.json:3: 'data' member 'a' misses key 'type' diff --git a/tests/qapi-schema/event-member-invalid-dict.json b/tests/qapi-schema/event-member-invalid-dict.json index ee6f3ecb6f..e58560abca 100644 --- a/tests/qapi-schema/event-member-invalid-dict.json +++ b/tests/qapi-schema/event-member-invalid-dict.json @@ -1,2 +1,4 @@ +# event 'data' member with dict value is (longhand) argument +# definition, not inline complex type { 'event': 'EVENT_A', 'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } } diff --git a/tests/qapi-schema/flat-union-inline-invalid-dict.json b/tests/qapi-schema/flat-union-inline-invalid-dict.json index 62c7cda617..1779712795 100644 --- a/tests/qapi-schema/flat-union-inline-invalid-dict.json +++ b/tests/qapi-schema/flat-union-inline-invalid-dict.json @@ -1,5 +1,5 @@ -# we require branches to be a struct name -# TODO: should we allow anonymous inline branch types? +# union 'data' member with dict value is (longhand) branch +# definition, not inline complex type { 'enum': 'TestEnum', 'data': [ 'value1', 'value2' ] } { 'struct': 'Base', diff --git a/tests/qapi-schema/nested-struct-data-invalid-dict.err b/tests/qapi-schema/nested-struct-data-invalid-dict.err index c044b2b17a..375e155fe6 100644 --- a/tests/qapi-schema/nested-struct-data-invalid-dict.err +++ b/tests/qapi-schema/nested-struct-data-invalid-dict.err @@ -1,2 +1,2 @@ nested-struct-data-invalid-dict.json: In command 'foo': -nested-struct-data-invalid-dict.json:2: 'data' member 'a' misses key 'type' +nested-struct-data-invalid-dict.json:3: 'data' member 'a' misses key 'type' diff --git a/tests/qapi-schema/nested-struct-data-invalid-dict.json b/tests/qapi-schema/nested-struct-data-invalid-dict.json index efbe773ded..aa37b85e19 100644 --- a/tests/qapi-schema/nested-struct-data-invalid-dict.json +++ b/tests/qapi-schema/nested-struct-data-invalid-dict.json @@ -1,3 +1,4 @@ -# inline subtypes collide with our desired future use of defaults +# command 'data' member with dict value is (longhand) argument +# definition, not inline complex type { 'command': 'foo', 'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } } diff --git a/tests/qapi-schema/nested-struct-data.json b/tests/qapi-schema/nested-struct-data.json index 5b8a40cca3..2980d45d05 100644 --- a/tests/qapi-schema/nested-struct-data.json +++ b/tests/qapi-schema/nested-struct-data.json @@ -1,3 +1,3 @@ -# inline subtypes collide with our desired future use of defaults +# {} is not a valid type reference { 'command': 'foo', 'data': { 'a' : { 'type': {} }, 'b' : 'str' } } diff --git a/tests/qapi-schema/struct-member-invalid-dict.err b/tests/qapi-schema/struct-member-invalid-dict.err index 0621aecfbd..f9b3f33551 100644 --- a/tests/qapi-schema/struct-member-invalid-dict.err +++ b/tests/qapi-schema/struct-member-invalid-dict.err @@ -1,2 +1,2 @@ struct-member-invalid-dict.json: In struct 'foo': -struct-member-invalid-dict.json:2: 'data' member '*a' misses key 'type' +struct-member-invalid-dict.json:3: 'data' member '*a' misses key 'type' diff --git a/tests/qapi-schema/struct-member-invalid-dict.json b/tests/qapi-schema/struct-member-invalid-dict.json index 9fe0d455a9..bc3d62ae63 100644 --- a/tests/qapi-schema/struct-member-invalid-dict.json +++ b/tests/qapi-schema/struct-member-invalid-dict.json @@ -1,3 +1,4 @@ -# Long form of member must have a value member 'type' +# struct 'data' member with dict value is (longhand) member +# definition, not inline complex type { 'struct': 'foo', 'data': { '*a': { 'case': 'foo' } } } From 27ae2f0787ae42eca9ec34961d2269d7a1fc5230 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:01 +0100 Subject: [PATCH 04/29] tests/qapi-schema: Belatedly update comment on alternate clash Commit 0426d53c65 "qapi: Simplify visiting of alternate types" eliminated the implicit alternate enum, but neglected to update a comment about it in a test. Do that now. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-5-armbru@redhat.com> Reviewed-by: John Snow --- tests/qapi-schema/alternate-clash.err | 2 +- tests/qapi-schema/alternate-clash.json | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err index f58b977f7b..0fe02f2c99 100644 --- a/tests/qapi-schema/alternate-clash.err +++ b/tests/qapi-schema/alternate-clash.err @@ -1,2 +1,2 @@ alternate-clash.json: In alternate 'Alt1': -alternate-clash.json:7: branch 'a_b' collides with branch 'a-b' +alternate-clash.json:4: branch 'a_b' collides with branch 'a-b' diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json index 9a59b88ced..039c4be658 100644 --- a/tests/qapi-schema/alternate-clash.json +++ b/tests/qapi-schema/alternate-clash.json @@ -1,8 +1,5 @@ # Alternate branch name collision # Reject an alternate that would result in a collision in generated C -# names (this would try to generate two enum values 'ALT1_KIND_A_B'). -# TODO: In the future, if alternates are simplified to not generate -# the implicit Alt1Kind enum, we would still have a collision with the -# resulting C union trying to have two members named 'a_b'. +# names (this would try to generate two union members named 'a_b'). { 'alternate': 'Alt1', 'data': { 'a-b': 'bool', 'a_b': 'int' } } From 1444989a3a4e8399e366ceecf4ed5bbd2d83c727 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:02 +0100 Subject: [PATCH 05/29] tests/qapi-schema: Drop TODO comment on simple unions Simple unions don't need more features, they need to die. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-6-armbru@redhat.com> Reviewed-by: John Snow --- tests/qapi-schema/flat-union-no-base.err | 2 +- tests/qapi-schema/flat-union-no-base.json | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err index 9bd595bcfb..5167565b00 100644 --- a/tests/qapi-schema/flat-union-no-base.err +++ b/tests/qapi-schema/flat-union-no-base.err @@ -1,2 +1,2 @@ flat-union-no-base.json: In union 'TestUnion': -flat-union-no-base.json:9: 'discriminator' requires 'base' +flat-union-no-base.json:8: 'discriminator' requires 'base' diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json index ffc4c6f0e6..327877b563 100644 --- a/tests/qapi-schema/flat-union-no-base.json +++ b/tests/qapi-schema/flat-union-no-base.json @@ -1,5 +1,4 @@ # flat unions require a base -# TODO: simple unions should be able to use an enum discriminator { 'struct': 'TestTypeA', 'data': { 'string': 'str' } } { 'struct': 'TestTypeB', From 73c40b07c6fffcb2725f4c9d3f361967e39aef97 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:03 +0100 Subject: [PATCH 06/29] tests/qapi-schema: Tweak to demonstrate buggy member name check Member name 'u' and names starting with 'has-' or 'has_' are reserved for the generator. check_type() enforces this, covered by tests reserved-member-u and reserved-member-has. These tests neglect to cover optional members, where the name starts with '*'. Tweak reserved-member-u to fix that. Test reserved-member-has still covers non-optional members. This demonstrates the reserved member name check is broken for optional members. The next commit will fix it. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-7-armbru@redhat.com> Reviewed-by: John Snow [Commit message improved slightly] --- tests/qapi-schema/reserved-member-u.err | 2 -- tests/qapi-schema/reserved-member-u.json | 3 ++- tests/qapi-schema/reserved-member-u.out | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err index 231d552494..e69de29bb2 100644 --- a/tests/qapi-schema/reserved-member-u.err +++ b/tests/qapi-schema/reserved-member-u.err @@ -1,2 +0,0 @@ -reserved-member-u.json: In struct 'Oops': -reserved-member-u.json:7: 'data' member 'u' uses reserved name diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json index 1eaf0f301c..15005abb09 100644 --- a/tests/qapi-schema/reserved-member-u.json +++ b/tests/qapi-schema/reserved-member-u.json @@ -4,4 +4,5 @@ # This is true even for non-unions, because it is possible to convert a # struct to flat union while remaining backwards compatible in QMP. # TODO - we could munge the member name to 'q_u' to avoid the collision -{ 'struct': 'Oops', 'data': { 'u': 'str' } } +# BUG: not rejected +{ 'struct': 'Oops', 'data': { '*u': 'str' } } diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out index e69de29bb2..6a3705518b 100644 --- a/tests/qapi-schema/reserved-member-u.out +++ b/tests/qapi-schema/reserved-member-u.out @@ -0,0 +1,14 @@ +module ./builtin +object q_empty +enum QType + prefix QTYPE + member none + member qnull + member qnum + member qstring + member qdict + member qlist + member qbool +module reserved-member-u.json +object Oops + member u: str optional=True From dbfe3c7c289c6b95a920b4e2a178e583c17c62a8 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:04 +0100 Subject: [PATCH 07/29] qapi: Fix to reject optional members with reserved names check_type() fails to reject optional members with reserved names, because it neglects to strip off the leading '*'. Fix that. The stripping in check_name_str() is now useless. Drop. Also drop the "no leading '*'" assertion, because valid_name.match() ensures it can't fail. Fixes: 9fb081e0b98409556d023c7193eeb68947cd1211 Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-8-armbru@redhat.com> Reviewed-by: John Snow --- scripts/qapi/expr.py | 9 ++++----- tests/qapi-schema/reserved-member-u.err | 2 ++ tests/qapi-schema/reserved-member-u.json | 1 - tests/qapi-schema/reserved-member-u.out | 14 -------------- 4 files changed, 6 insertions(+), 20 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 2fcaaa2497..cf09fa9fd3 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -34,12 +34,10 @@ def check_name_is_str(name, info, source): def check_name_str(name, info, source, - allow_optional=False, enum_member=False, + enum_member=False, permit_upper=False): membername = name - if allow_optional and name.startswith('*'): - membername = name[1:] # Enum members can start with a digit, because the generated C # code always prefixes it with the enum name if enum_member and membername[0].isdigit(): @@ -52,7 +50,6 @@ def check_name_str(name, info, source, if not permit_upper and name.lower() != name: raise QAPISemError( info, "%s uses uppercase in name" % source) - assert not membername.startswith('*') def check_defn_name_str(name, info, meta): @@ -171,8 +168,10 @@ def check_type(value, info, source, # value is a dictionary, check that each member is okay for (key, arg) in value.items(): key_source = "%s member '%s'" % (source, key) + if key.startswith('*'): + key = key[1:] check_name_str(key, info, key_source, - allow_optional=True, permit_upper=permit_upper) + permit_upper=permit_upper) 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/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err index e69de29bb2..b58e599a00 100644 --- a/tests/qapi-schema/reserved-member-u.err +++ b/tests/qapi-schema/reserved-member-u.err @@ -0,0 +1,2 @@ +reserved-member-u.json: In struct 'Oops': +reserved-member-u.json:7: 'data' member '*u' uses reserved name diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json index 15005abb09..2bfb8f59b6 100644 --- a/tests/qapi-schema/reserved-member-u.json +++ b/tests/qapi-schema/reserved-member-u.json @@ -4,5 +4,4 @@ # This is true even for non-unions, because it is possible to convert a # struct to flat union while remaining backwards compatible in QMP. # TODO - we could munge the member name to 'q_u' to avoid the collision -# BUG: not rejected { 'struct': 'Oops', 'data': { '*u': 'str' } } diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out index 6a3705518b..e69de29bb2 100644 --- a/tests/qapi-schema/reserved-member-u.out +++ b/tests/qapi-schema/reserved-member-u.out @@ -1,14 +0,0 @@ -module ./builtin -object q_empty -enum QType - prefix QTYPE - member none - member qnull - member qnum - member qstring - member qdict - member qlist - member qbool -module reserved-member-u.json -object Oops - member u: str optional=True From 5fbc78dd3675832062894aeca89a52c90a96f954 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:05 +0100 Subject: [PATCH 08/29] qapi: Permit flat union members for any tag value Flat union branch names match the tag enum's member names. Omitted branches default to "no members for this tag value". Branch names starting with a digit get rejected like "'data' member '0' has an invalid name". However, omitting the branch works. This is because flat union tag values get checked twice: as enum member name, and as union branch name. The former accepts leading digits, the latter doesn't. Branches whose names start with a digit therefore cannot have members. Feels wrong. Get rid of the restriction by skipping the latter check. This can expose c_name() to input it can't handle: a name starting with a digit. Improve it to return a valid C identifier for any input. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-9-armbru@redhat.com> Reviewed-by: Eric Blake [Commit message rewritten] --- scripts/qapi/common.py | 8 ++++---- scripts/qapi/expr.py | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 11b86beeab..cbd3fd81d3 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -18,7 +18,6 @@ from typing import Optional, Sequence #: Magic string that gets removed along with all space to its right. EATSPACE = '\033EATSPACE.' POINTER_SUFFIX = ' *' + EATSPACE -_C_NAME_TRANS = str.maketrans('.-', '__') def camel_to_upper(value: str) -> str: @@ -109,9 +108,10 @@ def c_name(name: str, protect: bool = True) -> str: 'not_eq', 'or', 'or_eq', 'xor', 'xor_eq']) # namespace pollution: polluted_words = set(['unix', 'errno', 'mips', 'sparc', 'i386']) - name = name.translate(_C_NAME_TRANS) - if protect and (name in c89_words | c99_words | c11_words | gcc_words - | cpp_words | polluted_words): + name = re.sub(r'[^A-Za-z0-9_]', '_', name) + if protect and (name in (c89_words | c99_words | c11_words | gcc_words + | cpp_words | polluted_words) + or name[0].isdigit()): return 'q_' + name return name diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index cf09fa9fd3..507550c340 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -246,7 +246,9 @@ def check_union(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key - check_name_str(key, info, source) + if discriminator is None: + check_name_str(key, info, source) + # else: name is in discriminator enum, which gets checked check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) check_type(value['type'], info, source, allow_array=not base) From 0825f62c842f2c07c5471391c6d7fd3f4fe83732 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:06 +0100 Subject: [PATCH 09/29] qapi: Lift enum-specific code out of check_name_str() check_name_str() masks leading digits when passed enum_member=True. Only check_enum() does. Lift the masking into check_enum(). Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-10-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 507550c340..e00467636c 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -34,18 +34,11 @@ def check_name_is_str(name, info, source): def check_name_str(name, info, source, - enum_member=False, permit_upper=False): - membername = name - - # Enum members can start with a digit, because the generated C - # code always prefixes it with the enum name - if enum_member and membername[0].isdigit(): - membername = 'D' + membername # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' # and 'q_obj_*' implicit type names. - if not valid_name.match(membername) or \ - c_name(membername, False).startswith('q_'): + if not valid_name.match(name) or \ + c_name(name, False).startswith('q_'): raise QAPISemError(info, "%s has an invalid name" % source) if not permit_upper and name.lower() != name: raise QAPISemError( @@ -213,11 +206,15 @@ def check_enum(expr, info): for m in members] for member in members: source = "'data' member" + member_name = member['name'] check_keys(member, info, source, ['name'], ['if']) - check_name_is_str(member['name'], info, source) - source = "%s '%s'" % (source, member['name']) - check_name_str(member['name'], info, source, - enum_member=True, permit_upper=permit_upper) + check_name_is_str(member_name, info, source) + source = "%s '%s'" % (source, member_name) + # Enum members may start with a digit + if member_name[0].isdigit(): + member_name = 'd' + member_name # Hack: hide the digit + check_name_str(member_name, info, source, + permit_upper=permit_upper) check_if(member, info, source) From eaab06faa5540e02e4f4782c1a650c9805a36671 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:07 +0100 Subject: [PATCH 10/29] qapi: Rework name checking in preparation of stricter checking Naming rules differ for the various kinds of names. To prepare enforcing them, define functions to check them: check_name_upper(), check_name_lower(), and check_name_camel(). For now, these merely wrap around check_name_str(), but that will change shortly. Replace the other uses of check_name_str() by appropriate uses of the wrappers. No change in behavior just yet. check_name_str() now returns the name without downstream and x- prefix, for use by the wrappers in later patches. Requires tweaking regexp @valid_name. It accepts the same strings as before. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-11-armbru@redhat.com> Reviewed-by: Eric Blake [Commit message improved] --- scripts/qapi/expr.py | 51 +++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index e00467636c..30285fe334 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -21,11 +21,12 @@ from .common import c_name from .error import QAPISemError -# Names must be letters, numbers, -, and _. They must start with letter, -# except for downstream extensions which must start with __RFQDN_. -# Dots are only valid in the downstream extension prefix. -valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' - '[a-zA-Z][a-zA-Z0-9_-]*$') +# Names consist of letters, digits, -, and _, starting with a letter. +# An experimental name is prefixed with x-. A name of a downstream +# extension is prefixed with __RFQDN_. The latter prefix goes first. +valid_name = re.compile(r'(__[a-z0-9.-]+_)?' + r'(x-)?' + r'([a-z][a-z0-9_-]*)$', re.IGNORECASE) def check_name_is_str(name, info, source): @@ -37,16 +38,38 @@ def check_name_str(name, info, source, permit_upper=False): # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' # and 'q_obj_*' implicit type names. - if not valid_name.match(name) or \ - c_name(name, False).startswith('q_'): + match = valid_name.match(name) + if not match or c_name(name, False).startswith('q_'): raise QAPISemError(info, "%s has an invalid name" % source) if not permit_upper and name.lower() != name: raise QAPISemError( info, "%s uses uppercase in name" % source) + return match.group(3) + + +def check_name_upper(name, info, source): + stem = check_name_str(name, info, source, permit_upper=True) + # TODO reject '[a-z-]' in @stem + + +def check_name_lower(name, info, source, + permit_upper=False): + stem = check_name_str(name, info, source, permit_upper) + # TODO reject '_' in stem + + +def check_name_camel(name, info, source): + stem = check_name_str(name, info, source, permit_upper=True) + # TODO reject '[_-]' in stem, require CamelCase def check_defn_name_str(name, info, meta): - check_name_str(name, info, meta, permit_upper=True) + if meta == 'event': + check_name_upper(name, info, meta) + elif meta == 'command': + check_name_lower(name, info, meta, permit_upper=True) + else: + check_name_camel(name, info, meta) if name.endswith('Kind') or name.endswith('List'): raise QAPISemError( info, "%s name should not end in '%s'" % (meta, name[-4:])) @@ -163,8 +186,7 @@ def check_type(value, info, source, key_source = "%s member '%s'" % (source, key) if key.startswith('*'): key = key[1:] - check_name_str(key, info, key_source, - permit_upper=permit_upper) + check_name_lower(key, info, key_source, permit_upper) 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']) @@ -186,7 +208,7 @@ def check_features(features, info): check_keys(f, info, source, ['name'], ['if']) check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) - check_name_str(f['name'], info, source) + check_name_lower(f['name'], info, source) check_if(f, info, source) @@ -213,8 +235,7 @@ def check_enum(expr, info): # Enum members may start with a digit if member_name[0].isdigit(): member_name = 'd' + member_name # Hack: hide the digit - check_name_str(member_name, info, source, - permit_upper=permit_upper) + check_name_lower(member_name, info, source, permit_upper) check_if(member, info, source) @@ -244,7 +265,7 @@ def check_union(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key if discriminator is None: - check_name_str(key, info, source) + check_name_lower(key, info, source) # else: name is in discriminator enum, which gets checked check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) @@ -258,7 +279,7 @@ def check_alternate(expr, info): raise QAPISemError(info, "'data' must not be empty") for (key, value) in members.items(): source = "'data' member '%s'" % key - check_name_str(key, info, source) + check_name_lower(key, info, source) check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) check_type(value['type'], info, source) From d224e0c092653f0b9cff77ba6852147687b1bedb Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:08 +0100 Subject: [PATCH 11/29] qapi: Move uppercase rejection to check_name_lower() check_name_lower() is the only user of check_name_str() using permit_upper=False. Move the associated code from check_name_str() to check_name_lower(), and drop the parameter. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-12-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 30285fe334..a815060ee2 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -34,32 +34,31 @@ def check_name_is_str(name, info, source): raise QAPISemError(info, "%s requires a string name" % source) -def check_name_str(name, info, source, - permit_upper=False): +def check_name_str(name, info, source): # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' # and 'q_obj_*' implicit type names. match = valid_name.match(name) if not match or c_name(name, False).startswith('q_'): raise QAPISemError(info, "%s has an invalid name" % source) - if not permit_upper and name.lower() != name: - raise QAPISemError( - info, "%s uses uppercase in name" % source) return match.group(3) def check_name_upper(name, info, source): - stem = check_name_str(name, info, source, permit_upper=True) + stem = check_name_str(name, info, source) # TODO reject '[a-z-]' in @stem def check_name_lower(name, info, source, permit_upper=False): - stem = check_name_str(name, info, source, permit_upper) + stem = check_name_str(name, info, source) + if not permit_upper and name.lower() != name: + raise QAPISemError( + info, "%s uses uppercase in name" % source) # TODO reject '_' in stem def check_name_camel(name, info, source): - stem = check_name_str(name, info, source, permit_upper=True) + stem = check_name_str(name, info, source) # TODO reject '[_-]' in stem, require CamelCase From 00ffe242d64f7622965c52c62adb06fd9664ada8 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:09 +0100 Subject: [PATCH 12/29] qapi: Consistently permit any case in downstream prefixes We require lowercase __RFQDN_ downstream prefixes only where we require the prefixed name to be lowercase. Don't; permit any case in __RFQDN_ prefixes anywhere. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-13-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index a815060ee2..b5fb0be48b 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -51,7 +51,7 @@ def check_name_upper(name, info, source): def check_name_lower(name, info, source, permit_upper=False): stem = check_name_str(name, info, source) - if not permit_upper and name.lower() != name: + if not permit_upper and re.search(r'[A-Z]', stem): raise QAPISemError( info, "%s uses uppercase in name" % source) # TODO reject '_' in stem From d4f4cae8de19d2bdfcf09cdc4676e9b99857dcf2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:10 +0100 Subject: [PATCH 13/29] qapi: Enforce event naming rules Event names should be ALL_CAPS with words separated by underscore. Enforce this. The only offenders are in tests/. Fix them. Existing test event-case covers the new error. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-14-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 4 +++- tests/qapi-schema/doc-good.json | 4 ++-- tests/qapi-schema/doc-good.out | 4 ++-- tests/qapi-schema/doc-good.txt | 2 +- tests/qapi-schema/doc-invalid-return.json | 4 ++-- tests/qapi-schema/event-case.err | 2 ++ tests/qapi-schema/event-case.json | 2 -- tests/qapi-schema/event-case.out | 14 -------------- tests/qapi-schema/qapi-schema-test.json | 6 +++--- tests/qapi-schema/qapi-schema-test.out | 8 ++++---- tests/unit/test-qmp-event.c | 6 +++--- 11 files changed, 22 insertions(+), 34 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index b5fb0be48b..c065505b27 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -45,7 +45,9 @@ def check_name_str(name, info, source): def check_name_upper(name, info, source): stem = check_name_str(name, info, source) - # TODO reject '[a-z-]' in @stem + if re.search(r'[a-z-]', stem): + raise QAPISemError( + info, "name of %s must not use lowercase or '-'" % source) def check_name_lower(name, info, source, diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json index e9af0857db..423ea23e07 100644 --- a/tests/qapi-schema/doc-good.json +++ b/tests/qapi-schema/doc-good.json @@ -179,10 +179,10 @@ 'features': [ 'cmd-feat1', 'cmd-feat2' ] } ## -# @EVT-BOXED: +# @EVT_BOXED: # Features: # @feat3: a feature ## -{ 'event': 'EVT-BOXED', 'boxed': true, +{ 'event': 'EVT_BOXED', 'boxed': true, 'features': [ 'feat3' ], 'data': 'Object' } diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 715b0bbc1a..8f54ceff2e 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -63,7 +63,7 @@ command cmd-boxed Object -> None gen=True success_response=True boxed=True oob=False preconfig=False feature cmd-feat1 feature cmd-feat2 -event EVT-BOXED Object +event EVT_BOXED Object boxed=True feature feat3 doc freeform @@ -211,7 +211,7 @@ another feature -> in <- out -doc symbol=EVT-BOXED +doc symbol=EVT_BOXED body= feature=feat3 diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt index 6ca03d49d0..726727af74 100644 --- a/tests/qapi-schema/doc-good.txt +++ b/tests/qapi-schema/doc-good.txt @@ -272,7 +272,7 @@ Example <- out -"EVT-BOXED" (Event) +"EVT_BOXED" (Event) ------------------- diff --git a/tests/qapi-schema/doc-invalid-return.json b/tests/qapi-schema/doc-invalid-return.json index 1ba45de414..95e7583930 100644 --- a/tests/qapi-schema/doc-invalid-return.json +++ b/tests/qapi-schema/doc-invalid-return.json @@ -1,7 +1,7 @@ # Events can't have 'Returns' section ## -# @foo: +# @FOO: # Returns: blah ## -{ 'event': 'foo' } +{ 'event': 'FOO' } diff --git a/tests/qapi-schema/event-case.err b/tests/qapi-schema/event-case.err index e69de29bb2..d3007cfa63 100644 --- a/tests/qapi-schema/event-case.err +++ b/tests/qapi-schema/event-case.err @@ -0,0 +1,2 @@ +event-case.json: In event 'oops': +event-case.json:1: name of event must not use lowercase or '-' diff --git a/tests/qapi-schema/event-case.json b/tests/qapi-schema/event-case.json index 3a92d8b610..4d8a5d8a71 100644 --- a/tests/qapi-schema/event-case.json +++ b/tests/qapi-schema/event-case.json @@ -1,3 +1 @@ -# TODO: might be nice to enforce naming conventions; but until then this works -# even though events should usually be ALL_CAPS { 'event': 'oops' } diff --git a/tests/qapi-schema/event-case.out b/tests/qapi-schema/event-case.out index 9ae44052ac..e69de29bb2 100644 --- a/tests/qapi-schema/event-case.out +++ b/tests/qapi-schema/event-case.out @@ -1,14 +0,0 @@ -module ./builtin -object q_empty -enum QType - prefix QTYPE - member none - member qnull - member qnum - member qstring - member qdict - member qlist - member qbool -module event-case.json -event oops None - boxed=False diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 12ec588b52..a355321258 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -249,7 +249,7 @@ { 'command': 'TestCmdReturnDefThree', 'returns': 'UserDefThree' } -{ 'event': 'TestIfEvent', 'data': +{ 'event': 'TEST_IF_EVENT', 'data': { 'foo': 'TestIfStruct', 'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } }, 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' } @@ -324,8 +324,8 @@ 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'] } ] } -{ 'event': 'TEST-EVENT-FEATURES0', +{ 'event': 'TEST_EVENT_FEATURES0', 'data': 'FeatureStruct1' } -{ 'event': 'TEST-EVENT-FEATURES1', +{ 'event': 'TEST_EVENT_FEATURES1', 'features': [ 'deprecated' ] } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index f5741df97f..882d0e7c56 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -349,12 +349,12 @@ command TestCmdReturnDefThree None -> UserDefThree gen=True success_response=True boxed=False oob=False preconfig=False array TestIfEnumList TestIfEnum if ['defined(TEST_IF_ENUM)'] -object q_obj_TestIfEvent-arg +object q_obj_TEST_IF_EVENT-arg member foo: TestIfStruct optional=False member bar: TestIfEnumList optional=False if ['defined(TEST_IF_EVT_BAR)'] if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'] -event TestIfEvent q_obj_TestIfEvent-arg +event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg boxed=False if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)'] object FeatureStruct0 @@ -440,9 +440,9 @@ command test-command-cond-features3 None -> None gen=True success_response=True boxed=False oob=False preconfig=False feature feature1 if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'] -event TEST-EVENT-FEATURES0 FeatureStruct1 +event TEST_EVENT_FEATURES0 FeatureStruct1 boxed=False -event TEST-EVENT-FEATURES1 None +event TEST_EVENT_FEATURES1 None boxed=False feature deprecated module include/sub-module.json diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c index 047f44ff9a..d58c3b78f2 100644 --- a/tests/unit/test-qmp-event.c +++ b/tests/unit/test-qmp-event.c @@ -143,7 +143,7 @@ static void test_event_d(TestEventData *data, static void test_event_deprecated(TestEventData *data, const void *unused) { - data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES1' }"); + data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES1' }"); memset(&compat_policy, 0, sizeof(compat_policy)); @@ -163,7 +163,7 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused) { memset(&compat_policy, 0, sizeof(compat_policy)); - data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0'," + data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0'," " 'data': { 'foo': 42 } }"); qapi_event_send_test_event_features0(42); g_assert(data->emitted); @@ -172,7 +172,7 @@ static void test_event_deprecated_data(TestEventData *data, const void *unused) compat_policy.has_deprecated_output = true; compat_policy.deprecated_output = COMPAT_POLICY_OUTPUT_HIDE; - data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST-EVENT-FEATURES0' }"); + data->expect = qdict_from_jsonf_nofail("{ 'event': 'TEST_EVENT_FEATURES0' }"); qapi_event_send_test_event_features0(42); g_assert(data->emitted); From 3e6c8a633113fb6a60369c40cf2061de50727bf6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:11 +0100 Subject: [PATCH 14/29] qapi: Enforce type naming rules Type names should be CamelCase. Enforce this. The only offenders are in tests/. Fix them. Add test type-case to cover the new error. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-15-armbru@redhat.com> Reviewed-by: Eric Blake [Regexp simplified, new test made more robust] --- scripts/qapi/expr.py | 3 ++- tests/qapi-schema/doc-bad-union-member.json | 4 ++-- tests/qapi-schema/double-type.err | 2 +- tests/qapi-schema/double-type.json | 2 +- tests/qapi-schema/features-deprecated-type.err | 2 +- tests/qapi-schema/features-deprecated-type.json | 2 +- tests/qapi-schema/meson.build | 1 + tests/qapi-schema/redefined-builtin.err | 4 ++-- tests/qapi-schema/redefined-builtin.json | 4 ++-- tests/qapi-schema/redefined-type.err | 6 +++--- tests/qapi-schema/redefined-type.json | 4 ++-- tests/qapi-schema/struct-data-invalid.err | 2 +- tests/qapi-schema/struct-data-invalid.json | 2 +- tests/qapi-schema/struct-member-invalid-dict.err | 2 +- tests/qapi-schema/struct-member-invalid-dict.json | 2 +- tests/qapi-schema/struct-member-invalid.err | 2 +- tests/qapi-schema/struct-member-invalid.json | 2 +- tests/qapi-schema/type-case.err | 2 ++ tests/qapi-schema/type-case.json | 2 ++ tests/qapi-schema/type-case.out | 0 tests/qapi-schema/unknown-expr-key.err | 2 +- tests/qapi-schema/unknown-expr-key.json | 2 +- 22 files changed, 30 insertions(+), 24 deletions(-) create mode 100644 tests/qapi-schema/type-case.err create mode 100644 tests/qapi-schema/type-case.json create mode 100644 tests/qapi-schema/type-case.out diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index c065505b27..7bd15559de 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -61,7 +61,8 @@ def check_name_lower(name, info, source, def check_name_camel(name, info, source): stem = check_name_str(name, info, source) - # TODO reject '[_-]' in stem, require CamelCase + if not re.match(r'[A-Z][A-Za-z0-9]*[a-z][A-Za-z0-9]*$', stem): + raise QAPISemError(info, "name of %s must use CamelCase" % source) def check_defn_name_str(name, info, meta): diff --git a/tests/qapi-schema/doc-bad-union-member.json b/tests/qapi-schema/doc-bad-union-member.json index d611435f6a..bd231a0109 100644 --- a/tests/qapi-schema/doc-bad-union-member.json +++ b/tests/qapi-schema/doc-bad-union-member.json @@ -11,9 +11,9 @@ 'data': { 'nothing': 'Empty' } } { 'struct': 'Base', - 'data': { 'type': 'T' } } + 'data': { 'type': 'FrobType' } } { 'struct': 'Empty', 'data': { } } -{ 'enum': 'T', 'data': ['nothing'] } +{ 'enum': 'FrobType', 'data': ['nothing'] } diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err index 71fc4dbb52..576e716197 100644 --- a/tests/qapi-schema/double-type.err +++ b/tests/qapi-schema/double-type.err @@ -1,3 +1,3 @@ -double-type.json: In struct 'bar': +double-type.json: In struct 'Bar': double-type.json:2: struct has unknown key 'command' Valid keys are 'base', 'data', 'features', 'if', 'struct'. diff --git a/tests/qapi-schema/double-type.json b/tests/qapi-schema/double-type.json index 911fa7af50..2c0809f38d 100644 --- a/tests/qapi-schema/double-type.json +++ b/tests/qapi-schema/double-type.json @@ -1,2 +1,2 @@ # we reject an expression with ambiguous metatype -{ 'command': 'foo', 'struct': 'bar', 'data': { } } +{ 'command': 'foo', 'struct': 'Bar', 'data': { } } diff --git a/tests/qapi-schema/features-deprecated-type.err b/tests/qapi-schema/features-deprecated-type.err index af4ffe20aa..ddaedf604e 100644 --- a/tests/qapi-schema/features-deprecated-type.err +++ b/tests/qapi-schema/features-deprecated-type.err @@ -1,2 +1,2 @@ -features-deprecated-type.json: In struct 'S': +features-deprecated-type.json: In struct 'Foo': features-deprecated-type.json:2: feature 'deprecated' is not supported for types diff --git a/tests/qapi-schema/features-deprecated-type.json b/tests/qapi-schema/features-deprecated-type.json index 4b5bf5b86e..265849b1f7 100644 --- a/tests/qapi-schema/features-deprecated-type.json +++ b/tests/qapi-schema/features-deprecated-type.json @@ -1,3 +1,3 @@ # Feature 'deprecated' is not supported for types -{ 'struct': 'S', 'data': {}, +{ 'struct': 'Foo', 'data': {}, 'features': [ 'deprecated' ] } diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index d5fa035507..ba11cb76ac 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -180,6 +180,7 @@ schemas = [ 'trailing-comma-list.json', 'trailing-comma-object.json', 'type-bypass-bad-gen.json', + 'type-case.json', 'unclosed-list.json', 'unclosed-object.json', 'unclosed-string.json', diff --git a/tests/qapi-schema/redefined-builtin.err b/tests/qapi-schema/redefined-builtin.err index 58c7e42ffc..92bc62dc76 100644 --- a/tests/qapi-schema/redefined-builtin.err +++ b/tests/qapi-schema/redefined-builtin.err @@ -1,2 +1,2 @@ -redefined-builtin.json: In struct 'size': -redefined-builtin.json:2: built-in type 'size' is already defined +redefined-builtin.json: In struct 'QType': +redefined-builtin.json:2: enum type 'QType' is already defined diff --git a/tests/qapi-schema/redefined-builtin.json b/tests/qapi-schema/redefined-builtin.json index 45b8a550ad..cad555cc73 100644 --- a/tests/qapi-schema/redefined-builtin.json +++ b/tests/qapi-schema/redefined-builtin.json @@ -1,2 +1,2 @@ -# we reject types that duplicate builtin names -{ 'struct': 'size', 'data': { 'myint': 'size' } } +# we reject types that clash with predefined types +{ 'struct': 'QType', 'data': { 'myint': 'size' } } diff --git a/tests/qapi-schema/redefined-type.err b/tests/qapi-schema/redefined-type.err index b7103fc15f..5e5406f811 100644 --- a/tests/qapi-schema/redefined-type.err +++ b/tests/qapi-schema/redefined-type.err @@ -1,4 +1,4 @@ -redefined-type.json: In enum 'foo': -redefined-type.json:3: 'foo' is already defined -redefined-type.json: In struct 'foo': +redefined-type.json: In enum 'Foo': +redefined-type.json:3: 'Foo' is already defined +redefined-type.json: In struct 'Foo': redefined-type.json:2: previous definition diff --git a/tests/qapi-schema/redefined-type.json b/tests/qapi-schema/redefined-type.json index a09e768bae..291453e70d 100644 --- a/tests/qapi-schema/redefined-type.json +++ b/tests/qapi-schema/redefined-type.json @@ -1,3 +1,3 @@ # we reject types defined more than once -{ 'struct': 'foo', 'data': { 'one': 'str' } } -{ 'enum': 'foo', 'data': [ 'two' ] } +{ 'struct': 'Foo', 'data': { 'one': 'str' } } +{ 'enum': 'Foo', 'data': [ 'two' ] } diff --git a/tests/qapi-schema/struct-data-invalid.err b/tests/qapi-schema/struct-data-invalid.err index 5ed4bec573..23cbfc60ea 100644 --- a/tests/qapi-schema/struct-data-invalid.err +++ b/tests/qapi-schema/struct-data-invalid.err @@ -1,2 +1,2 @@ -struct-data-invalid.json: In struct 'foo': +struct-data-invalid.json: In struct 'Foo': struct-data-invalid.json:1: 'data' should be an object or type name diff --git a/tests/qapi-schema/struct-data-invalid.json b/tests/qapi-schema/struct-data-invalid.json index 9adbc3bb6b..00ad11ef94 100644 --- a/tests/qapi-schema/struct-data-invalid.json +++ b/tests/qapi-schema/struct-data-invalid.json @@ -1,2 +1,2 @@ -{ 'struct': 'foo', +{ 'struct': 'Foo', 'data': false } diff --git a/tests/qapi-schema/struct-member-invalid-dict.err b/tests/qapi-schema/struct-member-invalid-dict.err index f9b3f33551..517793cc9b 100644 --- a/tests/qapi-schema/struct-member-invalid-dict.err +++ b/tests/qapi-schema/struct-member-invalid-dict.err @@ -1,2 +1,2 @@ -struct-member-invalid-dict.json: In struct 'foo': +struct-member-invalid-dict.json: In struct 'Foo': struct-member-invalid-dict.json:3: 'data' member '*a' misses key 'type' diff --git a/tests/qapi-schema/struct-member-invalid-dict.json b/tests/qapi-schema/struct-member-invalid-dict.json index bc3d62ae63..df5d018f65 100644 --- a/tests/qapi-schema/struct-member-invalid-dict.json +++ b/tests/qapi-schema/struct-member-invalid-dict.json @@ -1,4 +1,4 @@ # struct 'data' member with dict value is (longhand) member # definition, not inline complex type -{ 'struct': 'foo', +{ 'struct': 'Foo', 'data': { '*a': { 'case': 'foo' } } } diff --git a/tests/qapi-schema/struct-member-invalid.err b/tests/qapi-schema/struct-member-invalid.err index 9a2c934538..7e01a41d7c 100644 --- a/tests/qapi-schema/struct-member-invalid.err +++ b/tests/qapi-schema/struct-member-invalid.err @@ -1,2 +1,2 @@ -struct-member-invalid.json: In struct 'foo': +struct-member-invalid.json: In struct 'Foo': struct-member-invalid.json:1: 'data' member 'a' should be a type name diff --git a/tests/qapi-schema/struct-member-invalid.json b/tests/qapi-schema/struct-member-invalid.json index 8f172f7a87..a4cd860c67 100644 --- a/tests/qapi-schema/struct-member-invalid.json +++ b/tests/qapi-schema/struct-member-invalid.json @@ -1,2 +1,2 @@ -{ 'struct': 'foo', +{ 'struct': 'Foo', 'data': { 'a': false } } diff --git a/tests/qapi-schema/type-case.err b/tests/qapi-schema/type-case.err new file mode 100644 index 0000000000..36d2de2d00 --- /dev/null +++ b/tests/qapi-schema/type-case.err @@ -0,0 +1,2 @@ +type-case.json: In struct 'not-a-camel': +type-case.json:2: name of struct must use CamelCase diff --git a/tests/qapi-schema/type-case.json b/tests/qapi-schema/type-case.json new file mode 100644 index 0000000000..a43c68e7eb --- /dev/null +++ b/tests/qapi-schema/type-case.json @@ -0,0 +1,2 @@ +# Type names should use CamelCase +{ 'struct': 'not-a-camel', 'data': {} } diff --git a/tests/qapi-schema/type-case.out b/tests/qapi-schema/type-case.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err index c5f395bf79..f2538e3ce7 100644 --- a/tests/qapi-schema/unknown-expr-key.err +++ b/tests/qapi-schema/unknown-expr-key.err @@ -1,3 +1,3 @@ -unknown-expr-key.json: In struct 'bar': +unknown-expr-key.json: In struct 'Bar': unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony' Valid keys are 'base', 'data', 'features', 'if', 'struct'. diff --git a/tests/qapi-schema/unknown-expr-key.json b/tests/qapi-schema/unknown-expr-key.json index 13292d75ed..8003a0c36e 100644 --- a/tests/qapi-schema/unknown-expr-key.json +++ b/tests/qapi-schema/unknown-expr-key.json @@ -1,2 +1,2 @@ # we reject an expression with unknown top-level keys -{ 'struct': 'bar', 'data': { 'string': 'str'}, 'bogus': { }, 'phony': { } } +{ 'struct': 'Bar', 'data': { 'string': 'str'}, 'bogus': { }, 'phony': { } } From 492db12ec3b42be6f971ba8436e080bc096b58b5 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:12 +0100 Subject: [PATCH 15/29] tests/qapi-schema: Rename redefined-builtin to redefined-predefined The previous commit changed this test to clash with a predefined enum type, not a built-in type. Adjust its name. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-16-armbru@redhat.com> Reviewed-by: Eric Blake --- tests/qapi-schema/meson.build | 2 +- tests/qapi-schema/redefined-builtin.err | 2 -- tests/qapi-schema/redefined-predefined.err | 2 ++ .../{redefined-builtin.json => redefined-predefined.json} | 0 .../{redefined-builtin.out => redefined-predefined.out} | 0 5 files changed, 3 insertions(+), 3 deletions(-) delete mode 100644 tests/qapi-schema/redefined-builtin.err create mode 100644 tests/qapi-schema/redefined-predefined.err rename tests/qapi-schema/{redefined-builtin.json => redefined-predefined.json} (100%) rename tests/qapi-schema/{redefined-builtin.out => redefined-predefined.out} (100%) diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index ba11cb76ac..664f9ee22d 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -152,9 +152,9 @@ schemas = [ 'pragma-returns-whitelist-crap.json', 'qapi-schema-test.json', 'quoted-structural-chars.json', - 'redefined-builtin.json', 'redefined-command.json', 'redefined-event.json', + 'redefined-predefined.json', 'redefined-type.json', 'reserved-command-q.json', 'reserved-enum-q.json', diff --git a/tests/qapi-schema/redefined-builtin.err b/tests/qapi-schema/redefined-builtin.err deleted file mode 100644 index 92bc62dc76..0000000000 --- a/tests/qapi-schema/redefined-builtin.err +++ /dev/null @@ -1,2 +0,0 @@ -redefined-builtin.json: In struct 'QType': -redefined-builtin.json:2: enum type 'QType' is already defined diff --git a/tests/qapi-schema/redefined-predefined.err b/tests/qapi-schema/redefined-predefined.err new file mode 100644 index 0000000000..2924dde60b --- /dev/null +++ b/tests/qapi-schema/redefined-predefined.err @@ -0,0 +1,2 @@ +redefined-predefined.json: In struct 'QType': +redefined-predefined.json:2: enum type 'QType' is already defined diff --git a/tests/qapi-schema/redefined-builtin.json b/tests/qapi-schema/redefined-predefined.json similarity index 100% rename from tests/qapi-schema/redefined-builtin.json rename to tests/qapi-schema/redefined-predefined.json diff --git a/tests/qapi-schema/redefined-builtin.out b/tests/qapi-schema/redefined-predefined.out similarity index 100% rename from tests/qapi-schema/redefined-builtin.out rename to tests/qapi-schema/redefined-predefined.out From 4a67bd31a4a45773ed1e33ebd06ff949ff9525d7 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:13 +0100 Subject: [PATCH 16/29] qapi: Factor out QAPISchemaParser._check_pragma_list_of_str() Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-17-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/parser.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 116afe549a..8eed69333f 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -119,6 +119,13 @@ class QAPISchemaParser: return QAPISchemaParser(incl_fname, previously_included, info) + def _check_pragma_list_of_str(self, name, value, info): + if (not isinstance(value, list) + or any([not isinstance(elt, str) for elt in value])): + raise QAPISemError( + info, + "pragma %s must be a list of strings" % name) + def _pragma(self, name, value, info): if name == 'doc-required': if not isinstance(value, bool): @@ -126,18 +133,10 @@ class QAPISchemaParser: "pragma 'doc-required' must be boolean") info.pragma.doc_required = value elif name == 'returns-whitelist': - if (not isinstance(value, list) - or any([not isinstance(elt, str) for elt in value])): - raise QAPISemError( - info, - "pragma returns-whitelist must be a list of strings") + self._check_pragma_list_of_str(name, value, info) info.pragma.returns_whitelist = value elif name == 'name-case-whitelist': - if (not isinstance(value, list) - or any([not isinstance(elt, str) for elt in value])): - raise QAPISemError( - info, - "pragma name-case-whitelist must be a list of strings") + self._check_pragma_list_of_str(name, value, info) info.pragma.name_case_whitelist = value else: raise QAPISemError(info, "unknown pragma '%s'" % name) From e90a61e3cc1ab30a2069173aee8b592933d827a1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:14 +0100 Subject: [PATCH 17/29] tests/qapi-schema: Rename pragma-*-crap to pragma-value-not-* Rename pragma-doc-required-crap to pragma-not-bool, pragma-returns-whitelist-crap to pragma-value-not-list, and pragma-name-case-whitelist-crap to pragma-value-not-list-of-str. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-18-armbru@redhat.com> Reviewed-by: Eric Blake --- tests/qapi-schema/meson.build | 6 +++--- tests/qapi-schema/pragma-doc-required-crap.err | 1 - tests/qapi-schema/pragma-name-case-whitelist-crap.err | 1 - tests/qapi-schema/pragma-returns-whitelist-crap.err | 1 - tests/qapi-schema/pragma-value-not-bool.err | 1 + ...ma-doc-required-crap.json => pragma-value-not-bool.json} | 2 +- ...agma-doc-required-crap.out => pragma-value-not-bool.out} | 0 tests/qapi-schema/pragma-value-not-list-of-str.err | 1 + ...hitelist-crap.json => pragma-value-not-list-of-str.json} | 2 +- ...-whitelist-crap.out => pragma-value-not-list-of-str.out} | 0 tests/qapi-schema/pragma-value-not-list.err | 1 + ...-case-whitelist-crap.json => pragma-value-not-list.json} | 2 +- ...returns-whitelist-crap.out => pragma-value-not-list.out} | 0 13 files changed, 9 insertions(+), 9 deletions(-) delete mode 100644 tests/qapi-schema/pragma-doc-required-crap.err delete mode 100644 tests/qapi-schema/pragma-name-case-whitelist-crap.err delete mode 100644 tests/qapi-schema/pragma-returns-whitelist-crap.err create mode 100644 tests/qapi-schema/pragma-value-not-bool.err rename tests/qapi-schema/{pragma-doc-required-crap.json => pragma-value-not-bool.json} (55%) rename tests/qapi-schema/{pragma-doc-required-crap.out => pragma-value-not-bool.out} (100%) create mode 100644 tests/qapi-schema/pragma-value-not-list-of-str.err rename tests/qapi-schema/{pragma-returns-whitelist-crap.json => pragma-value-not-list-of-str.json} (57%) rename tests/qapi-schema/{pragma-name-case-whitelist-crap.out => pragma-value-not-list-of-str.out} (100%) create mode 100644 tests/qapi-schema/pragma-value-not-list.err rename tests/qapi-schema/{pragma-name-case-whitelist-crap.json => pragma-value-not-list.json} (50%) rename tests/qapi-schema/{pragma-returns-whitelist-crap.out => pragma-value-not-list.out} (100%) diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index 664f9ee22d..ffc276b765 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -144,12 +144,12 @@ schemas = [ 'oob-coroutine.json', 'oob-test.json', 'allow-preconfig-test.json', - 'pragma-doc-required-crap.json', 'pragma-extra-junk.json', - 'pragma-name-case-whitelist-crap.json', 'pragma-non-dict.json', 'pragma-unknown.json', - 'pragma-returns-whitelist-crap.json', + 'pragma-value-not-bool.json', + 'pragma-value-not-list-of-str.json', + 'pragma-value-not-list.json', 'qapi-schema-test.json', 'quoted-structural-chars.json', 'redefined-command.json', diff --git a/tests/qapi-schema/pragma-doc-required-crap.err b/tests/qapi-schema/pragma-doc-required-crap.err deleted file mode 100644 index 717062cb14..0000000000 --- a/tests/qapi-schema/pragma-doc-required-crap.err +++ /dev/null @@ -1 +0,0 @@ -pragma-doc-required-crap.json:3: pragma 'doc-required' must be boolean diff --git a/tests/qapi-schema/pragma-name-case-whitelist-crap.err b/tests/qapi-schema/pragma-name-case-whitelist-crap.err deleted file mode 100644 index fbea90d6c5..0000000000 --- a/tests/qapi-schema/pragma-name-case-whitelist-crap.err +++ /dev/null @@ -1 +0,0 @@ -pragma-name-case-whitelist-crap.json:3: pragma name-case-whitelist must be a list of strings diff --git a/tests/qapi-schema/pragma-returns-whitelist-crap.err b/tests/qapi-schema/pragma-returns-whitelist-crap.err deleted file mode 100644 index 69784259df..0000000000 --- a/tests/qapi-schema/pragma-returns-whitelist-crap.err +++ /dev/null @@ -1 +0,0 @@ -pragma-returns-whitelist-crap.json:3: pragma returns-whitelist must be a list of strings diff --git a/tests/qapi-schema/pragma-value-not-bool.err b/tests/qapi-schema/pragma-value-not-bool.err new file mode 100644 index 0000000000..6247539616 --- /dev/null +++ b/tests/qapi-schema/pragma-value-not-bool.err @@ -0,0 +1 @@ +pragma-value-not-bool.json:3: pragma 'doc-required' must be boolean diff --git a/tests/qapi-schema/pragma-doc-required-crap.json b/tests/qapi-schema/pragma-value-not-bool.json similarity index 55% rename from tests/qapi-schema/pragma-doc-required-crap.json rename to tests/qapi-schema/pragma-value-not-bool.json index ed763c5ffc..feb489f15b 100644 --- a/tests/qapi-schema/pragma-doc-required-crap.json +++ b/tests/qapi-schema/pragma-value-not-bool.json @@ -1,3 +1,3 @@ -# 'doc-required' must be bool +# pragma value must be bool { 'pragma': { 'doc-required': {} } } diff --git a/tests/qapi-schema/pragma-doc-required-crap.out b/tests/qapi-schema/pragma-value-not-bool.out similarity index 100% rename from tests/qapi-schema/pragma-doc-required-crap.out rename to tests/qapi-schema/pragma-value-not-bool.out diff --git a/tests/qapi-schema/pragma-value-not-list-of-str.err b/tests/qapi-schema/pragma-value-not-list-of-str.err new file mode 100644 index 0000000000..44870fe5d9 --- /dev/null +++ b/tests/qapi-schema/pragma-value-not-list-of-str.err @@ -0,0 +1 @@ +pragma-value-not-list-of-str.json:3: pragma returns-whitelist must be a list of strings diff --git a/tests/qapi-schema/pragma-returns-whitelist-crap.json b/tests/qapi-schema/pragma-value-not-list-of-str.json similarity index 57% rename from tests/qapi-schema/pragma-returns-whitelist-crap.json rename to tests/qapi-schema/pragma-value-not-list-of-str.json index f6b81b093f..12bbbed2e8 100644 --- a/tests/qapi-schema/pragma-returns-whitelist-crap.json +++ b/tests/qapi-schema/pragma-value-not-list-of-str.json @@ -1,3 +1,3 @@ -# 'returns-whitelist' must be list of strings +# pragma value must be list of strings { 'pragma': { 'returns-whitelist': [ 'good', [ 'bad' ] ] } } diff --git a/tests/qapi-schema/pragma-name-case-whitelist-crap.out b/tests/qapi-schema/pragma-value-not-list-of-str.out similarity index 100% rename from tests/qapi-schema/pragma-name-case-whitelist-crap.out rename to tests/qapi-schema/pragma-value-not-list-of-str.out diff --git a/tests/qapi-schema/pragma-value-not-list.err b/tests/qapi-schema/pragma-value-not-list.err new file mode 100644 index 0000000000..21736c5723 --- /dev/null +++ b/tests/qapi-schema/pragma-value-not-list.err @@ -0,0 +1 @@ +pragma-value-not-list.json:3: pragma name-case-whitelist must be a list of strings diff --git a/tests/qapi-schema/pragma-name-case-whitelist-crap.json b/tests/qapi-schema/pragma-value-not-list.json similarity index 50% rename from tests/qapi-schema/pragma-name-case-whitelist-crap.json rename to tests/qapi-schema/pragma-value-not-list.json index 734e1c617b..2c61a97dd3 100644 --- a/tests/qapi-schema/pragma-name-case-whitelist-crap.json +++ b/tests/qapi-schema/pragma-value-not-list.json @@ -1,3 +1,3 @@ -# 'name-case-whitelist' must be list of strings +# pragma value must be list { 'pragma': { 'name-case-whitelist': false } } diff --git a/tests/qapi-schema/pragma-returns-whitelist-crap.out b/tests/qapi-schema/pragma-value-not-list.out similarity index 100% rename from tests/qapi-schema/pragma-returns-whitelist-crap.out rename to tests/qapi-schema/pragma-value-not-list.out From ef8b3829f6d194c856d7db34e14117e8ed90a396 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:15 +0100 Subject: [PATCH 18/29] tests/qapi-schema: Rename returns-whitelist to returns-bad-type This test covers returning "bad" types. Pragma returns-whitelist is just one aspect. Naming it returns-whitelist is suboptimal. Rename to returns-bad-type. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-19-armbru@redhat.com> Reviewed-by: Eric Blake --- tests/qapi-schema/meson.build | 2 +- tests/qapi-schema/returns-bad-type.err | 2 ++ .../{returns-whitelist.json => returns-bad-type.json} | 0 .../qapi-schema/{returns-whitelist.out => returns-bad-type.out} | 0 tests/qapi-schema/returns-whitelist.err | 2 -- 5 files changed, 3 insertions(+), 3 deletions(-) create mode 100644 tests/qapi-schema/returns-bad-type.err rename tests/qapi-schema/{returns-whitelist.json => returns-bad-type.json} (100%) rename tests/qapi-schema/{returns-whitelist.out => returns-bad-type.out} (100%) delete mode 100644 tests/qapi-schema/returns-whitelist.err diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index ffc276b765..4e7635f0a8 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -166,9 +166,9 @@ schemas = [ 'reserved-type-list.json', 'returns-alternate.json', 'returns-array-bad.json', + 'returns-bad-type.json', 'returns-dict.json', 'returns-unknown.json', - 'returns-whitelist.json', 'string-code-point-31.json', 'string-code-point-127.json', 'struct-base-clash-deep.json', diff --git a/tests/qapi-schema/returns-bad-type.err b/tests/qapi-schema/returns-bad-type.err new file mode 100644 index 0000000000..2c270de9ad --- /dev/null +++ b/tests/qapi-schema/returns-bad-type.err @@ -0,0 +1,2 @@ +returns-bad-type.json: In command 'no-way-this-will-get-whitelisted': +returns-bad-type.json:14: command's 'returns' cannot take array type ['int'] diff --git a/tests/qapi-schema/returns-whitelist.json b/tests/qapi-schema/returns-bad-type.json similarity index 100% rename from tests/qapi-schema/returns-whitelist.json rename to tests/qapi-schema/returns-bad-type.json diff --git a/tests/qapi-schema/returns-whitelist.out b/tests/qapi-schema/returns-bad-type.out similarity index 100% rename from tests/qapi-schema/returns-whitelist.out rename to tests/qapi-schema/returns-bad-type.out diff --git a/tests/qapi-schema/returns-whitelist.err b/tests/qapi-schema/returns-whitelist.err deleted file mode 100644 index c6e46b9b86..0000000000 --- a/tests/qapi-schema/returns-whitelist.err +++ /dev/null @@ -1,2 +0,0 @@ -returns-whitelist.json: In command 'no-way-this-will-get-whitelisted': -returns-whitelist.json:14: command's 'returns' cannot take array type ['int'] From b86df374784897c58b965939c9913c2a6c590426 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:16 +0100 Subject: [PATCH 19/29] qapi: Rename pragma *-whitelist to *-exceptions Rename pragma returns-whitelist to command-returns-exceptions, and name-case-whitelist to member-name-case-exceptions. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-20-armbru@redhat.com> Reviewed-by: Eric Blake --- docs/devel/qapi-code-gen.txt | 21 ++++++++++--------- qapi/pragma.json | 4 ++-- qga/qapi-schema.json | 2 +- scripts/qapi/expr.py | 4 ++-- scripts/qapi/parser.py | 8 +++---- scripts/qapi/schema.py | 2 +- scripts/qapi/source.py | 8 +++---- tests/qapi-schema/enum-member-case.json | 2 +- .../pragma-value-not-list-of-str.err | 2 +- .../pragma-value-not-list-of-str.json | 2 +- tests/qapi-schema/pragma-value-not-list.err | 2 +- tests/qapi-schema/pragma-value-not-list.json | 3 +-- tests/qapi-schema/qapi-schema-test.json | 2 +- tests/qapi-schema/returns-bad-type.json | 2 +- 14 files changed, 32 insertions(+), 32 deletions(-) diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt index 6906a06ad2..23e9823019 100644 --- a/docs/devel/qapi-code-gen.txt +++ b/docs/devel/qapi-code-gen.txt @@ -147,9 +147,10 @@ prevent incomplete include files. === Pragma directives === Syntax: - PRAGMA = { 'pragma': { '*doc-required': BOOL, - '*returns-whitelist': [ STRING, ... ], - '*name-case-whitelist': [ STRING, ... ] } } + PRAGMA = { 'pragma': { + '*doc-required': BOOL, + '*command-returns-exceptions': [ STRING, ... ], + '*member-name-exceptions': [ STRING, ... ] } } The pragma directive lets you control optional generator behavior. @@ -159,11 +160,11 @@ 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 'returns-whitelist' takes a list of command names that may +Pragma 'command-returns-exceptions' takes a list of commands that may violate the rules on permitted return types. Default is none. -Pragma 'name-case-whitelist' takes a list of names that may violate -rules on use of upper- vs. lower-case letters. Default is none. +Pragma 'member-name-exceptions' takes a list of types whose member +names may contain uppercase letters. Default is none. === Enumeration types === @@ -490,9 +491,9 @@ are the arguments. A union type requires 'boxed': true. Member 'returns' defines the command's return type. It defaults to an empty struct type. It must normally be a complex type or an array of a complex type. To return anything else, the command must be listed -in pragma 'returns-whitelist'. If you do this, extending the command -to return additional information will be harder. Use of -'returns-whitelist' for new commands is strongly discouraged. +in pragma 'commands-returns-exceptions'. If you do this, extending +the command to return additional information will be harder. Use of +the pragma for new commands is strongly discouraged. A command's error responses are not specified in the QAPI schema. Error conditions should be documented in comments. @@ -755,7 +756,7 @@ 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 'name-case-whitelist' lets you violate the rules on use of +Pragma 'member-name-exceptions' lets you violate the rules on use of upper and lower case. Use for new code is strongly discouraged. diff --git a/qapi/pragma.json b/qapi/pragma.json index 7f158e183d..4895848c5e 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -4,13 +4,13 @@ # add to them! { 'pragma': { # Commands allowed to return a non-dictionary: - 'returns-whitelist': [ + 'command-returns-exceptions': [ 'human-monitor-command', 'qom-get', 'query-tpm-models', 'query-tpm-types', 'ringbuf-read' ], - 'name-case-whitelist': [ + 'member-name-exceptions': [ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status 'BlockdevVmdkSubformat', # all members, to match VMDK spec spellings 'BlockdevVmdkAdapterType', # legacyESX, to match VMDK spec spellings diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 9a82b7e952..2b08b761c2 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -20,7 +20,7 @@ # add to them! { 'pragma': { # Commands allowed to return a non-dictionary: - 'returns-whitelist': [ + 'command-returns-exceptions': [ 'guest-file-open', 'guest-fsfreeze-freeze', 'guest-fsfreeze-freeze-list', diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 7bd15559de..85493efbac 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -181,7 +181,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.name_case_whitelist + permit_upper = allow_dict in info.pragma.member_name_exceptions # value is a dictionary, check that each member is okay for (key, arg) in value.items(): @@ -224,7 +224,7 @@ def check_enum(expr, info): if prefix is not None and not isinstance(prefix, str): raise QAPISemError(info, "'prefix' must be a string") - permit_upper = name in info.pragma.name_case_whitelist + permit_upper = name in info.pragma.member_name_exceptions members[:] = [m if isinstance(m, dict) else {'name': m} for m in members] diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 8eed69333f..c16b8b6995 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -132,12 +132,12 @@ class QAPISchemaParser: raise QAPISemError(info, "pragma 'doc-required' must be boolean") info.pragma.doc_required = value - elif name == 'returns-whitelist': + elif name == 'command-returns-exceptions': self._check_pragma_list_of_str(name, value, info) - info.pragma.returns_whitelist = value - elif name == 'name-case-whitelist': + info.pragma.command_returns_exceptions = value + elif name == 'member-name-exceptions': self._check_pragma_list_of_str(name, value, info) - info.pragma.name_case_whitelist = value + info.pragma.member_name_exceptions = value else: raise QAPISemError(info, "unknown pragma '%s'" % name) diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index ff16578f6d..703b446fd2 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -779,7 +779,7 @@ class QAPISchemaCommand(QAPISchemaEntity): if self._ret_type_name: self.ret_type = schema.resolve_type( self._ret_type_name, self.info, "command's 'returns'") - if self.name not in self.info.pragma.returns_whitelist: + if self.name not in self.info.pragma.command_returns_exceptions: typ = self.ret_type if isinstance(typ, QAPISchemaArrayType): typ = self.ret_type.element_type diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py index d7a79a9b8a..d8f9ec377f 100644 --- a/scripts/qapi/source.py +++ b/scripts/qapi/source.py @@ -21,10 +21,10 @@ class QAPISchemaPragma: def __init__(self) -> None: # Are documentation comments required? self.doc_required = False - # Whitelist of commands allowed to return a non-dictionary - self.returns_whitelist: List[str] = [] - # Whitelist of entities allowed to violate case conventions - self.name_case_whitelist: List[str] = [] + # Commands allowed to return a non-dictionary + self.command_returns_exceptions: List[str] = [] + # Types whose member names may violate case conventions + self.member_name_exceptions: List[str] = [] class QAPISourceInfo: diff --git a/tests/qapi-schema/enum-member-case.json b/tests/qapi-schema/enum-member-case.json index f8af3e4622..b8969d224d 100644 --- a/tests/qapi-schema/enum-member-case.json +++ b/tests/qapi-schema/enum-member-case.json @@ -1,4 +1,4 @@ # Member names should be 'lower-case' unless the enum is whitelisted -{ 'pragma': { 'name-case-whitelist': [ 'UuidInfo' ] } } +{ 'pragma': { 'member-name-exceptions': [ 'UuidInfo' ] } } { 'enum': 'UuidInfo', 'data': [ 'Value' ] } # UuidInfo is whitelisted { 'enum': 'NoWayThisWillGetWhitelisted', 'data': [ 'Value' ] } diff --git a/tests/qapi-schema/pragma-value-not-list-of-str.err b/tests/qapi-schema/pragma-value-not-list-of-str.err index 44870fe5d9..b81983dd85 100644 --- a/tests/qapi-schema/pragma-value-not-list-of-str.err +++ b/tests/qapi-schema/pragma-value-not-list-of-str.err @@ -1 +1 @@ -pragma-value-not-list-of-str.json:3: pragma returns-whitelist must be a list of strings +pragma-value-not-list-of-str.json:3: pragma command-returns-exceptions must be a list of strings diff --git a/tests/qapi-schema/pragma-value-not-list-of-str.json b/tests/qapi-schema/pragma-value-not-list-of-str.json index 12bbbed2e8..27811cff73 100644 --- a/tests/qapi-schema/pragma-value-not-list-of-str.json +++ b/tests/qapi-schema/pragma-value-not-list-of-str.json @@ -1,3 +1,3 @@ # pragma value must be list of strings -{ 'pragma': { 'returns-whitelist': [ 'good', [ 'bad' ] ] } } +{ 'pragma': { 'command-returns-exceptions': [ 'good', [ 'bad' ] ] } } diff --git a/tests/qapi-schema/pragma-value-not-list.err b/tests/qapi-schema/pragma-value-not-list.err index 21736c5723..45dde1bf05 100644 --- a/tests/qapi-schema/pragma-value-not-list.err +++ b/tests/qapi-schema/pragma-value-not-list.err @@ -1 +1 @@ -pragma-value-not-list.json:3: pragma name-case-whitelist must be a list of strings +pragma-value-not-list.json:2: pragma member-name-exceptions must be a list of strings diff --git a/tests/qapi-schema/pragma-value-not-list.json b/tests/qapi-schema/pragma-value-not-list.json index 2c61a97dd3..3faa340c3b 100644 --- a/tests/qapi-schema/pragma-value-not-list.json +++ b/tests/qapi-schema/pragma-value-not-list.json @@ -1,3 +1,2 @@ # pragma value must be list - -{ 'pragma': { 'name-case-whitelist': false } } +{ 'pragma': { 'member-name-exceptions': false } } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index a355321258..16b03bf308 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -7,7 +7,7 @@ # Whitelists to permit QAPI rule violations { 'pragma': { # Commands allowed to return a non-dictionary: - 'returns-whitelist': [ + 'command-returns-exceptions': [ 'guest-get-time', 'guest-sync' ] } } diff --git a/tests/qapi-schema/returns-bad-type.json b/tests/qapi-schema/returns-bad-type.json index da209329b1..0dd96e739e 100644 --- a/tests/qapi-schema/returns-bad-type.json +++ b/tests/qapi-schema/returns-bad-type.json @@ -1,6 +1,6 @@ # we enforce that 'returns' be a dict or array of dict unless whitelisted -{ 'pragma': { 'returns-whitelist': [ +{ 'pragma': { 'command-returns-exceptions': [ 'human-monitor-command', 'query-tpm-models', 'guest-get-time' ] } } { 'command': 'human-monitor-command', From b48a1033041c52c2ae12bd38a2caa36fe46ef466 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:17 +0100 Subject: [PATCH 20/29] qapi/pragma: Streamline comments on member-name-exceptions Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-21-armbru@redhat.com> Reviewed-by: Eric Blake --- qapi/pragma.json | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/qapi/pragma.json b/qapi/pragma.json index 4895848c5e..4c47c802d1 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -10,11 +10,13 @@ 'query-tpm-models', 'query-tpm-types', 'ringbuf-read' ], - 'member-name-exceptions': [ - 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status - 'BlockdevVmdkSubformat', # all members, to match VMDK spec spellings - 'BlockdevVmdkAdapterType', # legacyESX, to match VMDK spec spellings - 'QapiErrorClass', # all members, visible through errors - 'UuidInfo', # UUID, visible through query-uuid - 'X86CPURegister32' # all members, visible indirectly through qom-get + # Externally visible types whose member names may use uppercase + 'member-name-exceptions': [ # visible in: + 'ACPISlotType', # query-acpi-ospm-status + 'BlockdevVmdkAdapterType', # blockdev-create (to match VMDK spec) + 'BlockdevVmdkSubformat', # blockdev-create (to match VMDK spec) + 'QapiErrorClass', # QMP error replies + 'UuidInfo', # query-uuid + 'X86CPURegister32' # qom-get of x86 CPU properties + # feature-words, filtered-features ] } } From 6e2e12a70c0b7f7fe71a7938b9c49bdaa608ce58 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:18 +0100 Subject: [PATCH 21/29] tests-qmp-cmds: Drop unused and incorrect qmp_TestIfCmd() Commit 967c885108 "qapi: add 'if' to top-level expressions" added command TestIfCmd with an 'if' condition. It also added the qmp_TestIfCmd() to go with it, guarded by the corresponding #if. Commit ccadd6bcba "qapi: Add 'if' to implicit struct members" changed the command, but not the function. Compiles only because we don't satisfy the #if. Instead of fixing the function, simply drop it. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-22-armbru@redhat.com> Reviewed-by: Eric Blake --- tests/unit/test-qmp-cmds.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c index 266db074b4..99973dde7c 100644 --- a/tests/unit/test-qmp-cmds.c +++ b/tests/unit/test-qmp-cmds.c @@ -13,13 +13,6 @@ static QmpCommandList qmp_commands; -#if defined(TEST_IF_STRUCT) && defined(TEST_IF_CMD) -UserDefThree *qmp_TestIfCmd(TestIfStruct *foo, Error **errp) -{ - return NULL; -} -#endif - UserDefThree *qmp_TestCmdReturnDefThree(Error **errp) { return NULL; From 9af4b6b9e80daeab2ce47664ff422b5e421814de Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:19 +0100 Subject: [PATCH 22/29] qapi: Prepare for rejecting underscore in command and member names Command names and member names within a type should be all lower case with words separated by a hyphen. We also accept underscore. Rework check_name_lower() to optionally reject underscores, but don't use that option, yet. Update expected test output for the changed error message. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-23-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 24 ++++++++++++++---------- tests/qapi-schema/args-member-case.err | 2 +- tests/qapi-schema/enum-member-case.err | 2 +- tests/qapi-schema/union-branch-case.err | 2 +- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 85493efbac..e416cc5162 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -51,12 +51,13 @@ def check_name_upper(name, info, source): def check_name_lower(name, info, source, - permit_upper=False): + permit_upper=False, + permit_underscore=False): stem = check_name_str(name, info, source) - if not permit_upper and re.search(r'[A-Z]', stem): + if ((not permit_upper and re.search(r'[A-Z]', stem)) + or (not permit_underscore and '_' in stem)): raise QAPISemError( - info, "%s uses uppercase in name" % source) - # TODO reject '_' in stem + info, "name of %s must not use uppercase or '_'" % source) def check_name_camel(name, info, source): @@ -69,7 +70,8 @@ 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) + check_name_lower(name, info, meta, + permit_upper=True, permit_underscore=True) else: check_name_camel(name, info, meta) if name.endswith('Kind') or name.endswith('List'): @@ -188,7 +190,8 @@ def check_type(value, info, source, key_source = "%s member '%s'" % (source, key) if key.startswith('*'): key = key[1:] - check_name_lower(key, info, key_source, permit_upper) + check_name_lower(key, info, key_source, + permit_upper, permit_underscore=True) 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']) @@ -210,7 +213,7 @@ def check_features(features, info): check_keys(f, info, source, ['name'], ['if']) check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) - check_name_lower(f['name'], info, source) + check_name_lower(f['name'], info, source, permit_underscore=True) check_if(f, info, source) @@ -237,7 +240,8 @@ def check_enum(expr, info): # Enum members may start with a digit if member_name[0].isdigit(): member_name = 'd' + member_name # Hack: hide the digit - check_name_lower(member_name, info, source, permit_upper) + check_name_lower(member_name, info, source, + permit_upper, permit_underscore=True) check_if(member, info, source) @@ -267,7 +271,7 @@ def check_union(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key if discriminator is None: - check_name_lower(key, info, source) + check_name_lower(key, info, source, permit_underscore=True) # else: name is in discriminator enum, which gets checked check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) @@ -281,7 +285,7 @@ def check_alternate(expr, info): raise QAPISemError(info, "'data' must not be empty") for (key, value) in members.items(): source = "'data' member '%s'" % key - check_name_lower(key, info, source) + check_name_lower(key, info, source, permit_underscore=True) check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) check_type(value['type'], info, source) diff --git a/tests/qapi-schema/args-member-case.err b/tests/qapi-schema/args-member-case.err index 4f33dbbc38..25c3910a17 100644 --- a/tests/qapi-schema/args-member-case.err +++ b/tests/qapi-schema/args-member-case.err @@ -1,2 +1,2 @@ args-member-case.json: In command 'no-way-this-will-get-whitelisted': -args-member-case.json:2: 'data' member 'Arg' uses uppercase in name +args-member-case.json:2: name of 'data' member 'Arg' must not use uppercase or '_' diff --git a/tests/qapi-schema/enum-member-case.err b/tests/qapi-schema/enum-member-case.err index 8b3aefe37a..c813522c50 100644 --- a/tests/qapi-schema/enum-member-case.err +++ b/tests/qapi-schema/enum-member-case.err @@ -1,2 +1,2 @@ enum-member-case.json: In enum 'NoWayThisWillGetWhitelisted': -enum-member-case.json:4: 'data' member 'Value' uses uppercase in name +enum-member-case.json:4: name of 'data' member 'Value' must not use uppercase or '_' diff --git a/tests/qapi-schema/union-branch-case.err b/tests/qapi-schema/union-branch-case.err index b1e9417303..d2d5cb8993 100644 --- a/tests/qapi-schema/union-branch-case.err +++ b/tests/qapi-schema/union-branch-case.err @@ -1,2 +1,2 @@ union-branch-case.json: In union 'Uni': -union-branch-case.json:2: 'data' member 'Branch' uses uppercase in name +union-branch-case.json:2: name of 'data' member 'Branch' must not use uppercase or '_' From e744708a7783624292f8c405ca840f50a10b0003 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:20 +0100 Subject: [PATCH 23/29] qapi: Enforce feature naming rules Feature names should use '-', not '_'. Enforce this. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-24-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index e416cc5162..d778107c18 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -213,7 +213,7 @@ def check_features(features, info): check_keys(f, info, source, ['name'], ['if']) check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) - check_name_lower(f['name'], info, source, permit_underscore=True) + check_name_lower(f['name'], info, source) check_if(f, info, source) From 05ebf841efac494d8bd1f6d74642c3e9a3df4c19 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:21 +0100 Subject: [PATCH 24/29] qapi: Enforce command naming rules 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 Message-Id: <20210323094025.3569441-25-armbru@redhat.com> Reviewed-by: Eric Blake --- docs/devel/qapi-code-gen.txt | 8 ++++++-- qapi/pragma.json | 18 +++++++++++++++++ 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 ++++++++++++------------- tests/unit/test-qmp-cmds.c | 10 +++++----- 8 files changed, 62 insertions(+), 31 deletions(-) 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/scripts/qapi/expr.py b/scripts/qapi/expr.py index d778107c18..9193e68763 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 @@ class QAPISchemaParser: 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)'] 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' } } }")); From e75d4225b76842ec899f25e8ff39b070119f033f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:22 +0100 Subject: [PATCH 25/29] tests/qapi-schema: Switch member name clash test to struct Test args-name-clash covers command parameter name clash. This effectively covers struct member name clash as well. The next commit will make parameter name clash impossible. Convert args-name-clash from testing command to testing a struct, and rename it to struct-member-name-clash. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-26-armbru@redhat.com> Reviewed-by: Eric Blake [Commit message typo fixed] --- tests/qapi-schema/args-name-clash.err | 2 -- tests/qapi-schema/meson.build | 2 +- tests/qapi-schema/struct-member-name-clash.err | 2 ++ .../{args-name-clash.json => struct-member-name-clash.json} | 2 +- .../{args-name-clash.out => struct-member-name-clash.out} | 0 5 files changed, 4 insertions(+), 4 deletions(-) delete mode 100644 tests/qapi-schema/args-name-clash.err create mode 100644 tests/qapi-schema/struct-member-name-clash.err rename tests/qapi-schema/{args-name-clash.json => struct-member-name-clash.json} (64%) rename tests/qapi-schema/{args-name-clash.out => struct-member-name-clash.out} (100%) diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err deleted file mode 100644 index 3e04817bc0..0000000000 --- a/tests/qapi-schema/args-name-clash.err +++ /dev/null @@ -1,2 +0,0 @@ -args-name-clash.json: In command 'oops': -args-name-clash.json:4: parameter 'a_b' collides with parameter 'a-b' diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index 4e7635f0a8..8ba6917132 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -30,7 +30,6 @@ schemas = [ 'args-member-array-bad.json', 'args-member-case.json', 'args-member-unknown.json', - 'args-name-clash.json', 'args-union.json', 'args-unknown.json', 'bad-base.json', @@ -177,6 +176,7 @@ schemas = [ 'struct-member-if-invalid.json', 'struct-member-invalid-dict.json', 'struct-member-invalid.json', + 'struct-member-name-clash.json', 'trailing-comma-list.json', 'trailing-comma-object.json', 'type-bypass-bad-gen.json', diff --git a/tests/qapi-schema/struct-member-name-clash.err b/tests/qapi-schema/struct-member-name-clash.err new file mode 100644 index 0000000000..6ac042d59d --- /dev/null +++ b/tests/qapi-schema/struct-member-name-clash.err @@ -0,0 +1,2 @@ +struct-member-name-clash.json: In struct 'Oops': +struct-member-name-clash.json:4: member 'a_b' collides with member 'a-b' diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/struct-member-name-clash.json similarity index 64% rename from tests/qapi-schema/args-name-clash.json rename to tests/qapi-schema/struct-member-name-clash.json index 61423cb893..3fb69cc2ce 100644 --- a/tests/qapi-schema/args-name-clash.json +++ b/tests/qapi-schema/struct-member-name-clash.json @@ -1,4 +1,4 @@ # C member name collision # Reject members that clash when mapped to C names (we would have two 'a_b' # members). -{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } +{ 'struct': 'Oops', 'data': { 'a-b': 'str', 'a_b': 'str' } } diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/struct-member-name-clash.out similarity index 100% rename from tests/qapi-schema/args-name-clash.out rename to tests/qapi-schema/struct-member-name-clash.out From 5aceeac04de50e3a9d5c2a965379324659a94be0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:23 +0100 Subject: [PATCH 26/29] qapi: Enforce struct member naming rules 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 Message-Id: <20210323094025.3569441-27-armbru@redhat.com> Reviewed-by: Eric Blake --- 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(-) 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 9193e68763..ba9f7ad350 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' } } From 407efbf9e776ade8e8d09b778851834f91b225a1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:24 +0100 Subject: [PATCH 27/29] qapi: Enforce enum member naming rules Enum 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 Message-Id: <20210323094025.3569441-28-armbru@redhat.com> Reviewed-by: Eric Blake --- qapi/pragma.json | 8 ++++++++ scripts/qapi/expr.py | 5 +++-- tests/qapi-schema/enum-clash-member.err | 2 +- tests/qapi-schema/enum-clash-member.json | 1 + 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/qapi/pragma.json b/qapi/pragma.json index f422a1a2ac..b4e17167e1 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -32,12 +32,15 @@ 'member-name-exceptions': [ # visible in: 'ACPISlotType', # query-acpi-ospm-status 'AcpiTableOptions', # -acpitable + 'BlkdebugEvent', # blockdev-add, -blockdev 'BlkdebugSetStateOptions', # blockdev-add, -blockdev 'BlockDeviceInfo', # query-block 'BlockDeviceStats', # query-blockstats 'BlockDeviceTimedStats', # query-blockstats 'BlockIOThrottle', # block_set_io_throttle 'BlockInfo', # query-block + 'BlockdevAioOptions', # blockdev-add, -blockdev + 'BlockdevDriver', # blockdev-add, query-blockstats, ... 'BlockdevVmdkAdapterType', # blockdev-create (to match VMDK spec) 'BlockdevVmdkSubformat', # blockdev-create (to match VMDK spec) 'ColoCompareProperties', # object_add, -object @@ -46,10 +49,15 @@ 'FilterRewriterProperties', # object_add, -object 'InputLinuxProperties', # object_add, -object 'NetdevTapOptions', # netdev_add, query-netdev, -netdev + 'ObjectType', # object-add, -object + 'PCIELinkSpeed', # internal only 'PciBusInfo', # query-pci 'PciDeviceInfo', # query-pci 'PciMemoryRegion', # query-pci + 'QKeyCode', # send-key, input-sent-event 'QapiErrorClass', # QMP error replies + 'SshHostKeyCheckMode', # blockdev-add, -blockdev + 'SysEmuTarget', # query-cpu-fast, query-target 'UuidInfo', # query-uuid 'VncClientInfo', # query-vnc, query-vnc-servers, ... 'X86CPURegister32' # qom-get of x86 CPU properties diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index ba9f7ad350..d968609c48 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -229,7 +229,7 @@ def check_enum(expr, info): if prefix is not None and not isinstance(prefix, str): raise QAPISemError(info, "'prefix' must be a string") - permit_upper = name in info.pragma.member_name_exceptions + permissive = name in info.pragma.member_name_exceptions members[:] = [m if isinstance(m, dict) else {'name': m} for m in members] @@ -243,7 +243,8 @@ def check_enum(expr, info): if member_name[0].isdigit(): member_name = 'd' + member_name # Hack: hide the digit check_name_lower(member_name, info, source, - permit_upper, permit_underscore=True) + permit_upper=permissive, + permit_underscore=permissive) check_if(member, info, source) diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err index 5986571427..e4eb102ae2 100644 --- a/tests/qapi-schema/enum-clash-member.err +++ b/tests/qapi-schema/enum-clash-member.err @@ -1,2 +1,2 @@ enum-clash-member.json: In enum 'MyEnum': -enum-clash-member.json:2: value 'one_two' collides with value 'one-two' +enum-clash-member.json:3: value 'one_two' collides with value 'one-two' diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json index b6928b8bfd..82bcbf724b 100644 --- a/tests/qapi-schema/enum-clash-member.json +++ b/tests/qapi-schema/enum-clash-member.json @@ -1,2 +1,3 @@ # we reject enums where members will clash when mapped to C enum +{ 'pragma': { 'member-name-exceptions': [ 'MyEnum' ] } } { 'enum': 'MyEnum', 'data': [ 'one-two', 'one_two' ] } From d83b47646ec2bdf4f7be9c2078f1bcbbb0544b2e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 10:40:25 +0100 Subject: [PATCH 28/29] qapi: Enforce union and alternate branch naming rules Union branch names should use '-', not '_'. Enforce this. The only offenders are in tests/. Fix them. Signed-off-by: Markus Armbruster Message-Id: <20210323094025.3569441-29-armbru@redhat.com> Reviewed-by: Eric Blake [Commit message typo fixed] --- scripts/qapi/expr.py | 4 ++-- tests/qapi-schema/alternate-clash.err | 2 +- tests/qapi-schema/alternate-clash.json | 6 ++++-- tests/qapi-schema/qapi-schema-test.json | 2 +- tests/qapi-schema/qapi-schema-test.out | 4 ++-- tests/qapi-schema/union-clash-branches.err | 2 +- tests/qapi-schema/union-clash-branches.json | 6 ++++-- 7 files changed, 15 insertions(+), 11 deletions(-) diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index d968609c48..540b3982b1 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -274,7 +274,7 @@ def check_union(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key if discriminator is None: - check_name_lower(key, info, source, permit_underscore=True) + check_name_lower(key, info, source) # else: name is in discriminator enum, which gets checked check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) @@ -288,7 +288,7 @@ def check_alternate(expr, info): raise QAPISemError(info, "'data' must not be empty") for (key, value) in members.items(): source = "'data' member '%s'" % key - check_name_lower(key, info, source, permit_underscore=True) + check_name_lower(key, info, source) check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) check_type(value['type'], info, source) diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err index 0fe02f2c99..caa2d42e3f 100644 --- a/tests/qapi-schema/alternate-clash.err +++ b/tests/qapi-schema/alternate-clash.err @@ -1,2 +1,2 @@ alternate-clash.json: In alternate 'Alt1': -alternate-clash.json:4: branch 'a_b' collides with branch 'a-b' +alternate-clash.json:6: name of 'data' member 'a_b' must not use uppercase or '_' diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json index 039c4be658..87f061a74a 100644 --- a/tests/qapi-schema/alternate-clash.json +++ b/tests/qapi-schema/alternate-clash.json @@ -1,5 +1,7 @@ # Alternate branch name collision -# Reject an alternate that would result in a collision in generated C -# names (this would try to generate two union members named 'a_b'). +# Naming rules make collision impossible (even with the pragma). If +# that wasn't the case, then we'd get a collision in generated C: two +# union members a_b. +{ 'pragma': { 'member-name-exceptions': [ 'Alt1' ] } } { 'alternate': 'Alt1', 'data': { 'a-b': 'bool', 'a_b': 'int' } } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 387678acbb..84b9d41f15 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -231,7 +231,7 @@ { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct', - 'union_bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} }, + 'bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} }, 'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' } { 'command': 'test-if-union-cmd', diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 51efe5d7cd..e0b8a5f0b6 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -309,14 +309,14 @@ object q_obj_TestStruct-wrapper member data: TestStruct optional=False enum TestIfUnionKind member foo - member union_bar + member bar if ['defined(TEST_IF_UNION_BAR)'] if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'] object TestIfUnion member type: TestIfUnionKind optional=False tag type case foo: q_obj_TestStruct-wrapper - case union_bar: q_obj_str-wrapper + case bar: q_obj_str-wrapper if ['defined(TEST_IF_UNION_BAR)'] if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)'] object q_obj_test-if-union-cmd-arg diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err index 73bbc2cabd..ef53645728 100644 --- a/tests/qapi-schema/union-clash-branches.err +++ b/tests/qapi-schema/union-clash-branches.err @@ -1,2 +1,2 @@ union-clash-branches.json: In union 'TestUnion': -union-clash-branches.json:4: branch 'a_b' collides with branch 'a-b' +union-clash-branches.json:6: name of 'data' member 'a_b' must not use uppercase or '_' diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json index 3bece8c948..7bdda0b0da 100644 --- a/tests/qapi-schema/union-clash-branches.json +++ b/tests/qapi-schema/union-clash-branches.json @@ -1,5 +1,7 @@ # Union branch name collision -# Reject a union that would result in a collision in generated C names (this -# would try to generate two members 'a_b'). +# Naming rules make collision impossible (even with the pragma). If +# that wasn't the case, then we'd get collisions in generated C: two +# union members a_b, and two enum members TEST_UNION_A_B. +{ 'pragma': { 'member-name-exceptions': [ 'TestUnion' ] } } { 'union': 'TestUnion', 'data': { 'a-b': 'int', 'a_b': 'str' } } From bdabafc6836edc0f34732cac473899cb4e77a296 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 23 Mar 2021 11:19:51 +0100 Subject: [PATCH 29/29] block: Remove monitor command block_passwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Command block_passwd always fails since Commit c01c214b69 "block: remove all encryption handling APIs" (v2.10.0) turned block_passwd into a stub that always fails, and hardcoded encryption_key_missing to false in query-named-block-nodes and query-block. Commit ad1324e044 "block: remove 'encryption_key_missing' flag from QAPI" just landed. Complete the cleanup job: remove block_passwd. Cc: Daniel P. Berrangé Signed-off-by: Markus Armbruster Message-Id: <20210323101951.3686029-1-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé --- block/monitor/block-hmp-cmds.c | 10 ---------- blockdev.c | 8 -------- hmp-commands.hx | 15 --------------- qapi/block-core.json | 14 -------------- qapi/pragma.json | 1 - 5 files changed, 48 deletions(-) diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index 75d7fa9510..ebf1033f31 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -515,16 +515,6 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, error); } -void hmp_block_passwd(Monitor *mon, const QDict *qdict) -{ - const char *device = qdict_get_str(qdict, "device"); - const char *password = qdict_get_str(qdict, "password"); - Error *err = NULL; - - qmp_block_passwd(true, device, false, NULL, password, &err); - hmp_handle_error(mon, err); -} - void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) { Error *err = NULL; diff --git a/blockdev.c b/blockdev.c index cf70bb4e43..621cc3b7c4 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2407,14 +2407,6 @@ exit: job_txn_unref(block_job_txn); } -void qmp_block_passwd(bool has_device, const char *device, - bool has_node_name, const char *node_name, - const char *password, Error **errp) -{ - error_setg(errp, - "Setting block passwords directly is no longer supported"); -} - BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node, const char *name, Error **errp) diff --git a/hmp-commands.hx b/hmp-commands.hx index 9b88c45174..435c591a1c 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1493,21 +1493,6 @@ SRST used by another monitor command. ERST - { - .name = "block_passwd", - .args_type = "device:B,password:s", - .params = "block_passwd device password", - .help = "set the password of encrypted block devices", - .cmd = hmp_block_passwd, - }, - -SRST -``block_passwd`` *device* *password* - Set the encrypted device *device* password to *password* - - This command is now obsolete and will always return an error since 2.10 -ERST - { .name = "block_set_io_throttle", .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l", diff --git a/qapi/block-core.json b/qapi/block-core.json index 1c3f1deb03..6d227924d0 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1207,20 +1207,6 @@ ## { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] } -## -# @block_passwd: -# -# This command sets the password of a block device that has not been open -# with a password and requires one. -# -# This command is now obsolete and will always return an error since 2.10 -# -## -{ 'command': 'block_passwd', - 'data': { '*device': 'str', - '*node-name': 'str', - 'password': 'str' } } - ## # @block_resize: # diff --git a/qapi/pragma.json b/qapi/pragma.json index b4e17167e1..3bc0335d1f 100644 --- a/qapi/pragma.json +++ b/qapi/pragma.json @@ -6,7 +6,6 @@ # Commands allowed to return a non-dictionary: 'command-name-exceptions': [ 'add_client', - 'block_passwd', 'block_resize', 'block_set_io_throttle', 'client_migrate_info',