* [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 6:47 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type Eric Blake
` (10 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Previously, qapi-types and qapi-visit filtered out implicit
objects during visit_object_type() by using 'info' (works since
implicit objects do not [yet] have associated info); meanwhile
qapi-introspect filtered out all schema types on the first pass
by returning a python type from visit_begin(), which was then
used in QAPISchema.visit(). Rather than keeping these ad hoc
approaches, add a new visitor callback visit_predicate() which
returns False to skip a given entity, and which defaults to
True unless overridden. Use the new mechanism to simplify all
three visitors that need filtering. No change to the generated
code.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch
---
scripts/qapi-introspect.py | 5 ++++-
scripts/qapi-types.py | 18 ++++++++++--------
scripts/qapi-visit.py | 16 +++++++++-------
scripts/qapi.py | 11 +++++++----
4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 7d39320..f30fac3 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -54,7 +54,6 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
self._jsons = []
self._used_types = []
self._name_map = {}
- return QAPISchemaType # don't visit types for now
def visit_end(self):
# visit the types that are actually used
@@ -82,6 +81,10 @@ const char %(c_name)s[] = %(c_string)s;
self._used_types = None
self._name_map = None
+ def visit_predicate(self, entity):
+ # Ignore types on first pass; visit_end() will pick up used types
+ return not isinstance(entity, QAPISchemaType)
+
def _name(self, name):
if self._unmask:
return name
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d405f8d..c5b71b0 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -233,6 +233,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.decl = self._btin + self.decl
self._btin = None
+ def visit_predicate(self, entity):
+ return not isinstance(entity, QAPISchemaObjectType) or entity.info
+
def _gen_type_cleanup(self, name):
self.decl += gen_type_cleanup_decl(name)
self.defn += gen_type_cleanup(name)
@@ -254,14 +257,13 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self._gen_type_cleanup(name)
def visit_object_type(self, name, info, base, members, variants):
- if info:
- self._fwdecl += gen_fwd_object_or_array(name)
- if variants:
- assert not members # not implemented
- self.decl += gen_union(name, base, variants)
- else:
- self.decl += gen_struct(name, base, members)
- self._gen_type_cleanup(name)
+ self._fwdecl += gen_fwd_object_or_array(name)
+ if variants:
+ assert not members # not implemented
+ self.decl += gen_union(name, base, variants)
+ else:
+ self.decl += gen_struct(name, base, members)
+ self._gen_type_cleanup(name)
def visit_alternate_type(self, name, info, variants):
self._fwdecl += gen_fwd_object_or_array(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4f97781..0f47614 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -333,6 +333,9 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self.decl = self._btin + self.decl
self._btin = None
+ def visit_predicate(self, entity):
+ return not isinstance(entity, QAPISchemaObjectType) or entity.info
+
def visit_enum_type(self, name, info, values, prefix):
self.decl += gen_visit_decl(name, scalar=True)
self.defn += gen_visit_enum(name)
@@ -349,13 +352,12 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self.defn += defn
def visit_object_type(self, name, info, base, members, variants):
- if info:
- self.decl += gen_visit_decl(name)
- if variants:
- assert not members # not implemented
- self.defn += gen_visit_union(name, base, variants)
- else:
- self.defn += gen_visit_struct(name, base, members)
+ self.decl += gen_visit_decl(name)
+ if variants:
+ assert not members # not implemented
+ self.defn += gen_visit_union(name, base, variants)
+ else:
+ self.defn += gen_visit_struct(name, base, members)
def visit_alternate_type(self, name, info, variants):
self.decl += gen_visit_decl(name)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 26cff3f..7d359c8 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -811,6 +811,9 @@ class QAPISchemaVisitor(object):
def visit_end(self):
pass
+ def visit_predicate(self, entity):
+ return True
+
def visit_builtin_type(self, name, info, json_type):
pass
@@ -1304,10 +1307,10 @@ class QAPISchema(object):
ent.check(self)
def visit(self, visitor):
- ignore = visitor.visit_begin(self)
- for name in sorted(self._entity_dict.keys()):
- if not ignore or not isinstance(self._entity_dict[name], ignore):
- self._entity_dict[name].visit(visitor)
+ visitor.visit_begin(self)
+ for (name, entity) in sorted(self._entity_dict.items()):
+ if visitor.visit_predicate(entity):
+ entity.visit(visitor)
visitor.visit_end()
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering Eric Blake
@ 2015-10-02 6:47 ` Markus Armbruster
2015-10-02 12:16 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 6:47 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Previously, qapi-types and qapi-visit filtered out implicit
> objects during visit_object_type() by using 'info' (works since
> implicit objects do not [yet] have associated info); meanwhile
> qapi-introspect filtered out all schema types on the first pass
> by returning a python type from visit_begin(), which was then
> used in QAPISchema.visit(). Rather than keeping these ad hoc
> approaches, add a new visitor callback visit_predicate() which
> returns False to skip a given entity, and which defaults to
> True unless overridden. Use the new mechanism to simplify all
> three visitors that need filtering. No change to the generated
> code.
Let's call it visit_wanted().
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: new patch
> ---
> scripts/qapi-introspect.py | 5 ++++-
> scripts/qapi-types.py | 18 ++++++++++--------
> scripts/qapi-visit.py | 16 +++++++++-------
> scripts/qapi.py | 11 +++++++----
> 4 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 7d39320..f30fac3 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -54,7 +54,6 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
> self._jsons = []
> self._used_types = []
> self._name_map = {}
> - return QAPISchemaType # don't visit types for now
>
> def visit_end(self):
> # visit the types that are actually used
> @@ -82,6 +81,10 @@ const char %(c_name)s[] = %(c_string)s;
> self._used_types = None
> self._name_map = None
>
> + def visit_predicate(self, entity):
> + # Ignore types on first pass; visit_end() will pick up used types
> + return not isinstance(entity, QAPISchemaType)
> +
> def _name(self, name):
> if self._unmask:
> return name
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index d405f8d..c5b71b0 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -233,6 +233,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self.decl = self._btin + self.decl
> self._btin = None
>
> + def visit_predicate(self, entity):
> + return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +
This is the faithful translation. But the left hand side is superfluous
because non-types laways have info, isn't it?
> def _gen_type_cleanup(self, name):
> self.decl += gen_type_cleanup_decl(name)
> self.defn += gen_type_cleanup(name)
> @@ -254,14 +257,13 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self._gen_type_cleanup(name)
>
> def visit_object_type(self, name, info, base, members, variants):
> - if info:
> - self._fwdecl += gen_fwd_object_or_array(name)
> - if variants:
> - assert not members # not implemented
> - self.decl += gen_union(name, base, variants)
> - else:
> - self.decl += gen_struct(name, base, members)
> - self._gen_type_cleanup(name)
> + self._fwdecl += gen_fwd_object_or_array(name)
> + if variants:
> + assert not members # not implemented
> + self.decl += gen_union(name, base, variants)
> + else:
> + self.decl += gen_struct(name, base, members)
> + self._gen_type_cleanup(name)
>
> def visit_alternate_type(self, name, info, variants):
> self._fwdecl += gen_fwd_object_or_array(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 4f97781..0f47614 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -333,6 +333,9 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> self.decl = self._btin + self.decl
> self._btin = None
>
> + def visit_predicate(self, entity):
> + return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +
Likewise.
> def visit_enum_type(self, name, info, values, prefix):
> self.decl += gen_visit_decl(name, scalar=True)
> self.defn += gen_visit_enum(name)
> @@ -349,13 +352,12 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> self.defn += defn
>
> def visit_object_type(self, name, info, base, members, variants):
> - if info:
> - self.decl += gen_visit_decl(name)
> - if variants:
> - assert not members # not implemented
> - self.defn += gen_visit_union(name, base, variants)
> - else:
> - self.defn += gen_visit_struct(name, base, members)
> + self.decl += gen_visit_decl(name)
> + if variants:
> + assert not members # not implemented
> + self.defn += gen_visit_union(name, base, variants)
> + else:
> + self.defn += gen_visit_struct(name, base, members)
>
> def visit_alternate_type(self, name, info, variants):
> self.decl += gen_visit_decl(name)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 26cff3f..7d359c8 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -811,6 +811,9 @@ class QAPISchemaVisitor(object):
> def visit_end(self):
> pass
>
> + def visit_predicate(self, entity):
> + return True
> +
> def visit_builtin_type(self, name, info, json_type):
> pass
>
> @@ -1304,10 +1307,10 @@ class QAPISchema(object):
> ent.check(self)
>
> def visit(self, visitor):
> - ignore = visitor.visit_begin(self)
> - for name in sorted(self._entity_dict.keys()):
> - if not ignore or not isinstance(self._entity_dict[name], ignore):
> - self._entity_dict[name].visit(visitor)
> + visitor.visit_begin(self)
> + for (name, entity) in sorted(self._entity_dict.items()):
> + if visitor.visit_predicate(entity):
> + entity.visit(visitor)
> visitor.visit_end()
Much nicer than my ad hoc hackery!
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering
2015-10-02 6:47 ` Markus Armbruster
@ 2015-10-02 12:16 ` Eric Blake
0 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 12:16 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2947 bytes --]
On 10/02/2015 12:47 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Previously, qapi-types and qapi-visit filtered out implicit
>> objects during visit_object_type() by using 'info' (works since
>> implicit objects do not [yet] have associated info); meanwhile
>> qapi-introspect filtered out all schema types on the first pass
>> by returning a python type from visit_begin(), which was then
>> used in QAPISchema.visit(). Rather than keeping these ad hoc
>> approaches, add a new visitor callback visit_predicate() which
>> returns False to skip a given entity, and which defaults to
>> True unless overridden. Use the new mechanism to simplify all
>> three visitors that need filtering. No change to the generated
>> code.
>
> Let's call it visit_wanted().
Ah, that's nicer.
>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v6: new patch
>> +++ b/scripts/qapi-types.py
>> @@ -233,6 +233,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>> self.decl = self._btin + self.decl
>> self._btin = None
>>
>> + def visit_predicate(self, entity):
>> + return not isinstance(entity, QAPISchemaObjectType) or entity.info
>> +
>
> This is the faithful translation. But the left hand side is superfluous
> because non-types laways have info, isn't it?
Not yet. 3/12 adds info to arrays, but even then, intList still has
info of None. And 4/12 adds info to implicit enums. Without the left
half, we choke hard for any implicit UnionKind enum type being omitted
as implicit.
However, I do think you're on to something - the only time name[0] ==
':' is for objects (implicit enums are NameKind, but since we don't have
anonymous unions, we won't have an enum starting with :; likewise,
arrays are NameList, but you can only have an array of a named type).
So if I hoist is_implicit() to the QAPISchemaEntity level, then it
should only ever return True for an object, and while _this_ patch has
to keep the left side, patch 2 gets the shorter conditional.
>> @@ -1304,10 +1307,10 @@ class QAPISchema(object):
>> ent.check(self)
>>
>> def visit(self, visitor):
>> - ignore = visitor.visit_begin(self)
>> - for name in sorted(self._entity_dict.keys()):
>> - if not ignore or not isinstance(self._entity_dict[name], ignore):
>> - self._entity_dict[name].visit(visitor)
>> + visitor.visit_begin(self)
>> + for (name, entity) in sorted(self._entity_dict.items()):
>> + if visitor.visit_predicate(entity):
>> + entity.visit(visitor)
>> visitor.visit_end()
>
> Much nicer than my ad hoc hackery!
>
Yeah, I like how it turned out.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 7:02 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types Eric Blake
` (9 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
A future patch will enable error reporting from the various
QAPISchema*.check() methods. But to report an error related
to an implicit type, we'll need to associate a location with
the type (the same location as the top-level entity that is
causing the creation of the implicit type), and once we do
that, keying off of whether foo.info exists is no longer a
viable way to determine if foo is an implicit type.
Instead, add an is_implicit() method to QAPISchemaObjectType,
and use that function where needed. (Done at the ObjectType
level, since we already know all builtins and arrays are
implicit, no commands or events are implicit, and we don't
have any differences in generated code for regular vs.
implicit enums.)
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split 11/46 into pieces; don't rename _info yet; rework atop
nicer filtering mechanism, including no need to change visitor
signature
---
scripts/qapi-types.py | 3 ++-
scripts/qapi-visit.py | 3 ++-
scripts/qapi.py | 10 +++++++---
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index c5b71b0..6bac5b3 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -234,7 +234,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self._btin = None
def visit_predicate(self, entity):
- return not isinstance(entity, QAPISchemaObjectType) or entity.info
+ return not (isinstance(entity, QAPISchemaObjectType) and
+ entity.is_implicit())
def _gen_type_cleanup(self, name):
self.decl += gen_type_cleanup_decl(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0f47614..2957c85 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -334,7 +334,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self._btin = None
def visit_predicate(self, entity):
- return not isinstance(entity, QAPISchemaObjectType) or entity.info
+ return not (isinstance(entity, QAPISchemaObjectType) and
+ entity.is_implicit())
def visit_enum_type(self, name, info, values, prefix):
self.decl += gen_visit_decl(name, scalar=True)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 7d359c8..8123ab3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -970,12 +970,15 @@ class QAPISchemaObjectType(QAPISchemaType):
self.variants.check(schema, members, seen)
self.members = members
+ def is_implicit(self):
+ return self.name[0] == ':'
+
def c_name(self):
- assert self.info
+ assert not self.is_implicit()
return QAPISchemaType.c_name(self)
def c_type(self, is_param=False):
- assert self.info
+ assert not self.is_implicit()
return QAPISchemaType.c_type(self)
def json_type(self):
@@ -1043,7 +1046,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
# This function exists to support ugly simple union special cases
# TODO get rid of them, and drop the function
def simple_union_type(self):
- if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
+ if isinstance(self.type,
+ QAPISchemaObjectType) and self.type.is_implicit():
assert len(self.type.members) == 1
assert not self.type.variants
return self.type.members[0].type
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type Eric Blake
@ 2015-10-02 7:02 ` Markus Armbruster
2015-10-02 12:54 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 7:02 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> A future patch will enable error reporting from the various
> QAPISchema*.check() methods. But to report an error related
> to an implicit type, we'll need to associate a location with
> the type (the same location as the top-level entity that is
> causing the creation of the implicit type), and once we do
> that, keying off of whether foo.info exists is no longer a
> viable way to determine if foo is an implicit type.
>
> Instead, add an is_implicit() method to QAPISchemaObjectType,
> and use that function where needed. (Done at the ObjectType
> level, since we already know all builtins and arrays are
> implicit, no commands or events are implicit, and we don't
> have any differences in generated code for regular vs.
> implicit enums.)
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: split 11/46 into pieces; don't rename _info yet; rework atop
> nicer filtering mechanism, including no need to change visitor
> signature
> ---
> scripts/qapi-types.py | 3 ++-
> scripts/qapi-visit.py | 3 ++-
> scripts/qapi.py | 10 +++++++---
> 3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index c5b71b0..6bac5b3 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -234,7 +234,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self._btin = None
>
> def visit_predicate(self, entity):
> - return not isinstance(entity, QAPISchemaObjectType) or entity.info
> + return not (isinstance(entity, QAPISchemaObjectType) and
> + entity.is_implicit())
Aha, now the left hand side becomes necessary to guard the
.is_implicit(). It stays superfluous if you make .is_implicit() a
QAPISchemaEntity method.
>
> def _gen_type_cleanup(self, name):
> self.decl += gen_type_cleanup_decl(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 0f47614..2957c85 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -334,7 +334,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> self._btin = None
>
> def visit_predicate(self, entity):
> - return not isinstance(entity, QAPISchemaObjectType) or entity.info
> + return not (isinstance(entity, QAPISchemaObjectType) and
> + entity.is_implicit())
>
> def visit_enum_type(self, name, info, values, prefix):
> self.decl += gen_visit_decl(name, scalar=True)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7d359c8..8123ab3 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -970,12 +970,15 @@ class QAPISchemaObjectType(QAPISchemaType):
> self.variants.check(schema, members, seen)
> self.members = members
>
> + def is_implicit(self):
> + return self.name[0] == ':'
> +
If this test is here to stay, perhaps add a comment pointing to
_make_implicit_object_type().
> def c_name(self):
> - assert self.info
> + assert not self.is_implicit()
> return QAPISchemaType.c_name(self)
>
> def c_type(self, is_param=False):
> - assert self.info
> + assert not self.is_implicit()
> return QAPISchemaType.c_type(self)
>
> def json_type(self):
> @@ -1043,7 +1046,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> # This function exists to support ugly simple union special cases
> # TODO get rid of them, and drop the function
> def simple_union_type(self):
> - if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
> + if isinstance(self.type,
> + QAPISchemaObjectType) and self.type.is_implicit():
I figure you break this line in the argument list to avoid a backslash
for line continuation. I know PEP8 doesn't like them, but I like line
breaks away from top level operators even less. I feel it should be
broken after the and operator, even though that'll require wrapping the
condition in parenthesis.
> assert len(self.type.members) == 1
> assert not self.type.variants
> return self.type.members[0].type
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 7:02 ` Markus Armbruster
@ 2015-10-02 12:54 ` Eric Blake
2015-10-02 14:12 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 12:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3879 bytes --]
On 10/02/2015 01:02 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> A future patch will enable error reporting from the various
>> QAPISchema*.check() methods. But to report an error related
>> to an implicit type, we'll need to associate a location with
>> the type (the same location as the top-level entity that is
>> causing the creation of the implicit type), and once we do
>> that, keying off of whether foo.info exists is no longer a
>> viable way to determine if foo is an implicit type.
>>
>> Instead, add an is_implicit() method to QAPISchemaObjectType,
>> and use that function where needed. (Done at the ObjectType
>> level, since we already know all builtins and arrays are
>> implicit, no commands or events are implicit, and we don't
>> have any differences in generated code for regular vs.
>> implicit enums.)
>> +++ b/scripts/qapi-types.py
>> @@ -234,7 +234,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>> self._btin = None
>>
>> def visit_predicate(self, entity):
>> - return not isinstance(entity, QAPISchemaObjectType) or entity.info
>> + return not (isinstance(entity, QAPISchemaObjectType) and
>> + entity.is_implicit())
>
> Aha, now the left hand side becomes necessary to guard the
> .is_implicit(). It stays superfluous if you make .is_implicit() a
> QAPISchemaEntity method.
See discussion on 1/12 - actually, it _becomes_ superfluous in this
patch once we make it a QAPISchemaEntity method.
>> +++ b/scripts/qapi.py
>> @@ -970,12 +970,15 @@ class QAPISchemaObjectType(QAPISchemaType):
>> self.variants.check(schema, members, seen)
>> self.members = members
>>
>> + def is_implicit(self):
>> + return self.name[0] == ':'
Actually, this only works for implicit objects. Implicit enums instead
have self.name[-4:] == 'Kind'. But qapi-types cares about implicit
objects only. So if I hoist this, I may need something like:
def is_implicit(self, type=None):
if type and not isinstance(self, type):
return Fals
if isinstance(self, QAPISchemaObjectType):
return self.name[0] == ':'
if isinstance(self, QAPISchemaEnumType):
return self.name[-4:] == 'Kind'
return False
where qapi-types would call entity.is_implicit(QAPISchemaObjectType).
>> +
>
> If this test is here to stay, perhaps add a comment pointing to
> _make_implicit_object_type().
Indeed.
>> @@ -1043,7 +1046,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>> # This function exists to support ugly simple union special cases
>> # TODO get rid of them, and drop the function
>> def simple_union_type(self):
>> - if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
>> + if isinstance(self.type,
>> + QAPISchemaObjectType) and self.type.is_implicit():
>
> I figure you break this line in the argument list to avoid a backslash
> for line continuation. I know PEP8 doesn't like them, but I like line
> breaks away from top level operators even less. I feel it should be
> broken after the and operator, even though that'll require wrapping the
> condition in parenthesis.
Sadly, this form causes pep8 to complain about continuation at the same
indentation as the body:
if (cond1 and
cond2):
body
But it seems that pep8 and pylint don't mind the backslash in:
if cond1 and \
cond2:
body
where the indentation is okay. I guess trying to avoid the \ is not
worth it, if the tools don't complain about it, and that this was a case
of me prematurely guessing (incorrectly) about what the tools don't like.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 12:54 ` Eric Blake
@ 2015-10-02 14:12 ` Markus Armbruster
2015-10-02 14:33 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 14:12 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 01:02 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> A future patch will enable error reporting from the various
>>> QAPISchema*.check() methods. But to report an error related
>>> to an implicit type, we'll need to associate a location with
>>> the type (the same location as the top-level entity that is
>>> causing the creation of the implicit type), and once we do
>>> that, keying off of whether foo.info exists is no longer a
>>> viable way to determine if foo is an implicit type.
>>>
>>> Instead, add an is_implicit() method to QAPISchemaObjectType,
>>> and use that function where needed. (Done at the ObjectType
>>> level, since we already know all builtins and arrays are
>>> implicit, no commands or events are implicit, and we don't
>>> have any differences in generated code for regular vs.
>>> implicit enums.)
>
>>> +++ b/scripts/qapi-types.py
>>> @@ -234,7 +234,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>> self._btin = None
>>>
>>> def visit_predicate(self, entity):
>>> - return not isinstance(entity, QAPISchemaObjectType) or entity.info
>>> + return not (isinstance(entity, QAPISchemaObjectType) and
>>> + entity.is_implicit())
>>
>> Aha, now the left hand side becomes necessary to guard the
>> .is_implicit(). It stays superfluous if you make .is_implicit() a
>> QAPISchemaEntity method.
>
> See discussion on 1/12 - actually, it _becomes_ superfluous in this
> patch once we make it a QAPISchemaEntity method.
>
>>> +++ b/scripts/qapi.py
>>> @@ -970,12 +970,15 @@ class QAPISchemaObjectType(QAPISchemaType):
>>> self.variants.check(schema, members, seen)
>>> self.members = members
>>>
>>> + def is_implicit(self):
>>> + return self.name[0] == ':'
>
> Actually, this only works for implicit objects. Implicit enums instead
> have self.name[-4:] == 'Kind'. But qapi-types cares about implicit
> objects only. So if I hoist this, I may need something like:
>
> def is_implicit(self, type=None):
> if type and not isinstance(self, type):
> return Fals
> if isinstance(self, QAPISchemaObjectType):
> return self.name[0] == ':'
> if isinstance(self, QAPISchemaEnumType):
> return self.name[-4:] == 'Kind'
> return False
>
> where qapi-types would call entity.is_implicit(QAPISchemaObjectType).
Do it the OO-way: QAPISchemaEntity.is_implicit() returns False. Any
subclass that can have implicitly defined instances overrides it:
QAPISchemaObjectType.is_implicit() tests for ':' prefix,
QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
outlawing it for user enums, if we don't do it already), and so forth.
>>> +
>>
>> If this test is here to stay, perhaps add a comment pointing to
>> _make_implicit_object_type().
>
> Indeed.
>
>
>>> @@ -1043,7 +1046,8 @@ class
>>> QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>>> # This function exists to support ugly simple union special cases
>>> # TODO get rid of them, and drop the function
>>> def simple_union_type(self):
>>> - if isinstance(self.type, QAPISchemaObjectType) and not
>>> self.type.info:
>>> + if isinstance(self.type,
>>> + QAPISchemaObjectType) and self.type.is_implicit():
>>
>> I figure you break this line in the argument list to avoid a backslash
>> for line continuation. I know PEP8 doesn't like them, but I like line
>> breaks away from top level operators even less. I feel it should be
>> broken after the and operator, even though that'll require wrapping the
>> condition in parenthesis.
>
> Sadly, this form causes pep8 to complain about continuation at the same
> indentation as the body:
>
> if (cond1 and
> cond2):
> body
>
> But it seems that pep8 and pylint don't mind the backslash in:
>
> if cond1 and \
> cond2:
> body
>
> where the indentation is okay. I guess trying to avoid the \ is not
> worth it, if the tools don't complain about it, and that this was a case
> of me prematurely guessing (incorrectly) about what the tools don't like.
Quoting PEP8:
When the conditional part of an if -statement is long enough to
require that it be written across multiple lines, it's worth noting
that the combination of a two character keyword (i.e. if ), plus a
single space, plus an opening parenthesis creates a natural 4-space
indent for the subsequent lines of the multiline conditional. This
can produce a visual conflict with the indented suite of code nested
inside the if -statement, which would also naturally be indented to
4 spaces. This PEP takes no explicit position on how (or whether) to
further visually distinguish such conditional lines from the nested
suite inside the if -statement. Acceptable options in this situation
include, but are not limited to:
# No extra indentation.
if (this_is_one_thing and
that_is_another_thing):
do_something()
# Add a comment, which will provide some distinction in editors
# supporting syntax highlighting.
if (this_is_one_thing and
that_is_another_thing):
# Since both conditions are true, we can frobnicate.
do_something()
# Add some extra indentation on the conditional continuation line.
if (this_is_one_thing
and that_is_another_thing):
do_something()
The last one could make pep8 and pylint happy.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 14:12 ` Markus Armbruster
@ 2015-10-02 14:33 ` Eric Blake
2015-10-02 16:48 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 14:33 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
[-- Attachment #1: Type: text/plain, Size: 2017 bytes --]
On 10/02/2015 08:12 AM, Markus Armbruster wrote:
>> Actually, this only works for implicit objects. Implicit enums instead
>> have self.name[-4:] == 'Kind'. But qapi-types cares about implicit
>> objects only. So if I hoist this, I may need something like:
>>
>> def is_implicit(self, type=None):
>> if type and not isinstance(self, type):
>> return Fals
>> if isinstance(self, QAPISchemaObjectType):
>> return self.name[0] == ':'
>> if isinstance(self, QAPISchemaEnumType):
>> return self.name[-4:] == 'Kind'
>> return False
>>
>> where qapi-types would call entity.is_implicit(QAPISchemaObjectType).
>
> Do it the OO-way: QAPISchemaEntity.is_implicit() returns False. Any
> subclass that can have implicitly defined instances overrides it:
> QAPISchemaObjectType.is_implicit() tests for ':' prefix,
> QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
> outlawing it for user enums, if we don't do it already), and so forth.
But there's still the issue of filtering by subclass. For the
qapi-types visit_needed() filter, I either write:
if isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
or what I want to write:
if entity.is_implicit(QAPISchemaObjectType)
while still allowing the common case of is_implicit() when I don't care
which type is doing the testing. Since python doesn't allow method
overloads, I'd have to repeat the:
def is_implicit(self, type=None):
if type and not isinstance(self, type):
return False
prefix in each subclass that overrides the basic version.
>> where the indentation is okay. I guess trying to avoid the \ is not
>> worth it, if the tools don't complain about it, and that this was a case
>> of me prematurely guessing (incorrectly) about what the tools don't like.
>
> Quoting PEP8:
Thanks; that helps.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 14:33 ` Eric Blake
@ 2015-10-02 16:48 ` Markus Armbruster
2015-10-02 16:57 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 16:48 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, Michael Roth, ehabkost, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 08:12 AM, Markus Armbruster wrote:
>
>>> Actually, this only works for implicit objects. Implicit enums instead
>>> have self.name[-4:] == 'Kind'. But qapi-types cares about implicit
>>> objects only. So if I hoist this, I may need something like:
>>>
>>> def is_implicit(self, type=None):
>>> if type and not isinstance(self, type):
>>> return Fals
>>> if isinstance(self, QAPISchemaObjectType):
>>> return self.name[0] == ':'
>>> if isinstance(self, QAPISchemaEnumType):
>>> return self.name[-4:] == 'Kind'
>>> return False
>>>
>>> where qapi-types would call entity.is_implicit(QAPISchemaObjectType).
>>
>> Do it the OO-way: QAPISchemaEntity.is_implicit() returns False. Any
>> subclass that can have implicitly defined instances overrides it:
>> QAPISchemaObjectType.is_implicit() tests for ':' prefix,
>> QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
>> outlawing it for user enums, if we don't do it already), and so forth.
>
> But there's still the issue of filtering by subclass. For the
> qapi-types visit_needed() filter, I either write:
>
> if isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
If is_implicit() is defined on entities, then this shrinks to just
entity.is_implicit()
because no non-type entity is implicit.
> or what I want to write:
>
> if entity.is_implicit(QAPISchemaObjectType)
I'm not even sure what that's supposed to mean :)
> while still allowing the common case of is_implicit() when I don't care
> which type is doing the testing. Since python doesn't allow method
> overloads, I'd have to repeat the:
>
> def is_implicit(self, type=None):
> if type and not isinstance(self, type):
> return False
>
> prefix in each subclass that overrides the basic version.
AH, you seem to propose to define E.is_implicit(T) as "E is implicitly
defined and not an instance of T". Why not simply keep the two
predicates seperate? Am I missing something?
[...]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 16:48 ` Markus Armbruster
@ 2015-10-02 16:57 ` Eric Blake
2015-10-02 22:40 ` Eric Blake
2015-10-06 8:32 ` Markus Armbruster
0 siblings, 2 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 16:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, Michael Roth, ehabkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]
On 10/02/2015 10:48 AM, Markus Armbruster wrote:
>>> Do it the OO-way: QAPISchemaEntity.is_implicit() returns False. Any
>>> subclass that can have implicitly defined instances overrides it:
>>> QAPISchemaObjectType.is_implicit() tests for ':' prefix,
>>> QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
>>> outlawing it for user enums, if we don't do it already), and so forth.
>>
>> But there's still the issue of filtering by subclass. For the
>> qapi-types visit_needed() filter, I either write:
>>
>> if isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
>
> If is_implicit() is defined on entities, then this shrinks to just
>
> entity.is_implicit()
>
> because no non-type entity is implicit.
QAPISchemaEnumType is not a subclass of QAPISchemaObjectType, but it can
indeed be implicit (currently depends on whether its name ends in
"Kind"). Right now, the code we generate doesn't care about whether
enums are implicit. But having enum.is_implicit() return False merely
because it is not an ObjectType feels wrong; if anything, we'd want to
name it is_implicit_object() if we are only returning whether an object
is implicit.
>
>> or what I want to write:
>>
>> if entity.is_implicit(QAPISchemaObjectType)
>
> I'm not even sure what that's supposed to mean :)
If entity is an implicit ObjectType, return True. If it is implicit but
not an object (such as an implicit enum), or is not implicit (regardless
of whether it is an ObjectType), then return False.
And if you don't need filtering by python type, then
entity.is_implicit() (shorthand for entity.is_implicit(None)) then gives
the correct answer whether entity is ObjectType or EnumType or something
else.
>
>> while still allowing the common case of is_implicit() when I don't care
>> which type is doing the testing. Since python doesn't allow method
>> overloads, I'd have to repeat the:
>>
>> def is_implicit(self, type=None):
>> if type and not isinstance(self, type):
>> return False
>>
>> prefix in each subclass that overrides the basic version.
>
> AH, you seem to propose to define E.is_implicit(T) as "E is implicitly
> defined and not an instance of T". Why not simply keep the two
> predicates seperate? Am I missing something?
No, I was thinking E.is_implicit(T) is "E is implicitly defined and IS
an instance of T", while E.is_implicit(None) (the default) doesn't care
about type relations. To get that semantic, each override of the base
class .is_implicit() has to first filter out all non-T, before going
into its class-specific tests (ObjectType for leading ':', EnumType for
trailing 'Kind').
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 16:57 ` Eric Blake
@ 2015-10-02 22:40 ` Eric Blake
2015-10-06 8:32 ` Markus Armbruster
1 sibling, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 22:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, marcandre.lureau, Michael Roth, ehabkost
[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]
On 10/02/2015 10:57 AM, Eric Blake wrote:
> On 10/02/2015 10:48 AM, Markus Armbruster wrote:
>
>>>> Do it the OO-way: QAPISchemaEntity.is_implicit() returns False. Any
>>>> subclass that can have implicitly defined instances overrides it:
>>>> QAPISchemaObjectType.is_implicit() tests for ':' prefix,
>>>> QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
>>>> outlawing it for user enums, if we don't do it already), and so forth.
>>>
>>> But there's still the issue of filtering by subclass. For the
>>> qapi-types visit_needed() filter, I either write:
>>>
>>> or what I want to write:
>>>
>>> if entity.is_implicit(QAPISchemaObjectType)
>>
>> I'm not even sure what that's supposed to mean :)
>
> If entity is an implicit ObjectType, return True. If it is implicit but
> not an object (such as an implicit enum), or is not implicit (regardless
> of whether it is an ObjectType), then return False.
>
> And if you don't need filtering by python type, then
> entity.is_implicit() (shorthand for entity.is_implicit(None)) then gives
> the correct answer whether entity is ObjectType or EnumType or something
> else.
>
>>
>>> while still allowing the common case of is_implicit() when I don't care
>>> which type is doing the testing. Since python doesn't allow method
>>> overloads, I'd have to repeat the:
>>>
>>> def is_implicit(self, type=None):
>>> if type and not isinstance(self, type):
>>> return False
>>>
>>> prefix in each subclass that overrides the basic version.
Or, as I just realized, split the public interface from the private
interface:
class QAPISchemaEntity...
def is_implicit(self, typ=None):
if typ and not isinstance(self, typ):
return False
return self._is_implicit()
def _is_implicit(self):
return not self.info
class QAPISchemaObjectTypeMember...
def _is_implicit(self):
return self.name[0] == ':'
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-02 16:57 ` Eric Blake
2015-10-02 22:40 ` Eric Blake
@ 2015-10-06 8:32 ` Markus Armbruster
2015-10-06 11:56 ` Eric Blake
1 sibling, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-06 8:32 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, marcandre.lureau, Michael Roth, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 10:48 AM, Markus Armbruster wrote:
>
>>>> Do it the OO-way: QAPISchemaEntity.is_implicit() returns False. Any
>>>> subclass that can have implicitly defined instances overrides it:
>>>> QAPISchemaObjectType.is_implicit() tests for ':' prefix,
>>>> QAPISchemaEnumType.is_implicit() tests for 'Kind' suffix (requires
>>>> outlawing it for user enums, if we don't do it already), and so forth.
>>>
>>> But there's still the issue of filtering by subclass. For the
>>> qapi-types visit_needed() filter, I either write:
>>>
>>> if isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
>>
>> If is_implicit() is defined on entities, then this shrinks to just
>>
>> entity.is_implicit()
>>
>> because no non-type entity is implicit.
>
> QAPISchemaEnumType is not a subclass of QAPISchemaObjectType, but it can
> indeed be implicit (currently depends on whether its name ends in
> "Kind"). Right now, the code we generate doesn't care about whether
> enums are implicit. But having enum.is_implicit() return False merely
> because it is not an ObjectType feels wrong; if anything, we'd want to
> name it is_implicit_object() if we are only returning whether an object
> is implicit.
You're right; I got confused. I think the most straightforward way to
test "is entity an implicitly defined object type" is the way you wrote
it:
isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
If we define is_implicit() on entities, either ordering of operands
works.
>>> or what I want to write:
>>>
>>> if entity.is_implicit(QAPISchemaObjectType)
>>
>> I'm not even sure what that's supposed to mean :)
>
> If entity is an implicit ObjectType, return True. If it is implicit but
> not an object (such as an implicit enum), or is not implicit (regardless
> of whether it is an ObjectType), then return False.
>
> And if you don't need filtering by python type, then
> entity.is_implicit() (shorthand for entity.is_implicit(None)) then gives
> the correct answer whether entity is ObjectType or EnumType or something
> else.
I find
isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
more obvious than
entity.is_implicit(QAPISchemaObjectType)
[...]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-06 8:32 ` Markus Armbruster
@ 2015-10-06 11:56 ` Eric Blake
2015-10-06 13:31 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-06 11:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, marcandre.lureau, Michael Roth, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]
On 10/06/2015 02:32 AM, Markus Armbruster wrote:
>> If entity is an implicit ObjectType, return True. If it is implicit but
>> not an object (such as an implicit enum), or is not implicit (regardless
>> of whether it is an ObjectType), then return False.
>>
>> And if you don't need filtering by python type, then
>> entity.is_implicit() (shorthand for entity.is_implicit(None)) then gives
>> the correct answer whether entity is ObjectType or EnumType or something
>> else.
>
> I find
>
> isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
>
> more obvious than
>
> entity.is_implicit(QAPISchemaObjectType)
Well, I had already written v7 before this request of yours; do you want
to see how it turned out, before deciding if I need a v8 that throws the
isinstance check burden back on the caller?
http://thread.gmane.org/gmane.comp.emulators.qemu/366810/focus=366809
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type
2015-10-06 11:56 ` Eric Blake
@ 2015-10-06 13:31 ` Markus Armbruster
0 siblings, 0 replies; 46+ messages in thread
From: Markus Armbruster @ 2015-10-06 13:31 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/06/2015 02:32 AM, Markus Armbruster wrote:
>
>>> If entity is an implicit ObjectType, return True. If it is implicit but
>>> not an object (such as an implicit enum), or is not implicit (regardless
>>> of whether it is an ObjectType), then return False.
>>>
>>> And if you don't need filtering by python type, then
>>> entity.is_implicit() (shorthand for entity.is_implicit(None)) then gives
>>> the correct answer whether entity is ObjectType or EnumType or something
>>> else.
>>
>> I find
>>
>> isinstance(entity, QAPISchemaObjectType) and entity.is_implicit()
>>
>> more obvious than
>>
>> entity.is_implicit(QAPISchemaObjectType)
>
> Well, I had already written v7 before this request of yours; do you want
> to see how it turned out, before deciding if I need a v8 that throws the
> isinstance check burden back on the caller?
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/366810/focus=366809
I certainly want to review your v7 first.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 01/12] qapi: Use predicate callback to determine visit filtering Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 02/12] qapi: Don't use info as witness of implicit object type Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 8:06 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier Eric Blake
` (8 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Commit ac88219a had several TODO markers about whether we needed
to automatically create the corresponding array type alongside
any other type. It turns out that most of the time, we don't!
As part of lazy creation of array types, this patch now assigns
an 'info' to array types at their point of first instantiation,
rather than leaving it None.
There are a few exceptions: 1) We have a few situations where we
use an array type in internal code but do not expose that type
through QMP; fix it by declaring a dummy type that forces the
generator to see that we want to use the array type.
2) The builtin arrays (such as intList for QAPI ['int']) must
always be generated, because of the way our QAPI_TYPES_BUILTIN
compile guard works: we have situations (at the very least
tests/test-qmp-output-visitor.c) that include both top-level
"qapi-types.h" (via "error.h") and a secondary
"test-qapi-types.h". If we were to only emit the builtin types
when used locally, then the first .h file would not include all
types, but the second .h does not declare anything at all because
the first .h set QAPI_TYPES_BUILTIN, and we would end up with
compilation error due to things like unknown type 'int8List'.
Actually, we may need to revisit how we do type guards, and
change from a single QAPI_TYPES_BUILTIN over to a different
usage pattern that does one #ifdef per qapi type - right now,
the only types that are declared multiple times between two qapi
.json files for inclusion by a single .c file happen to be the
builtin arrays. But now that we have QAPI 'include' statements,
it is logical to assume that we will soon reach a point where
we want to reuse non-builtin types (yes, I'm thinking about what
it will take to add introspection to QGA, where we will want to
reuse the SchemaInfo type and friends). One #ifdef per type
will help ensure that generating the same qapi type into more
than one qapi-types.h won't cause collisions when both are
included in the same .c file; but we also have to solve how to
avoid creating duplicate qapi-types.c entry points. So that
is a problem left for another day.
Interestingly, the introspection output is unchanged - this is
because we already cull all types that are not indirectly
reachable from a command or event, so introspection was already
using only a subset of array types. The subset of types
introspected is now a much larger percentage of the overall set
of array types emitted in qapi-types.h (since the larger set
shrunk), but still not 100% (proof that the array types emitted
for our new Dummy struct, and the new Dummy struct itself, don't
affect QMP). Of course, the rest of the generated output is
drastically shrunk (a number of unused array types are no longer
generated) [for best results, diff the generated files with
'git diff --patience --no-index pre post'].
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch
---
qapi-schema.json | 10 +++++++++
scripts/qapi.py | 39 ++++++++++++++++-----------------
tests/qapi-schema/qapi-schema-test.json | 4 ++++
tests/qapi-schema/qapi-schema-test.out | 3 +++
4 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b0520c..98bf0b2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3396,6 +3396,16 @@
'features': 'int' } }
##
+# @Dummy
+#
+# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
+#
+# Since 2.5
+##
+{ 'struct': 'Dummy', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
+
+
+##
# @RxState:
#
# Packets receiving state
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8123ab3..15640b6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1143,7 +1143,7 @@ class QAPISchema(object):
def _def_builtin_type(self, name, json_type, c_type, c_null):
self._def_entity(QAPISchemaBuiltinType(name, json_type,
c_type, c_null))
- self._make_array_type(name) # TODO really needed?
+ self._make_array_type(name, None)
def _def_predefineds(self):
for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'),
@@ -1170,10 +1170,10 @@ class QAPISchema(object):
self._def_entity(QAPISchemaEnumType(name, None, values, None))
return name
- def _make_array_type(self, element_type):
+ def _make_array_type(self, element_type, info):
name = element_type + 'List'
if not self.lookup_type(name):
- self._def_entity(QAPISchemaArrayType(name, None, element_type))
+ self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
def _make_implicit_object_type(self, name, role, members):
@@ -1190,20 +1190,19 @@ class QAPISchema(object):
data = expr['data']
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
- self._make_array_type(name) # TODO really needed?
- def _make_member(self, name, typ):
+ def _make_member(self, name, typ, info):
optional = False
if name.startswith('*'):
name = name[1:]
optional = True
if isinstance(typ, list):
assert len(typ) == 1
- typ = self._make_array_type(typ[0])
+ typ = self._make_array_type(typ[0], info)
return QAPISchemaObjectTypeMember(name, typ, optional)
- def _make_members(self, data):
- return [self._make_member(key, value)
+ def _make_members(self, data, info):
+ return [self._make_member(key, value, info)
for (key, value) in data.iteritems()]
def _def_struct_type(self, expr, info):
@@ -1211,19 +1210,19 @@ class QAPISchema(object):
base = expr.get('base')
data = expr['data']
self._def_entity(QAPISchemaObjectType(name, info, base,
- self._make_members(data),
+ self._make_members(data, info),
None))
- self._make_array_type(name) # TODO really needed?
def _make_variant(self, case, typ):
return QAPISchemaObjectTypeVariant(case, typ)
- def _make_simple_variant(self, case, typ):
+ def _make_simple_variant(self, case, typ, info):
if isinstance(typ, list):
assert len(typ) == 1
- typ = self._make_array_type(typ[0])
+ typ = self._make_array_type(typ[0], info)
typ = self._make_implicit_object_type(typ, 'wrapper',
- [self._make_member('data', typ)])
+ [self._make_member('data', typ,
+ info)])
return QAPISchemaObjectTypeVariant(case, typ)
def _make_tag_enum(self, type_name, variants):
@@ -1240,16 +1239,15 @@ class QAPISchema(object):
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
else:
- variants = [self._make_simple_variant(key, value)
+ variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
tag_enum = self._make_tag_enum(name, variants)
self._def_entity(
QAPISchemaObjectType(name, info, base,
- self._make_members(OrderedDict()),
+ self._make_members(OrderedDict(), info),
QAPISchemaObjectTypeVariants(tag_name,
tag_enum,
variants)))
- self._make_array_type(name) # TODO really needed?
def _def_alternate_type(self, expr, info):
name = expr['alternate']
@@ -1262,7 +1260,6 @@ class QAPISchema(object):
QAPISchemaObjectTypeVariants(None,
tag_enum,
variants)))
- self._make_array_type(name) # TODO really needed?
def _def_command(self, expr, info):
name = expr['command']
@@ -1272,10 +1269,11 @@ class QAPISchema(object):
success_response = expr.get('success-response', True)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(name, 'arg',
- self._make_members(data))
+ self._make_members(data,
+ info))
if isinstance(rets, list):
assert len(rets) == 1
- rets = self._make_array_type(rets[0])
+ rets = self._make_array_type(rets[0], info)
self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
success_response))
@@ -1284,7 +1282,8 @@ class QAPISchema(object):
data = expr.get('data')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(name, 'arg',
- self._make_members(data))
+ self._make_members(data,
+ info))
self._def_entity(QAPISchemaEvent(name, info, data))
def _def_exprs(self):
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index abe59fd..020ff2e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -31,6 +31,10 @@
'data': { 'string0': 'str',
'dict1': 'UserDefTwoDict' } }
+# dummy struct to force generation of array types not otherwise mentioned
+{ 'struct': 'ForceArrays',
+ 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } }
+
# for testing unions
# Among other things, test that a name collision between branches does
# not cause any problems (since only one branch can be in use at a time),
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8f81784..2a8c82e 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -86,6 +86,9 @@ object EventStructOne
member struct1: UserDefOne optional=False
member string: str optional=False
member enum2: EnumOne optional=True
+object ForceArrays
+ member unused1: UserDefOneList optional=False
+ member unused2: UserDefTwoList optional=False
object NestedEnumsOne
member enum1: EnumOne optional=False
member enum2: EnumOne optional=True
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types Eric Blake
@ 2015-10-02 8:06 ` Markus Armbruster
2015-10-02 13:05 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 8:06 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Woot!
Eric Blake <eblake@redhat.com> writes:
> Commit ac88219a had several TODO markers about whether we needed
> to automatically create the corresponding array type alongside
> any other type. It turns out that most of the time, we don't!
>
> As part of lazy creation of array types, this patch now assigns
> an 'info' to array types at their point of first instantiation,
> rather than leaving it None.
I guess our general for info should be:
For explicitly defined entities, info points to the (explicit)
definition. For implicitly defined entities, it points to a place that
triggers implicit definition. For some kinds of entities, multiple
places may exist, and info points to one of them.
First one we encounter is as good as any, and easy to compute.
> There are a few exceptions: 1) We have a few situations where we
> use an array type in internal code but do not expose that type
> through QMP; fix it by declaring a dummy type that forces the
> generator to see that we want to use the array type.
>
> 2) The builtin arrays (such as intList for QAPI ['int']) must
> always be generated, because of the way our QAPI_TYPES_BUILTIN
> compile guard works: we have situations (at the very least
> tests/test-qmp-output-visitor.c) that include both top-level
> "qapi-types.h" (via "error.h") and a secondary
> "test-qapi-types.h". If we were to only emit the builtin types
> when used locally, then the first .h file would not include all
> types, but the second .h does not declare anything at all because
> the first .h set QAPI_TYPES_BUILTIN, and we would end up with
> compilation error due to things like unknown type 'int8List'.
Yes.
> Actually, we may need to revisit how we do type guards, and
> change from a single QAPI_TYPES_BUILTIN over to a different
> usage pattern that does one #ifdef per qapi type - right now,
> the only types that are declared multiple times between two qapi
> .json files for inclusion by a single .c file happen to be the
> builtin arrays. But now that we have QAPI 'include' statements,
> it is logical to assume that we will soon reach a point where
> we want to reuse non-builtin types (yes, I'm thinking about what
> it will take to add introspection to QGA, where we will want to
> reuse the SchemaInfo type and friends). One #ifdef per type
> will help ensure that generating the same qapi type into more
> than one qapi-types.h won't cause collisions when both are
> included in the same .c file; but we also have to solve how to
> avoid creating duplicate qapi-types.c entry points. So that
> is a problem left for another day.
Yes, let's leave it for later.
> Interestingly, the introspection output is unchanged - this is
> because we already cull all types that are not indirectly
> reachable from a command or event, so introspection was already
> using only a subset of array types. The subset of types
> introspected is now a much larger percentage of the overall set
> of array types emitted in qapi-types.h (since the larger set
> shrunk), but still not 100% (proof that the array types emitted
> for our new Dummy struct, and the new Dummy struct itself, don't
> affect QMP). Of course, the rest of the generated output is
> drastically shrunk (a number of unused array types are no longer
> generated) [for best results, diff the generated files with
> 'git diff --patience --no-index pre post'].
Here, you're talking about the effect on generated code. I'd put the
specific case of introspection last, in a paragraph of its own.
How much is "drastically"? Here's what I get:
$ git-diff --patience --no-index --stat o n
{o => n}/qapi-types.c | 2239 +-----------------------------
{o => n}/qapi-types.h | 1800 +-----------------------
{o => n}/qapi-visit.c | 3605 +------------------------------------------------
{o => n}/qapi-visit.h | 151 +--
4 files changed, 23 insertions(+), 7772 deletions(-)
$ wc o/* | tail -n 1
34738 90018 985738 total
$ wc n/* | tail -n 1
26989 71981 788721 total
That's a >20% shrink.
$ grep -c 'typedef .*List;' {o,n}/qapi-types.h
o/qapi-types.h:219
n/qapi-types.h:69
Less than a third of the array types are still around. Includes 14
built-in array types.
Compiled size with -O2 for me, before:
text data bss dec hex filename
39235 4576 0 43811 ab23 qapi-types.o
159424 0 0 159424 26ec0 qapi-visit.o
After:
text data bss dec hex filename
24931 4576 0 29507 7343 qapi-types.o
115581 0 0 115581 1c37d qapi-visit.o
A 30% shrink. Lovely.
Further reductions are probably possible. One step at a time.
Generating code makes profligate wastefulness so much easier.
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: new patch
> ---
> qapi-schema.json | 10 +++++++++
> scripts/qapi.py | 39 ++++++++++++++++-----------------
> tests/qapi-schema/qapi-schema-test.json | 4 ++++
> tests/qapi-schema/qapi-schema-test.out | 3 +++
> 4 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8b0520c..98bf0b2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3396,6 +3396,16 @@
> 'features': 'int' } }
>
> ##
> +# @Dummy
> +#
> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'Dummy', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
DummyToForceArrays?
> +
> +
> +##
> # @RxState:
> #
> # Packets receiving state
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8123ab3..15640b6 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1143,7 +1143,7 @@ class QAPISchema(object):
> def _def_builtin_type(self, name, json_type, c_type, c_null):
> self._def_entity(QAPISchemaBuiltinType(name, json_type,
> c_type, c_null))
> - self._make_array_type(name) # TODO really needed?
> + self._make_array_type(name, None)
Should we keep a TODO here?
>
> def _def_predefineds(self):
> for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'),
> @@ -1170,10 +1170,10 @@ class QAPISchema(object):
> self._def_entity(QAPISchemaEnumType(name, None, values, None))
> return name
>
> - def _make_array_type(self, element_type):
> + def _make_array_type(self, element_type, info):
> name = element_type + 'List'
> if not self.lookup_type(name):
> - self._def_entity(QAPISchemaArrayType(name, None, element_type))
> + self._def_entity(QAPISchemaArrayType(name, info, element_type))
> return name
>
> def _make_implicit_object_type(self, name, role, members):
> @@ -1190,20 +1190,19 @@ class QAPISchema(object):
> data = expr['data']
> prefix = expr.get('prefix')
> self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
> - self._make_array_type(name) # TODO really needed?
>
> - def _make_member(self, name, typ):
> + def _make_member(self, name, typ, info):
> optional = False
> if name.startswith('*'):
> name = name[1:]
> optional = True
> if isinstance(typ, list):
> assert len(typ) == 1
> - typ = self._make_array_type(typ[0])
> + typ = self._make_array_type(typ[0], info)
> return QAPISchemaObjectTypeMember(name, typ, optional)
>
> - def _make_members(self, data):
> - return [self._make_member(key, value)
> + def _make_members(self, data, info):
> + return [self._make_member(key, value, info)
> for (key, value) in data.iteritems()]
>
> def _def_struct_type(self, expr, info):
> @@ -1211,19 +1210,19 @@ class QAPISchema(object):
> base = expr.get('base')
> data = expr['data']
> self._def_entity(QAPISchemaObjectType(name, info, base,
> - self._make_members(data),
> + self._make_members(data, info),
> None))
> - self._make_array_type(name) # TODO really needed?
>
> def _make_variant(self, case, typ):
> return QAPISchemaObjectTypeVariant(case, typ)
>
> - def _make_simple_variant(self, case, typ):
> + def _make_simple_variant(self, case, typ, info):
> if isinstance(typ, list):
> assert len(typ) == 1
> - typ = self._make_array_type(typ[0])
> + typ = self._make_array_type(typ[0], info)
> typ = self._make_implicit_object_type(typ, 'wrapper',
> - [self._make_member('data', typ)])
> + [self._make_member('data', typ,
> + info)])
Consider a hanging indent here:
typ = self._make_implicit_object_type(
typ, 'wrapper', [self._make_member('data', typ, info)])
> return QAPISchemaObjectTypeVariant(case, typ)
>
> def _make_tag_enum(self, type_name, variants):
> @@ -1240,16 +1239,15 @@ class QAPISchema(object):
> variants = [self._make_variant(key, value)
> for (key, value) in data.iteritems()]
> else:
> - variants = [self._make_simple_variant(key, value)
> + variants = [self._make_simple_variant(key, value, info)
> for (key, value) in data.iteritems()]
> tag_enum = self._make_tag_enum(name, variants)
> self._def_entity(
> QAPISchemaObjectType(name, info, base,
> - self._make_members(OrderedDict()),
> + self._make_members(OrderedDict(), info),
> QAPISchemaObjectTypeVariants(tag_name,
> tag_enum,
> variants)))
> - self._make_array_type(name) # TODO really needed?
>
> def _def_alternate_type(self, expr, info):
> name = expr['alternate']
> @@ -1262,7 +1260,6 @@ class QAPISchema(object):
> QAPISchemaObjectTypeVariants(None,
> tag_enum,
> variants)))
> - self._make_array_type(name) # TODO really needed?
>
> def _def_command(self, expr, info):
> name = expr['command']
> @@ -1272,10 +1269,11 @@ class QAPISchema(object):
> success_response = expr.get('success-response', True)
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(name, 'arg',
> - self._make_members(data))
> + self._make_members(data,
> + info))
Hanging indent?
> if isinstance(rets, list):
> assert len(rets) == 1
> - rets = self._make_array_type(rets[0])
> + rets = self._make_array_type(rets[0], info)
> self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
> success_response))
>
> @@ -1284,7 +1282,8 @@ class QAPISchema(object):
> data = expr.get('data')
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(name, 'arg',
> - self._make_members(data))
> + self._make_members(data,
> + info))
Hanging indent?
> self._def_entity(QAPISchemaEvent(name, info, data))
>
> def _def_exprs(self):
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index abe59fd..020ff2e 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -31,6 +31,10 @@
> 'data': { 'string0': 'str',
> 'dict1': 'UserDefTwoDict' } }
>
> +# dummy struct to force generation of array types not otherwise mentioned
> +{ 'struct': 'ForceArrays',
> + 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } }
> +
> # for testing unions
> # Among other things, test that a name collision between branches does
> # not cause any problems (since only one branch can be in use at a time),
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 8f81784..2a8c82e 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -86,6 +86,9 @@ object EventStructOne
> member struct1: UserDefOne optional=False
> member string: str optional=False
> member enum2: EnumOne optional=True
> +object ForceArrays
> + member unused1: UserDefOneList optional=False
> + member unused2: UserDefTwoList optional=False
> object NestedEnumsOne
> member enum1: EnumOne optional=False
> member enum2: EnumOne optional=True
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types
2015-10-02 8:06 ` Markus Armbruster
@ 2015-10-02 13:05 ` Eric Blake
2015-10-06 8:35 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 13:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]
On 10/02/2015 02:06 AM, Markus Armbruster wrote:
> Woot!
>
> Eric Blake <eblake@redhat.com> writes:
>
>> Commit ac88219a had several TODO markers about whether we needed
>> to automatically create the corresponding array type alongside
>> any other type. It turns out that most of the time, we don't!
>>
>> As part of lazy creation of array types, this patch now assigns
>> an 'info' to array types at their point of first instantiation,
>> rather than leaving it None.
>
> I guess our general for info should be:
s/general/general description/ ?
>
> For explicitly defined entities, info points to the (explicit)
> definition. For implicitly defined entities, it points to a place that
> triggers implicit definition. For some kinds of entities, multiple
> places may exist, and info points to one of them.
>
Right now, we don't document any of the fields, but I can certainly add
that comment.
>
> Here, you're talking about the effect on generated code. I'd put the
> specific case of introspection last, in a paragraph of its own.
>
Improving the commit message is easier than redoing bad code :)
>> +++ b/qapi-schema.json
>> @@ -3396,6 +3396,16 @@
>> 'features': 'int' } }
>>
>> ##
>> +# @Dummy
>> +#
>> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
>> +#
>> +# Since 2.5
>> +##
>> +{ 'struct': 'Dummy', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
>
> DummyToForceArrays?
Sure.
>
>> +
>> +
>> +##
>> # @RxState:
>> #
>> # Packets receiving state
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 8123ab3..15640b6 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1143,7 +1143,7 @@ class QAPISchema(object):
>> def _def_builtin_type(self, name, json_type, c_type, c_null):
>> self._def_entity(QAPISchemaBuiltinType(name, json_type,
>> c_type, c_null))
>> - self._make_array_type(name) # TODO really needed?
>> + self._make_array_type(name, None)
>
> Should we keep a TODO here?
Sure, with better text explaining the guard issue on types shared in
multiple qapi-types.h files.
>> typ = self._make_implicit_object_type(typ, 'wrapper',
>> - [self._make_member('data', typ)])
>> + [self._make_member('data', typ,
>> + info)])
>
> Consider a hanging indent here:
>
> typ = self._make_implicit_object_type(
> typ, 'wrapper', [self._make_member('data', typ, info)])
Okay. pep8 and pylint like it (I'm not used to doing it, but it does
look better, and emacs didn't fight me too hard).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types
2015-10-02 13:05 ` Eric Blake
@ 2015-10-06 8:35 ` Markus Armbruster
0 siblings, 0 replies; 46+ messages in thread
From: Markus Armbruster @ 2015-10-06 8:35 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 02:06 AM, Markus Armbruster wrote:
>> Woot!
>>
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Commit ac88219a had several TODO markers about whether we needed
>>> to automatically create the corresponding array type alongside
>>> any other type. It turns out that most of the time, we don't!
>>>
>>> As part of lazy creation of array types, this patch now assigns
>>> an 'info' to array types at their point of first instantiation,
>>> rather than leaving it None.
>>
>> I guess our general for info should be:
>
> s/general/general description/ ?
General idea.
Looks like my typing engine doesn't run on all cylinders on Fridays...
>> For explicitly defined entities, info points to the (explicit)
>> definition. For implicitly defined entities, it points to a place that
>> triggers implicit definition. For some kinds of entities, multiple
>> places may exist, and info points to one of them.
>>
>
> Right now, we don't document any of the fields, but I can certainly add
> that comment.
>
>
>>
>> Here, you're talking about the effect on generated code. I'd put the
>> specific case of introspection last, in a paragraph of its own.
>>
>
> Improving the commit message is easier than redoing bad code :)
>
>>> +++ b/qapi-schema.json
>>> @@ -3396,6 +3396,16 @@
>>> 'features': 'int' } }
>>>
>>> ##
>>> +# @Dummy
>>> +#
>>> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
>>> +#
>>> +# Since 2.5
>>> +##
>>> +{ 'struct': 'Dummy', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
>>
>> DummyToForceArrays?
>
> Sure.
>
>>
>>> +
>>> +
>>> +##
>>> # @RxState:
>>> #
>>> # Packets receiving state
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 8123ab3..15640b6 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -1143,7 +1143,7 @@ class QAPISchema(object):
>>> def _def_builtin_type(self, name, json_type, c_type, c_null):
>>> self._def_entity(QAPISchemaBuiltinType(name, json_type,
>>> c_type, c_null))
>>> - self._make_array_type(name) # TODO really needed?
>>> + self._make_array_type(name, None)
>>
>> Should we keep a TODO here?
>
> Sure, with better text explaining the guard issue on types shared in
> multiple qapi-types.h files.
>
>
>>> typ = self._make_implicit_object_type(typ, 'wrapper',
>>> - [self._make_member('data', typ)])
>>> + [self._make_member('data', typ,
>>> + info)])
>>
>> Consider a hanging indent here:
>>
>> typ = self._make_implicit_object_type(
>> typ, 'wrapper', [self._make_member('data', typ, info)])
>
>
> Okay. pep8 and pylint like it (I'm not used to doing it, but it does
> look better, and emacs didn't fight me too hard).
I'm still getting used to hanging indents myself. When in Rome...
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (2 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 8:34 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type Eric Blake
` (7 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
For simple unions, we were creating the implicit 'type' tag
member during the QAPISchemaObjectTypeVariants constructor.
This is different from every other implicit QAPISchemaEntity
object, which get created by QAPISchema methods. Hoist the
creation to the caller, and pass the entity rather than the
string name, so that we have the nice property that no
entities are created as a side effect within a different
entity. A later patch will then have an easier time of
associating location info with each entity creation.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: New patch
---
scripts/qapi.py | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 15640b6..255001a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1010,18 +1010,16 @@ class QAPISchemaObjectTypeMember(object):
class QAPISchemaObjectTypeVariants(object):
- def __init__(self, tag_name, tag_enum, variants):
+ def __init__(self, tag_name, tag_member, variants):
assert tag_name is None or isinstance(tag_name, str)
- assert tag_enum is None or isinstance(tag_enum, str)
+ assert (tag_member is None or
+ isinstance(tag_member, QAPISchemaObjectTypeMember))
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self.tag_name = tag_name
if tag_name:
- assert not tag_enum
- self.tag_member = None
- else:
- self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
- False)
+ assert tag_member is None
+ self.tag_member = tag_member
self.variants = variants
def check(self, schema, members, seen):
@@ -1226,8 +1224,9 @@ class QAPISchema(object):
return QAPISchemaObjectTypeVariant(case, typ)
def _make_tag_enum(self, type_name, variants):
- return self._make_implicit_enum_type(type_name,
- [v.name for v in variants])
+ typ = self._make_implicit_enum_type(type_name,
+ [v.name for v in variants])
+ return QAPISchemaObjectTypeMember('type', typ, False)
def _def_union_type(self, expr, info):
name = expr['union']
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier Eric Blake
@ 2015-10-02 8:34 ` Markus Armbruster
2015-10-02 13:08 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 8:34 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> For simple unions, we were creating the implicit 'type' tag
> member during the QAPISchemaObjectTypeVariants constructor.
> This is different from every other implicit QAPISchemaEntity
> object, which get created by QAPISchema methods. Hoist the
> creation to the caller, and pass the entity rather than the
> string name, so that we have the nice property that no
> entities are created as a side effect within a different
> entity. A later patch will then have an easier time of
> associating location info with each entity creation.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: New patch
> ---
> scripts/qapi.py | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 15640b6..255001a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1010,18 +1010,16 @@ class QAPISchemaObjectTypeMember(object):
>
>
> class QAPISchemaObjectTypeVariants(object):
> - def __init__(self, tag_name, tag_enum, variants):
> + def __init__(self, tag_name, tag_member, variants):
> assert tag_name is None or isinstance(tag_name, str)
> - assert tag_enum is None or isinstance(tag_enum, str)
> + assert (tag_member is None or
> + isinstance(tag_member, QAPISchemaObjectTypeMember))
> for v in variants:
> assert isinstance(v, QAPISchemaObjectTypeVariant)
> self.tag_name = tag_name
> if tag_name:
> - assert not tag_enum
> - self.tag_member = None
> - else:
> - self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
> - False)
> + assert tag_member is None
Since the conditional degenerates to just checking the "either tag_name
of tag_member" precondition, let's move it to right after the checking
of tag_name and tag_member, i.e. before the loop.
I'd also simplify to
assert not tag_name != not tag_member
or
bool(tag_name) != bool(tag_member)
> + self.tag_member = tag_member
> self.variants = variants
Before:
__init__() preconditions involving the tag:
if flat union:
tag_name is the name of the tag member
tag_enum is None
else simple union or alternate:
tag_name is None
tag_enum is the name of the implicitly defined enumeration type
__init__() postconditions:
if flat union:
self.tag_name is the name of the tag member
self.tag_member is None
else simple union or alternate:
self.tag_name is None
self.tag_member is the implicitly defined tag member
check() postconditions:
if flat union:
self.tag_name is the name of the tag member
else simple union or alternate:
self.tag_name is None
self.tag_member is the tag member
After:
__init__() preconditions involving the tag:
if flat union:
tag_name is the name of the tag member
tag_member is None
else simple union or alternate:
tag_name is None
tag_member is the implicitly defined tag member
__init__() postconditions:
exactly as before
check() postconditions:
exactly as before
Okay. Slightly simpler even.
>
> def check(self, schema, members, seen):
> @@ -1226,8 +1224,9 @@ class QAPISchema(object):
> return QAPISchemaObjectTypeVariant(case, typ)
>
> def _make_tag_enum(self, type_name, variants):
> - return self._make_implicit_enum_type(type_name,
> - [v.name for v in variants])
> + typ = self._make_implicit_enum_type(type_name,
> + [v.name for v in variants])
> + return QAPISchemaObjectTypeMember('type', typ, False)
I think this function should now be called _make_tag_member(), or
_make_implicit_tag_member(), or _make_implicit_tag().
>
> def _def_union_type(self, expr, info):
> name = expr['union']
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier
2015-10-02 8:34 ` Markus Armbruster
@ 2015-10-02 13:08 ` Eric Blake
0 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 13:08 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2851 bytes --]
On 10/02/2015 02:34 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> For simple unions, we were creating the implicit 'type' tag
>> member during the QAPISchemaObjectTypeVariants constructor.
>> This is different from every other implicit QAPISchemaEntity
>> object, which get created by QAPISchema methods. Hoist the
>> creation to the caller, and pass the entity rather than the
>> string name, so that we have the nice property that no
>> entities are created as a side effect within a different
>> entity. A later patch will then have an easier time of
>> associating location info with each entity creation.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> class QAPISchemaObjectTypeVariants(object):
>> - def __init__(self, tag_name, tag_enum, variants):
>> + def __init__(self, tag_name, tag_member, variants):
>> assert tag_name is None or isinstance(tag_name, str)
>> - assert tag_enum is None or isinstance(tag_enum, str)
>> + assert (tag_member is None or
>> + isinstance(tag_member, QAPISchemaObjectTypeMember))
>> for v in variants:
>> assert isinstance(v, QAPISchemaObjectTypeVariant)
>> self.tag_name = tag_name
>> if tag_name:
>> - assert not tag_enum
>> - self.tag_member = None
>> - else:
>> - self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
>> - False)
>> + assert tag_member is None
>
> Since the conditional degenerates to just checking the "either tag_name
> of tag_member" precondition, let's move it to right after the checking
> of tag_name and tag_member, i.e. before the loop.
>
> I'd also simplify to
>
> assert not tag_name != not tag_member
>
> or
>
> bool(tag_name) != bool(tag_member)
Sure, that works. I'll also add comments on the preconditions.
>> @@ -1226,8 +1224,9 @@ class QAPISchema(object):
>> return QAPISchemaObjectTypeVariant(case, typ)
>>
>> def _make_tag_enum(self, type_name, variants):
>> - return self._make_implicit_enum_type(type_name,
>> - [v.name for v in variants])
>> + typ = self._make_implicit_enum_type(type_name,
>> + [v.name for v in variants])
>> + return QAPISchemaObjectTypeMember('type', typ, False)
>
> I think this function should now be called _make_tag_member(), or
> _make_implicit_tag_member(), or _make_implicit_tag().
>
Makes sense. I'll probably use _make_implicit_tag().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (3 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 04/12] qapi: Create simple union type member earlier Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 8:54 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member Eric Blake
` (6 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
A future patch will enable deferred error detection in the
various QAPISchema*.check() methods (rather than the current
ad hoc parse checks). But that means the user can request
a QAPI entity that will only fail validation after it has
been initialized. Since all errors have to have an
associated 'info' location, we need a location to be
associated with all user-triggered implicit types. The
intuitive info to use is the location of the enclosing
entity that caused the creation of the implicit type.
Note that we do not anticipate builtin types being used in
an error message (as they are not part of the user's QAPI
input, the user can't cause a semantic error in their
behavior), so we exempt those types from requiring info, by
setting a flag to track the completion of _def_predefineds().
No change to the generated code.
RFC: I used a class-level static flag to track whether we expected
'info is None' when creating a QAPISchemaEntity. This is gross,
because the flag will only be set on the first QAPISchema() instance
(it works because none of our client scripts ever instantiate more
than one schema). But the only other thing I could think of would
be passing the QAPISchema instance into the constructor for each
QAPISchemaEntity, which is a lot of churn. Any better ideas on how
best to do the assertion, or should I just drop it?
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: improve commit message, track implicit enum info, rebase
on new lazy array handling
---
scripts/qapi.py | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 255001a..19cca97 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -792,6 +792,7 @@ class QAPISchemaEntity(object):
def __init__(self, name, info):
assert isinstance(name, str)
self.name = name
+ assert info or not QAPISchema.predefined_initialized
self.info = info
def c_name(self):
@@ -1114,6 +1115,8 @@ class QAPISchemaEvent(QAPISchemaEntity):
class QAPISchema(object):
+ predefined_initialized = False
+
def __init__(self, fname):
try:
self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
@@ -1122,6 +1125,7 @@ class QAPISchema(object):
exit(1)
self._entity_dict = {}
self._def_predefineds()
+ QAPISchema.predefined_initialized = True
self._def_exprs()
self.check()
@@ -1163,9 +1167,9 @@ class QAPISchema(object):
[], None)
self._def_entity(self.the_empty_object_type)
- def _make_implicit_enum_type(self, name, values):
+ def _make_implicit_enum_type(self, name, info, values):
name = name + 'Kind'
- self._def_entity(QAPISchemaEnumType(name, None, values, None))
+ self._def_entity(QAPISchemaEnumType(name, info, values, None))
return name
def _make_array_type(self, element_type, info):
@@ -1174,12 +1178,12 @@ class QAPISchema(object):
self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
- def _make_implicit_object_type(self, name, role, members):
+ def _make_implicit_object_type(self, name, info, role, members):
if not members:
return None
name = ':obj-%s-%s' % (name, role)
if not self.lookup_entity(name, QAPISchemaObjectType):
- self._def_entity(QAPISchemaObjectType(name, None, None,
+ self._def_entity(QAPISchemaObjectType(name, info, None,
members, None))
return name
@@ -1218,13 +1222,13 @@ class QAPISchema(object):
if isinstance(typ, list):
assert len(typ) == 1
typ = self._make_array_type(typ[0], info)
- typ = self._make_implicit_object_type(typ, 'wrapper',
+ typ = self._make_implicit_object_type(typ, info, 'wrapper',
[self._make_member('data', typ,
info)])
return QAPISchemaObjectTypeVariant(case, typ)
- def _make_tag_enum(self, type_name, variants):
- typ = self._make_implicit_enum_type(type_name,
+ def _make_tag_enum(self, type_name, info, variants):
+ typ = self._make_implicit_enum_type(type_name, info,
[v.name for v in variants])
return QAPISchemaObjectTypeMember('type', typ, False)
@@ -1240,7 +1244,7 @@ class QAPISchema(object):
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
- tag_enum = self._make_tag_enum(name, variants)
+ tag_enum = self._make_tag_enum(name, info, variants)
self._def_entity(
QAPISchemaObjectType(name, info, base,
self._make_members(OrderedDict(), info),
@@ -1253,7 +1257,7 @@ class QAPISchema(object):
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
- tag_enum = self._make_tag_enum(name, variants)
+ tag_enum = self._make_tag_enum(name, info, variants)
self._def_entity(
QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,
@@ -1267,7 +1271,7 @@ class QAPISchema(object):
gen = expr.get('gen', True)
success_response = expr.get('success-response', True)
if isinstance(data, OrderedDict):
- data = self._make_implicit_object_type(name, 'arg',
+ data = self._make_implicit_object_type(name, info, 'arg',
self._make_members(data,
info))
if isinstance(rets, list):
@@ -1280,7 +1284,7 @@ class QAPISchema(object):
name = expr['event']
data = expr.get('data')
if isinstance(data, OrderedDict):
- data = self._make_implicit_object_type(name, 'arg',
+ data = self._make_implicit_object_type(name, info, 'arg',
self._make_members(data,
info))
self._def_entity(QAPISchemaEvent(name, info, data))
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type Eric Blake
@ 2015-10-02 8:54 ` Markus Armbruster
2015-10-02 14:07 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 8:54 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> A future patch will enable deferred error detection in the
> various QAPISchema*.check() methods (rather than the current
> ad hoc parse checks).
What's "deferred" about them?
Perhaps simply: A future patch will move error checking into the various
QAPISchema*.check() methods.
> But that means the user can request
> a QAPI entity that will only fail validation after it has
> been initialized.
I'm not sure I get this sentence.
> Since all errors have to have an
> associated 'info' location, we need a location to be
> associated with all user-triggered implicit types. The
> intuitive info to use is the location of the enclosing
> entity that caused the creation of the implicit type.
Yes.
> Note that we do not anticipate builtin types being used in
> an error message (as they are not part of the user's QAPI
> input, the user can't cause a semantic error in their
> behavior), so we exempt those types from requiring info,
Yes, no errors should be reported for built-in entities.
Sometimes, an error message for one entity wants to refer to some other
entity. We'll have to take care not to blindly use info then.
> by
> setting a flag to track the completion of _def_predefineds().
Explaining the implementation here seems premature.
> No change to the generated code.
>
> RFC: I used a class-level static flag to track whether we expected
> 'info is None' when creating a QAPISchemaEntity. This is gross,
> because the flag will only be set on the first QAPISchema() instance
> (it works because none of our client scripts ever instantiate more
> than one schema). But the only other thing I could think of would
> be passing the QAPISchema instance into the constructor for each
> QAPISchemaEntity, which is a lot of churn. Any better ideas on how
> best to do the assertion, or should I just drop it?
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
I'd check in QAPISchema._def_entity().
Patch looks good otherwise.
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type
2015-10-02 8:54 ` Markus Armbruster
@ 2015-10-02 14:07 ` Eric Blake
2015-10-02 16:07 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 14:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]
On 10/02/2015 02:54 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> A future patch will enable deferred error detection in the
>> various QAPISchema*.check() methods (rather than the current
>> ad hoc parse checks).
>
> What's "deferred" about them?
With ad hoc parse checks, we validate the .json before calling
QAPISchemaEntity constructors. With QAPISchemaEntity.check(), the
constructor is called on various strings, but the strings may not
resolve; we don't know about the problem until check() is called.
>
> Perhaps simply: A future patch will move error checking into the various
> QAPISchema*.check() methods.
>
>> But that means the user can request
>> a QAPI entity that will only fail validation after it has
>> been initialized.
>
> I'm not sure I get this sentence.
Trying to point out that while pre-patch, the check() method was only
run on well-formed entities, now post-patch it can raise errors that we
chose not to detect prior to __init__ time.
>> RFC: I used a class-level static flag to track whether we expected
>> 'info is None' when creating a QAPISchemaEntity. This is gross,
>> because the flag will only be set on the first QAPISchema() instance
>> (it works because none of our client scripts ever instantiate more
>> than one schema). But the only other thing I could think of would
>> be passing the QAPISchema instance into the constructor for each
>> QAPISchemaEntity, which is a lot of churn. Any better ideas on how
>> best to do the assertion, or should I just drop it?
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> I'd check in QAPISchema._def_entity().
Ah, instead of an assert in QAPISchemaEntity.__init__() (which requires
a leaky abstraction), instead write the assert into QAPISchema (so the
flag can now be instance-local). Makes sense; I'll play with the idea.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type
2015-10-02 14:07 ` Eric Blake
@ 2015-10-02 16:07 ` Markus Armbruster
2015-10-02 16:13 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 16:07 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 02:54 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> A future patch will enable deferred error detection in the
>>> various QAPISchema*.check() methods (rather than the current
>>> ad hoc parse checks).
>>
>> What's "deferred" about them?
>
> With ad hoc parse checks, we validate the .json before calling
> QAPISchemaEntity constructors. With QAPISchemaEntity.check(), the
> constructor is called on various strings, but the strings may not
> resolve; we don't know about the problem until check() is called.
I guess I'd say something like
A future patch will move some error checking from the parser to the
various QAPISchema*.check() methods. These run only after parsing
completes.
>>
>> Perhaps simply: A future patch will move error checking into the various
>> QAPISchema*.check() methods.
>>
>>> But that means the user can request
>>> a QAPI entity that will only fail validation after it has
>>> been initialized.
>>
>> I'm not sure I get this sentence.
>
> Trying to point out that while pre-patch, the check() method was only
> run on well-formed entities, now post-patch it can raise errors that we
> chose not to detect prior to __init__ time.
>
>>> RFC: I used a class-level static flag to track whether we expected
>>> 'info is None' when creating a QAPISchemaEntity. This is gross,
>>> because the flag will only be set on the first QAPISchema() instance
>>> (it works because none of our client scripts ever instantiate more
>>> than one schema). But the only other thing I could think of would
>>> be passing the QAPISchema instance into the constructor for each
>>> QAPISchemaEntity, which is a lot of churn. Any better ideas on how
>>> best to do the assertion, or should I just drop it?
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> I'd check in QAPISchema._def_entity().
>
> Ah, instead of an assert in QAPISchemaEntity.__init__() (which requires
> a leaky abstraction), instead write the assert into QAPISchema (so the
> flag can now be instance-local). Makes sense; I'll play with the idea.
:)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type
2015-10-02 16:07 ` Markus Armbruster
@ 2015-10-02 16:13 ` Eric Blake
0 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 16:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]
On 10/02/2015 10:07 AM, Markus Armbruster wrote:
>>>> RFC: I used a class-level static flag to track whether we expected
>>>> 'info is None' when creating a QAPISchemaEntity. This is gross,
>>>> because the flag will only be set on the first QAPISchema() instance
>>>> (it works because none of our client scripts ever instantiate more
>>>> than one schema). But the only other thing I could think of would
>>>> be passing the QAPISchema instance into the constructor for each
>>>> QAPISchemaEntity, which is a lot of churn. Any better ideas on how
>>>> best to do the assertion, or should I just drop it?
>>>
>>> I'd check in QAPISchema._def_entity().
>>
>> Ah, instead of an assert in QAPISchemaEntity.__init__() (which requires
>> a leaky abstraction), instead write the assert into QAPISchema (so the
>> flag can now be instance-local). Makes sense; I'll play with the idea.
>
> :)
Oh, and this means accessing QAPISchemaEntity.info outside of the class,
which absolutely kills off my idea of renaming it to _info for hiding
purposes (so patch 12/12 is now officially gone).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (4 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 05/12] qapi: Track location that created an implicit type Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 9:50 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names Eric Blake
` (5 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Future commits will migrate semantic checking away from parsing
and over to the various QAPISchema*.check() methods. But to
report an error message about an incorrect semantic use of a
member of an object type, we need to know which type, command,
or event owns the member. Rather than making all the check()
methods have to pass around additional information, it is easier
to have each member track who owns it in the first place.
The source information is intended for human consumption in
error messages, and a new describe() method is added to access
the resulting information. For example, given the qapi:
{ 'command': 'foo', 'data': { 'string': 'str' } }
an implementation of visit_command() that calls
arg_type.members[0].describe()
will see "'string' (member of foo arguments)".
Where implicit types are involved, the code intentionally tries
to pick the name of the owner of that implicit type, rather than
the type name itself (a user reading the error message should be
able to grep for the problem in their original file, but will not
be able to locate a generated implicit name).
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase on new lazy array creation and simple union 'type'
motion; tweak commit message
---
scripts/qapi.py | 53 +++++++++++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 19cca97..880de94 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -993,14 +993,16 @@ class QAPISchemaObjectType(QAPISchemaType):
class QAPISchemaObjectTypeMember(object):
- def __init__(self, name, typ, optional):
+ def __init__(self, name, typ, optional, owner):
assert isinstance(name, str)
assert isinstance(typ, str)
assert isinstance(optional, bool)
+ assert isinstance(owner, str) and ':' not in owner
self.name = name
self._type_name = typ
self.type = None
self.optional = optional
+ self._owner = owner
def check(self, schema, all_members, seen):
assert self.name not in seen
@@ -1009,6 +1011,9 @@ class QAPISchemaObjectTypeMember(object):
all_members.append(self)
seen[self.name] = self
+ def describe(self):
+ return "'%s' (member of %s)" % (self.name, self._owner)
+
class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_member, variants):
@@ -1035,13 +1040,16 @@ class QAPISchemaObjectTypeVariants(object):
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
- def __init__(self, name, typ):
- QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
+ def __init__(self, name, typ, owner):
+ QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
def check(self, schema, tag_type, seen):
QAPISchemaObjectTypeMember.check(self, schema, [], seen)
assert self.name in tag_type.values
+ def describe(self):
+ return "'%s' (branch of %s)" % (self.name, self._owner)
+
# This function exists to support ugly simple union special cases
# TODO get rid of them, and drop the function
def simple_union_type(self):
@@ -1193,7 +1201,7 @@ class QAPISchema(object):
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
- def _make_member(self, name, typ, info):
+ def _make_member(self, name, typ, info, owner):
optional = False
if name.startswith('*'):
name = name[1:]
@@ -1201,10 +1209,10 @@ class QAPISchema(object):
if isinstance(typ, list):
assert len(typ) == 1
typ = self._make_array_type(typ[0], info)
- return QAPISchemaObjectTypeMember(name, typ, optional)
+ return QAPISchemaObjectTypeMember(name, typ, optional, owner)
- def _make_members(self, data, info):
- return [self._make_member(key, value, info)
+ def _make_members(self, data, info, owner):
+ return [self._make_member(key, value, info, owner)
for (key, value) in data.iteritems()]
def _def_struct_type(self, expr, info):
@@ -1212,25 +1220,26 @@ class QAPISchema(object):
base = expr.get('base')
data = expr['data']
self._def_entity(QAPISchemaObjectType(name, info, base,
- self._make_members(data, info),
+ self._make_members(data, info,
+ name),
None))
- def _make_variant(self, case, typ):
- return QAPISchemaObjectTypeVariant(case, typ)
+ def _make_variant(self, case, typ, owner):
+ return QAPISchemaObjectTypeVariant(case, typ, owner)
- def _make_simple_variant(self, case, typ, info):
+ def _make_simple_variant(self, case, typ, info, owner):
if isinstance(typ, list):
assert len(typ) == 1
typ = self._make_array_type(typ[0], info)
typ = self._make_implicit_object_type(typ, info, 'wrapper',
[self._make_member('data', typ,
- info)])
- return QAPISchemaObjectTypeVariant(case, typ)
+ info, owner)])
+ return QAPISchemaObjectTypeVariant(case, typ, owner)
def _make_tag_enum(self, type_name, info, variants):
typ = self._make_implicit_enum_type(type_name, info,
[v.name for v in variants])
- return QAPISchemaObjectTypeMember('type', typ, False)
+ return QAPISchemaObjectTypeMember('type', typ, False, type_name)
def _def_union_type(self, expr, info):
name = expr['union']
@@ -1239,15 +1248,15 @@ class QAPISchema(object):
tag_name = expr.get('discriminator')
tag_enum = None
if tag_name:
- variants = [self._make_variant(key, value)
+ variants = [self._make_variant(key, value, name)
for (key, value) in data.iteritems()]
else:
- variants = [self._make_simple_variant(key, value, info)
+ variants = [self._make_simple_variant(key, value, info, name)
for (key, value) in data.iteritems()]
tag_enum = self._make_tag_enum(name, info, variants)
self._def_entity(
QAPISchemaObjectType(name, info, base,
- self._make_members(OrderedDict(), info),
+ self._make_members(OrderedDict(), info, name),
QAPISchemaObjectTypeVariants(tag_name,
tag_enum,
variants)))
@@ -1255,7 +1264,7 @@ class QAPISchema(object):
def _def_alternate_type(self, expr, info):
name = expr['alternate']
data = expr['data']
- variants = [self._make_variant(key, value)
+ variants = [self._make_variant(key, value, name)
for (key, value) in data.iteritems()]
tag_enum = self._make_tag_enum(name, info, variants)
self._def_entity(
@@ -1271,9 +1280,11 @@ class QAPISchema(object):
gen = expr.get('gen', True)
success_response = expr.get('success-response', True)
if isinstance(data, OrderedDict):
+ owner = name + ' arguments'
data = self._make_implicit_object_type(name, info, 'arg',
self._make_members(data,
- info))
+ info,
+ owner))
if isinstance(rets, list):
assert len(rets) == 1
rets = self._make_array_type(rets[0], info)
@@ -1284,9 +1295,11 @@ class QAPISchema(object):
name = expr['event']
data = expr.get('data')
if isinstance(data, OrderedDict):
+ owner = name + ' data'
data = self._make_implicit_object_type(name, info, 'arg',
self._make_members(data,
- info))
+ info,
+ owner))
self._def_entity(QAPISchemaEvent(name, info, data))
def _def_exprs(self):
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member Eric Blake
@ 2015-10-02 9:50 ` Markus Armbruster
2015-10-02 14:48 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 9:50 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Future commits will migrate semantic checking away from parsing
> and over to the various QAPISchema*.check() methods. But to
> report an error message about an incorrect semantic use of a
> member of an object type, we need to know which type, command,
> or event owns the member. Rather than making all the check()
> methods have to pass around additional information, it is easier
> to have each member track who owns it in the first place.
>
> The source information is intended for human consumption in
> error messages, and a new describe() method is added to access
> the resulting information. For example, given the qapi:
> { 'command': 'foo', 'data': { 'string': 'str' } }
> an implementation of visit_command() that calls
> arg_type.members[0].describe()
> will see "'string' (member of foo arguments)".
Peeking ahead a bit, I see two describe(), one for ordinary members
returning
"'%s' (member of %s)" % (self.name, self._owner)
and one for variant members returning
"'%s' (branch of %s)" % (self.name, self._owner)
The name _owner makes me expect it's the owning types name, but that's
not always the case, as we shall see. How is it related to info and to
the owning type's name then?
In your example (implicit arguments type):
arg_type.members[0]._owner is 'foo arguments'.
arg_type.members[0] has no info.
arg_type.name is ':obj-foo-arg'
arg_type.info is something like
info {'line': 10, 'parent': None, 'file': 'example-schema.json'}
pointing to the definition of command 'foo'. It's actually the
command's info, inherited by its implicit argument type.
Here, _owner is merely a variation of the owning type's name geared for
human readers.
Example of explicit arguments type:
{ 'struct': 'BarArgs', 'data': { 'string': 'str' } }
{ 'command': 'bar', 'data': 'BarArgs' }
Here, we get:
arg_type.members[0]._owner is 'BarArgs'.
arg_type.members[0] has no info.
arg_type.name is 'BarArgs'
arg_type.info is something like
info {'line': 12, 'parent': None, 'file': 'example-schema.json'}
pointing to the definition of command 'bar. Again, it's the
command's info, inherited by its implicit argument type.
Here, _owner *is* the owning type's name.
So, _owner is a more readable name we make up when the other name for
the same thing isn't readable. However, we make up that other name,
too! Begs the question why we don't simply make it readable right away.
Naturally, we still need to make up names collision-free. But as far as
I can tell, nothing stops us from picking ':obj-foo arguments' instead
of ':obj-foo-arg', and when we talk to users strip off the common prefix
':obj-' we prepend to avoid collisions.
> Where implicit types are involved, the code intentionally tries
> to pick the name of the owner of that implicit type, rather than
> the type name itself (a user reading the error message should be
> able to grep for the problem in their original file, but will not
> be able to locate a generated implicit name).
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Let's discuss the above before I review the actual patch closely.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member
2015-10-02 9:50 ` Markus Armbruster
@ 2015-10-02 14:48 ` Eric Blake
2015-10-02 17:05 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 14:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4934 bytes --]
On 10/02/2015 03:50 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Future commits will migrate semantic checking away from parsing
>> and over to the various QAPISchema*.check() methods. But to
>> report an error message about an incorrect semantic use of a
>> member of an object type, we need to know which type, command,
>> or event owns the member. Rather than making all the check()
>> methods have to pass around additional information, it is easier
>> to have each member track who owns it in the first place.
>>
>> The source information is intended for human consumption in
>> error messages, and a new describe() method is added to access
>> the resulting information. For example, given the qapi:
>> { 'command': 'foo', 'data': { 'string': 'str' } }
>> an implementation of visit_command() that calls
>> arg_type.members[0].describe()
>> will see "'string' (member of foo arguments)".
>
> Peeking ahead a bit, I see two describe(), one for ordinary members
> returning
>
> "'%s' (member of %s)" % (self.name, self._owner)
>
> and one for variant members returning
>
> "'%s' (branch of %s)" % (self.name, self._owner)
>
> The name _owner makes me expect it's the owning types name, but that's
> not always the case, as we shall see. How is it related to info and to
> the owning type's name then?
>
> In your example (implicit arguments type):
>
> arg_type.members[0]._owner is 'foo arguments'.
>
> arg_type.members[0] has no info.
Well, none of the members have info - they are not subclassed from
QAPISchemaEntity.
>
> arg_type.name is ':obj-foo-arg'
>
> arg_type.info is something like
>
> info {'line': 10, 'parent': None, 'file': 'example-schema.json'}
>
> pointing to the definition of command 'foo'. It's actually the
> command's info, inherited by its implicit argument type.
>
> Here, _owner is merely a variation of the owning type's name geared for
> human readers.
Oh, I think I see where you are going with this - why is 'owner' a
string, rather than the actual QAPISchemaType python object? And is it
worth following the pattern used in other classes, where __init__ gets a
string naming the type, and then check() resolves that name to the
actual type? At which point, we could do member.owner.info to access
the info of the type that owns the member?
But there's a chicken-and-egg situation - we don't know what the type
will be named until we call _make_implicit_object_type(), but that
function requires that we have already pre-constructed the
QAPISchemaObjectTypeMembers array (which means we can't pre-construct
the members with the type name embedded).
We'd have to refactor things to generate the type name, then construct
the members, then construct the type (doable, but probably involves
splitting this patch for ease of review).
>
> Example of explicit arguments type:
>
> { 'struct': 'BarArgs', 'data': { 'string': 'str' } }
> { 'command': 'bar', 'data': 'BarArgs' }
>
> Here, we get:
>
> arg_type.members[0]._owner is 'BarArgs'.
>
> arg_type.members[0] has no info.
Again, because NO members have info.
>
> arg_type.name is 'BarArgs'
>
> arg_type.info is something like
>
> info {'line': 12, 'parent': None, 'file': 'example-schema.json'}
>
> pointing to the definition of command 'bar. Again, it's the
> command's info, inherited by its implicit argument type.
>
> Here, _owner *is* the owning type's name.
>
> So, _owner is a more readable name we make up when the other name for
> the same thing isn't readable. However, we make up that other name,
> too! Begs the question why we don't simply make it readable right away.
>
> Naturally, we still need to make up names collision-free. But as far as
> I can tell, nothing stops us from picking ':obj-foo arguments' instead
> of ':obj-foo-arg', and when we talk to users strip off the common prefix
> ':obj-' we prepend to avoid collisions.
Might be doable, but then we'd have to generate the implicit object name
prior to creating its Member objects (thus splitting
_make_implicit_object_type() into two parts).
>
>> Where implicit types are involved, the code intentionally tries
>> to pick the name of the owner of that implicit type, rather than
>> the type name itself (a user reading the error message should be
>> able to grep for the problem in their original file, but will not
>> be able to locate a generated implicit name).
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> Let's discuss the above before I review the actual patch closely.
What do you think - would that refactoring be worth it?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member
2015-10-02 14:48 ` Eric Blake
@ 2015-10-02 17:05 ` Markus Armbruster
2015-10-02 22:35 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 17:05 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 03:50 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Future commits will migrate semantic checking away from parsing
>>> and over to the various QAPISchema*.check() methods. But to
>>> report an error message about an incorrect semantic use of a
>>> member of an object type, we need to know which type, command,
>>> or event owns the member. Rather than making all the check()
>>> methods have to pass around additional information, it is easier
>>> to have each member track who owns it in the first place.
>>>
>>> The source information is intended for human consumption in
>>> error messages, and a new describe() method is added to access
>>> the resulting information. For example, given the qapi:
>>> { 'command': 'foo', 'data': { 'string': 'str' } }
>>> an implementation of visit_command() that calls
>>> arg_type.members[0].describe()
>>> will see "'string' (member of foo arguments)".
>>
>> Peeking ahead a bit, I see two describe(), one for ordinary members
>> returning
>>
>> "'%s' (member of %s)" % (self.name, self._owner)
>>
>> and one for variant members returning
>>
>> "'%s' (branch of %s)" % (self.name, self._owner)
>>
>> The name _owner makes me expect it's the owning types name, but that's
>> not always the case, as we shall see. How is it related to info and to
>> the owning type's name then?
>>
>> In your example (implicit arguments type):
>>
>> arg_type.members[0]._owner is 'foo arguments'.
>>
>> arg_type.members[0] has no info.
>
> Well, none of the members have info - they are not subclassed from
> QAPISchemaEntity.
>
>>
>> arg_type.name is ':obj-foo-arg'
>>
>> arg_type.info is something like
>>
>> info {'line': 10, 'parent': None, 'file': 'example-schema.json'}
>>
>> pointing to the definition of command 'foo'. It's actually the
>> command's info, inherited by its implicit argument type.
>>
>> Here, _owner is merely a variation of the owning type's name geared for
>> human readers.
>
> Oh, I think I see where you are going with this - why is 'owner' a
> string, rather than the actual QAPISchemaType python object? And is it
> worth following the pattern used in other classes, where __init__ gets a
> string naming the type, and then check() resolves that name to the
> actual type? At which point, we could do member.owner.info to access
> the info of the type that owns the member?
We generally refer to types by their name until check(). In check(), we
then do things like
self.type = schema.lookup_type(self._type_name)
Done that way because in the general case, you can't resolve type names
to types before check(), and doing it that way even in cases where you
could keeps things nicely regular.
We don't currently create back-references from member to type. Instead,
we rely on context. Tends to be simpler as long as the context is
readily available. If we find the lack of back-references complicates
things, we can of course add them. Slightly spooky, because Python
lacks a real garbage collector, and cyclic references can make it leak.
> But there's a chicken-and-egg situation - we don't know what the type
> will be named until we call _make_implicit_object_type(), but that
> function requires that we have already pre-constructed the
> QAPISchemaObjectTypeMembers array (which means we can't pre-construct
> the members with the type name embedded).
>
> We'd have to refactor things to generate the type name, then construct
> the members, then construct the type (doable, but probably involves
> splitting this patch for ease of review).
Not the only way, see below.
>> Example of explicit arguments type:
>>
>> { 'struct': 'BarArgs', 'data': { 'string': 'str' } }
>> { 'command': 'bar', 'data': 'BarArgs' }
>>
>> Here, we get:
>>
>> arg_type.members[0]._owner is 'BarArgs'.
>>
>> arg_type.members[0] has no info.
>
> Again, because NO members have info.
>
>>
>> arg_type.name is 'BarArgs'
>>
>> arg_type.info is something like
>>
>> info {'line': 12, 'parent': None, 'file': 'example-schema.json'}
>>
>> pointing to the definition of command 'bar. Again, it's the
>> command's info, inherited by its implicit argument type.
>>
>> Here, _owner *is* the owning type's name.
>>
>> So, _owner is a more readable name we make up when the other name for
>> the same thing isn't readable. However, we make up that other name,
>> too! Begs the question why we don't simply make it readable right away.
>>
>> Naturally, we still need to make up names collision-free. But as far as
>> I can tell, nothing stops us from picking ':obj-foo arguments' instead
>> of ':obj-foo-arg', and when we talk to users strip off the common prefix
>> ':obj-' we prepend to avoid collisions.
>
> Might be doable, but then we'd have to generate the implicit object name
> prior to creating its Member objects (thus splitting
> _make_implicit_object_type() into two parts).
def _make_implicit_object_type(self, name, role, members):
if not members:
return None
(1) name = ':obj-%s-%s' % (name, role)
if not self.lookup_entity(name, QAPISchemaObjectType):
(2) self._def_entity(QAPISchemaObjectType(name, None, None,
members, None))
return name
We create the implicit object type name at (1), and we associate the
type and its members in (2), by creating references from type to
members. Isn't that the natural place for creating back-references,
too? Assuming we need them.
>>> Where implicit types are involved, the code intentionally tries
>>> to pick the name of the owner of that implicit type, rather than
>>> the type name itself (a user reading the error message should be
>>> able to grep for the problem in their original file, but will not
>>> be able to locate a generated implicit name).
>>>
>>> No change to generated code.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> Let's discuss the above before I review the actual patch closely.
>
> What do you think - would that refactoring be worth it?
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member
2015-10-02 17:05 ` Markus Armbruster
@ 2015-10-02 22:35 ` Eric Blake
0 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 22:35 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]
On 10/02/2015 11:05 AM, Markus Armbruster wrote:
>> Might be doable, but then we'd have to generate the implicit object name
>> prior to creating its Member objects (thus splitting
>> _make_implicit_object_type() into two parts).
>
> def _make_implicit_object_type(self, name, role, members):
> if not members:
> return None
> (1) name = ':obj-%s-%s' % (name, role)
> if not self.lookup_entity(name, QAPISchemaObjectType):
> (2) self._def_entity(QAPISchemaObjectType(name, None, None,
> members, None))
> return name
>
> We create the implicit object type name at (1), and we associate the
> type and its members in (2), by creating references from type to
> members. Isn't that the natural place for creating back-references,
> too? Assuming we need them.
Okay, I don't think we need a backref; a string name is good enough. And
I like your idea of making the implicit object name something where the
suffix is usable as-is. So I'm currently playing with:
class QAPISchemaObjectTypeMember...
def __init__...
self.owner = None
def check...
assert self.owner
class QAPISchemaObjectType...
def __init__(..., members):
for m in members:
assert isinstance(m, QAPISchemaObjectTypeMember)
assert not m.owner
m.owner = self.name
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (5 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 13:19 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check() Eric Blake
` (4 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Detect attempts to declare two object members that would result
in the same C member name, by keying the 'seen' dictionary off
of the C name rather than the qapi name. Doing this was made
easier by adding a member.c_name() helper function.
As this is the first error raised within the QAPISchema*.check()
methods, we also have to pass 'info' through the call stack, and
fix the overall 'try' to display errors detected during the
check() phase.
This fixes a couple of previously-broken tests, and the
resulting error messages demonstrate the utility of the
.describe() method added previously. No change to generated
code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to earlier testsuite and info improvements
---
scripts/qapi.py | 46 ++++++++++++++++----------
tests/qapi-schema/args-name-clash.err | 1 +
tests/qapi-schema/args-name-clash.exit | 2 +-
tests/qapi-schema/args-name-clash.json | 6 ++--
tests/qapi-schema/args-name-clash.out | 6 ----
tests/qapi-schema/flat-union-clash-branch.err | 1 +
tests/qapi-schema/flat-union-clash-branch.exit | 2 +-
tests/qapi-schema/flat-union-clash-branch.json | 9 ++---
tests/qapi-schema/flat-union-clash-branch.out | 14 --------
9 files changed, 40 insertions(+), 47 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 880de94..1acf02b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -103,6 +103,7 @@ class QAPISchemaError(Exception):
class QAPIExprError(Exception):
def __init__(self, expr_info, msg):
Exception.__init__(self)
+ assert expr_info
self.info = expr_info
self.msg = msg
@@ -964,11 +965,12 @@ class QAPISchemaObjectType(QAPISchemaType):
members = []
seen = {}
for m in members:
- seen[m.name] = m
+ assert m.c_name() not in seen
+ seen[m.c_name()] = m
for m in self.local_members:
- m.check(schema, members, seen)
+ m.check(schema, self.info, members, seen)
if self.variants:
- self.variants.check(schema, members, seen)
+ self.variants.check(schema, self.info, members, seen)
self.members = members
def is_implicit(self):
@@ -1004,12 +1006,19 @@ class QAPISchemaObjectTypeMember(object):
self.optional = optional
self._owner = owner
- def check(self, schema, all_members, seen):
- assert self.name not in seen
+ def check(self, schema, info, all_members, seen):
+ if self.c_name() in seen:
+ raise QAPIExprError(info,
+ "%s collides with %s"
+ % (self.describe(),
+ seen[self.c_name()].describe()))
self.type = schema.lookup_type(self._type_name)
assert self.type
all_members.append(self)
- seen[self.name] = self
+ seen[self.c_name()] = self
+
+ def c_name(self):
+ return c_name(self.name)
def describe(self):
return "'%s' (member of %s)" % (self.name, self._owner)
@@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
- def check(self, schema, members, seen):
+ def check(self, schema, info, members, seen):
if self.tag_name:
- self.tag_member = seen[self.tag_name]
+ self.tag_member = seen[c_name(self.tag_name)]
+ assert self.tag_name == self.tag_member.name
else:
- self.tag_member.check(schema, members, seen)
+ self.tag_member.check(schema, info, members, seen)
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
vseen = dict(seen)
- v.check(schema, self.tag_member.type, vseen)
+ v.check(schema, info, self.tag_member.type, vseen)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ, owner):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
- def check(self, schema, tag_type, seen):
- QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+ def check(self, schema, info, tag_type, seen):
+ QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
assert self.name in tag_type.values
def describe(self):
@@ -1069,7 +1079,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
- self.variants.check(schema, [], {})
+ self.variants.check(schema, self.info, [], {})
def json_type(self):
return 'value'
@@ -1128,14 +1138,14 @@ class QAPISchema(object):
def __init__(self, fname):
try:
self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
+ self._entity_dict = {}
+ self._def_predefineds()
+ QAPISchema.predefined_initialized = True
+ self._def_exprs()
+ self.check()
except (QAPISchemaError, QAPIExprError), err:
print >>sys.stderr, err
exit(1)
- self._entity_dict = {}
- self._def_predefineds()
- QAPISchema.predefined_initialized = True
- self._def_exprs()
- self.check()
def _def_entity(self, ent):
assert ent.name not in self._entity_dict
diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
index e69de29..66f609c 100644
--- a/tests/qapi-schema/args-name-clash.err
+++ b/tests/qapi-schema/args-name-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-name-clash.json:5: 'a_b' (member of oops arguments) collides with 'a-b' (member of oops arguments)
diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-name-clash.exit
+++ b/tests/qapi-schema/args-name-clash.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
index 9e8f889..3fe4ea5 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/args-name-clash.json
@@ -1,5 +1,5 @@
# C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'a_b' members. Either reject this at parse time, or munge the C names
-# to avoid the collision.
+# Reject members that clash when mapped to C names (we would have two 'a_b'
+# members). It would also be possible to munge the C names to avoid the
+# collision, but unlikely to be worth the effort.
{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
index 9b2f6e4..e69de29 100644
--- a/tests/qapi-schema/args-name-clash.out
+++ b/tests/qapi-schema/args-name-clash.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops-arg
- member a-b: str optional=False
- member a_b: str optional=False
-command oops :obj-oops-arg -> None
- gen=True success_response=True
diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
index e69de29..0190d79 100644
--- a/tests/qapi-schema/flat-union-clash-branch.err
+++ b/tests/qapi-schema/flat-union-clash-branch.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ b/tests/qapi-schema/flat-union-clash-branch.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
index e593336..a6c302f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -1,8 +1,9 @@
# Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
-# (one from the base member, the other from the branch name). We should
-# either reject the collision at parse time, or munge the generated branch
-# name to allow this to compile.
+# Reject attempts to use a branch name that would clash with a non-variant
+# member, when mapped to C names (here, we would have two 'c_d' members,
+# one from the base member, the other from the branch name).
+# TODO: We could munge the generated branch name to avoid the collision and
+# allow this to compile.
{ 'enum': 'TestEnum',
'data': [ 'base', 'c-d' ] }
{ 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
index 8e0da73..e69de29 100644
--- a/tests/qapi-schema/flat-union-clash-branch.out
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -1,14 +0,0 @@
-object :empty
-object Base
- member enum1: TestEnum optional=False
- member c_d: str optional=True
-object Branch1
- member string: str optional=False
-object Branch2
- member value: int optional=False
-enum TestEnum ['base', 'c-d']
-object TestUnion
- base Base
- tag enum1
- case base: Branch1
- case c-d: Branch2
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names Eric Blake
@ 2015-10-02 13:19 ` Markus Armbruster
2015-10-02 15:12 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 13:19 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Detect attempts to declare two object members that would result
> in the same C member name, by keying the 'seen' dictionary off
> of the C name rather than the qapi name. Doing this was made
> easier by adding a member.c_name() helper function.
This gains protection against colliding C names. It keeps protection
against colliding QMP names as long as QMP name collision implies C name
collision. I think that's the case, but it's definitely worth spelling
out in the code, and possibly in the commit message.
> As this is the first error raised within the QAPISchema*.check()
> methods, we also have to pass 'info' through the call stack, and
> fix the overall 'try' to display errors detected during the
> check() phase.
Could also be a separate patch, if the parts are easier to review.
> This fixes a couple of previously-broken tests, and the
Just two.
> resulting error messages demonstrate the utility of the
> .describe() method added previously. No change to generated
> code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: rebase to earlier testsuite and info improvements
> ---
> scripts/qapi.py | 46 ++++++++++++++++----------
> tests/qapi-schema/args-name-clash.err | 1 +
> tests/qapi-schema/args-name-clash.exit | 2 +-
> tests/qapi-schema/args-name-clash.json | 6 ++--
> tests/qapi-schema/args-name-clash.out | 6 ----
> tests/qapi-schema/flat-union-clash-branch.err | 1 +
> tests/qapi-schema/flat-union-clash-branch.exit | 2 +-
> tests/qapi-schema/flat-union-clash-branch.json | 9 ++---
> tests/qapi-schema/flat-union-clash-branch.out | 14 --------
> 9 files changed, 40 insertions(+), 47 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 880de94..1acf02b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -103,6 +103,7 @@ class QAPISchemaError(Exception):
> class QAPIExprError(Exception):
> def __init__(self, expr_info, msg):
> Exception.__init__(self)
> + assert expr_info
> self.info = expr_info
> self.msg = msg
>
Assertion should hold before the patch. Could therefore be added in a
separate patch before this one. Only if we have enough material for
a separate patch, or it makes this one materially easier to review.
> @@ -964,11 +965,12 @@ class QAPISchemaObjectType(QAPISchemaType):
> members = []
> seen = {}
> for m in members:
> - seen[m.name] = m
> + assert m.c_name() not in seen
> + seen[m.c_name()] = m
Assertion should hold before the patch.
> for m in self.local_members:
> - m.check(schema, members, seen)
> + m.check(schema, self.info, members, seen)
> if self.variants:
> - self.variants.check(schema, members, seen)
> + self.variants.check(schema, self.info, members, seen)
> self.members = members
>
> def is_implicit(self):
> @@ -1004,12 +1006,19 @@ class QAPISchemaObjectTypeMember(object):
> self.optional = optional
> self._owner = owner
>
> - def check(self, schema, all_members, seen):
> - assert self.name not in seen
> + def check(self, schema, info, all_members, seen):
> + if self.c_name() in seen:
> + raise QAPIExprError(info,
> + "%s collides with %s"
> + % (self.describe(),
> + seen[self.c_name()].describe()))
> self.type = schema.lookup_type(self._type_name)
> assert self.type
> all_members.append(self)
> - seen[self.name] = self
> + seen[self.c_name()] = self
> +
> + def c_name(self):
> + return c_name(self.name)
Why wrap function c_name() in a method? Why not simply call the
function?
It's method in QAPISchemaEntity only because this lets us add special
cases in a neat way.
>
> def describe(self):
> return "'%s' (member of %s)" % (self.name, self._owner)
> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
> self.tag_member = tag_member
> self.variants = variants
>
> - def check(self, schema, members, seen):
> + def check(self, schema, info, members, seen):
> if self.tag_name:
> - self.tag_member = seen[self.tag_name]
> + self.tag_member = seen[c_name(self.tag_name)]
> + assert self.tag_name == self.tag_member.name
Assertion should hold before the patch, but it's kind of trivial then.
> else:
> - self.tag_member.check(schema, members, seen)
> + self.tag_member.check(schema, info, members, seen)
> assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> for v in self.variants:
> vseen = dict(seen)
> - v.check(schema, self.tag_member.type, vseen)
> + v.check(schema, info, self.tag_member.type, vseen)
>
>
> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> def __init__(self, name, typ, owner):
> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
>
> - def check(self, schema, tag_type, seen):
> - QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> + def check(self, schema, info, tag_type, seen):
> + QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
> assert self.name in tag_type.values
>
> def describe(self):
> @@ -1069,7 +1079,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
> self.variants = variants
>
> def check(self, schema):
> - self.variants.check(schema, [], {})
> + self.variants.check(schema, self.info, [], {})
>
> def json_type(self):
> return 'value'
> @@ -1128,14 +1138,14 @@ class QAPISchema(object):
> def __init__(self, fname):
> try:
> self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
> + self._entity_dict = {}
> + self._def_predefineds()
> + QAPISchema.predefined_initialized = True
> + self._def_exprs()
> + self.check()
> except (QAPISchemaError, QAPIExprError), err:
> print >>sys.stderr, err
> exit(1)
> - self._entity_dict = {}
> - self._def_predefineds()
> - QAPISchema.predefined_initialized = True
> - self._def_exprs()
> - self.check()
Moving code into the try wholesale is okay because we catch only our own
exceptions.
>
> def _def_entity(self, ent):
> assert ent.name not in self._entity_dict
> diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
> index e69de29..66f609c 100644
> --- a/tests/qapi-schema/args-name-clash.err
> +++ b/tests/qapi-schema/args-name-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-name-clash.json:5: 'a_b' (member of oops arguments) collides with 'a-b' (member of oops arguments)
> diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/args-name-clash.exit
> +++ b/tests/qapi-schema/args-name-clash.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
> index 9e8f889..3fe4ea5 100644
> --- a/tests/qapi-schema/args-name-clash.json
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -1,5 +1,5 @@
> # C member name collision
> -# FIXME - This parses, but fails to compile, because the C struct is given
> -# two 'a_b' members. Either reject this at parse time, or munge the C names
> -# to avoid the collision.
> +# Reject members that clash when mapped to C names (we would have two 'a_b'
> +# members). It would also be possible to munge the C names to avoid the
> +# collision, but unlikely to be worth the effort.
> { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
> index 9b2f6e4..e69de29 100644
> --- a/tests/qapi-schema/args-name-clash.out
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -1,6 +0,0 @@
> -object :empty
> -object :obj-oops-arg
> - member a-b: str optional=False
> - member a_b: str optional=False
> -command oops :obj-oops-arg -> None
> - gen=True success_response=True
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
> index e69de29..0190d79 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.err
> +++ b/tests/qapi-schema/flat-union-clash-branch.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.exit
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
> index e593336..a6c302f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.json
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -1,8 +1,9 @@
> # Flat union branch name collision
> -# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
> -# (one from the base member, the other from the branch name). We should
> -# either reject the collision at parse time, or munge the generated branch
> -# name to allow this to compile.
> +# Reject attempts to use a branch name that would clash with a non-variant
> +# member, when mapped to C names (here, we would have two 'c_d' members,
> +# one from the base member, the other from the branch name).
> +# TODO: We could munge the generated branch name to avoid the collision and
> +# allow this to compile.
> { 'enum': 'TestEnum',
> 'data': [ 'base', 'c-d' ] }
> { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
> index 8e0da73..e69de29 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.out
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -1,14 +0,0 @@
> -object :empty
> -object Base
> - member enum1: TestEnum optional=False
> - member c_d: str optional=True
> -object Branch1
> - member string: str optional=False
> -object Branch2
> - member value: int optional=False
> -enum TestEnum ['base', 'c-d']
> -object TestUnion
> - base Base
> - tag enum1
> - case base: Branch1
> - case c-d: Branch2
I'm fine with not splitting this patch. I'd be also fine with splitting
it up. Your choice.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names
2015-10-02 13:19 ` Markus Armbruster
@ 2015-10-02 15:12 ` Eric Blake
2015-10-02 17:11 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 15:12 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4429 bytes --]
On 10/02/2015 07:19 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Detect attempts to declare two object members that would result
>> in the same C member name, by keying the 'seen' dictionary off
>> of the C name rather than the qapi name. Doing this was made
>> easier by adding a member.c_name() helper function.
>
> This gains protection against colliding C names. It keeps protection
> against colliding QMP names as long as QMP name collision implies C name
> collision. I think that's the case, but it's definitely worth spelling
> out in the code, and possibly in the commit message.
>
>> As this is the first error raised within the QAPISchema*.check()
>> methods, we also have to pass 'info' through the call stack, and
>> fix the overall 'try' to display errors detected during the
>> check() phase.
>
> Could also be a separate patch, if the parts are easier to review.
I'm still debating about that.
>> + def check(self, schema, info, all_members, seen):
>> + if self.c_name() in seen:
>> + raise QAPIExprError(info,
>> + "%s collides with %s"
>> + % (self.describe(),
>> + seen[self.c_name()].describe()))
>> self.type = schema.lookup_type(self._type_name)
>> assert self.type
>> all_members.append(self)
>> - seen[self.name] = self
>> + seen[self.c_name()] = self
>> +
>> + def c_name(self):
>> + return c_name(self.name)
>
> Why wrap function c_name() in a method? Why not simply call the
> function?
'self.c_name()' is shorter than 'c_name(self.name)'. And I already had
long lines with that seen[self.c_name()].describe() pattern.
>
> It's method in QAPISchemaEntity only because this lets us add special
> cases in a neat way.
True, but I _did_ mention in the commit message that I did it for less
typing.
But as to special cases, yes, I have one in mind (although I have not
played with it yet). In v5 19/46 Simplify visiting of alternate types,
I want to change alternates to have variants.tag_member == None, and the
generated C code to use 'qtype_code type;' as the tag variable. In the
v5 representation, it led to a lot of special-casing (many uses of
QAPISchemaObjectTypeVariants became more complex because tag_member was
not always defined). But now that I've been working on these front-end
patches, my idea was to do something like:
class QAPISchemaVariantTag(QAPISchemaObjectTypeMember):
def __init__(self, info):
QAPISchemaObjectTypeMember(self, 'type', None, False)
def c_type(self):
return 'qtype_code'
and then, we WOULD need member.c_type() rather than member.type.c_type()
(at which point having member.c_name() to match member.c_type() makes
more sense).
>> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
>> self.tag_member = tag_member
>> self.variants = variants
>>
>> - def check(self, schema, members, seen):
>> + def check(self, schema, info, members, seen):
>> if self.tag_name:
>> - self.tag_member = seen[self.tag_name]
>> + self.tag_member = seen[c_name(self.tag_name)]
>> + assert self.tag_name == self.tag_member.name
>
> Assertion should hold before the patch, but it's kind of trivial then.
My worry here was that if I have:
{ 'enum': 'Enum', 'data': ['a'] }
{ 'base': 'Base', 'data': { 'b-c': 'Enum' } }
{ 'union': 'Flat', 'base': 'Base', 'discriminator': 'b_c',
'data': { 'a': 'Struct...' } }
the old ad hoc parser rejects 'b_c' as not being a member of Enum (bad
discriminator). But once we convert from the old parser to the check()
method (basically, anywhere the check() methods now have an assert will
become if statements that raise an exception), we need to make sure that
we don't get confused by the fact that seen[c_name('b_c')] maps to the
same value as seen[c_name('b-c')], so that we are still flagging the
flat union as invalid.
>
> I'm fine with not splitting this patch. I'd be also fine with splitting
> it up. Your choice.
I'm still thinking about it; may depend on how much other refactoring I do.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names
2015-10-02 15:12 ` Eric Blake
@ 2015-10-02 17:11 ` Markus Armbruster
2015-10-03 1:01 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 17:11 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 07:19 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Detect attempts to declare two object members that would result
>>> in the same C member name, by keying the 'seen' dictionary off
>>> of the C name rather than the qapi name. Doing this was made
>>> easier by adding a member.c_name() helper function.
>>
>> This gains protection against colliding C names. It keeps protection
>> against colliding QMP names as long as QMP name collision implies C name
>> collision. I think that's the case, but it's definitely worth spelling
>> out in the code, and possibly in the commit message.
>>
>>> As this is the first error raised within the QAPISchema*.check()
>>> methods, we also have to pass 'info' through the call stack, and
>>> fix the overall 'try' to display errors detected during the
>>> check() phase.
>>
>> Could also be a separate patch, if the parts are easier to review.
>
> I'm still debating about that.
>
>
>>> + def check(self, schema, info, all_members, seen):
>>> + if self.c_name() in seen:
>>> + raise QAPIExprError(info,
>>> + "%s collides with %s"
>>> + % (self.describe(),
>>> + seen[self.c_name()].describe()))
>>> self.type = schema.lookup_type(self._type_name)
>>> assert self.type
>>> all_members.append(self)
>>> - seen[self.name] = self
>>> + seen[self.c_name()] = self
>>> +
>>> + def c_name(self):
>>> + return c_name(self.name)
>>
>> Why wrap function c_name() in a method? Why not simply call the
>> function?
>
> 'self.c_name()' is shorter than 'c_name(self.name)'. And I already had
> long lines with that seen[self.c_name()].describe() pattern.
You could also try a local variable: cnam = c_name(self.name).
>> It's method in QAPISchemaEntity only because this lets us add special
>> cases in a neat way.
>
> True, but I _did_ mention in the commit message that I did it for less
> typing.
>
> But as to special cases, yes, I have one in mind (although I have not
> played with it yet). In v5 19/46 Simplify visiting of alternate types,
> I want to change alternates to have variants.tag_member == None, and the
> generated C code to use 'qtype_code type;' as the tag variable. In the
> v5 representation, it led to a lot of special-casing (many uses of
> QAPISchemaObjectTypeVariants became more complex because tag_member was
> not always defined). But now that I've been working on these front-end
> patches, my idea was to do something like:
>
> class QAPISchemaVariantTag(QAPISchemaObjectTypeMember):
> def __init__(self, info):
> QAPISchemaObjectTypeMember(self, 'type', None, False)
>
> def c_type(self):
> return 'qtype_code'
>
> and then, we WOULD need member.c_type() rather than member.type.c_type()
> (at which point having member.c_name() to match member.c_type() makes
> more sense).
I'm afraid I don't have enough context to grok this late on Friday :)
>>> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object):
>>> self.tag_member = tag_member
>>> self.variants = variants
>>>
>>> - def check(self, schema, members, seen):
>>> + def check(self, schema, info, members, seen):
>>> if self.tag_name:
>>> - self.tag_member = seen[self.tag_name]
>>> + self.tag_member = seen[c_name(self.tag_name)]
>>> + assert self.tag_name == self.tag_member.name
>>
>> Assertion should hold before the patch, but it's kind of trivial then.
>
> My worry here was that if I have:
>
> { 'enum': 'Enum', 'data': ['a'] }
> { 'base': 'Base', 'data': { 'b-c': 'Enum' } }
> { 'union': 'Flat', 'base': 'Base', 'discriminator': 'b_c',
> 'data': { 'a': 'Struct...' } }
>
> the old ad hoc parser rejects 'b_c' as not being a member of Enum (bad
> discriminator). But once we convert from the old parser to the check()
> method (basically, anywhere the check() methods now have an assert will
> become if statements that raise an exception), we need to make sure that
> we don't get confused by the fact that seen[c_name('b_c')] maps to the
> same value as seen[c_name('b-c')], so that we are still flagging the
> flat union as invalid.
I had already flagged a few assertions as "holds before the patch" when
I got here, and felt an urge to flag this one, too, for completeness. I
don't object to you adding the assertion.
>> I'm fine with not splitting this patch. I'd be also fine with splitting
>> it up. Your choice.
>
> I'm still thinking about it; may depend on how much other refactoring I do.
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names
2015-10-02 17:11 ` Markus Armbruster
@ 2015-10-03 1:01 ` Eric Blake
2015-10-06 8:41 ` Markus Armbruster
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-03 1:01 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]
On 10/02/2015 11:11 AM, Markus Armbruster wrote:
>>> Why wrap function c_name() in a method? Why not simply call the
>>> function?
>>
>> 'self.c_name()' is shorter than 'c_name(self.name)'. And I already had
>> long lines with that seen[self.c_name()].describe() pattern.
>
> You could also try a local variable: cnam = c_name(self.name).
>
>>> It's method in QAPISchemaEntity only because this lets us add special
>>> cases in a neat way.
>>
>> True, but I _did_ mention in the commit message that I did it for less
>> typing.
>>
>> But as to special cases, yes, I have one in mind (although I have not
>> played with it yet).
>
> I'm afraid I don't have enough context to grok this late on Friday :)
Here's another case I have in mind. Right now, we have special code
littered in qapi-types and qapi-visit to track that the
QAPISchemaObjectTypeMember for simple unions is named 'type' in QMP but
'kind' in C code. Having member.c_name() return 'kind' would simplify
that code.
So, what I will do for v7 is rework the patches to independently
implement and use member.c_name() (possibly by also creating a special
subclass of QAPISchemaObjectTypeMember), showing how it makes the
clients easier, and then this patch can also make use of that work.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names
2015-10-03 1:01 ` Eric Blake
@ 2015-10-06 8:41 ` Markus Armbruster
0 siblings, 0 replies; 46+ messages in thread
From: Markus Armbruster @ 2015-10-06 8:41 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, Michael Roth, ehabkost, qemu-devel
Eric Blake <eblake@redhat.com> writes:
> On 10/02/2015 11:11 AM, Markus Armbruster wrote:
>
>>>> Why wrap function c_name() in a method? Why not simply call the
>>>> function?
>>>
>>> 'self.c_name()' is shorter than 'c_name(self.name)'. And I already had
>>> long lines with that seen[self.c_name()].describe() pattern.
>>
>> You could also try a local variable: cnam = c_name(self.name).
>>
>>>> It's method in QAPISchemaEntity only because this lets us add special
>>>> cases in a neat way.
>>>
>>> True, but I _did_ mention in the commit message that I did it for less
>>> typing.
>>>
>>> But as to special cases, yes, I have one in mind (although I have not
>>> played with it yet).
>
>>
>> I'm afraid I don't have enough context to grok this late on Friday :)
>
> Here's another case I have in mind. Right now, we have special code
> littered in qapi-types and qapi-visit to track that the
> QAPISchemaObjectTypeMember for simple unions is named 'type' in QMP but
> 'kind' in C code. Having member.c_name() return 'kind' would simplify
> that code.
Yes, but naming it 'type' in C will simplify things even more :)
Might make sense as an intermediate step all the same. However, if this
is the only reason for having a c_name() member, the member will become
superfluous when we clean up the type vs. kind stupidity.
> So, what I will do for v7 is rework the patches to independently
> implement and use member.c_name() (possibly by also creating a special
> subclass of QAPISchemaObjectTypeMember), showing how it makes the
> clients easier, and then this patch can also make use of that work.
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check()
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (6 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 14:00 ` Markus Armbruster
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 09/12] qapi: Defer duplicate enum value " Eric Blake
` (3 subsequent siblings)
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
With the previous commit, we have two different locations for
detecting member name clashes - one at parse time, and another
at QAPISchema*.check() time. Consolidate some of the checks
into a single place, which is also in line with our TODO to
eventually defer all of the parse time semantic checking into
the newer schema code. The check_member_clash() function is
no longer needed.
The wording of several error messages has changed, but in many
cases feels like an improvement rather than a regression. The
recent change to avoid an assertion failure when a flat union
branch name collides with its discriminator name is also
handled nicely by this code; but there is more work needed
before we can detect all collisions in simple union branch
names done by the old code.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to earlier testsuite improvements, fold in cleanup
of flat-union-clash-type
---
scripts/qapi.py | 46 +++++++++------------------
tests/qapi-schema/flat-union-clash-member.err | 2 +-
tests/qapi-schema/flat-union-clash-type.err | 2 +-
tests/qapi-schema/struct-base-clash-deep.err | 2 +-
tests/qapi-schema/struct-base-clash.err | 2 +-
5 files changed, 19 insertions(+), 35 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1acf02b..e58c5a8 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False,
'enum'])
-def check_member_clash(expr_info, base_name, data, source=""):
- base = find_struct(base_name)
- assert base
- base_members = base['data']
- for key in data.keys():
- if key.startswith('*'):
- key = key[1:]
- if key in base_members or "*" + key in base_members:
- raise QAPIExprError(expr_info,
- "Member name '%s'%s clashes with base '%s'"
- % (key, source, base_name))
- if base.get('base'):
- check_member_clash(expr_info, base['base'], data, source)
-
-
def check_command(expr, expr_info):
name = expr['command']
@@ -592,15 +577,9 @@ def check_union(expr, expr_info):
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
- # Each value must name a known type; furthermore, in flat unions,
- # branches must be a struct with no overlapping member names
+ # Each value must name a known type
check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
value, allow_array=not base, allow_metas=allow_metas)
- if base:
- branch_struct = find_struct(value)
- assert branch_struct
- check_member_clash(expr_info, base, branch_struct['data'],
- " of branch '%s'" % key)
# If the discriminator names an enum type, then all members
# of 'data' must also be members of the enum type, which in turn
@@ -611,11 +590,6 @@ def check_union(expr, expr_info):
"Discriminator value '%s' is not found in "
"enum '%s'" %
(key, enum_define["enum_name"]))
- if discriminator in enum_define['enum_values']:
- raise QAPIExprError(expr_info,
- "Discriminator name '%s' collides with "
- "enum value in '%s'" %
- (discriminator, enum_define["enum_name"]))
# Otherwise, check for conflicts in the generated enum
else:
@@ -690,8 +664,6 @@ def check_struct(expr, expr_info):
allow_dict=True, allow_optional=True)
check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
allow_metas=['struct'])
- if expr.get('base'):
- check_member_clash(expr_info, expr['base'], expr['data'])
def check_keys(expr_elem, meta, required, optional=[]):
@@ -1046,16 +1018,28 @@ class QAPISchemaObjectTypeVariants(object):
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
vseen = dict(seen)
- v.check(schema, info, self.tag_member.type, vseen)
+ v.check(schema, info, self.tag_member.type, vseen,
+ self.tag_name is not None)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ, owner):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
- def check(self, schema, info, tag_type, seen):
+ def check(self, schema, info, tag_type, seen, flat):
QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
assert self.name in tag_type.values
+ if flat:
+ self.type.check(schema)
+ assert isinstance(self.type.members, list)
+ assert not self.type.variants # not implemented
+ for m in self.type.members:
+ if m.c_name() in seen:
+ raise QAPIExprError(info,
+ "Member '%s' of branch '%s' collides "
+ "with %s"
+ % (m.name, self.name,
+ seen[m.c_name()].describe()))
def describe(self):
return "'%s' (branch of %s)" % (self.name, self._owner)
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
index 2f0397a..57dd478 100644
--- a/tests/qapi-schema/flat-union-clash-member.err
+++ b/tests/qapi-schema/flat-union-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
+tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
index b44dd40..3f3248b 100644
--- a/tests/qapi-schema/flat-union-clash-type.err
+++ b/tests/qapi-schema/flat-union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
+tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
index f7a25a3..e2d7943 100644
--- a/tests/qapi-schema/struct-base-clash-deep.err
+++ b/tests/qapi-schema/struct-base-clash-deep.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
index 3a9f66b..c52f33d 100644
--- a/tests/qapi-schema/struct-base-clash.err
+++ b/tests/qapi-schema/struct-base-clash.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check()
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check() Eric Blake
@ 2015-10-02 14:00 ` Markus Armbruster
2015-10-02 15:52 ` Eric Blake
0 siblings, 1 reply; 46+ messages in thread
From: Markus Armbruster @ 2015-10-02 14:00 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> With the previous commit, we have two different locations for
> detecting member name clashes - one at parse time, and another
> at QAPISchema*.check() time. Consolidate some of the checks
> into a single place, which is also in line with our TODO to
> eventually defer all of the parse time semantic checking into
> the newer schema code. The check_member_clash() function is
> no longer needed.
>
> The wording of several error messages has changed, but in many
> cases feels like an improvement rather than a regression. The
> recent change to avoid an assertion failure when a flat union
> branch name collides with its discriminator name is also
Which patch was that again?
> handled nicely by this code; but there is more work needed
> before we can detect all collisions in simple union branch
> names done by the old code.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: rebase to earlier testsuite improvements, fold in cleanup
> of flat-union-clash-type
> ---
> scripts/qapi.py | 46 +++++++++------------------
> tests/qapi-schema/flat-union-clash-member.err | 2 +-
> tests/qapi-schema/flat-union-clash-type.err | 2 +-
> tests/qapi-schema/struct-base-clash-deep.err | 2 +-
> tests/qapi-schema/struct-base-clash.err | 2 +-
> 5 files changed, 19 insertions(+), 35 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1acf02b..e58c5a8 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False,
> 'enum'])
>
>
> -def check_member_clash(expr_info, base_name, data, source=""):
> - base = find_struct(base_name)
> - assert base
> - base_members = base['data']
> - for key in data.keys():
> - if key.startswith('*'):
> - key = key[1:]
> - if key in base_members or "*" + key in base_members:
> - raise QAPIExprError(expr_info,
> - "Member name '%s'%s clashes with base '%s'"
> - % (key, source, base_name))
> - if base.get('base'):
> - check_member_clash(expr_info, base['base'], data, source)
> -
> -
> def check_command(expr, expr_info):
> name = expr['command']
>
> @@ -592,15 +577,9 @@ def check_union(expr, expr_info):
> for (key, value) in members.items():
> check_name(expr_info, "Member of union '%s'" % name, key)
>
> - # Each value must name a known type; furthermore, in flat unions,
> - # branches must be a struct with no overlapping member names
> + # Each value must name a known type
> check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
> value, allow_array=not base, allow_metas=allow_metas)
> - if base:
> - branch_struct = find_struct(value)
> - assert branch_struct
> - check_member_clash(expr_info, base, branch_struct['data'],
> - " of branch '%s'" % key)
>
> # If the discriminator names an enum type, then all members
> # of 'data' must also be members of the enum type, which in turn
> @@ -611,11 +590,6 @@ def check_union(expr, expr_info):
# If the discriminator names an enum type, then all members
# of 'data' must also be members of the enum type, which in turn
# must not collide with the discriminator name.
Should this comment be updated for the code removal below?
if enum_define:
if key not in enum_define['enum_values']:
raise QAPIExprError(expr_info,
> "Discriminator value '%s' is not found in "
> "enum '%s'" %
> (key, enum_define["enum_name"]))
> - if discriminator in enum_define['enum_values']:
> - raise QAPIExprError(expr_info,
> - "Discriminator name '%s' collides with "
> - "enum value in '%s'" %
> - (discriminator, enum_define["enum_name"]))
>
> # Otherwise, check for conflicts in the generated enum
> else:
> @@ -690,8 +664,6 @@ def check_struct(expr, expr_info):
> allow_dict=True, allow_optional=True)
> check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
> allow_metas=['struct'])
> - if expr.get('base'):
> - check_member_clash(expr_info, expr['base'], expr['data'])
>
>
> def check_keys(expr_elem, meta, required, optional=[]):
> @@ -1046,16 +1018,28 @@ class QAPISchemaObjectTypeVariants(object):
> assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> for v in self.variants:
> vseen = dict(seen)
> - v.check(schema, info, self.tag_member.type, vseen)
> + v.check(schema, info, self.tag_member.type, vseen,
> + self.tag_name is not None)
>
>
> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> def __init__(self, name, typ, owner):
> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
>
> - def check(self, schema, info, tag_type, seen):
> + def check(self, schema, info, tag_type, seen, flat):
> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
> assert self.name in tag_type.values
> + if flat:
Any way to avoid the conditional?
I've tried hard to make simple unions mere sugar for flat ones. There
are a few special cases left where we need to distinguish the two, but
they're all marked TODO.
Can we have a brief comment explaing what we're checking here? We
generally don't have such comments in check() methods so far. Sort of
okay as long as they merely assert, but you're now starting to move
semantic analysis into them, which raises the bar.
> + self.type.check(schema)
Uh, careful.
It's always okay for a check() method to call the check() of a child in
the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling
m.check() for its members. A tree has no cycles.
Calling it on anything else requires a non-trivial correctness argument.
Example: QAPISchemaObjectType.check() calls self.base.check(). Okay,
because no type may be a base of itself, and therefore a cycle would be
an error. The code takes care to detect cycles.
I think the correctness argument here would be "no type contain itself
as variant member, and therefore a cycle would be an error." Do we
detect cycles?
> + assert isinstance(self.type.members, list)
Implied by QAPISchemaObjectType.check()'s postcondition. Feels
redundant here, in particular since you iterate over it next anyway.
> + assert not self.type.variants # not implemented
> + for m in self.type.members:
> + if m.c_name() in seen:
> + raise QAPIExprError(info,
> + "Member '%s' of branch '%s' collides "
> + "with %s"
Hanging indentation would avoid the split string.
> + % (m.name, self.name,
> + seen[m.c_name()].describe()))
>
> def describe(self):
> return "'%s' (branch of %s)" % (self.name, self._owner)
> diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
> index 2f0397a..57dd478 100644
> --- a/tests/qapi-schema/flat-union-clash-member.err
> +++ b/tests/qapi-schema/flat-union-clash-member.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
> +tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
> index b44dd40..3f3248b 100644
> --- a/tests/qapi-schema/flat-union-clash-type.err
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
> +tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
> diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
> index f7a25a3..e2d7943 100644
> --- a/tests/qapi-schema/struct-base-clash-deep.err
> +++ b/tests/qapi-schema/struct-base-clash-deep.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
> diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
> index 3a9f66b..c52f33d 100644
> --- a/tests/qapi-schema/struct-base-clash.err
> +++ b/tests/qapi-schema/struct-base-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check()
2015-10-02 14:00 ` Markus Armbruster
@ 2015-10-02 15:52 ` Eric Blake
0 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 15:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 5413 bytes --]
On 10/02/2015 08:00 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> With the previous commit, we have two different locations for
>> detecting member name clashes - one at parse time, and another
>> at QAPISchema*.check() time. Consolidate some of the checks
>> into a single place, which is also in line with our TODO to
>> eventually defer all of the parse time semantic checking into
>> the newer schema code. The check_member_clash() function is
>> no longer needed.
>>
>> The wording of several error messages has changed, but in many
>> cases feels like an improvement rather than a regression. The
>> recent change to avoid an assertion failure when a flat union
>> branch name collides with its discriminator name is also
>
> Which patch was that again?
v7a 6/18, http://repo.or.cz/qemu/ericb.git/commit/ede42e8e
[uggh - gnu.org archives are still quite lagging, or I'd point to a mail]
>
>> handled nicely by this code; but there is more work needed
>> before we can detect all collisions in simple union branch
>> names done by the old code.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> @@ -611,11 +590,6 @@ def check_union(expr, expr_info):
> # If the discriminator names an enum type, then all members
> # of 'data' must also be members of the enum type, which in turn
> # must not collide with the discriminator name.
>
> Should this comment be updated for the code removal below?
Oh, right, revert both hunks of the earlier commit since we are now
doing it in a less hacky manner.
>> + v.check(schema, info, self.tag_member.type, vseen,
>> + self.tag_name is not None)
>>
>>
>> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>> def __init__(self, name, typ, owner):
>> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
>>
>> - def check(self, schema, info, tag_type, seen):
>> + def check(self, schema, info, tag_type, seen, flat):
>> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>> assert self.name in tag_type.values
>> + if flat:
>
> Any way to avoid the conditional?
Not that I could come up with when I first tried, but it's been a while,
so I can try again. I remember running into an assertion failure about
nested ObjectType.check() calls if I did it unconditionally, since we
allow the following:
{ 'struct': 'Fork', 'data': { '*fork': 'Union' } }
{ 'union': 'Union', 'data': { 'branch': 'Fork' } }
We must NOT call self.type.check() on Fork (because we are not embedding
the members of Fork into the simple union) - but then again, what we are
really doing in the QAPISchema is visiting
'branch':':obj-Union-branch-wrapper', not 'Fork'. And then there's
alternates, where branches aren't necessarily QAPISchemaObjectType in
the first place.
>
> I've tried hard to make simple unions mere sugar for flat ones. There
> are a few special cases left where we need to distinguish the two, but
> they're all marked TODO.
>
> Can we have a brief comment explaing what we're checking here? We
> generally don't have such comments in check() methods so far. Sort of
> okay as long as they merely assert, but you're now starting to move
> semantic analysis into them, which raises the bar.
Yeah, I agree with adding more documentation about the checks.
>
>> + self.type.check(schema)
>
> Uh, careful.
>
> It's always okay for a check() method to call the check() of a child in
> the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling
> m.check() for its members. A tree has no cycles.
>
> Calling it on anything else requires a non-trivial correctness argument.
> Example: QAPISchemaObjectType.check() calls self.base.check(). Okay,
> because no type may be a base of itself, and therefore a cycle would be
> an error. The code takes care to detect cycles.
>
> I think the correctness argument here would be "no type contain itself
> as variant member, and therefore a cycle would be an error." Do we
> detect cycles?
Not in this patch, but the assertion failure I ran into is the very
reason I wrote a cycle detection patch next (11/12 in this series);
maybe I should rebase that patch first?
But you are correct that a flat union must not include itself as one of
the branch types. I originally tried to test that in v5 3/46; your
review convinced me it is no different than any other branch type that
injects the same QMP members into the flat union, so I revamped the test
in v6 to be a self-inheritance test instead, and then we dropped it
after v7. (Search for "fork" in
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05733.html)
>
>> + assert isinstance(self.type.members, list)
>
> Implied by QAPISchemaObjectType.check()'s postcondition. Feels
> redundant here, in particular since you iterate over it next anyway.
Probably redundant, but I added it when I hit a cycle loop, to make sure
that I wasn't still hitting a cycle (the 'if flat:' condition was the
only way I could figure at the time to ensure I didn't hit this assertion).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 09/12] qapi: Defer duplicate enum value checks to schema check()
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (7 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check() Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 10/12] qapi: Correct error for union branch 'kind' clash Eric Blake
` (2 subsequent siblings)
11 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Similar to the previous commit, move the detection of a collision
in enum values from parse time to QAPISchemaEnumType.check().
This happens to also detect collisions in union branch names,
so for a decent error message, we have to determine if the enum
is implicit (and if so where the real collision lies).
Testing this showed that the test union-bad-branch and
union-clash-branches were basically testing the same thing;
with the minor difference that the former clashes only in the
enum, while the latter also clashes in the C union member
names that would be generated. So delete the weaker test.
However, the union-clash-type test exposes one weakness in this
patch - it claims the collision is with 'type', even though the
C struct only has a duplicate 'kind'. We will eventually fix
the generated C to use 'type' like QMP already does, but in the
short term, the next patch will fix things to find the correct
collision.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch
---
scripts/qapi.py | 44 ++++++++++++------------------
tests/Makefile | 1 -
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/enum-max-member.err | 2 +-
tests/qapi-schema/union-bad-branch.err | 1 -
tests/qapi-schema/union-bad-branch.exit | 1 -
tests/qapi-schema/union-bad-branch.json | 8 ------
tests/qapi-schema/union-bad-branch.out | 0
tests/qapi-schema/union-clash-branches.err | 2 +-
tests/qapi-schema/union-clash-type.err | 2 +-
tests/qapi-schema/union-clash-type.json | 2 ++
tests/qapi-schema/union-max.err | 2 +-
13 files changed, 25 insertions(+), 44 deletions(-)
delete mode 100644 tests/qapi-schema/union-bad-branch.err
delete mode 100644 tests/qapi-schema/union-bad-branch.exit
delete mode 100644 tests/qapi-schema/union-bad-branch.json
delete mode 100644 tests/qapi-schema/union-bad-branch.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e58c5a8..d90399a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -530,7 +530,6 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
# Two types of unions, determined by discriminator.
@@ -591,34 +590,16 @@ def check_union(expr, expr_info):
"enum '%s'" %
(key, enum_define["enum_name"]))
- # Otherwise, check for conflicts in the generated enum
- else:
- c_key = camel_to_upper(key)
- if c_key in values:
- raise QAPIExprError(expr_info,
- "Union '%s' member '%s' clashes with '%s'"
- % (name, key, values[c_key]))
- values[c_key] = key
-
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
- values = {'MAX': '(automatic)'}
types_seen = {}
# Check every branch
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
- # Check for conflicts in the generated enum
- c_key = camel_to_upper(key)
- if c_key in values:
- raise QAPIExprError(expr_info,
- "Alternate '%s' member '%s' clashes with '%s'"
- % (name, key, values[c_key]))
- values[c_key] = key
-
# Ensure alternates have no type conflicts.
check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
value,
@@ -637,7 +618,6 @@ def check_enum(expr, expr_info):
name = expr['enum']
members = expr.get('data')
prefix = expr.get('prefix')
- values = {'MAX': '(automatic)'}
if not isinstance(members, list):
raise QAPIExprError(expr_info,
@@ -648,12 +628,6 @@ def check_enum(expr, expr_info):
for member in members:
check_name(expr_info, "Member of enum '%s'" % name, member,
enum_member=True)
- key = camel_to_upper(member)
- if key in values:
- raise QAPIExprError(expr_info,
- "Enum '%s' member '%s' clashes with '%s'"
- % (name, member, values[key]))
- values[key] = member
def check_struct(expr, expr_info):
@@ -873,7 +847,23 @@ class QAPISchemaEnumType(QAPISchemaType):
self.prefix = prefix
def check(self, schema):
- assert len(set(self.values)) == len(self.values)
+ seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
+ for value in self.values:
+ c_value = c_enum_const(self.name, value)
+ if c_value in seen:
+ if self.name[-4:] == 'Kind':
+ owner = schema.lookup_type(self.name[:-4])
+ assert owner
+ if isinstance(owner, QAPISchemaAlternateType):
+ description = "Alternate '%s' branch" % owner.name
+ else:
+ description = "Union '%s' branch" % owner.name
+ else:
+ description = "Enum '%s' value" % self.name
+ raise QAPIExprError(self.info,
+ "%s '%s' clashes with '%s'"
+ % (description, value, seen[c_value]))
+ seen[c_value] = value
def c_type(self, is_param=False):
return c_name(self.name)
diff --git a/tests/Makefile b/tests/Makefile
index edf310d..79fb154 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -332,7 +332,6 @@ qapi-schema += unclosed-list.json
qapi-schema += unclosed-object.json
qapi-schema += unclosed-string.json
qapi-schema += unicode-str.json
-qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
qapi-schema += union-clash-branches.json
qapi-schema += union-clash-data.json
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
index a475ab6..7fd3069 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
index 48bd136..84030c5 100644
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'
+tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err
index f77837f..6b9ef9b 100644
--- a/tests/qapi-schema/enum-max-member.err
+++ b/tests/qapi-schema/enum-max-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)'
+tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' value 'max' clashes with '(automatic MAX)'
diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err
deleted file mode 100644
index 8822735..0000000
--- a/tests/qapi-schema/union-bad-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/union-bad-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json
deleted file mode 100644
index 913aa38..0000000
--- a/tests/qapi-schema/union-bad-branch.json
+++ /dev/null
@@ -1,8 +0,0 @@
-# we reject normal unions where branches would collide in C
-{ 'struct': 'One',
- 'data': { 'string': 'str' } }
-{ 'struct': 'Two',
- 'data': { 'number': 'int' } }
-{ 'union': 'MyUnion',
- 'data': { 'one': 'One',
- 'ONE': 'Two' } }
diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err
index 005c48d..d8f1265 100644
--- a/tests/qapi-schema/union-clash-branches.err
+++ b/tests/qapi-schema/union-clash-branches.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
index a5dead1..451d5f2 100644
--- a/tests/qapi-schema/union-clash-type.err
+++ b/tests/qapi-schema/union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' clashes with '(automatic)'
+tests/qapi-schema/union-clash-type.json:10: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)
diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json
index cfc256b..6101e49 100644
--- a/tests/qapi-schema/union-clash-type.json
+++ b/tests/qapi-schema/union-clash-type.json
@@ -2,6 +2,8 @@
# Reject this, because we would have a clash in generated C, between the
# simple union's implicit tag member 'kind' and the branch name 'kind'
# within the union.
+# FIXME: Right now, the test claims the collision is on 'type'; if the
+# second branch is renamed, the code mistakenly parses successfully.
# TODO: Even when the generated C is switched to use 'type' rather than
# 'kind', to match the QMP spelling, the collision should still be detected.
# Or, we could munge the branch name to allow compilation.
diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err
index 55ce439..b93beae 100644
--- a/tests/qapi-schema/union-max.err
+++ b/tests/qapi-schema/union-max.err
@@ -1 +1 @@
-tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with '(automatic)'
+tests/qapi-schema/union-max.json:2: Union 'Union' branch 'max' clashes with '(automatic MAX)'
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH v6 10/12] qapi: Correct error for union branch 'kind' clash
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (8 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 09/12] qapi: Defer duplicate enum value " Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-03 17:56 ` Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 11/12] qapi: Detect base class loops Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 12/12] RFC: qapi: Hide _info member Eric Blake
11 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
The error message when a simple union or alternate contains a
branch named 'kind' is ugly, because it is tied to the Schema
member named 'type'. A future patch will fix the generated C
to match QMP, but until that point, we can hack things with
a temporary subclass to make the error message reflect the
actually collision.
Rename alternate-clash to alternate-clash-members, and add a
new test alternate-clash-type. While similar to the earlier
addition of union-clash-type, we have one major difference: a
future patch will be simplifying alternates to not need an
implict AlternateKind enum, but we still need to detect the
collision with the resulting C 'qtype_code type;' tag.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: New patch
---
scripts/qapi.py | 14 ++++++++++++++
tests/Makefile | 3 ++-
tests/qapi-schema/alternate-clash-members.err | 1 +
.../{alternate-clash.exit => alternate-clash-members.exit} | 0
.../{alternate-clash.json => alternate-clash-members.json} | 0
.../{alternate-clash.out => alternate-clash-members.out} | 0
tests/qapi-schema/alternate-clash-type.err | 1 +
tests/qapi-schema/alternate-clash-type.exit | 1 +
tests/qapi-schema/alternate-clash-type.json | 10 ++++++++++
tests/qapi-schema/alternate-clash-type.out | 0
tests/qapi-schema/alternate-clash.err | 1 -
tests/qapi-schema/union-clash-type.err | 2 +-
tests/qapi-schema/union-clash-type.json | 2 --
13 files changed, 30 insertions(+), 5 deletions(-)
create mode 100644 tests/qapi-schema/alternate-clash-members.err
rename tests/qapi-schema/{alternate-clash.exit => alternate-clash-members.exit} (100%)
rename tests/qapi-schema/{alternate-clash.json => alternate-clash-members.json} (100%)
rename tests/qapi-schema/{alternate-clash.out => alternate-clash-members.out} (100%)
create mode 100644 tests/qapi-schema/alternate-clash-type.err
create mode 100644 tests/qapi-schema/alternate-clash-type.exit
create mode 100644 tests/qapi-schema/alternate-clash-type.json
create mode 100644 tests/qapi-schema/alternate-clash-type.out
delete mode 100644 tests/qapi-schema/alternate-clash.err
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d90399a..6c224c6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1008,10 +1008,24 @@ class QAPISchemaObjectTypeVariants(object):
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
vseen = dict(seen)
+ # TODO Ugly special case for simple unions, where the C member
+ # is named 'kind' instead of 'type'.
+ if not self.tag_name:
+ vseen['kind'] = QAPISchemaKindHack('kind',
+ vseen.pop('type')._owner)
v.check(schema, info, self.tag_member.type, vseen,
self.tag_name is not None)
+# TODO Ugly hack for simple unions, where the C member is named 'kind'
+class QAPISchemaKindHack(QAPISchemaObjectTypeMember):
+ def __init__(self, name, owner):
+ QAPISchemaObjectTypeMember.__init__(self, name, '', False, owner)
+
+ def describe(self):
+ return "'%s' (implicit tag of %s)" % (self.name, self._owner)
+
+
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ, owner):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False, owner)
diff --git a/tests/Makefile b/tests/Makefile
index 79fb154..db0f9a6 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -223,7 +223,8 @@ $(foreach target,$(SYSEMU_TARGET_LIST), \
qapi-schema += alternate-array.json
qapi-schema += alternate-base.json
-qapi-schema += alternate-clash.json
+qapi-schema += alternate-clash-members.json
+qapi-schema += alternate-clash-type.json
qapi-schema += alternate-conflict-dict.json
qapi-schema += alternate-conflict-string.json
qapi-schema += alternate-empty.json
diff --git a/tests/qapi-schema/alternate-clash-members.err b/tests/qapi-schema/alternate-clash-members.err
new file mode 100644
index 0000000..0adf737
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-members.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-clash-members.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/alternate-clash.exit b/tests/qapi-schema/alternate-clash-members.exit
similarity index 100%
rename from tests/qapi-schema/alternate-clash.exit
rename to tests/qapi-schema/alternate-clash-members.exit
diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash-members.json
similarity index 100%
rename from tests/qapi-schema/alternate-clash.json
rename to tests/qapi-schema/alternate-clash-members.json
diff --git a/tests/qapi-schema/alternate-clash.out b/tests/qapi-schema/alternate-clash-members.out
similarity index 100%
rename from tests/qapi-schema/alternate-clash.out
rename to tests/qapi-schema/alternate-clash-members.out
diff --git a/tests/qapi-schema/alternate-clash-type.err b/tests/qapi-schema/alternate-clash-type.err
new file mode 100644
index 0000000..cdd2090
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-clash-type.json:9: 'kind' (branch of Alt1) collides with 'kind' (implicit tag of Alt1)
diff --git a/tests/qapi-schema/alternate-clash-type.exit b/tests/qapi-schema/alternate-clash-type.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-clash-type.json b/tests/qapi-schema/alternate-clash-type.json
new file mode 100644
index 0000000..629584b
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.json
@@ -0,0 +1,10 @@
+# Alternate branch 'type'
+# Reject this, because we would have a clash in generated C, between the
+# alternate's implicit tag member 'kind' and the branch name 'kind'
+# within the alternate.
+# TODO: Even if alternates are simplified in the future to use a simpler
+# 'qtype_code type' tag, rather than a full QAPISchemaObjectTypeMember,
+# we must still flag the collision, or else munge the generated C branch
+# names to allow compilation.
+{ 'alternate': 'Alt1',
+ 'data': { 'kind': 'str', 'type': 'int' } }
diff --git a/tests/qapi-schema/alternate-clash-type.out b/tests/qapi-schema/alternate-clash-type.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
deleted file mode 100644
index 7fd3069..0000000
--- a/tests/qapi-schema/alternate-clash.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
index 451d5f2..ce7f8cd 100644
--- a/tests/qapi-schema/union-clash-type.err
+++ b/tests/qapi-schema/union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-type.json:10: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)
+tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion) collides with 'kind' (implicit tag of TestUnion)
diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json
index 6101e49..cfc256b 100644
--- a/tests/qapi-schema/union-clash-type.json
+++ b/tests/qapi-schema/union-clash-type.json
@@ -2,8 +2,6 @@
# Reject this, because we would have a clash in generated C, between the
# simple union's implicit tag member 'kind' and the branch name 'kind'
# within the union.
-# FIXME: Right now, the test claims the collision is on 'type'; if the
-# second branch is renamed, the code mistakenly parses successfully.
# TODO: Even when the generated C is switched to use 'type' rather than
# 'kind', to match the QMP spelling, the collision should still be detected.
# Or, we could munge the branch name to allow compilation.
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [PATCH v6 10/12] qapi: Correct error for union branch 'kind' clash
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 10/12] qapi: Correct error for union branch 'kind' clash Eric Blake
@ 2015-10-03 17:56 ` Eric Blake
0 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-03 17:56 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, Michael Roth, ehabkost, armbru
[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]
On 10/01/2015 10:31 PM, Eric Blake wrote:
> The error message when a simple union or alternate contains a
> branch named 'kind' is ugly, because it is tied to the Schema
> member named 'type'. A future patch will fix the generated C
> to match QMP, but until that point, we can hack things with
> a temporary subclass to make the error message reflect the
> actually collision.
I found a cleaner way to do this, and will be dropping this patch,
except for the testsuite improvements which I will be merging into 9/12.
>
> Rename alternate-clash to alternate-clash-members, and add a
> new test alternate-clash-type. While similar to the earlier
> addition of union-clash-type, we have one major difference: a
> future patch will be simplifying alternates to not need an
> implict AlternateKind enum, but we still need to detect the
> collision with the resulting C 'qtype_code type;' tag.
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [PATCH v6 11/12] qapi: Detect base class loops
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (9 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 10/12] qapi: Correct error for union branch 'kind' clash Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 12/12] RFC: qapi: Hide _info member Eric Blake
11 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
It should be fairly obvious that qapi base classes need to
form an acyclic graph, since QMP cannot specify the same
key more than once, while base classes are included as flat
members alongside other members added by the child. But prior
to Markus' introspection commits (such as commit 75ebcd7f),
the test in isolation would cause python to exit with a
complaint about unbounded nesting; and after his patches (in
particular ac88219a), it triggers an assertion failure. This
patch includes both the test and the fix, since the .err file
output for an assertion failure is too hard to rebase when
other patches cause line number changes.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to earlier info changes
---
scripts/qapi.py | 6 +++++-
tests/Makefile | 1 +
tests/qapi-schema/base-cycle.err | 1 +
tests/qapi-schema/base-cycle.exit | 1 +
tests/qapi-schema/base-cycle.json | 3 +++
tests/qapi-schema/base-cycle.out | 0
6 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/base-cycle.err
create mode 100644 tests/qapi-schema/base-cycle.exit
create mode 100644 tests/qapi-schema/base-cycle.json
create mode 100644 tests/qapi-schema/base-cycle.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6c224c6..c226cd9 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -913,7 +913,11 @@ class QAPISchemaObjectType(QAPISchemaType):
self.members = None
def check(self, schema):
- assert self.members is not False # not running in cycles
+ if self.members is False: # check for cycles
+ assert self._base_name
+ raise QAPIExprError(self.info,
+ "Object %s cyclically depends on %s"
+ % (self.name, self._base_name))
if self.members:
return
self.members = False # mark as being checked
diff --git a/tests/Makefile b/tests/Makefile
index db0f9a6..8ae8140 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -249,6 +249,7 @@ qapi-schema += bad-ident.json
qapi-schema += bad-type-bool.json
qapi-schema += bad-type-dict.json
qapi-schema += bad-type-int.json
+qapi-schema += base-cycle.json
qapi-schema += command-int.json
qapi-schema += comments.json
qapi-schema += double-data.json
diff --git a/tests/qapi-schema/base-cycle.err b/tests/qapi-schema/base-cycle.err
new file mode 100644
index 0000000..e0221b5
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.err
@@ -0,0 +1 @@
+tests/qapi-schema/base-cycle.json:2: Object Base1 cyclically depends on Base2
diff --git a/tests/qapi-schema/base-cycle.exit b/tests/qapi-schema/base-cycle.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/base-cycle.json b/tests/qapi-schema/base-cycle.json
new file mode 100644
index 0000000..2866772
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.json
@@ -0,0 +1,3 @@
+# we reject a loop in base classes
+{ 'struct': 'Base1', 'base': 'Base2', 'data': {} }
+{ 'struct': 'Base2', 'base': 'Base1', 'data': {} }
diff --git a/tests/qapi-schema/base-cycle.out b/tests/qapi-schema/base-cycle.out
new file mode 100644
index 0000000..e69de29
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [PATCH v6 12/12] RFC: qapi: Hide _info member
2015-10-02 4:31 [Qemu-devel] [PATCH v6 00/12] post-introspection cleanups, subset B Eric Blake
` (10 preceding siblings ...)
2015-10-02 4:31 ` [Qemu-devel] [PATCH v6 11/12] qapi: Detect base class loops Eric Blake
@ 2015-10-02 4:31 ` Eric Blake
11 siblings, 0 replies; 46+ messages in thread
From: Eric Blake @ 2015-10-02 4:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Now that nothing but sub-classes are using QAPISchemaEntity.info,
rename it to _info to ensure that external users do not revert
to using it again.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split from 11/46; probably worth dropping
---
scripts/qapi.py | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c226cd9..7b8d976 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -740,7 +740,7 @@ class QAPISchemaEntity(object):
assert isinstance(name, str)
self.name = name
assert info or not QAPISchema.predefined_initialized
- self.info = info
+ self._info = info
def c_name(self):
return c_name(self.name)
@@ -834,7 +834,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
return self._json_type_name
def visit(self, visitor):
- visitor.visit_builtin_type(self.name, self.info, self.json_type())
+ visitor.visit_builtin_type(self.name, self._info, self.json_type())
class QAPISchemaEnumType(QAPISchemaType):
@@ -860,7 +860,7 @@ class QAPISchemaEnumType(QAPISchemaType):
description = "Union '%s' branch" % owner.name
else:
description = "Enum '%s' value" % self.name
- raise QAPIExprError(self.info,
+ raise QAPIExprError(self._info,
"%s '%s' clashes with '%s'"
% (description, value, seen[c_value]))
seen[c_value] = value
@@ -876,7 +876,7 @@ class QAPISchemaEnumType(QAPISchemaType):
return 'string'
def visit(self, visitor):
- visitor.visit_enum_type(self.name, self.info,
+ visitor.visit_enum_type(self.name, self._info,
self.values, self.prefix)
@@ -895,7 +895,7 @@ class QAPISchemaArrayType(QAPISchemaType):
return 'array'
def visit(self, visitor):
- visitor.visit_array_type(self.name, self.info, self.element_type)
+ visitor.visit_array_type(self.name, self._info, self.element_type)
class QAPISchemaObjectType(QAPISchemaType):
@@ -915,7 +915,7 @@ class QAPISchemaObjectType(QAPISchemaType):
def check(self, schema):
if self.members is False: # check for cycles
assert self._base_name
- raise QAPIExprError(self.info,
+ raise QAPIExprError(self._info,
"Object %s cyclically depends on %s"
% (self.name, self._base_name))
if self.members:
@@ -934,9 +934,9 @@ class QAPISchemaObjectType(QAPISchemaType):
assert m.c_name() not in seen
seen[m.c_name()] = m
for m in self.local_members:
- m.check(schema, self.info, members, seen)
+ m.check(schema, self._info, members, seen)
if self.variants:
- self.variants.check(schema, self.info, members, seen)
+ self.variants.check(schema, self._info, members, seen)
self.members = members
def is_implicit(self):
@@ -954,9 +954,9 @@ class QAPISchemaObjectType(QAPISchemaType):
return 'object'
def visit(self, visitor):
- visitor.visit_object_type(self.name, self.info,
+ visitor.visit_object_type(self.name, self._info,
self.base, self.local_members, self.variants)
- visitor.visit_object_type_flat(self.name, self.info,
+ visitor.visit_object_type_flat(self.name, self._info,
self.members, self.variants)
@@ -1071,13 +1071,13 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
- self.variants.check(schema, self.info, [], {})
+ self.variants.check(schema, self._info, [], {})
def json_type(self):
return 'value'
def visit(self, visitor):
- visitor.visit_alternate_type(self.name, self.info, self.variants)
+ visitor.visit_alternate_type(self.name, self._info, self.variants)
class QAPISchemaCommand(QAPISchemaEntity):
@@ -1102,7 +1102,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
assert isinstance(self.ret_type, QAPISchemaType)
def visit(self, visitor):
- visitor.visit_command(self.name, self.info,
+ visitor.visit_command(self.name, self._info,
self.arg_type, self.ret_type,
self.gen, self.success_response)
@@ -1121,7 +1121,7 @@ class QAPISchemaEvent(QAPISchemaEntity):
assert not self.arg_type.variants # not implemented
def visit(self, visitor):
- visitor.visit_event(self.name, self.info, self.arg_type)
+ visitor.visit_event(self.name, self._info, self.arg_type)
class QAPISchema(object):
--
2.4.3
^ permalink raw reply related [flat|nested] 46+ messages in thread