Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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.