From 8712fa5333ad348da20034b717dd814219d1ec11 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 26 Oct 2015 16:34:41 -0600 Subject: qapi: More idiomatic string operations Rather than slicing the end of a string, we can use python's endswith(). And rather than creating a set of characters, we can search for a character within a string. Signed-off-by: Eric Blake Message-Id: <1445898903-12082-3-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 9d53255..3af4c2c 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -172,7 +172,7 @@ class QAPISchemaParser(object): if self.tok == '#': self.cursor = self.src.find('\n', self.cursor) - elif self.tok in ['{', '}', ':', ',', '[', ']']: + elif self.tok in "{}:,[]": return elif self.tok == "'": string = '' @@ -390,7 +390,7 @@ def add_name(name, info, meta, implicit=False): raise QAPIExprError(info, "%s '%s' is already defined" % (all_names[name], name)) - if not implicit and name[-4:] == 'Kind': + if not implicit and name.endswith('Kind'): raise QAPIExprError(info, "%s '%s' should not end in 'Kind'" % (meta, name)) @@ -910,7 +910,7 @@ class QAPISchemaEnumType(QAPISchemaType): def is_implicit(self): # See QAPISchema._make_implicit_enum_type() - return self.name[-4:] == 'Kind' + return self.name.endswith('Kind') def c_type(self, is_param=False): return c_name(self.name) -- cgit v1.1 From 255960dd374d4497d6ea537305f1b0d8a3433789 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 26 Oct 2015 16:34:43 -0600 Subject: qapi: Reserve '*List' type names for list types Type names ending in 'List' can clash with qapi list types in generated C. We don't currently use such names. It is easier to outlaw them now than to worry about how to resolve such a clash in the future. For precedence, see commit 4dc2e69, which did the same for names ending in 'Kind' versus implicit enum types for qapi unions. Update the testsuite to match. Signed-off-by: Eric Blake Message-Id: <1445898903-12082-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 3af4c2c..d53b5c4 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -390,10 +390,10 @@ def add_name(name, info, meta, implicit=False): raise QAPIExprError(info, "%s '%s' is already defined" % (all_names[name], name)) - if not implicit and name.endswith('Kind'): + if not implicit and (name.endswith('Kind') or name.endswith('List')): raise QAPIExprError(info, - "%s '%s' should not end in 'Kind'" - % (meta, name)) + "%s '%s' should not end in '%s'" + % (meta, name, name[-4:])) all_names[name] = meta @@ -1196,9 +1196,7 @@ class QAPISchema(object): return name def _make_array_type(self, element_type, info): - # TODO fooList namespace is not reserved; user can create collisions, - # or abuse our type system with ['fooList'] for 2D array - name = element_type + 'List' + name = element_type + 'List' # Use namespace reserved by add_name() if not self.lookup_type(name): self._def_entity(QAPISchemaArrayType(name, info, element_type)) return name -- cgit v1.1 From 9fb081e0b98409556d023c7193eeb68947cd1211 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 26 Oct 2015 16:34:44 -0600 Subject: qapi: Reserve 'q_*' and 'has_*' member names c_name() produces names starting with 'q_' when protecting a dictionary member name that would fail to directly compile, but in doing so can cause clashes with any member name already beginning with 'q-' or 'q_'. Likewise, we create a C name 'has_' for any optional member that can clash with any member name beginning with 'has-' or 'has_'. Technically, rather than blindly reserving the namespace, we could try to complain about user names only when an actual collision occurs, or even teach c_name() how to munge names to avoid collisions. But it is not trivial, especially when collisions can occur across multiple types (such as via inheritance or flat unions). Besides, no existing .json files are trying to use these names. So it's easier to just outright forbid the potential for collision. We can always relax things in the future if a real need arises for QMP to express member names that have been forbidden here. 'has_' only has to be reserved for struct/union member names, while 'q_' is reserved everywhere (matching the fact that only members can be optional, while we use c_name() for munging both members and entities). Note that we could relax 'q_' restrictions on entities independently from member names; for example, c_name('qmp_' + 'unix') would result in a different function name than our current 'qmp_' + c_name('unix'). Update and add tests to cover the new error messages. Signed-off-by: Eric Blake Message-Id: <1445898903-12082-6-git-send-email-eblake@redhat.com> [Consistently pass protect=False to c_name(); commit message tweaked slightly] Signed-off-by: Markus Armbruster --- scripts/qapi.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index d53b5c4..3ff7b11 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -376,7 +376,9 @@ def check_name(expr_info, source, name, allow_optional=False, # code always prefixes it with the enum name if enum_member: membername = '_' + membername - if not valid_name.match(membername): + # Reserve the entire 'q_' namespace for c_name() + if not valid_name.match(membername) or \ + c_name(membername, False).startswith('q_'): raise QAPIExprError(expr_info, "%s uses invalid name '%s'" % (source, name)) @@ -488,6 +490,10 @@ def check_type(expr_info, source, value, allow_array=False, for (key, arg) in value.items(): check_name(expr_info, "Member of %s" % source, key, allow_optional=allow_optional) + if c_name(key, False).startswith('has_'): + raise QAPIExprError(expr_info, + "Member of %s uses reserved name '%s'" + % (source, key)) # Todo: allow dictionaries to represent default values of # an optional argument. check_type(expr_info, "Member '%s' of %s" % (key, source), arg, -- cgit v1.1 From f51d8fab44b231aa299d8de24cfdf9ba41ef4a21 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 26 Oct 2015 16:34:51 -0600 Subject: qapi: Start converting to new qapi union layout We have two issues with our qapi union layout: 1) Even though the QMP wire format spells the tag 'type', the C code spells it 'kind', requiring some hacks in the generator. 2) The C struct uses an anonymous union, which places all tag values in the same namespace as all non-variant members. This leads to spurious collisions if a tag value matches a non-variant member's name. This patch is the front end for a series that converts to a saner qapi union layout. By the end of the series, we will no longer have the type/kind mismatch, and all tag values will be under a named union, which requires clients to access 'obj->u.value' instead of 'obj->value'. But since the conversion touches a number of files, it is easiest if we temporarily support BOTH layouts simultaneously. Given a simple union qapi type: { 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } } make the following changes in generated qapi-types.h: | struct Foo { |- FooKind kind; |- union { /* union tag is @kind */ |+ union { |+ FooKind kind; |+ FooKind type; |+ }; |+ union { /* union tag is @type */ | void *data; | int64_t a; | bool b; |+ union { /* union tag is @type */ |+ void *data; |+ int64_t a; |+ bool b; |+ } u; | }; | }; Flat unions do not need the anonymous union for the tag member, as we already fixed that to use the member name instead of 'kind' back in commit 0f61af3e. One additional change is needed in qapi.py: check_union() now needs to check for collisions with 'type' in addition to those with 'kind'. Later, when the conversions are complete, we will remove the duplication hacks, and also drop the check_union() restrictions. Note, however, that we do not rename the generated enum, which is still 'FooKind'. A further patch could generate implicit enums as 'FooType', but while the generator already reserved the '*Kind' namespace (commit 4dc2e69), there are already QMP constructs with '*Type' naming, which means changing our reservation namespace would have lots of churn to C code to deal with a forced name change. Signed-off-by: Eric Blake Message-Id: <1445898903-12082-13-git-send-email-eblake@redhat.com> [Commit message tweaked slightly] Signed-off-by: Markus Armbruster --- scripts/qapi.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 3ff7b11..00a1620 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -548,7 +548,8 @@ def check_union(expr, expr_info): base = expr.get('base') discriminator = expr.get('discriminator') members = expr['data'] - values = {'MAX': '(automatic)', 'KIND': '(automatic)'} + values = {'MAX': '(automatic)', 'KIND': '(automatic)', + 'TYPE': '(automatic)'} # Two types of unions, determined by discriminator. -- cgit v1.1 From 5e59baf90a72cd25d38a3134edc029f4f022da74 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 26 Oct 2015 16:35:02 -0600 Subject: qapi: Reserve 'u' member name Now that we have separated union tag values from colliding with non-variant C names, by naming the union 'u', we should reserve this name for our use. Note that we want to forbid 'u' even in a struct with no variants, because it is possible for a future qemu release to extend QMP in a backwards-compatible manner while converting from a struct to a flat union. Fortunately, no existing clients were using this member name. If we ever find the need for QMP to have a member 'u', we could at that time relax things, perhaps by having c_name() munge the QMP member to 'q_u'. Note that we cannot forbid 'u' everywhere (by adding the rejection code to check_name()), because the existing QKeyCode enum already uses it; therefore we only reserve it as a struct type member name. Signed-off-by: Eric Blake Message-Id: <1445898903-12082-24-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts/qapi.py') diff --git a/scripts/qapi.py b/scripts/qapi.py index 00a1620..7c50cc4 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -490,7 +490,7 @@ def check_type(expr_info, source, value, allow_array=False, for (key, arg) in value.items(): check_name(expr_info, "Member of %s" % source, key, allow_optional=allow_optional) - if c_name(key, False).startswith('has_'): + if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPIExprError(expr_info, "Member of %s uses reserved name '%s'" % (source, key)) -- cgit v1.1