Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members

[Markus Armbruster <armbru@redhat.com>]

A bit more detail in the commit message would make this patch easier to
review.

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py                         | 39 ++++++++++++++++++++++++++++-----
>  tests/Makefile.include                  |  1 -
>  tests/qapi-schema/enum-dict-member.err  |  1 -
>  tests/qapi-schema/enum-dict-member.exit |  1 -
>  tests/qapi-schema/enum-dict-member.json |  2 --
>  tests/qapi-schema/enum-dict-member.out  |  0
>  tests/qapi-schema/qapi-schema-test.json |  5 +++--
>  tests/qapi-schema/qapi-schema-test.out  |  3 ++-
>  tests/qapi-schema/test-qapi.py          |  2 ++
>  9 files changed, 41 insertions(+), 13 deletions(-)
>  delete mode 100644 tests/qapi-schema/enum-dict-member.err
>  delete mode 100644 tests/qapi-schema/enum-dict-member.exit
>  delete mode 100644 tests/qapi-schema/enum-dict-member.json
>  delete mode 100644 tests/qapi-schema/enum-dict-member.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 386a577a59..1535de9ce7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -659,6 +659,14 @@ def check_if(expr, info):
>              info, "'if' condition must be a string or a list of strings")
>  
>  
> +def check_unknown_keys(info, dict, allowed_keys):
> +    diff = set(dict) - allowed_keys
> +    if not diff:
> +        return
> +    raise QAPISemError(info, "Dictionnary has unknown keys: %s (allowed: %s)" %

s/Dictionnary/Dictionary/

> +        (', '.join(diff), ', '.join(allowed_keys)))

I'm afraid this duplicates a part of check_keys().  Could it be factored
out?

> +
> +
>  def check_type(info, source, value, allow_array=False,
>                 allow_dict=False, allow_optional=False,
>                 allow_metas=[]):
> @@ -739,6 +747,10 @@ def check_event(expr, info):
>                 allow_metas=meta)
>  
>  
> +def enum_get_values(expr):
> +    return [e if isinstance(e, str) else e['name'] for e in expr['data']]
> +
> +
>  def check_union(expr, info):
>      name = expr['union']
>      base = expr.get('base')
> @@ -798,7 +810,7 @@ def check_union(expr, info):
>          # If the discriminator names an enum type, then all members
>          # of 'data' must also be members of the enum type.
>          if enum_define:
> -            if key not in enum_define['data']:
> +            if key not in enum_get_values(enum_define):
>                  raise QAPISemError(info,
>                                     "Discriminator value '%s' is not found in "
>                                     "enum '%s'"
> @@ -806,7 +818,7 @@ def check_union(expr, info):
>  
>      # If discriminator is user-defined, ensure all values are covered
>      if enum_define:
> -        for value in enum_define['data']:
> +        for value in enum_get_values(enum_define):
>              if value not in members.keys():
>                  raise QAPISemError(info, "Union '%s' data missing '%s' branch"
>                                     % (name, value))
> @@ -837,7 +849,7 @@ def check_alternate(expr, info):
>          if qtype == 'QTYPE_QSTRING':
>              enum_expr = enum_types.get(value)
>              if enum_expr:
> -                for v in enum_expr['data']:
> +                for v in enum_get_values(enum_expr):
>                      if v in ['on', 'off']:
>                          conflicting.add('QTYPE_QBOOL')
>                      if re.match(r'[-+0-9.]', v): # lazy, could be tightened
> @@ -865,6 +877,14 @@ def check_enum(expr, info):
>          raise QAPISemError(info,
>                             "Enum '%s' requires a string for 'prefix'" % name)
>      for member in members:
> +        if isinstance(member, dict):
> +            if 'name' not in member:
> +                raise QAPISemError(info, "Dictionary member of enum '%s' must "
> +                                   "have a 'name' key" % name)
> +            if 'if' in member:
> +                check_if(member, info)
> +            check_unknown_keys(info, member, {'name', 'if'})
> +            member = member['name']
>          check_name(info, "Member of enum '%s'" % name, member,
>                     enum_member=True)
>  
> @@ -1280,9 +1300,11 @@ class QAPISchemaObjectType(QAPISchemaType):
>  class QAPISchemaMember(object):
>      role = 'member'
>  
> -    def __init__(self, name):
> +    def __init__(self, name, ifcond=None):
>          assert isinstance(name, str)
> +        assert ifcond is None or isinstance(ifcond, str)
>          self.name = name
> +        self.ifcond = ifcond
>          self.owner = None
>  
>      def set_owner(self, name):

QAPISchemaObjectTypeMember inherits .ifcond.  Peeking ahead: looks like
it'll get used when we add conditions to object type members, PATCH
23ff.  Okay.  Mentioning it in the commit message wouldn't hurt, though.

> @@ -1559,7 +1581,14 @@ class QAPISchema(object):
>                                              qtype_values, 'QTYPE'))
>  
>      def _make_enum_members(self, values):
> -        return [QAPISchemaMember(v) for v in values]
> +        enum = []
> +        for v in values:
> +            ifcond = None
> +            if isinstance(v, dict):
> +                ifcond = v.get('if')
> +                v = v['name']
> +            enum.append(QAPISchemaMember(v, ifcond))


