diff mbox series

[v3,19/50] qapi: add 'if' to enum members

Message ID 20170911110623.24981-20-marcandre.lureau@redhat.com
State New
Headers show
Series Hi, | expand

Commit Message

Marc-André Lureau Sept. 11, 2017, 11:05 a.m. UTC 
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

Comments

Markus Armbruster Dec. 8, 2017, 8:38 a.m. UTC | #1 
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
Marc-André Lureau Jan. 11, 2018, 9:24 p.m. UTC | #2 
Hi

On Fri, Dec 8, 2017 at 9:38 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 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/

ok

>
>> +        (', '.join(diff), ', '.join(allowed_keys)))
>
> I'm afraid this duplicates a part of check_keys().  Could it be factored
> out?

done

>
>> +
>> +
>>  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.

It will *also* get used. Ok, updated commit message.

>
>> @@ -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))
>

sure

>> +        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.

I replaced enum-dict-member by enum-bad-member. I left the int-member
test, because it has also the purpose to check the lexer.

>
>> 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?

It could, but it is not necessary until conditionals are added to
verify introspection generation.

>
>> 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
>
diff mbox series

Patch

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)" %
+        (', '.join(diff), ', '.join(allowed_keys)))
+
+
 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):
@@ -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))
+        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
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' },
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