* [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name
@ 2014-02-20 5:54 Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values Wenchao Xia
` (10 more replies)
0 siblings, 11 replies; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
This series address two issues:
1. support using enum as discriminator in union.
For example, if we have following define in qapi schema:
{ 'enum': 'EnumOne',
'data': [ 'value1', 'value2', 'value3' ] }
{ 'type': 'UserDefBase0',
'data': { 'base-string0': 'str', 'base-enum0': 'EnumOne' } }
Before this series, discriminator in union must be a string, and a
hidden enum type as discriminator is generated. After this series,
qapi schema can directly use predefined enum type:
{ 'union': 'UserDefEnumDiscriminatorUnion',
'base': 'UserDefBase0',
'discriminator' : 'base-enum0',
'data': { 'value1' : 'UserDefA',
'value2' : 'UserDefInherit',
'value3' : 'UserDefB' } }
The benefit is that every thing is defined explicitly in schema file,
the discriminator enum type can be used in other API define in schema,
and a compile time check will be put to verify the correctness according
to enum define. Currently BlockdevOptions used discriminator which can
be converted, in the future other union can also use enum discriminator.
The implement is done by:
1.1 remember the enum defines by qapi scripts.(patch 1)
1.2 use the remembered enum define to check correctness at compile
time.(patch 3), more strict check(patch 2)
1.3 use the same enum name generation rule to avoid C code mismatch,
esp for "case [ENUM_VALUE]" in qapi-visit.c.(patch 4,5)
1.4 switch the code path, when pre-defined enum type is used as discriminator,
don't generate a hidden enum type, use the enum type instead, add
docs/qapi-code-gen.txt.(Patch 6)
1.5 test case shows how it looks like.(Patch 7)
1.6 convert BlockdevOptions. (Patch 8)
2. Better enum name generation
Before this patch, AIOContext->A_I_O_CONTEXT, after this patch,
AIOContet->AIO_CONTEXT. Since previous patch has foldered enum
name generation codes into one function, it is done easily by modifying
it.(Patch 9)
Changes from RFC:
Mainly address Eric's comments: fix typo, add patch 2 to allow partly mapping
enum value in union, add related test case, remove direct inherit support "_base"
and related test case. RFC series at:
http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg00363.html
v2:
General:
3/8: use Raise exception instead of sys.error.write in qapi.py.
Address Eric's comments:
2/8,3/8: more check for enum value at compile time, not allow partly mapping.
8/8: correspond test case change.
v3:
General:
move enum name generation patch to last in the series, add convert patch
8/9.
Address Luiz and Kevin's comments:
Better introduction.
6/9: renamed this patch, add docs/qapi-code-gen.txt part.
v4:
Address Eric's comments:
5/9: better commit message.
6/9: typo fix in doc.
9/9: typo fix, fix indentation, better incode comment.
v5:
Address Eric's comments:
6/10: doc typo fix.
8/10: new patch to remove string discriminator.
9/10: removed the string discriminator test case.
v6:
rebased on upstream by adding "blgdebug" and "blkverify" in qapi-schema.json
in patch 7/10.
v7:
The series is rebased on Markus's tree:
git://repo.or.cz/qemu/armbru.git branch qapi-scripts
Address Markus's comments:
2/11: typo fix in error message.
3/11: new patch to recording addtional line info in schema parsing.
4/11: move the check into qapi.py, report better with fp/line info.
7/11: move the UnionKind adding in qapi.py after 1st parsing and
with better error info.
9/11: move the check into qapi.py with fp/line info, test case
corresond change.
11/11: new patch for errpr path test case.
Wenchao Xia (11):
1 qapi script: remember enum values
2 qapi script: add check for duplicated key
3 qapi-script: remember line number in schema parsing
4 qapi script: check correctness of discriminator values in union
5 qapi script: code move for generate_enum_name()
6 qapi script: use same function to generate enum string
7 qapi script: support pre-defined enum type as discriminator in union
8 qapi: convert BlockdevOptions to use enum discriminator
9 qapi script: do not allow string discriminator
10 qapi script: do not add "_" for every capitalized char in enum
11 qapi test: add error path test for union
docs/qapi-code-gen.txt | 8 +-
include/qapi/qmp/qerror.h | 2 +-
qapi-schema.json | 14 ++-
scripts/qapi-types.py | 36 +++--
scripts/qapi-visit.py | 42 ++++--
scripts/qapi.py | 166 ++++++++++++++++++--
target-i386/cpu.c | 2 +-
tests/Makefile | 8 +-
tests/qapi-schema/comments.out | 2 +-
tests/qapi-schema/duplicate-key.err | 1 +
tests/qapi-schema/duplicate-key.exit | 1 +
tests/qapi-schema/duplicate-key.json | 2 +
tests/qapi-schema/qapi-schema-test.json | 9 +-
tests/qapi-schema/qapi-schema-test.out | 13 +-
tests/qapi-schema/union-enum-value-not-cover.err | 1 +
tests/qapi-schema/union-enum-value-not-cover.exit | 1 +
tests/qapi-schema/union-enum-value-not-cover.json | 16 ++
tests/qapi-schema/union-invalid-base.err | 1 +
tests/qapi-schema/union-invalid-base.exit | 1 +
tests/qapi-schema/union-invalid-base.json | 17 ++
.../union-invalid-discriminator-value.err | 1 +
.../union-invalid-discriminator-value.exit | 1 +
.../union-invalid-discriminator-value.json | 17 ++
tests/qapi-schema/union-invalid-discriminator.err | 1 +
tests/qapi-schema/union-invalid-discriminator.exit | 1 +
tests/qapi-schema/union-invalid-discriminator.json | 17 ++
tests/test-qmp-input-strict.c | 5 +-
tests/test-qmp-input-visitor.c | 10 +-
tests/test-qmp-output-visitor.c | 10 +-
29 files changed, 339 insertions(+), 67 deletions(-)
create mode 100644 tests/qapi-schema/duplicate-key.err
create mode 100644 tests/qapi-schema/duplicate-key.exit
create mode 100644 tests/qapi-schema/duplicate-key.json
create mode 100644 tests/qapi-schema/duplicate-key.out
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.err
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.exit
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.json
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.out
create mode 100644 tests/qapi-schema/union-invalid-base.err
create mode 100644 tests/qapi-schema/union-invalid-base.exit
create mode 100644 tests/qapi-schema/union-invalid-base.json
create mode 100644 tests/qapi-schema/union-invalid-base.out
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.err
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.exit
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.json
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.out
create mode 100644 tests/qapi-schema/union-invalid-discriminator.err
create mode 100644 tests/qapi-schema/union-invalid-discriminator.exit
create mode 100644 tests/qapi-schema/union-invalid-discriminator.json
create mode 100644 tests/qapi-schema/union-invalid-discriminator.out
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 12:05 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 02/11] qapi script: add check for duplicated key Wenchao Xia
` (9 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Later other scripts will need to check the enum values.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi.py | 18 ++++++++++++++----
| 2 +-
tests/qapi-schema/qapi-schema-test.out | 10 +++++-----
3 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f3c2a20..bd81f06 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -169,7 +169,7 @@ def parse_schema(fp):
for expr in schema.exprs:
if expr.has_key('enum'):
- add_enum(expr['enum'])
+ add_enum(expr['enum'], expr['data'])
elif expr.has_key('union'):
add_union(expr)
add_enum('%sKind' % expr['union'])
@@ -289,13 +289,23 @@ def find_union(name):
return union
return None
-def add_enum(name):
+def add_enum(name, enum_values = None):
global enum_types
- enum_types.append(name)
+ enum_types.append({"enum_name": name, "enum_values": enum_values})
+
+def find_enum(name):
+ global enum_types
+ for enum in enum_types:
+ if enum['enum_name'] == name:
+ return enum
+ return None
def is_enum(name):
global enum_types
- return (name in enum_types)
+ for enum in enum_types:
+ if enum['enum_name'] == name:
+ return True
+ return False
def c_type(name):
if name == 'str':
--git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index e3bd904..4ce3dcf 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,3 +1,3 @@
[OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])])]
-['Status']
+[{'enum_name': 'Status', 'enum_values': ['good', 'bad', 'ugly']}]
[]
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 89b53d4..01685d4 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -15,11 +15,11 @@
OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
OrderedDict([('command', 'user_def_cmd3'), ('data', OrderedDict([('a', 'int'), ('*b', 'int')])), ('returns', 'int')]),
OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
-['EnumOne',
- 'UserDefUnionKind',
- 'UserDefFlatUnionKind',
- 'UserDefAnonUnionKind',
- 'UserDefNativeListUnionKind']
+[{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
+ {'enum_name': 'UserDefUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
[OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 02/11] qapi script: add check for duplicated key
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 12:05 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing Wenchao Xia
` (8 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
It is bad that same key was specified twice, especially when a union have
two branches with same condition. This patch can prevent it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
scripts/qapi.py | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index bd81f06..3732fe1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -116,6 +116,8 @@ class QAPISchema:
if self.tok != ':':
raise QAPISchemaError(self, 'Expected ":"')
self.accept()
+ if key in expr:
+ raise QAPISchemaError(self, 'Duplicate key "%s"' % key)
expr[key] = self.get_expr(True)
if self.tok == '}':
self.accept()
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 02/11] qapi script: add check for duplicated key Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 12:22 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union Wenchao Xia
` (7 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Before this patch, 'QAPISchemaError' scans whole input until 'pos'
to get error line number. After this patch, the scan is avoided since
line number is remembered in schema parsing. This patch also benefits
other error report functions, which would be introduced later.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
scripts/qapi.py | 14 ++++++++------
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3732fe1..c504eb4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -39,12 +39,10 @@ class QAPISchemaError(Exception):
def __init__(self, schema, msg):
self.fp = schema.fp
self.msg = msg
- self.line = self.col = 1
- for ch in schema.src[0:schema.pos]:
- if ch == '\n':
- self.line += 1
- self.col = 1
- elif ch == '\t':
+ self.col = 1
+ self.line = schema.line
+ for ch in schema.src[schema.line_pos:schema.pos]:
+ if ch == '\t':
self.col = (self.col + 7) % 8 + 1
else:
self.col += 1
@@ -60,6 +58,8 @@ class QAPISchema:
if self.src == '' or self.src[-1] != '\n':
self.src += '\n'
self.cursor = 0
+ self.line = 1
+ self.line_pos = 0
self.exprs = []
self.accept()
@@ -100,6 +100,8 @@ class QAPISchema:
if self.cursor == len(self.src):
self.tok = None
return
+ self.line += 1
+ self.line_pos = self.cursor
elif not self.tok.isspace():
raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (2 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 14:43 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 05/11] qapi script: code move for generate_enum_name() Wenchao Xia
` (6 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
It will check whether base is set, whether discriminator is found
in base, whether the values specified are written correctly, and
whether all enum values are covered, when discriminator is a
pre-defined enum type. Exprs now have addtional info inside qapi.py
to form better error message.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
scripts/qapi.py | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c504eb4..8af8cfd 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
def __str__(self):
return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
+class QAPIExprError(Exception):
+ def __init__(self, expr_elem, msg):
+ self.fp = expr_elem['fp']
+ self.line = expr_elem['line']
+ self.msg = msg
+
+ def __str__(self):
+ return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
+
class QAPISchema:
def __init__(self, fp):
@@ -64,7 +73,11 @@ class QAPISchema:
self.accept()
while self.tok != None:
- self.exprs.append(self.get_expr(False))
+ line = self.line
+ expr_elem = {'expr': self.get_expr(False),
+ 'fp': fp,
+ 'line': line}
+ self.exprs.append(expr_elem)
def accept(self):
while True:
@@ -162,6 +175,66 @@ class QAPISchema:
raise QAPISchemaError(self, 'Expected "{", "[" or string')
return expr
+# This function can be used to check whether "base" is valid
+def find_base_fields(base):
+ base_struct_define = find_struct(base)
+ if not base_struct_define:
+ return None
+ return base_struct_define.get('data')
+
+# Return the discriminator enum define, if discriminator is specified in
+# @expr_elem["expr"] and it is a pre-defined enum type
+def discriminator_find_enum_define(expr_elem):
+ expr = expr_elem['expr']
+ discriminator = expr.get('discriminator')
+ base = expr.get('base')
+
+ # Only support discriminator when base present
+ if not (discriminator and base):
+ return None
+
+ base_fields = find_base_fields(base)
+
+ if not base_fields:
+ raise QAPIExprError(expr_elem,
+ "Base '%s' is not a valid type"
+ % base)
+
+ discriminator_type = base_fields.get(discriminator)
+
+ if not discriminator_type:
+ raise QAPIExprError(expr_elem,
+ "Discriminator '%s' not found in schema"
+ % discriminator)
+
+ return find_enum(discriminator_type)
+
+def check_union(expr_elem):
+ # If discriminator is specified and it is a pre-defined enum in schema,
+ # check its correctness
+ enum_define = discriminator_find_enum_define(expr_elem)
+ name = expr_elem['expr']['union']
+ members = expr_elem['expr']['data']
+ if enum_define:
+ for key in members:
+ if not key in enum_define['enum_values']:
+ raise QAPIExprError(expr_elem,
+ "Discriminator value '%s' is not found in "
+ "enum '%s'" %
+ (key, enum_define["enum_name"]))
+ for key in enum_define['enum_values']:
+ if not key in members:
+ raise QAPIExprError(expr_elem,
+ "Enum value '%s' is not covered by a "
+ "branch of union '%s'" %
+ (key, name))
+
+def check_exprs(schema):
+ for expr_elem in schema.exprs:
+ expr = expr_elem['expr']
+ if expr.has_key('union'):
+ check_union(expr_elem)
+
def parse_schema(fp):
try:
schema = QAPISchema(fp)
@@ -171,7 +244,8 @@ def parse_schema(fp):
exprs = []
- for expr in schema.exprs:
+ for expr_elem in schema.exprs:
+ expr = expr_elem['expr']
if expr.has_key('enum'):
add_enum(expr['enum'], expr['data'])
elif expr.has_key('union'):
@@ -181,6 +255,12 @@ def parse_schema(fp):
add_struct(expr)
exprs.append(expr)
+ try:
+ check_exprs(schema)
+ except QAPIExprError, e:
+ print >>sys.stderr, e
+ exit(1)
+
return exprs
def parse_args(typeinfo):
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 05/11] qapi script: code move for generate_enum_name()
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (3 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string Wenchao Xia
` (5 subsequent siblings)
10 siblings, 0 replies; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Later both qapi-types.py and qapi-visit.py need a common function
for enum name generation.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-types.py | 10 ----------
scripts/qapi.py | 10 ++++++++++
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2c6e0dc..35ad993 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -127,16 +127,6 @@ const char *%(name)s_lookup[] = {
''')
return ret
-def generate_enum_name(name):
- if name.isupper():
- return c_fun(name, False)
- new_name = ''
- for c in c_fun(name, False):
- if c.isupper():
- new_name += '_'
- new_name += c
- return new_name.lstrip('_').upper()
-
def generate_enum(name, values):
lookup_decl = mcgen('''
extern const char *%(name)s_lookup[];
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8af8cfd..c334ad3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -467,3 +467,13 @@ def guardend(name):
''',
name=guardname(name))
+
+def generate_enum_name(name):
+ if name.isupper():
+ return c_fun(name, False)
+ new_name = ''
+ for c in c_fun(name, False):
+ if c.isupper():
+ new_name += '_'
+ new_name += c
+ return new_name.lstrip('_').upper()
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (4 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 05/11] qapi script: code move for generate_enum_name() Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 15:20 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union Wenchao Xia
` (4 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Prior to this patch, qapi-visit.py used custom code to generate enum
names used for handling a qapi union. Fix it to instead reuse common
code, with identical generated results, and allowing future updates to
generation to only need to touch one place.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-types.py | 6 +++---
scripts/qapi-visit.py | 21 +++++++++++++++------
scripts/qapi.py | 15 +++++++++++----
3 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 35ad993..656a9a0 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -144,11 +144,11 @@ typedef enum %(name)s
i = 0
for value in enum_values:
+ enum_full_value_string = generate_enum_full_value_string(name, value)
enum_decl += mcgen('''
- %(abbrev)s_%(value)s = %(i)d,
+ %(enum_full_value_string)s = %(i)d,
''',
- abbrev=de_camel_case(name).upper(),
- value=generate_enum_name(value),
+ enum_full_value_string=enum_full_value_string,
i=i)
i += 1
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c6de9ae..87e6df7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -214,18 +214,23 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
''',
name=name)
+ # For anon union, always use the default enum type automatically generated
+ # as "'%sKind' % (name)"
+ discriminator_type_name = '%sKind' % (name)
+
for key in members:
assert (members[key] in builtin_types
or find_struct(members[key])
or find_union(members[key])), "Invalid anonymous union member"
+ enum_full_value_string = \
+ generate_enum_full_value_string(discriminator_type_name, key)
ret += mcgen('''
- case %(abbrev)s_KIND_%(enum)s:
+ case %(enum_full_value_string)s:
visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
break;
''',
- abbrev = de_camel_case(name).upper(),
- enum = c_fun(de_camel_case(key),False).upper(),
+ enum_full_value_string = enum_full_value_string,
c_type = type_name(members[key]),
c_name = c_fun(key))
@@ -255,7 +260,10 @@ def generate_visit_union(expr):
assert not base
return generate_visit_anon_union(name, members)
+ # There will always be a discriminator in the C switch code, by default it
+ # is an enum type generated silently as "'%sKind' % (name)"
ret = generate_visit_enum('%sKind' % name, members.keys())
+ discriminator_type_name = '%sKind' % (name)
if base:
base_fields = find_struct(base)['data']
@@ -313,13 +321,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
visit_end_implicit_struct(m, &err);
}'''
+ enum_full_value_string = \
+ generate_enum_full_value_string(discriminator_type_name, key)
ret += mcgen('''
- case %(abbrev)s_KIND_%(enum)s:
+ case %(enum_full_value_string)s:
''' + fmt + '''
break;
''',
- abbrev = de_camel_case(name).upper(),
- enum = c_fun(de_camel_case(key),False).upper(),
+ enum_full_value_string = enum_full_value_string,
c_type=type_name(members[key]),
c_name=c_fun(key))
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c334ad3..130dced 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -468,12 +468,19 @@ def guardend(name):
''',
name=guardname(name))
-def generate_enum_name(name):
- if name.isupper():
- return c_fun(name, False)
+def _generate_enum_value_string(value):
+ if value.isupper():
+ return c_fun(value, False)
new_name = ''
- for c in c_fun(name, False):
+ for c in c_fun(value, False):
if c.isupper():
new_name += '_'
new_name += c
return new_name.lstrip('_').upper()
+
+def generate_enum_full_value_string(enum_name, enum_value):
+ # generate abbrev string
+ abbrev_string = de_camel_case(enum_name).upper()
+ # generate value string
+ value_string = _generate_enum_value_string(enum_value)
+ return "%s_%s" % (abbrev_string, value_string)
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (5 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 16:38 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator Wenchao Xia
` (3 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
By default, any union will automatically generate a enum type as
"[UnionName]Kind" in C code, and it is duplicated when the discriminator
is specified as a pre-defined enum type in schema. After this patch,
the pre-defined enum type will be really used as the switch case
condition in generated C code, if discriminator is an enum field.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
docs/qapi-code-gen.txt | 8 ++++++--
scripts/qapi-types.py | 20 ++++++++++++++++----
scripts/qapi-visit.py | 27 ++++++++++++++++++++-------
scripts/qapi.py | 13 ++++++++++++-
4 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 0728f36..a2e7921 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -123,11 +123,15 @@ And it looks like this on the wire:
Flat union types avoid the nesting on the wire. They are used whenever a
specific field of the base type is declared as the discriminator ('type' is
-then no longer generated). The discriminator must always be a string field.
+then no longer generated). The discriminator can be a string field or a
+predefined enum field. If it is a string field, a hidden enum type will be
+generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
+will be done to verify the correctness. It is recommended to use an enum field.
The above example can then be modified as follows:
+ { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
{ 'type': 'BlockdevCommonOptions',
- 'data': { 'driver': 'str', 'readonly': 'bool' } }
+ 'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
{ 'union': 'BlockdevOptions',
'base': 'BlockdevCommonOptions',
'discriminator': 'driver',
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 656a9a0..4098c60 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -201,14 +201,22 @@ def generate_union(expr):
base = expr.get('base')
discriminator = expr.get('discriminator')
+ expr_elem = {'expr': expr}
+ enum_define = discriminator_find_enum_define(expr_elem)
+ if enum_define:
+ discriminator_type_name = enum_define['enum_name']
+ else:
+ discriminator_type_name = '%sKind' % (name)
+
ret = mcgen('''
struct %(name)s
{
- %(name)sKind kind;
+ %(discriminator_type_name)s kind;
union {
void *data;
''',
- name=name)
+ name=name,
+ discriminator_type_name=discriminator_type_name)
for key in typeinfo:
ret += mcgen('''
@@ -389,8 +397,12 @@ for expr in exprs:
fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
elif expr.has_key('union'):
ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
- ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
- fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
+ expr_elem = {'expr': expr}
+ enum_define = discriminator_find_enum_define(expr_elem)
+ if not enum_define:
+ ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
+ fdef.write(generate_enum_lookup('%sKind' % expr['union'],
+ expr['data'].keys()))
if expr.get('discriminator') == {}:
fdef.write(generate_anon_union_qtypes(expr))
else:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 87e6df7..08685a7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -260,10 +260,17 @@ def generate_visit_union(expr):
assert not base
return generate_visit_anon_union(name, members)
- # There will always be a discriminator in the C switch code, by default it
- # is an enum type generated silently as "'%sKind' % (name)"
- ret = generate_visit_enum('%sKind' % name, members.keys())
- discriminator_type_name = '%sKind' % (name)
+ expr_elem = {'expr': expr}
+ enum_define = discriminator_find_enum_define(expr_elem)
+ if enum_define:
+ # Use the predefined enum type as discriminator
+ ret = ""
+ discriminator_type_name = enum_define['enum_name']
+ else:
+ # There will always be a discriminator in the C switch code, by default it
+ # is an enum type generated silently as "'%sKind' % (name)"
+ ret = generate_visit_enum('%sKind' % name, members.keys())
+ discriminator_type_name = '%sKind' % (name)
if base:
base_fields = find_struct(base)['data']
@@ -303,11 +310,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
else:
desc_type = discriminator
ret += mcgen('''
- visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
+ visit_type_%(discriminator_type_name)s(m, &(*obj)->kind, "%(type)s", &err);
if (!err) {
switch ((*obj)->kind) {
''',
- name=name, type=desc_type)
+ discriminator_type_name=discriminator_type_name,
+ type=desc_type)
for key in members:
if not discriminator:
@@ -519,7 +527,12 @@ for expr in exprs:
ret += generate_visit_list(expr['union'], expr['data'])
fdef.write(ret)
- ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
+ expr_elem = {'expr': expr}
+ enum_define = discriminator_find_enum_define(expr_elem)
+ ret = ""
+ if not enum_define:
+ ret = generate_decl_enum('%sKind' % expr['union'],
+ expr['data'].keys())
ret += generate_declaration(expr['union'], expr['data'])
fdecl.write(ret)
elif expr.has_key('enum'):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 130dced..2a5eb59 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -250,11 +250,22 @@ def parse_schema(fp):
add_enum(expr['enum'], expr['data'])
elif expr.has_key('union'):
add_union(expr)
- add_enum('%sKind' % expr['union'])
elif expr.has_key('type'):
add_struct(expr)
exprs.append(expr)
+ # Try again for hidden UnionKind enum
+ for expr_elem in schema.exprs:
+ expr = expr_elem['expr']
+ if expr.has_key('union'):
+ try:
+ enum_define = discriminator_find_enum_define(expr_elem)
+ except QAPIExprError, e:
+ print >>sys.stderr, e
+ exit(1)
+ if not enum_define:
+ add_enum('%sKind' % expr['union'])
+
try:
check_exprs(schema)
except QAPIExprError, e:
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (6 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 17:59 ` Eric Blake
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator Wenchao Xia
` (2 subsequent siblings)
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
After this patch, hidden enum type BlockdevOptionsKind will not
be generated, and other API can use enum BlockdevDriver.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
qapi-schema.json | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index 7cfb5e5..190d237 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4120,6 +4120,18 @@
'*no-flush': 'bool' } }
##
+# @BlockdevDriver
+#
+# Drivers that are supported in block device operations.
+#
+# Since: 2.0
+##
+{ 'enum': 'BlockdevDriver',
+ 'data': [ 'file', 'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
+ 'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
+ 'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc' ] }
+
+##
# @BlockdevOptionsBase
#
# Options that are available for all block devices, independent of the block
@@ -4143,7 +4155,7 @@
# Since: 1.7
##
{ 'type': 'BlockdevOptionsBase',
- 'data': { 'driver': 'str',
+ 'data': { 'driver': 'BlockdevDriver',
'*id': 'str',
'*node-name': 'str',
'*discard': 'BlockdevDiscardOptions',
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (7 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 16:50 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 11/11] qapi test: add error path test for union Wenchao Xia
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
docs/qapi-code-gen.txt | 8 +++-----
scripts/qapi.py | 6 ++++++
tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
tests/qapi-schema/qapi-schema-test.out | 5 +++--
tests/test-qmp-input-strict.c | 5 ++++-
tests/test-qmp-input-visitor.c | 10 +++++++---
tests/test-qmp-output-visitor.c | 10 ++++++----
7 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index a2e7921..c92add9 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -123,11 +123,9 @@ And it looks like this on the wire:
Flat union types avoid the nesting on the wire. They are used whenever a
specific field of the base type is declared as the discriminator ('type' is
-then no longer generated). The discriminator can be a string field or a
-predefined enum field. If it is a string field, a hidden enum type will be
-generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
-will be done to verify the correctness. It is recommended to use an enum field.
-The above example can then be modified as follows:
+then no longer generated). The discriminator should be a predefined enum field,
+and a compile time check will be done to verify the correctness. The above
+example can then be modified as follows:
{ 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
{ 'type': 'BlockdevCommonOptions',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2a5eb59..c3c118b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -215,6 +215,7 @@ def check_union(expr_elem):
enum_define = discriminator_find_enum_define(expr_elem)
name = expr_elem['expr']['union']
members = expr_elem['expr']['data']
+ discriminator = expr_elem['expr'].get('discriminator')
if enum_define:
for key in members:
if not key in enum_define['enum_values']:
@@ -228,6 +229,11 @@ def check_union(expr_elem):
"Enum value '%s' is not covered by a "
"branch of union '%s'" %
(key, name))
+ elif discriminator:
+ # Do not allow string discriminator
+ raise QAPIExprError(expr_elem,
+ "Discriminator '%s' is not a pre-defined enum "
+ "type" % discriminator)
def check_exprs(schema):
for expr_elem in schema.exprs:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 471ba47..818c06d 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -37,10 +37,13 @@
'base': 'UserDefZero',
'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+{ 'type': 'UserDefUnionBase',
+ 'data': { 'string': 'str', 'enum1': 'EnumOne' } }
+
{ 'union': 'UserDefFlatUnion',
- 'base': 'UserDefOne',
- 'discriminator': 'string',
- 'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+ 'base': 'UserDefUnionBase',
+ 'discriminator': 'enum1',
+ 'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
# FIXME generated struct UserDefFlatUnion has members for direct base
# UserDefOne, but lacks members for indirect base UserDefZero
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 01685d4..6cd03f3 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -7,7 +7,8 @@
OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
- OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'), ('discriminator', 'string'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
@@ -17,7 +18,6 @@
OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
[{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
{'enum_name': 'UserDefUnionKind', 'enum_values': None},
- {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
{'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
{'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
[OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
@@ -27,4 +27,5 @@
OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 09fc1ef..c715a50 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -146,7 +146,10 @@ static void test_validate_union_flat(TestInputVisitorData *data,
Visitor *v;
Error *errp = NULL;
- v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
+ v = validate_test_init(data,
+ "{ 'enum1': 'value1',\
+ 'string': 'str', \
+ 'boolean': true }");
/* TODO when generator bug is fixed, add 'integer': 41 */
visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index f1ab541..b549746 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -310,14 +310,18 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
Error *err = NULL;
UserDefFlatUnion *tmp;
- v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
+ v = visitor_input_test_init(data,
+ "{ 'enum1': 'value1',\
+ 'string': 'str', \
+ 'boolean': true }");
/* TODO when generator bug is fixed, add 'integer': 41 */
visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
g_assert(err == NULL);
- g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
+ g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
+ g_assert_cmpstr(tmp->string, ==, "str");
/* TODO g_assert_cmpint(tmp->integer, ==, 41); */
- g_assert_cmpint(tmp->a->boolean, ==, true);
+ g_assert_cmpint(tmp->value1->boolean, ==, true);
qapi_free_UserDefFlatUnion(tmp);
}
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 5613396..6f4abb7 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -449,10 +449,11 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
Error *err = NULL;
UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
- tmp->kind = USER_DEF_UNION_KIND_A;
- tmp->a = g_malloc0(sizeof(UserDefA));
+ tmp->kind = ENUM_ONE_VALUE1;
+ tmp->string = g_strdup("str");
+ tmp->value1 = g_malloc0(sizeof(UserDefA));
/* TODO when generator bug is fixed: tmp->integer = 41; */
- tmp->a->boolean = true;
+ tmp->value1->boolean = true;
visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
g_assert(err == NULL);
@@ -461,7 +462,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
g_assert(qobject_type(arg) == QTYPE_QDICT);
qdict = qobject_to_qdict(arg);
- g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "a");
+ g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
+ g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
/* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (8 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
2014-02-20 16:54 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 11/11] qapi test: add error path test for union Wenchao Xia
10 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Now "enum AIOContext" will generate AIO_CONTEXT instead of A_I_O_CONTEXT,
"X86CPU" will generate X86_CPU instead of X86_C_P_U.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
include/qapi/qmp/qerror.h | 2 +-
scripts/qapi.py | 26 +++++++++++++++++++-------
target-i386/cpu.c | 2 +-
3 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 73c67b7..78db342 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -159,7 +159,7 @@ void qerror_report_err(Error *err);
ERROR_CLASS_GENERIC_ERROR, "Invalid JSON syntax"
#define QERR_KVM_MISSING_CAP \
- ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
+ ERROR_CLASS_KVM_MISSING_CAP, "Using KVM without %s, %s unavailable"
#define QERR_MIGRATION_ACTIVE \
ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c3c118b..548d559 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -485,19 +485,31 @@ def guardend(name):
''',
name=guardname(name))
-def _generate_enum_value_string(value):
+# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
+# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
+# ENUM24_Name -> ENUM24_NAME
+def _generate_enum_string(value):
+ c_fun_str = c_fun(value, False)
if value.isupper():
- return c_fun(value, False)
+ return c_fun_str
+
new_name = ''
- for c in c_fun(value, False):
- if c.isupper():
- new_name += '_'
+ l = len(c_fun_str)
+ for i in range(l):
+ c = c_fun_str[i]
+ # When c is upper and no "_" appears before, do more checks
+ if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
+ # Case 1: next string is lower
+ # Case 2: previous string is digit
+ if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
+ c_fun_str[i - 1].isdigit():
+ new_name += '_'
new_name += c
return new_name.lstrip('_').upper()
def generate_enum_full_value_string(enum_name, enum_value):
# generate abbrev string
- abbrev_string = de_camel_case(enum_name).upper()
+ abbrev_string = _generate_enum_string(enum_name)
# generate value string
- value_string = _generate_enum_value_string(enum_value)
+ value_string = _generate_enum_string(enum_value)
return "%s_%s" % (abbrev_string, value_string)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..c83ab0f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -315,7 +315,7 @@ typedef struct X86RegisterInfo32 {
} X86RegisterInfo32;
#define REGISTER(reg) \
- [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg }
+ [R_##reg] = { .name = #reg, .qapi_enum = X86_CPU_REGISTER32_##reg }
X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
REGISTER(EAX),
REGISTER(ECX),
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH V7 11/11] qapi test: add error path test for union
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
` (9 preceding siblings ...)
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum Wenchao Xia
@ 2014-02-20 5:54 ` Wenchao Xia
10 siblings, 0 replies; 33+ messages in thread
From: Wenchao Xia @ 2014-02-20 5:54 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, mdroth, armbru, lcapitulino, Wenchao Xia
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/Makefile | 8 +++++---
tests/qapi-schema/duplicate-key.err | 1 +
tests/qapi-schema/duplicate-key.exit | 1 +
tests/qapi-schema/duplicate-key.json | 2 ++
tests/qapi-schema/union-enum-value-not-cover.err | 1 +
tests/qapi-schema/union-enum-value-not-cover.exit | 1 +
tests/qapi-schema/union-enum-value-not-cover.json | 16 ++++++++++++++++
tests/qapi-schema/union-invalid-base.err | 1 +
tests/qapi-schema/union-invalid-base.exit | 1 +
tests/qapi-schema/union-invalid-base.json | 17 +++++++++++++++++
.../union-invalid-discriminator-value.err | 1 +
.../union-invalid-discriminator-value.exit | 1 +
.../union-invalid-discriminator-value.json | 17 +++++++++++++++++
tests/qapi-schema/union-invalid-discriminator.err | 1 +
tests/qapi-schema/union-invalid-discriminator.exit | 1 +
tests/qapi-schema/union-invalid-discriminator.json | 17 +++++++++++++++++
16 files changed, 84 insertions(+), 3 deletions(-)
create mode 100644 tests/qapi-schema/duplicate-key.err
create mode 100644 tests/qapi-schema/duplicate-key.exit
create mode 100644 tests/qapi-schema/duplicate-key.json
create mode 100644 tests/qapi-schema/duplicate-key.out
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.err
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.exit
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.json
create mode 100644 tests/qapi-schema/union-enum-value-not-cover.out
create mode 100644 tests/qapi-schema/union-invalid-base.err
create mode 100644 tests/qapi-schema/union-invalid-base.exit
create mode 100644 tests/qapi-schema/union-invalid-base.json
create mode 100644 tests/qapi-schema/union-invalid-base.out
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.err
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.exit
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.json
create mode 100644 tests/qapi-schema/union-invalid-discriminator-value.out
create mode 100644 tests/qapi-schema/union-invalid-discriminator.err
create mode 100644 tests/qapi-schema/union-invalid-discriminator.exit
create mode 100644 tests/qapi-schema/union-invalid-discriminator.json
create mode 100644 tests/qapi-schema/union-invalid-discriminator.out
diff --git a/tests/Makefile b/tests/Makefile
index 9a7d2f1..9219422 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -119,12 +119,14 @@ check-qtest-xtensa-y += tests/qom-test$(EXESUF)
check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
- comments.json empty.json funny-char.json indented-expr.json \
- missing-colon.json missing-comma-list.json \
+ comments.json duplicate-key.json empty.json funny-char.json \
+ indented-expr.json missing-colon.json missing-comma-list.json \
missing-comma-object.json non-objects.json \
qapi-schema-test.json quoted-structural-chars.json \
trailing-comma-list.json trailing-comma-object.json \
- unclosed-list.json unclosed-object.json unclosed-string.json)
+ unclosed-list.json unclosed-object.json unclosed-string.json \
+ union-invalid-base.json union-invalid-discriminator.json \
+ union-invalid-discriminator-value.json union-enum-value-not-cover.json)
GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
diff --git a/tests/qapi-schema/duplicate-key.err b/tests/qapi-schema/duplicate-key.err
new file mode 100644
index 0000000..0801c6a
--- /dev/null
+++ b/tests/qapi-schema/duplicate-key.err
@@ -0,0 +1 @@
+<stdin>:2:10: Duplicate key "key"
diff --git a/tests/qapi-schema/duplicate-key.exit b/tests/qapi-schema/duplicate-key.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/duplicate-key.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/duplicate-key.json b/tests/qapi-schema/duplicate-key.json
new file mode 100644
index 0000000..1b55d88
--- /dev/null
+++ b/tests/qapi-schema/duplicate-key.json
@@ -0,0 +1,2 @@
+{ 'key': 'value',
+ 'key': 'value' }
diff --git a/tests/qapi-schema/duplicate-key.out b/tests/qapi-schema/duplicate-key.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-enum-value-not-cover.err b/tests/qapi-schema/union-enum-value-not-cover.err
new file mode 100644
index 0000000..a2354b3
--- /dev/null
+++ b/tests/qapi-schema/union-enum-value-not-cover.err
@@ -0,0 +1 @@
+<stdin>:13: Enum value 'value2' is not covered by a branch of union 'TestUnion'
diff --git a/tests/qapi-schema/union-enum-value-not-cover.exit b/tests/qapi-schema/union-enum-value-not-cover.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-enum-value-not-cover.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-enum-value-not-cover.json b/tests/qapi-schema/union-enum-value-not-cover.json
new file mode 100644
index 0000000..c6a8432
--- /dev/null
+++ b/tests/qapi-schema/union-enum-value-not-cover.json
@@ -0,0 +1,16 @@
+{ 'enum': 'TestEnum',
+ 'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+ 'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+ 'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+ 'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+ 'base': 'TestBase',
+ 'discriminator': 'enum1',
+ 'data': { 'value1': 'TestTypeA' } }
diff --git a/tests/qapi-schema/union-enum-value-not-cover.out b/tests/qapi-schema/union-enum-value-not-cover.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
new file mode 100644
index 0000000..cf3229e
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.err
@@ -0,0 +1 @@
+<stdin>:13: Base 'TestBaseWrong' is not a valid type
diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json
new file mode 100644
index 0000000..cffc237
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+ 'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+ 'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+ 'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+ 'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+ 'base': 'TestBaseWrong',
+ 'discriminator': 'enum1',
+ 'data': { 'value1': 'TestTypeA',
+ 'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-invalid-discriminator-value.err b/tests/qapi-schema/union-invalid-discriminator-value.err
new file mode 100644
index 0000000..1125caf
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-discriminator-value.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
diff --git a/tests/qapi-schema/union-invalid-discriminator-value.exit b/tests/qapi-schema/union-invalid-discriminator-value.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-discriminator-value.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-invalid-discriminator-value.json b/tests/qapi-schema/union-invalid-discriminator-value.json
new file mode 100644
index 0000000..a624282
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-discriminator-value.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+ 'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+ 'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+ 'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+ 'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+ 'base': 'TestBase',
+ 'discriminator': 'enum1',
+ 'data': { 'value_wrong': 'TestTypeA',
+ 'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/union-invalid-discriminator-value.out b/tests/qapi-schema/union-invalid-discriminator-value.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-invalid-discriminator.err b/tests/qapi-schema/union-invalid-discriminator.err
new file mode 100644
index 0000000..c5eeda9
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-discriminator.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator 'enum_wrong' not found in schema
diff --git a/tests/qapi-schema/union-invalid-discriminator.exit b/tests/qapi-schema/union-invalid-discriminator.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-discriminator.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-invalid-discriminator.json b/tests/qapi-schema/union-invalid-discriminator.json
new file mode 100644
index 0000000..887157e
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-discriminator.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+ 'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+ 'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+ 'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+ 'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+ 'base': 'TestBase',
+ 'discriminator': 'enum_wrong',
+ 'data': { 'value1': 'TestTypeA',
+ 'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/union-invalid-discriminator.out b/tests/qapi-schema/union-invalid-discriminator.out
new file mode 100644
index 0000000..e69de29
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values Wenchao Xia
@ 2014-02-20 12:05 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 12:05 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Later other scripts will need to check the enum values.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> scripts/qapi.py | 18 ++++++++++++++----
> tests/qapi-schema/comments.out | 2 +-
> tests/qapi-schema/qapi-schema-test.out | 10 +++++-----
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f3c2a20..bd81f06 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -169,7 +169,7 @@ def parse_schema(fp):
>
> for expr in schema.exprs:
> if expr.has_key('enum'):
> - add_enum(expr['enum'])
> + add_enum(expr['enum'], expr['data'])
This is an explicitly defined enum.
> elif expr.has_key('union'):
> add_union(expr)
> add_enum('%sKind' % expr['union'])
elif expr.has_key('type'):
add_struct(expr)
exprs.append(expr)
# Try again for hidden UnionKind enum
for expr_elem in schema.exprs:
expr = expr_elem['expr']
if expr.has_key('union'):
try:
enum_define = discriminator_find_enum_define(expr_elem)
except QAPIExprError, e:
print >>sys.stderr, e
exit(1)
if not enum_define:
add_enum('%sKind' % expr['union'])
This is an implicitly defined enum.
> @@ -289,13 +289,23 @@ def find_union(name):
> return union
> return None
>
> -def add_enum(name):
> +def add_enum(name, enum_values = None):
> global enum_types
> - enum_types.append(name)
> + enum_types.append({"enum_name": name, "enum_values": enum_values})
You remember enum values only for the explicitly defined enums. Let's
see how that works out later in this series. In any case, mentioning it
in the commit message wouldn't hurt :)
> +
> +def find_enum(name):
> + global enum_types
> + for enum in enum_types:
> + if enum['enum_name'] == name:
> + return enum
> + return None
>
> def is_enum(name):
> global enum_types
> - return (name in enum_types)
> + for enum in enum_types:
> + if enum['enum_name'] == name:
> + return True
> + return False
Duplicates find_enum()'s loop. Consider simplifying to
def is_enum(name):
return find_enum(name) != None
>
> def c_type(name):
> if name == 'str':
> diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
> index e3bd904..4ce3dcf 100644
> --- a/tests/qapi-schema/comments.out
> +++ b/tests/qapi-schema/comments.out
> @@ -1,3 +1,3 @@
> [OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])])]
> -['Status']
> +[{'enum_name': 'Status', 'enum_values': ['good', 'bad', 'ugly']}]
> []
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 89b53d4..01685d4 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -15,11 +15,11 @@
> OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
> OrderedDict([('command', 'user_def_cmd3'), ('data', OrderedDict([('a', 'int'), ('*b', 'int')])), ('returns', 'int')]),
> OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
> -['EnumOne',
> - 'UserDefUnionKind',
> - 'UserDefFlatUnionKind',
> - 'UserDefAnonUnionKind',
> - 'UserDefNativeListUnionKind']
> +[{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
Explicitly defined enum with values.
> + {'enum_name': 'UserDefUnionKind', 'enum_values': None},
> + {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
> + {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
> + {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
Four implicitly defined enums without values.
> [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
> OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
> OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 02/11] qapi script: add check for duplicated key
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 02/11] qapi script: add check for duplicated key Wenchao Xia
@ 2014-02-20 12:05 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 12:05 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> It is bad that same key was specified twice, especially when a union have
> two branches with same condition. This patch can prevent it.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> scripts/qapi.py | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index bd81f06..3732fe1 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -116,6 +116,8 @@ class QAPISchema:
> if self.tok != ':':
> raise QAPISchemaError(self, 'Expected ":"')
> self.accept()
> + if key in expr:
> + raise QAPISchemaError(self, 'Duplicate key "%s"' % key)
> expr[key] = self.get_expr(True)
> if self.tok == '}':
> self.accept()
The test for this error is in 11/11. If you need to respin anyway,
consider adding error tests in the same patch as the error they test.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing Wenchao Xia
@ 2014-02-20 12:22 ` Markus Armbruster
2014-02-21 0:10 ` Wenchao Xia
0 siblings, 1 reply; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 12:22 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Before this patch, 'QAPISchemaError' scans whole input until 'pos'
> to get error line number. After this patch, the scan is avoided since
> line number is remembered in schema parsing. This patch also benefits
> other error report functions, which would be introduced later.
Not sure avoiding the scan is worthwhile, but since you coded it
already... no objections.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> scripts/qapi.py | 14 ++++++++------
> 1 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3732fe1..c504eb4 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -39,12 +39,10 @@ class QAPISchemaError(Exception):
> def __init__(self, schema, msg):
> self.fp = schema.fp
> self.msg = msg
> - self.line = self.col = 1
> - for ch in schema.src[0:schema.pos]:
> - if ch == '\n':
> - self.line += 1
> - self.col = 1
> - elif ch == '\t':
> + self.col = 1
> + self.line = schema.line
> + for ch in schema.src[schema.line_pos:schema.pos]:
> + if ch == '\t':
> self.col = (self.col + 7) % 8 + 1
Column computation is wrong. Should be something like
self.col = ((self.col + 7) & ~7) + 1
Not your fault, of course, and you don't have to fix it to get my R-by.
If you want to fix it, separate patch, and please include suitable
tests.
> else:
> self.col += 1
> @@ -60,6 +58,8 @@ class QAPISchema:
> if self.src == '' or self.src[-1] != '\n':
> self.src += '\n'
> self.cursor = 0
> + self.line = 1
> + self.line_pos = 0
> self.exprs = []
> self.accept()
>
> @@ -100,6 +100,8 @@ class QAPISchema:
> if self.cursor == len(self.src):
> self.tok = None
> return
> + self.line += 1
> + self.line_pos = self.cursor
> elif not self.tok.isspace():
> raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union Wenchao Xia
@ 2014-02-20 14:43 ` Markus Armbruster
2014-02-20 15:26 ` Eric Blake
0 siblings, 1 reply; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 14:43 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> It will check whether base is set, whether discriminator is found
> in base, whether the values specified are written correctly, and
> whether all enum values are covered, when discriminator is a
Please delete the comma in case you respin anyway.
> pre-defined enum type. Exprs now have addtional info inside qapi.py
> to form better error message.
This sounds like you're improving error messages. Not true; you're only
adding them. The improvement is compared to v6. I think you could drop
this sentence if you respin anyway.
Here's what we need to check:
If the object has no member 'union', it's not a union, and checking
it is somebody else's problem.
Else:
Member 'data' is an object. The members are discriminator
values, and their valus are types.
If the object has a member 'base', its value must name a complex
type. Call it "the base type".
If the union object has no member 'discriminator', it's an
ordinary union.
Else if the value of member 'discriminator' is {}, it's an
anonymous union.
Else, it's a flat union. The object must have a member 'base'.
The value of member 'discriminator' must name a member of the
base type. If this named member's value names an enum type,
then all members of 'data' must also be members of the enum
type.
I'd add test cases for all these errors first, and then fix qapi.py to
properly diagnose them.
The current qapi.py is lousy at diagnosing semantic errors :(
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> scripts/qapi.py | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c504eb4..8af8cfd 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
> def __str__(self):
> return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
>
> +class QAPIExprError(Exception):
> + def __init__(self, expr_elem, msg):
> + self.fp = expr_elem['fp']
> + self.line = expr_elem['line']
> + self.msg = msg
Entirely separate exception type. I would've tried to reuse
QAPISchemaError. Matter of taste; no need to change anything.
No column information. Can be added later if we care for it.
> +
> + def __str__(self):
> + return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
> +
> class QAPISchema:
>
> def __init__(self, fp):
> @@ -64,7 +73,11 @@ class QAPISchema:
> self.accept()
>
> while self.tok != None:
> - self.exprs.append(self.get_expr(False))
> + line = self.line
> + expr_elem = {'expr': self.get_expr(False),
> + 'fp': fp,
> + 'line': line}
> + self.exprs.append(expr_elem)
Here, you save the location of top-level expressions. More on that
below.
>
> def accept(self):
> while True:
> @@ -162,6 +175,66 @@ class QAPISchema:
> raise QAPISchemaError(self, 'Expected "{", "[" or string')
> return expr
>
> +# This function can be used to check whether "base" is valid
> +def find_base_fields(base):
> + base_struct_define = find_struct(base)
> + if not base_struct_define:
> + return None
> + return base_struct_define.get('data')
Why not base_struct_define['data'] ?
> +
> +# Return the discriminator enum define, if discriminator is specified in
> +# @expr_elem["expr"] and it is a pre-defined enum type
Actually, this function checks the discriminator against the base. Its
caller checks the flat union case
> +def discriminator_find_enum_define(expr_elem):
> + expr = expr_elem['expr']
> + discriminator = expr.get('discriminator')
> + base = expr.get('base')
Why not expr['base'] ?
> +
> + # Only support discriminator when base present
"Support"? Do you mean "recognize"?
> + if not (discriminator and base):
> + return None
> +
> + base_fields = find_base_fields(base)
> +
> + if not base_fields:
> + raise QAPIExprError(expr_elem,
> + "Base '%s' is not a valid type"
> + % base)
The test case for this error is in 11/11. If you need to respin anyway,
consider adding error tests in the same patch as the error they test.
The error is reported with the union's location instead of its base's
location. Best you can do as long as you save positions only for
top-level expressions. Can be improved later if we care.
> +
> + discriminator_type = base_fields.get(discriminator)
> +
> + if not discriminator_type:
> + raise QAPIExprError(expr_elem,
> + "Discriminator '%s' not found in schema"
> + % discriminator)
Suggest something like "Discriminator '%s' is not a member of base type '%s'"
> +
> + return find_enum(discriminator_type)
> +
> +def check_union(expr_elem):
> + # If discriminator is specified and it is a pre-defined enum in schema,
> + # check its correctness
> + enum_define = discriminator_find_enum_define(expr_elem)
> + name = expr_elem['expr']['union']
> + members = expr_elem['expr']['data']
> + if enum_define:
If the discriminator is of enum type:
> + for key in members:
> + if not key in enum_define['enum_values']:
> + raise QAPIExprError(expr_elem,
> + "Discriminator value '%s' is not found in "
> + "enum '%s'" %
> + (key, enum_define["enum_name"]))
Then all keys of the union's member 'data' need to be members of the
discriminator enum type. Good.
> + for key in enum_define['enum_values']:
> + if not key in members:
> + raise QAPIExprError(expr_elem,
> + "Enum value '%s' is not covered by a "
> + "branch of union '%s'" %
> + (key, name))
And every member of the discriminator enum type must also occur as key
of the union's member 'data'. Why?
Consider:
{ 'enum': 'FooEnum', 'data': [ 'plain', 'bells', 'whistles' ] }
{ 'type': 'CommonFooOptions',
'data': { 'type: 'FooType', 'readonly': 'bool' } }
{ 'union': 'FooOptions',
'base': 'CommonFooOptions',
'discriminator': 'type',
'data': { 'bells': 'BellsOptions',
'whistles': 'WhistlesOptions' } }
Type 'plain' doesn't have options beyond CommonFooOptions.
> +
> +def check_exprs(schema):
> + for expr_elem in schema.exprs:
> + expr = expr_elem['expr']
> + if expr.has_key('union'):
> + check_union(expr_elem)
> +
> def parse_schema(fp):
> try:
> schema = QAPISchema(fp)
> @@ -171,7 +244,8 @@ def parse_schema(fp):
>
> exprs = []
>
> - for expr in schema.exprs:
> + for expr_elem in schema.exprs:
> + expr = expr_elem['expr']
> if expr.has_key('enum'):
> add_enum(expr['enum'], expr['data'])
> elif expr.has_key('union'):
> @@ -181,6 +255,12 @@ def parse_schema(fp):
> add_struct(expr)
> exprs.append(expr)
>
> + try:
> + check_exprs(schema)
> + except QAPIExprError, e:
> + print >>sys.stderr, e
> + exit(1)
> +
> return exprs
>
> def parse_args(typeinfo):
I don't think your semantic checks are complete (compare against my
"Here's what we need to check"), but it's an improvement. We can do
"complete" on top.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string Wenchao Xia
@ 2014-02-20 15:20 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 15:20 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Prior to this patch, qapi-visit.py used custom code to generate enum
> names used for handling a qapi union. Fix it to instead reuse common
> code, with identical generated results, and allowing future updates to
> generation to only need to touch one place.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> scripts/qapi-types.py | 6 +++---
> scripts/qapi-visit.py | 21 +++++++++++++++------
> scripts/qapi.py | 15 +++++++++++----
> 3 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 35ad993..656a9a0 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -144,11 +144,11 @@ typedef enum %(name)s
>
> i = 0
> for value in enum_values:
> + enum_full_value_string = generate_enum_full_value_string(name, value)
> enum_decl += mcgen('''
> - %(abbrev)s_%(value)s = %(i)d,
> + %(enum_full_value_string)s = %(i)d,
> ''',
> - abbrev=de_camel_case(name).upper(),
> - value=generate_enum_name(value),
> + enum_full_value_string=enum_full_value_string,
> i=i)
> i += 1
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index c6de9ae..87e6df7 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -214,18 +214,23 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
> ''',
> name=name)
>
> + # For anon union, always use the default enum type automatically generated
> + # as "'%sKind' % (name)"
> + discriminator_type_name = '%sKind' % (name)
> +
> for key in members:
> assert (members[key] in builtin_types
> or find_struct(members[key])
> or find_union(members[key])), "Invalid anonymous union member"
>
> + enum_full_value_string = \
> + generate_enum_full_value_string(discriminator_type_name, key)
> ret += mcgen('''
> - case %(abbrev)s_KIND_%(enum)s:
> + case %(enum_full_value_string)s:
> visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
> break;
> ''',
> - abbrev = de_camel_case(name).upper(),
> - enum = c_fun(de_camel_case(key),False).upper(),
> + enum_full_value_string = enum_full_value_string,
> c_type = type_name(members[key]),
> c_name = c_fun(key))
>
> @@ -255,7 +260,10 @@ def generate_visit_union(expr):
> assert not base
> return generate_visit_anon_union(name, members)
>
> + # There will always be a discriminator in the C switch code, by default it
> + # is an enum type generated silently as "'%sKind' % (name)"
> ret = generate_visit_enum('%sKind' % name, members.keys())
> + discriminator_type_name = '%sKind' % (name)
>
> if base:
> base_fields = find_struct(base)['data']
> @@ -313,13 +321,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
> visit_end_implicit_struct(m, &err);
> }'''
>
> + enum_full_value_string = \
> + generate_enum_full_value_string(discriminator_type_name, key)
Long line due to very long identifiers. disc_type_name or disc_type
would do, I think. Also consider value instead of value_string.
> ret += mcgen('''
> - case %(abbrev)s_KIND_%(enum)s:
> + case %(enum_full_value_string)s:
> ''' + fmt + '''
> break;
> ''',
> - abbrev = de_camel_case(name).upper(),
> - enum = c_fun(de_camel_case(key),False).upper(),
> + enum_full_value_string = enum_full_value_string,
> c_type=type_name(members[key]),
> c_name=c_fun(key))
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c334ad3..130dced 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -468,12 +468,19 @@ def guardend(name):
> ''',
> name=guardname(name))
>
> -def generate_enum_name(name):
> - if name.isupper():
> - return c_fun(name, False)
> +def _generate_enum_value_string(value):
> + if value.isupper():
> + return c_fun(value, False)
> new_name = ''
> - for c in c_fun(name, False):
> + for c in c_fun(value, False):
> if c.isupper():
> new_name += '_'
> new_name += c
> return new_name.lstrip('_').upper()
> +
> +def generate_enum_full_value_string(enum_name, enum_value):
> + # generate abbrev string
> + abbrev_string = de_camel_case(enum_name).upper()
> + # generate value string
> + value_string = _generate_enum_value_string(enum_value)
> + return "%s_%s" % (abbrev_string, value_string)
These comments feel quite superfluous.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union
2014-02-20 14:43 ` Markus Armbruster
@ 2014-02-20 15:26 ` Eric Blake
2014-02-21 8:21 ` Markus Armbruster
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2014-02-20 15:26 UTC (permalink / raw)
To: Markus Armbruster, Wenchao Xia; +Cc: kwolf, mdroth, qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]
On 02/20/2014 07:43 AM, Markus Armbruster wrote:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> It will check whether base is set, whether discriminator is found
>> in base, whether the values specified are written correctly, and
>> whether all enum values are covered, when discriminator is a
>
> And every member of the discriminator enum type must also occur as key
> of the union's member 'data'. Why?
>
> Consider:
>
> { 'enum': 'FooEnum', 'data': [ 'plain', 'bells', 'whistles' ] }
>
> { 'type': 'CommonFooOptions',
> 'data': { 'type: 'FooType', 'readonly': 'bool' } }
> { 'union': 'FooOptions',
> 'base': 'CommonFooOptions',
> 'discriminator': 'type',
> 'data': { 'bells': 'BellsOptions',
> 'whistles': 'WhistlesOptions' } }
>
> Type 'plain' doesn't have options beyond CommonFooOptions.
I'd still rather make it explicit that we KNOW that this branch of the
union has no additional options:
{ 'union': 'FooOptions',
'base': 'CommonFooOptions',
'discriminator': 'type',
'data': { 'plain': {},
'bells': 'BellsOptions',
'whistles': 'WhistlesOptions' } }
to show that we explicitly thought about all the cases. We don't
currently have any such unions with an empty branch, but it would be
worth documenting in the qapi text and explicitly testing that it works
if we intend to support this.
--
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] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union Wenchao Xia
@ 2014-02-20 16:38 ` Markus Armbruster
2014-02-21 0:17 ` Wenchao Xia
0 siblings, 1 reply; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 16:38 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> By default, any union will automatically generate a enum type as
> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
> is specified as a pre-defined enum type in schema. After this patch,
> the pre-defined enum type will be really used as the switch case
> condition in generated C code, if discriminator is an enum field.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> docs/qapi-code-gen.txt | 8 ++++++--
> scripts/qapi-types.py | 20 ++++++++++++++++----
> scripts/qapi-visit.py | 27 ++++++++++++++++++++-------
> scripts/qapi.py | 13 ++++++++++++-
> 4 files changed, 54 insertions(+), 14 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 0728f36..a2e7921 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -123,11 +123,15 @@ And it looks like this on the wire:
>
> Flat union types avoid the nesting on the wire. They are used whenever a
> specific field of the base type is declared as the discriminator ('type' is
> -then no longer generated). The discriminator must always be a string field.
> +then no longer generated). The discriminator can be a string field or a
> +predefined enum field. If it is a string field, a hidden enum type will be
> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
> +will be done to verify the correctness. It is recommended to use an enum field.
> The above example can then be modified as follows:
>
> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
> { 'type': 'BlockdevCommonOptions',
> - 'data': { 'driver': 'str', 'readonly': 'bool' } }
> + 'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
> { 'union': 'BlockdevOptions',
> 'base': 'BlockdevCommonOptions',
> 'discriminator': 'driver',
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 656a9a0..4098c60 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -201,14 +201,22 @@ def generate_union(expr):
> base = expr.get('base')
> discriminator = expr.get('discriminator')
>
> + expr_elem = {'expr': expr}
> + enum_define = discriminator_find_enum_define(expr_elem)
expr_elem has no fp, line. What if discriminator_find_enum_define
throws a QAPIExprError?
More of the same below.
> + if enum_define:
> + discriminator_type_name = enum_define['enum_name']
> + else:
> + discriminator_type_name = '%sKind' % (name)
> +
> ret = mcgen('''
> struct %(name)s
> {
> - %(name)sKind kind;
> + %(discriminator_type_name)s kind;
> union {
> void *data;
> ''',
> - name=name)
> + name=name,
> + discriminator_type_name=discriminator_type_name)
>
> for key in typeinfo:
> ret += mcgen('''
> @@ -389,8 +397,12 @@ for expr in exprs:
> fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
> elif expr.has_key('union'):
> ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
> - ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
> - fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
> + expr_elem = {'expr': expr}
> + enum_define = discriminator_find_enum_define(expr_elem)
> + if not enum_define:
> + ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
> + fdef.write(generate_enum_lookup('%sKind' % expr['union'],
> + expr['data'].keys()))
Generate the implicit enum only when we don't have an explicit enum
discriminator. Good.
> if expr.get('discriminator') == {}:
> fdef.write(generate_anon_union_qtypes(expr))
> else:
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 87e6df7..08685a7 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -260,10 +260,17 @@ def generate_visit_union(expr):
> assert not base
> return generate_visit_anon_union(name, members)
>
> - # There will always be a discriminator in the C switch code, by default it
> - # is an enum type generated silently as "'%sKind' % (name)"
> - ret = generate_visit_enum('%sKind' % name, members.keys())
> - discriminator_type_name = '%sKind' % (name)
> + expr_elem = {'expr': expr}
> + enum_define = discriminator_find_enum_define(expr_elem)
> + if enum_define:
> + # Use the predefined enum type as discriminator
> + ret = ""
> + discriminator_type_name = enum_define['enum_name']
> + else:
> + # There will always be a discriminator in the C switch code, by default it
> + # is an enum type generated silently as "'%sKind' % (name)"
> + ret = generate_visit_enum('%sKind' % name, members.keys())
> + discriminator_type_name = '%sKind' % (name)
Generate the visit of the discriminator only when we don't have an
explicit enum discriminator (which gets visited elsewhere already).
Good.
>
> if base:
> base_fields = find_struct(base)['data']
> @@ -303,11 +310,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
> else:
> desc_type = discriminator
> ret += mcgen('''
> - visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
> + visit_type_%(discriminator_type_name)s(m, &(*obj)->kind, "%(type)s", &err);
Another long line due to very long identifier.
> if (!err) {
> switch ((*obj)->kind) {
> ''',
> - name=name, type=desc_type)
> + discriminator_type_name=discriminator_type_name,
> + type=desc_type)
>
> for key in members:
> if not discriminator:
> @@ -519,7 +527,12 @@ for expr in exprs:
> ret += generate_visit_list(expr['union'], expr['data'])
> fdef.write(ret)
>
> - ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
> + expr_elem = {'expr': expr}
> + enum_define = discriminator_find_enum_define(expr_elem)
> + ret = ""
> + if not enum_define:
> + ret = generate_decl_enum('%sKind' % expr['union'],
> + expr['data'].keys())
Generate the visitor for the implicit enum only when we don't have an
explicit enum discriminator (which has its own visitor already). Good.
> ret += generate_declaration(expr['union'], expr['data'])
> fdecl.write(ret)
> elif expr.has_key('enum'):
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 130dced..2a5eb59 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -250,11 +250,22 @@ def parse_schema(fp):
> add_enum(expr['enum'], expr['data'])
> elif expr.has_key('union'):
> add_union(expr)
> - add_enum('%sKind' % expr['union'])
> elif expr.has_key('type'):
> add_struct(expr)
> exprs.append(expr)
>
> + # Try again for hidden UnionKind enum
> + for expr_elem in schema.exprs:
> + expr = expr_elem['expr']
> + if expr.has_key('union'):
> + try:
> + enum_define = discriminator_find_enum_define(expr_elem)
> + except QAPIExprError, e:
> + print >>sys.stderr, e
> + exit(1)
> + if not enum_define:
> + add_enum('%sKind' % expr['union'])
> +
> try:
> check_exprs(schema)
> except QAPIExprError, e:
I guess you move this into its own loop because when base types are used
before they're defined, or an enum type is used for a discriminator
before it's defined, then discriminator_find_enum_define() complains.
Correct?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator Wenchao Xia
@ 2014-02-20 16:50 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 16:50 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> docs/qapi-code-gen.txt | 8 +++-----
> scripts/qapi.py | 6 ++++++
> tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
> tests/qapi-schema/qapi-schema-test.out | 5 +++--
> tests/test-qmp-input-strict.c | 5 ++++-
> tests/test-qmp-input-visitor.c | 10 +++++++---
> tests/test-qmp-output-visitor.c | 10 ++++++----
> 7 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index a2e7921..c92add9 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -123,11 +123,9 @@ And it looks like this on the wire:
>
> Flat union types avoid the nesting on the wire. They are used whenever a
> specific field of the base type is declared as the discriminator ('type' is
> -then no longer generated). The discriminator can be a string field or a
> -predefined enum field. If it is a string field, a hidden enum type will be
> -generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
> -will be done to verify the correctness. It is recommended to use an enum field.
> -The above example can then be modified as follows:
> +then no longer generated). The discriminator should be a predefined enum field,
> +and a compile time check will be done to verify the correctness. The above
> +example can then be modified as follows:
Suggest:
The discriminator must be of enumeration type. The above example...
>
> { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
> { 'type': 'BlockdevCommonOptions',
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2a5eb59..c3c118b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -215,6 +215,7 @@ def check_union(expr_elem):
> enum_define = discriminator_find_enum_define(expr_elem)
> name = expr_elem['expr']['union']
> members = expr_elem['expr']['data']
> + discriminator = expr_elem['expr'].get('discriminator')
> if enum_define:
> for key in members:
> if not key in enum_define['enum_values']:
> @@ -228,6 +229,11 @@ def check_union(expr_elem):
> "Enum value '%s' is not covered by a "
> "branch of union '%s'" %
> (key, name))
> + elif discriminator:
> + # Do not allow string discriminator
> + raise QAPIExprError(expr_elem,
> + "Discriminator '%s' is not a pre-defined enum "
> + "type" % discriminator)
I have no idea what "pre-defined" means here :)
Suggest:
"Discriminator '%s' must be of enumeration type"
>
> def check_exprs(schema):
> for expr_elem in schema.exprs:
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 471ba47..818c06d 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -37,10 +37,13 @@
> 'base': 'UserDefZero',
> 'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
>
> +{ 'type': 'UserDefUnionBase',
> + 'data': { 'string': 'str', 'enum1': 'EnumOne' } }
> +
> { 'union': 'UserDefFlatUnion',
> - 'base': 'UserDefOne',
> - 'discriminator': 'string',
> - 'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
> + 'base': 'UserDefUnionBase',
> + 'discriminator': 'enum1',
> + 'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
> # FIXME generated struct UserDefFlatUnion has members for direct base
> # UserDefOne, but lacks members for indirect base UserDefZero
>
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 01685d4..6cd03f3 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -7,7 +7,8 @@
> OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
> OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
> OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
> - OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'), ('discriminator', 'string'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
> + OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
> + OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
> OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
> OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
> OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
> @@ -17,7 +18,6 @@
> OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
> [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
> {'enum_name': 'UserDefUnionKind', 'enum_values': None},
> - {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
> {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
> {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
> [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
> @@ -27,4 +27,5 @@
> OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
> OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
> OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
> + OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
> OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index 09fc1ef..c715a50 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -146,7 +146,10 @@ static void test_validate_union_flat(TestInputVisitorData *data,
> Visitor *v;
> Error *errp = NULL;
>
> - v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
> + v = validate_test_init(data,
> + "{ 'enum1': 'value1',\
> + 'string': 'str', \
> + 'boolean': true }");
> /* TODO when generator bug is fixed, add 'integer': 41 */
>
Please use string concatenation rather than line continuation to avoid
long lines, like this:
v = validate_test_init(data,
"{ 'enum1': 'value1', "
"'string': 'str', "
"'boolean': true }");
The existing tests don't bother to avoid long lines, but that's not your
fault.
> visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index f1ab541..b549746 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -310,14 +310,18 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
> Error *err = NULL;
> UserDefFlatUnion *tmp;
>
> - v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
> + v = visitor_input_test_init(data,
> + "{ 'enum1': 'value1',\
> + 'string': 'str', \
> + 'boolean': true }");
Likewise.
> /* TODO when generator bug is fixed, add 'integer': 41 */
>
> visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
> g_assert(err == NULL);
> - g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
> + g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
> + g_assert_cmpstr(tmp->string, ==, "str");
> /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
> - g_assert_cmpint(tmp->a->boolean, ==, true);
> + g_assert_cmpint(tmp->value1->boolean, ==, true);
> qapi_free_UserDefFlatUnion(tmp);
> }
>
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 5613396..6f4abb7 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -449,10 +449,11 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
> Error *err = NULL;
>
> UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
> - tmp->kind = USER_DEF_UNION_KIND_A;
> - tmp->a = g_malloc0(sizeof(UserDefA));
> + tmp->kind = ENUM_ONE_VALUE1;
> + tmp->string = g_strdup("str");
> + tmp->value1 = g_malloc0(sizeof(UserDefA));
> /* TODO when generator bug is fixed: tmp->integer = 41; */
> - tmp->a->boolean = true;
> + tmp->value1->boolean = true;
>
> visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
> g_assert(err == NULL);
> @@ -461,7 +462,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
> g_assert(qobject_type(arg) == QTYPE_QDICT);
> qdict = qobject_to_qdict(arg);
>
> - g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "a");
> + g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
> + g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
> /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
> g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum Wenchao Xia
@ 2014-02-20 16:54 ` Markus Armbruster
2014-02-20 17:53 ` Eric Blake
0 siblings, 1 reply; 33+ messages in thread
From: Markus Armbruster @ 2014-02-20 16:54 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Now "enum AIOContext" will generate AIO_CONTEXT instead of A_I_O_CONTEXT,
> "X86CPU" will generate X86_CPU instead of X86_C_P_U.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> include/qapi/qmp/qerror.h | 2 +-
> scripts/qapi.py | 26 +++++++++++++++++++-------
> target-i386/cpu.c | 2 +-
> 3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 73c67b7..78db342 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -159,7 +159,7 @@ void qerror_report_err(Error *err);
> ERROR_CLASS_GENERIC_ERROR, "Invalid JSON syntax"
>
> #define QERR_KVM_MISSING_CAP \
> - ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
> + ERROR_CLASS_KVM_MISSING_CAP, "Using KVM without %s, %s unavailable"
>
> #define QERR_MIGRATION_ACTIVE \
> ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c3c118b..548d559 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -485,19 +485,31 @@ def guardend(name):
> ''',
> name=guardname(name))
>
> -def _generate_enum_value_string(value):
> +# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
> +# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
> +# ENUM24_Name -> ENUM24_NAME
> +def _generate_enum_string(value):
> + c_fun_str = c_fun(value, False)
> if value.isupper():
> - return c_fun(value, False)
> + return c_fun_str
> +
> new_name = ''
> - for c in c_fun(value, False):
> - if c.isupper():
> - new_name += '_'
> + l = len(c_fun_str)
> + for i in range(l):
> + c = c_fun_str[i]
> + # When c is upper and no "_" appears before, do more checks
> + if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
c_fun_str[i - 1]... what if i == 0?
> + # Case 1: next string is lower
> + # Case 2: previous string is digit
> + if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
> + c_fun_str[i - 1].isdigit():
> + new_name += '_'
> new_name += c
> return new_name.lstrip('_').upper()
>
> def generate_enum_full_value_string(enum_name, enum_value):
> # generate abbrev string
> - abbrev_string = de_camel_case(enum_name).upper()
> + abbrev_string = _generate_enum_string(enum_name)
> # generate value string
> - value_string = _generate_enum_value_string(enum_value)
> + value_string = _generate_enum_string(enum_value)
> return "%s_%s" % (abbrev_string, value_string)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0e8812a..c83ab0f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -315,7 +315,7 @@ typedef struct X86RegisterInfo32 {
> } X86RegisterInfo32;
>
> #define REGISTER(reg) \
> - [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg }
> + [R_##reg] = { .name = #reg, .qapi_enum = X86_CPU_REGISTER32_##reg }
> X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
> REGISTER(EAX),
> REGISTER(ECX),
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum
2014-02-20 16:54 ` Markus Armbruster
@ 2014-02-20 17:53 ` Eric Blake
2014-02-21 8:21 ` Markus Armbruster
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2014-02-20 17:53 UTC (permalink / raw)
To: Markus Armbruster, Wenchao Xia; +Cc: kwolf, mdroth, qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 410 bytes --]
On 02/20/2014 09:54 AM, Markus Armbruster wrote:
>> + # When c is upper and no "_" appears before, do more checks
>> + if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
>
> c_fun_str[i - 1]... what if i == 0?
How? We already had '(i > 0) and' prior to the use of i-1.
--
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] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator Wenchao Xia
@ 2014-02-20 17:59 ` Eric Blake
0 siblings, 0 replies; 33+ messages in thread
From: Eric Blake @ 2014-02-20 17:59 UTC (permalink / raw)
To: Wenchao Xia, qemu-devel; +Cc: mdroth, kwolf, armbru, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 496 bytes --]
On 02/19/2014 10:54 PM, Wenchao Xia wrote:
> After this patch, hidden enum type BlockdevOptionsKind will not
> be generated, and other API can use enum BlockdevDriver.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> qapi-schema.json | 14 +++++++++++++-
> 1 files changed, 13 insertions(+), 1 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
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] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing
2014-02-20 12:22 ` Markus Armbruster
@ 2014-02-21 0:10 ` Wenchao Xia
2014-02-21 13:04 ` Markus Armbruster
0 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-21 0:10 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
于 2014/2/20 20:22, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> Before this patch, 'QAPISchemaError' scans whole input until 'pos'
>> to get error line number. After this patch, the scan is avoided since
>> line number is remembered in schema parsing. This patch also benefits
>> other error report functions, which would be introduced later.
>
> Not sure avoiding the scan is worthwhile, but since you coded it
> already... no objections.
>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> scripts/qapi.py | 14 ++++++++------
>> 1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 3732fe1..c504eb4 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -39,12 +39,10 @@ class QAPISchemaError(Exception):
>> def __init__(self, schema, msg):
>> self.fp = schema.fp
>> self.msg = msg
>> - self.line = self.col = 1
>> - for ch in schema.src[0:schema.pos]:
>> - if ch == '\n':
>> - self.line += 1
>> - self.col = 1
>> - elif ch == '\t':
>> + self.col = 1
>> + self.line = schema.line
>> + for ch in schema.src[schema.line_pos:schema.pos]:
>> + if ch == '\t':
>> self.col = (self.col + 7) % 8 + 1
>
> Column computation is wrong. Should be something like
>
> self.col = ((self.col + 7) & ~7) + 1
>
> Not your fault, of course, and you don't have to fix it to get my R-by.
> If you want to fix it, separate patch, and please include suitable
> tests.
>
Thanks for your quick review, I'll respin it next week.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union
2014-02-20 16:38 ` Markus Armbruster
@ 2014-02-21 0:17 ` Wenchao Xia
2014-02-21 8:13 ` Markus Armbruster
0 siblings, 1 reply; 33+ messages in thread
From: Wenchao Xia @ 2014-02-21 0:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, lcapitulino, qemu-devel, mdroth
于 2014/2/21 0:38, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> By default, any union will automatically generate a enum type as
>> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
>> is specified as a pre-defined enum type in schema. After this patch,
>> the pre-defined enum type will be really used as the switch case
>> condition in generated C code, if discriminator is an enum field.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> docs/qapi-code-gen.txt | 8 ++++++--
>> scripts/qapi-types.py | 20 ++++++++++++++++----
>> scripts/qapi-visit.py | 27 ++++++++++++++++++++-------
>> scripts/qapi.py | 13 ++++++++++++-
>> 4 files changed, 54 insertions(+), 14 deletions(-)
>>
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index 0728f36..a2e7921 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -123,11 +123,15 @@ And it looks like this on the wire:
>>
>> Flat union types avoid the nesting on the wire. They are used whenever a
>> specific field of the base type is declared as the discriminator ('type' is
>> -then no longer generated). The discriminator must always be a string field.
>> +then no longer generated). The discriminator can be a string field or a
>> +predefined enum field. If it is a string field, a hidden enum type will be
>> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
>> +will be done to verify the correctness. It is recommended to use an enum field.
>> The above example can then be modified as follows:
>>
>> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>> { 'type': 'BlockdevCommonOptions',
>> - 'data': { 'driver': 'str', 'readonly': 'bool' } }
>> + 'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>> { 'union': 'BlockdevOptions',
>> 'base': 'BlockdevCommonOptions',
>> 'discriminator': 'driver',
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 656a9a0..4098c60 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -201,14 +201,22 @@ def generate_union(expr):
>> base = expr.get('base')
>> discriminator = expr.get('discriminator')
>>
>> + expr_elem = {'expr': expr}
>> + enum_define = discriminator_find_enum_define(expr_elem)
>
> expr_elem has no fp, line. What if discriminator_find_enum_define
> throws a QAPIExprError?
>
It shouldn't happen, since all error check happens in parse_schema().
> More of the same below.
>
>> + if enum_define:
>> + discriminator_type_name = enum_define['enum_name']
>> + else:
>> + discriminator_type_name = '%sKind' % (name)
>> +
>> ret = mcgen('''
>> struct %(name)s
>> {
>> - %(name)sKind kind;
>> + %(discriminator_type_name)s kind;
>> union {
>> void *data;
>> ''',
>> - name=name)
>> + name=name,
>> + discriminator_type_name=discriminator_type_name)
>>
>> for key in typeinfo:
>> ret += mcgen('''
>> @@ -389,8 +397,12 @@ for expr in exprs:
>> fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
>> elif expr.has_key('union'):
>> ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
>> - ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
>> - fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
>> + expr_elem = {'expr': expr}
>> + enum_define = discriminator_find_enum_define(expr_elem)
>> + if not enum_define:
>> + ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
>> + fdef.write(generate_enum_lookup('%sKind' % expr['union'],
>> + expr['data'].keys()))
>
> Generate the implicit enum only when we don't have an explicit enum
> discriminator. Good.
>
>> if expr.get('discriminator') == {}:
>> fdef.write(generate_anon_union_qtypes(expr))
>> else:
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 87e6df7..08685a7 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -260,10 +260,17 @@ def generate_visit_union(expr):
>> assert not base
>> return generate_visit_anon_union(name, members)
>>
>> - # There will always be a discriminator in the C switch code, by default it
>> - # is an enum type generated silently as "'%sKind' % (name)"
>> - ret = generate_visit_enum('%sKind' % name, members.keys())
>> - discriminator_type_name = '%sKind' % (name)
>> + expr_elem = {'expr': expr}
>> + enum_define = discriminator_find_enum_define(expr_elem)
>> + if enum_define:
>> + # Use the predefined enum type as discriminator
>> + ret = ""
>> + discriminator_type_name = enum_define['enum_name']
>> + else:
>> + # There will always be a discriminator in the C switch code, by default it
>> + # is an enum type generated silently as "'%sKind' % (name)"
>> + ret = generate_visit_enum('%sKind' % name, members.keys())
>> + discriminator_type_name = '%sKind' % (name)
>
> Generate the visit of the discriminator only when we don't have an
> explicit enum discriminator (which gets visited elsewhere already).
> Good.
>
>>
>> if base:
>> base_fields = find_struct(base)['data']
>> @@ -303,11 +310,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>> else:
>> desc_type = discriminator
>> ret += mcgen('''
>> - visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
>> + visit_type_%(discriminator_type_name)s(m, &(*obj)->kind, "%(type)s", &err);
>
> Another long line due to very long identifier.
>
Will improve.
>> if (!err) {
>> switch ((*obj)->kind) {
>> ''',
>> - name=name, type=desc_type)
>> + discriminator_type_name=discriminator_type_name,
>> + type=desc_type)
>>
>> for key in members:
>> if not discriminator:
>> @@ -519,7 +527,12 @@ for expr in exprs:
>> ret += generate_visit_list(expr['union'], expr['data'])
>> fdef.write(ret)
>>
>> - ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
>> + expr_elem = {'expr': expr}
>> + enum_define = discriminator_find_enum_define(expr_elem)
>> + ret = ""
>> + if not enum_define:
>> + ret = generate_decl_enum('%sKind' % expr['union'],
>> + expr['data'].keys())
>
> Generate the visitor for the implicit enum only when we don't have an
> explicit enum discriminator (which has its own visitor already). Good.
>
>> ret += generate_declaration(expr['union'], expr['data'])
>> fdecl.write(ret)
>> elif expr.has_key('enum'):
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 130dced..2a5eb59 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -250,11 +250,22 @@ def parse_schema(fp):
>> add_enum(expr['enum'], expr['data'])
>> elif expr.has_key('union'):
>> add_union(expr)
>> - add_enum('%sKind' % expr['union'])
>> elif expr.has_key('type'):
>> add_struct(expr)
>> exprs.append(expr)
>>
>> + # Try again for hidden UnionKind enum
>> + for expr_elem in schema.exprs:
>> + expr = expr_elem['expr']
>> + if expr.has_key('union'):
>> + try:
>> + enum_define = discriminator_find_enum_define(expr_elem)
>> + except QAPIExprError, e:
>> + print >>sys.stderr, e
>> + exit(1)
>> + if not enum_define:
>> + add_enum('%sKind' % expr['union'])
>> +
>> try:
>> check_exprs(schema)
>> except QAPIExprError, e:
>
> I guess you move this into its own loop because when base types are used
> before they're defined, or an enum type is used for a discriminator
> before it's defined, then discriminator_find_enum_define() complains.
> Correct?
>
Exactly, which allow enum define after usage in schema.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union
2014-02-21 0:17 ` Wenchao Xia
@ 2014-02-21 8:13 ` Markus Armbruster
2014-02-21 13:56 ` Eric Blake
0 siblings, 1 reply; 33+ messages in thread
From: Markus Armbruster @ 2014-02-21 8:13 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, mdroth, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2014/2/21 0:38, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> By default, any union will automatically generate a enum type as
>>> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
>>> is specified as a pre-defined enum type in schema. After this patch,
>>> the pre-defined enum type will be really used as the switch case
>>> condition in generated C code, if discriminator is an enum field.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>> docs/qapi-code-gen.txt | 8 ++++++--
>>> scripts/qapi-types.py | 20 ++++++++++++++++----
>>> scripts/qapi-visit.py | 27 ++++++++++++++++++++-------
>>> scripts/qapi.py | 13 ++++++++++++-
>>> 4 files changed, 54 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>>> index 0728f36..a2e7921 100644
>>> --- a/docs/qapi-code-gen.txt
>>> +++ b/docs/qapi-code-gen.txt
>>> @@ -123,11 +123,15 @@ And it looks like this on the wire:
>>>
>>> Flat union types avoid the nesting on the wire. They are used whenever a
>>> specific field of the base type is declared as the discriminator ('type' is
>>> -then no longer generated). The discriminator must always be a string field.
>>> +then no longer generated). The discriminator can be a string field or a
>>> +predefined enum field. If it is a string field, a hidden enum type will be
>>> +generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
>>> +will be done to verify the correctness. It is recommended to use an enum field.
>>> The above example can then be modified as follows:
>>>
>>> + { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>>> { 'type': 'BlockdevCommonOptions',
>>> - 'data': { 'driver': 'str', 'readonly': 'bool' } }
>>> + 'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
>>> { 'union': 'BlockdevOptions',
>>> 'base': 'BlockdevCommonOptions',
>>> 'discriminator': 'driver',
>>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>>> index 656a9a0..4098c60 100644
>>> --- a/scripts/qapi-types.py
>>> +++ b/scripts/qapi-types.py
>>> @@ -201,14 +201,22 @@ def generate_union(expr):
>>> base = expr.get('base')
>>> discriminator = expr.get('discriminator')
>>>
>>> + expr_elem = {'expr': expr}
>>> + enum_define = discriminator_find_enum_define(expr_elem)
>>
>> expr_elem has no fp, line. What if discriminator_find_enum_define
>> throws a QAPIExprError?
>>
> It shouldn't happen, since all error check happens in parse_schema().
Impossible errors often mean the abstractions aren't quite right. But
this series is progress, and I don't want to delay it by demanding
perfection. We can improve on it in tree if we want.
>> More of the same below.
[...]
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 130dced..2a5eb59 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -250,11 +250,22 @@ def parse_schema(fp):
>>> add_enum(expr['enum'], expr['data'])
>>> elif expr.has_key('union'):
>>> add_union(expr)
>>> - add_enum('%sKind' % expr['union'])
>>> elif expr.has_key('type'):
>>> add_struct(expr)
>>> exprs.append(expr)
>>>
>>> + # Try again for hidden UnionKind enum
>>> + for expr_elem in schema.exprs:
>>> + expr = expr_elem['expr']
>>> + if expr.has_key('union'):
>>> + try:
>>> + enum_define = discriminator_find_enum_define(expr_elem)
>>> + except QAPIExprError, e:
>>> + print >>sys.stderr, e
>>> + exit(1)
>>> + if not enum_define:
>>> + add_enum('%sKind' % expr['union'])
>>> +
>>> try:
>>> check_exprs(schema)
>>> except QAPIExprError, e:
>>
>> I guess you move this into its own loop because when base types are used
>> before they're defined, or an enum type is used for a discriminator
>> before it's defined, then discriminator_find_enum_define() complains.
>> Correct?
>>
> Exactly, which allow enum define after usage in schema.
Do we want to (have to?) support "use before define" in schemas? Eric,
what do you think?
If yes, we should add suitable tests. Outside the scope of this series.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union
2014-02-20 15:26 ` Eric Blake
@ 2014-02-21 8:21 ` Markus Armbruster
2014-02-21 13:49 ` Eric Blake
0 siblings, 1 reply; 33+ messages in thread
From: Markus Armbruster @ 2014-02-21 8:21 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, lcapitulino, Wenchao Xia, mdroth
Eric Blake <eblake@redhat.com> writes:
> On 02/20/2014 07:43 AM, Markus Armbruster wrote:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> It will check whether base is set, whether discriminator is found
>>> in base, whether the values specified are written correctly, and
>>> whether all enum values are covered, when discriminator is a
>
>>
>> And every member of the discriminator enum type must also occur as key
>> of the union's member 'data'. Why?
>>
>> Consider:
>>
>> { 'enum': 'FooEnum', 'data': [ 'plain', 'bells', 'whistles' ] }
>>
>> { 'type': 'CommonFooOptions',
>> 'data': { 'type: 'FooType', 'readonly': 'bool' } }
>> { 'union': 'FooOptions',
>> 'base': 'CommonFooOptions',
>> 'discriminator': 'type',
>> 'data': { 'bells': 'BellsOptions',
>> 'whistles': 'WhistlesOptions' } }
>>
>> Type 'plain' doesn't have options beyond CommonFooOptions.
>
> I'd still rather make it explicit that we KNOW that this branch of the
> union has no additional options:
>
> { 'union': 'FooOptions',
> 'base': 'CommonFooOptions',
> 'discriminator': 'type',
> 'data': { 'plain': {},
> 'bells': 'BellsOptions',
> 'whistles': 'WhistlesOptions' } }
>
> to show that we explicitly thought about all the cases. We don't
> currently have any such unions with an empty branch, but it would be
> worth documenting in the qapi text and explicitly testing that it works
> if we intend to support this.
Fair point. However, it requires 'plain': {} to work, and it doesn't in
my testing. When I add "'c': {}" to qapi-schema-test.json's
UserDefFlatUnion like this:
{ 'union': 'UserDefFlatUnion',
'base': 'UserDefOne',
'discriminator': 'string',
'data': { 'a' : 'UserDefA', 'b' : 'UserDefB', 'c': {} } }
the generator gives me
struct UserDefFlatUnion
{
UserDefFlatUnionKind kind;
union {
void *data;
UserDefA * a;
UserDefB * b;
---> void c;
};
bool has_enum1;
EnumOne enum1;
};
which doesn't compile. This is only the first compile error, there are
more.
We should extend the generator to permit {} before we insist on unions
covering all discriminator values explicitly. Because if we don't,
people will be compelled to add dummy fields.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum
2014-02-20 17:53 ` Eric Blake
@ 2014-02-21 8:21 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-21 8:21 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, lcapitulino, Wenchao Xia, mdroth
Eric Blake <eblake@redhat.com> writes:
> On 02/20/2014 09:54 AM, Markus Armbruster wrote:
>>> + # When c is upper and no "_" appears before, do more checks
>>> + if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
>>
>> c_fun_str[i - 1]... what if i == 0?
>
> How? We already had '(i > 0) and' prior to the use of i-1.
Blind on both eyes, sorry for the noise %-}
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing
2014-02-21 0:10 ` Wenchao Xia
@ 2014-02-21 13:04 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-21 13:04 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, mdroth, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Thanks for your quick review, I'll respin it next week.
Looking forward to it. I think we're very close to merge.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union
2014-02-21 8:21 ` Markus Armbruster
@ 2014-02-21 13:49 ` Eric Blake
2014-02-21 14:08 ` Markus Armbruster
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2014-02-21 13:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, lcapitulino, Wenchao Xia, mdroth
[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]
On 02/21/2014 01:21 AM, Markus Armbruster wrote:
>> I'd still rather make it explicit that we KNOW that this branch of the
>> union has no additional options:
>>
>> { 'union': 'FooOptions',
>> 'base': 'CommonFooOptions',
>> 'discriminator': 'type',
>> 'data': { 'plain': {},
>> 'bells': 'BellsOptions',
>> 'whistles': 'WhistlesOptions' } }
>>
>> to show that we explicitly thought about all the cases. We don't
>> currently have any such unions with an empty branch, but it would be
>> worth documenting in the qapi text and explicitly testing that it works
>> if we intend to support this.
>
> Fair point. However, it requires 'plain': {} to work, and it doesn't in
> my testing.
> We should extend the generator to permit {} before we insist on unions
> covering all discriminator values explicitly. Because if we don't,
> people will be compelled to add dummy fields.
So far, we have no one that should be omitting a branch. But yes, I
agree that we should update the generator to allow an empty branch.
In the context of _this_ patch, requiring that all branches are covered
doesn't hurt until a future patch actually has to add a dummy field or
fix the generator. I don't know if it's something Wenchao would like to
add in the next spin, or if we should just live this this patch being a
little over-ambitious on what it enforces in the absence of more generic
support for an explicit empty branch.
--
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] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union
2014-02-21 8:13 ` Markus Armbruster
@ 2014-02-21 13:56 ` Eric Blake
2014-02-21 14:39 ` Markus Armbruster
0 siblings, 1 reply; 33+ messages in thread
From: Eric Blake @ 2014-02-21 13:56 UTC (permalink / raw)
To: Markus Armbruster, Wenchao Xia; +Cc: kwolf, mdroth, qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]
On 02/21/2014 01:13 AM, Markus Armbruster wrote:
>>> I guess you move this into its own loop because when base types are used
>>> before they're defined, or an enum type is used for a discriminator
>>> before it's defined, then discriminator_find_enum_define() complains.
>>> Correct?
>>>
>> Exactly, which allow enum define after usage in schema.
>
> Do we want to (have to?) support "use before define" in schemas? Eric,
> what do you think?
Topological sorting is a nice goal; and unfortunately not possible in
qapi because we already have recursive types. We already have to
support use before define in schemas. But it still seems like a
two-pass parse is sufficient - in pass one, read all types, but without
paying attention to their contents; in pass two, resolve all types in
the order that they are encountered. Enums are not recursive, so it
will always possible to resolve an enum before resolving the base class
of a union, even if the enum definition occurred later than the union
type that is using the enum as its discriminator.
Now, just because we have to support use-before-define of recursive
types does not necessarily mean that we have to support
use-before-define of enums. Since enums are inherently not recursive,
it might be okay to state that any use of a discriminator must be after
the enum has already been declared. But for consistency, I think
supporting use-before-define is nicer; I also think there may come a day
where the schema file is so large that it would pay to do a one-time
sort and make all further insertions in alphabetical order (to make it
easier to find a given type name) - and I do not want to enforce that
enum types must sort lexicographically before any client using it as a
discriminator.
>
> If yes, we should add suitable tests. Outside the scope of this series.
Here, I agree - whether you enforce define-before-use for now, or plan
on supporting use-before-define, you need a test of an enum after the
union type to ensure that it either gives a sane error message or does
the right action. I actually think adding such a test IS part of the
scope of this series, as this is the series adding support for an enum
discriminator in the first place.
--
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] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union
2014-02-21 13:49 ` Eric Blake
@ 2014-02-21 14:08 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-21 14:08 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, mdroth, Wenchao Xia, qemu-devel, lcapitulino
Eric Blake <eblake@redhat.com> writes:
> On 02/21/2014 01:21 AM, Markus Armbruster wrote:
>
>>> I'd still rather make it explicit that we KNOW that this branch of the
>>> union has no additional options:
>>>
>>> { 'union': 'FooOptions',
>>> 'base': 'CommonFooOptions',
>>> 'discriminator': 'type',
>>> 'data': { 'plain': {},
>>> 'bells': 'BellsOptions',
>>> 'whistles': 'WhistlesOptions' } }
>>>
>>> to show that we explicitly thought about all the cases. We don't
>>> currently have any such unions with an empty branch, but it would be
>>> worth documenting in the qapi text and explicitly testing that it works
>>> if we intend to support this.
>>
>> Fair point. However, it requires 'plain': {} to work, and it doesn't in
>> my testing.
>
>> We should extend the generator to permit {} before we insist on unions
>> covering all discriminator values explicitly. Because if we don't,
>> people will be compelled to add dummy fields.
>
> So far, we have no one that should be omitting a branch. But yes, I
> agree that we should update the generator to allow an empty branch.
>
> In the context of _this_ patch, requiring that all branches are covered
> doesn't hurt until a future patch actually has to add a dummy field or
> fix the generator. I don't know if it's something Wenchao would like to
> add in the next spin, or if we should just live this this patch being a
> little over-ambitious on what it enforces in the absence of more generic
> support for an explicit empty branch.
Considering we're at v7, I'd like to get this series committed sooner
rather than later, and that means avoiding adding more functionality to
it. Only advice; Wenchao is of course free to add whatever he wants :)
My first preference is to drop the code enforcing all enum values are
covered from the patch for now. No new code to review. Doesn't
preclude adding the code back later.
I can also live with keeping the enforcing code. We'd have to watch out
for dummy fields until we implement support for {}.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union
2014-02-21 13:56 ` Eric Blake
@ 2014-02-21 14:39 ` Markus Armbruster
0 siblings, 0 replies; 33+ messages in thread
From: Markus Armbruster @ 2014-02-21 14:39 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, lcapitulino, Wenchao Xia, mdroth
Eric Blake <eblake@redhat.com> writes:
> On 02/21/2014 01:13 AM, Markus Armbruster wrote:
>
>>>> I guess you move this into its own loop because when base types are used
>>>> before they're defined, or an enum type is used for a discriminator
>>>> before it's defined, then discriminator_find_enum_define() complains.
>>>> Correct?
>>>>
>>> Exactly, which allow enum define after usage in schema.
>>
>> Do we want to (have to?) support "use before define" in schemas? Eric,
>> what do you think?
>
> Topological sorting is a nice goal; and unfortunately not possible in
> qapi because we already have recursive types. We already have to
> support use before define in schemas. But it still seems like a
> two-pass parse is sufficient - in pass one, read all types, but without
> paying attention to their contents; in pass two, resolve all types in
> the order that they are encountered. Enums are not recursive, so it
> will always possible to resolve an enum before resolving the base class
> of a union, even if the enum definition occurred later than the union
> type that is using the enum as its discriminator.
>
> Now, just because we have to support use-before-define of recursive
> types does not necessarily mean that we have to support
> use-before-define of enums. Since enums are inherently not recursive,
> it might be okay to state that any use of a discriminator must be after
> the enum has already been declared. But for consistency, I think
> supporting use-before-define is nicer; I also think there may come a day
> where the schema file is so large that it would pay to do a one-time
> sort and make all further insertions in alphabetical order (to make it
> easier to find a given type name) - and I do not want to enforce that
> enum types must sort lexicographically before any client using it as a
> discriminator.
Color me convinced.
>> If yes, we should add suitable tests. Outside the scope of this series.
>
> Here, I agree - whether you enforce define-before-use for now, or plan
> on supporting use-before-define, you need a test of an enum after the
> union type to ensure that it either gives a sane error message or does
> the right action. I actually think adding such a test IS part of the
> scope of this series, as this is the series adding support for an enum
> discriminator in the first place.
Wenchao, if it's not too much trouble...
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2014-02-21 14:39 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-20 5:54 [Qemu-devel] [PATCH V7 00/11] qapi script: support enum as discriminator and better enum name Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 01/11] qapi script: remember enum values Wenchao Xia
2014-02-20 12:05 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 02/11] qapi script: add check for duplicated key Wenchao Xia
2014-02-20 12:05 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing Wenchao Xia
2014-02-20 12:22 ` Markus Armbruster
2014-02-21 0:10 ` Wenchao Xia
2014-02-21 13:04 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union Wenchao Xia
2014-02-20 14:43 ` Markus Armbruster
2014-02-20 15:26 ` Eric Blake
2014-02-21 8:21 ` Markus Armbruster
2014-02-21 13:49 ` Eric Blake
2014-02-21 14:08 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 05/11] qapi script: code move for generate_enum_name() Wenchao Xia
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 06/11] qapi script: use same function to generate enum string Wenchao Xia
2014-02-20 15:20 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 07/11] qapi script: support pre-defined enum type as discriminator in union Wenchao Xia
2014-02-20 16:38 ` Markus Armbruster
2014-02-21 0:17 ` Wenchao Xia
2014-02-21 8:13 ` Markus Armbruster
2014-02-21 13:56 ` Eric Blake
2014-02-21 14:39 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator Wenchao Xia
2014-02-20 17:59 ` Eric Blake
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator Wenchao Xia
2014-02-20 16:50 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum Wenchao Xia
2014-02-20 16:54 ` Markus Armbruster
2014-02-20 17:53 ` Eric Blake
2014-02-21 8:21 ` Markus Armbruster
2014-02-20 5:54 ` [Qemu-devel] [PATCH V7 11/11] qapi test: add error path test for union Wenchao Xia
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.