I like brevity a lot, but if it's bought by assigning to a loop control
variable, I pass.  Cleaner:

           for v in values:
               if isinstance(v, dict):
                   name = v['name']
                   ifcond = v.get('if')
               else:
                   name = v
                   ifcond = None
           enum.append(QAPISchemaMember(name, ifcond))
                   
> +        return enum
>  
>      def _make_implicit_enum_type(self, name, info, ifcond, values):
>          # See also QAPISchemaObjectTypeMember._pretty_owner()
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 8dac7c9083..a9f0ddbe01 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -443,7 +443,6 @@ qapi-schema += empty.json
>  qapi-schema += enum-bad-name.json
>  qapi-schema += enum-bad-prefix.json
>  qapi-schema += enum-clash-member.json
> -qapi-schema += enum-dict-member.json
>  qapi-schema += enum-int-member.json
>  qapi-schema += enum-member-case.json
>  qapi-schema += enum-missing-data.json
> diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err
> deleted file mode 100644
> index 8ca146ea59..0000000000
> --- a/tests/qapi-schema/enum-dict-member.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/enum-dict-member.json:2: Member of enum 'MyEnum' requires a string name
> diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit
> deleted file mode 100644
> index d00491fd7e..0000000000
> --- a/tests/qapi-schema/enum-dict-member.exit
> +++ /dev/null
> @@ -1 +0,0 @@
> -1
> diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json
> deleted file mode 100644
> index 79672e0f09..0000000000
> --- a/tests/qapi-schema/enum-dict-member.json
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -# we reject any enum member that is not a string
> -{ 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }
> diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out
> deleted file mode 100644
> index e69de29bb2..0000000000

Hmm.  The dict instance of "enum value must be of an appropriate JSON
type" error class goes away in this patch, but the class remains.  Do we
still cover it?

There's enum-int-member.json, but it's not a good test: it dies in the
lexer.  I think we should replace the two tests by a single one.
Perhaps something like 'data': [ [] ] would do.

> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index dc2c444fc1..ad2b405d83 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -194,7 +194,8 @@
>  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>    'if': 'defined(TEST_IF_STRUCT)' }
>  
> -{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],
> +{ 'enum': 'TestIfEnum', 'data':
> +  [ 'foo', { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ],
>    'if': 'defined(TEST_IF_ENUM)' }
>  
>  { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },
> @@ -203,7 +204,7 @@
>  { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 'TestStruct' },
>    'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>  
> -{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
> +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct', 'bar': 'TestIfEnum' },
>    'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' }
>  
>  { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },

Should this hunk be in PATCH 04?

> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 9a7cafc269..8a0cf1a551 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -74,7 +74,7 @@ command TestIfCmd q_obj_TestIfCmd-arg -> None
>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)
>  enum TestIfEnum
>      member foo:
> -    member bar:
> +    member bar: if=defined(TEST_IF_ENUM_BAR)
>      if defined(TEST_IF_ENUM)
>  event TestIfEvent q_obj_TestIfEvent-arg
>     boxed=False
> @@ -228,6 +228,7 @@ object q_obj_EVENT_D-arg
>      member enum3: EnumOne optional=True
>  object q_obj_TestIfCmd-arg
>      member foo: TestIfStruct optional=False
> +    member bar: TestIfEnum optional=False
>      if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)
>  object q_obj_TestIfEvent-arg
>      member foo: TestIfStruct optional=False
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 67c6c1ecef..a86c3b5ee1 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>              print '    member %s:' % m.name,
>              if isinstance(m, QAPISchemaObjectTypeMember):
>                  print '%s optional=%s' % (m.type.name, m.optional),
> +            if m.ifcond:
> +                print 'if=%s' % m.ifcond,
>              print
>  
>      @staticmethod