[Qemu-devel] [PATCH 00/26] qapi: Another round of fixes and cleanups

[Qemu-devel] [PATCH 00/26] qapi: Another round of fixes and cleanups

[Markus Armbruster]

Previously posted as part of "[PATCH RFC v2 00/47] qapi: QMP
introspection".  Changes since then:
* Trivially rebased, R-bys retained unless noted otherwise.
* PATCH 01: Squash in update of qapi-code-gen.txt examples to current
  code.
* PATCH 04: Update qapi-code-gen.txt for the change.
* PATCH 05: Fix typo of --prefix in error message [Eric].
* PATCH 06: Clearer commit message [Eric].
* PATCH 07+08: Old PATCH 07 split up into bug fix and cleanup,
  crediting Eric for the bug report.  Generate comments on member
  inheritance [Eric], and move generated "union tag is" comment.  R-by
  dropped.
* PATCH 14+15: New, from Eric.
* PATCH 16+18: New.
* PATCH 19: Add note on events to commit message.  Drop invalid TODO
  [Eric].  Straightforward conflicts with new PATCH 18 resolved.
* PATCH 20: Fix events, too [Eric].  R-by dropped.
* PATCH 21+25: New.
* PATCH 26: Restore truncated commit message [Eric].  Update
  qapi-code-gen.txt for the change.
* Parches not mentioned are identical.

Eric Blake (2):
  qapi: Document that input visitor semantics are prone to leaks
  qapi: Document shortcoming with union 'data' branch

Markus Armbruster (24):
  qapi: Clarify docs on including the same file multiple times
  qapi: Clean up cgen() and mcgen()
  qapi: Simplify guardname()
  qapi-event: Clean up how name of enum QAPIEvent is made
  qapi: Reject -p arguments that break qapi-event.py
  qapi: Drop unused and useless parameters and variables
  qapi: Fix generated code when flat union has member 'kind'
  qapi: Generate a nicer struct for flat unions
  qapi-visit: Fix generated code when schema has forward refs
  qapi-visit: Replace list implicit_structs by set
  qapi-visit: Fix two name arguments passed to visitors
  tests/qapi-schema: Document alternate's enum lacks visit function
  tests/qapi-schema: Document events with base don't work
  qapi: Document flaws in checking of names
  tests/qapi-schema: Restore test case for flat union base bug
  tests/qapi-schema: Rename tests from data- to args-
  qapi-tests: New tests for union, alternate command arguments
  qapi: Fix to reject union command and event arguments
  qapi: Command returning anonymous type doesn't work, outlaw
  qapi-commands: Fix gen_err_check(e) for e and e != 'local_err'
  qapi-commands: Inline gen_marshal_output_call()
  qapi-commands: Don't feed output of mcgen() to mcgen() again
  qapi-commands: Drop useless initialization
  qapi: Generated code cleanup

 docs/qapi-code-gen.txt                             |  41 ++++----
 scripts/qapi-commands.py                           | 109 +++++++++-----------
 scripts/qapi-event.py                              |   6 +-
 scripts/qapi-types.py                              | 110 +++++++++++----------
 scripts/qapi-visit.py                              |  93 ++++++++++-------
 scripts/qapi.py                                    |  46 +++++----
 tests/Makefile                                     |   9 +-
 tests/qapi-schema/args-alternate.err               |   1 +
 ...ted-struct-returns.exit => args-alternate.exit} |   0
 tests/qapi-schema/args-alternate.json              |   3 +
 ...ested-struct-returns.out => args-alternate.out} |   0
 .../{data-array-empty.err => args-array-empty.err} |   2 +-
 .../{data-unknown.exit => args-array-empty.exit}   |   0
 ...data-array-empty.json => args-array-empty.json} |   0
 .../{data-unknown.out => args-array-empty.out}     |   0
 ...ta-array-unknown.err => args-array-unknown.err} |   2 +-
 ...member-unknown.exit => args-array-unknown.exit} |   0
 ...-array-unknown.json => args-array-unknown.json} |   0
 ...a-member-unknown.out => args-array-unknown.out} |   0
 tests/qapi-schema/args-int.err                     |   1 +
 .../{data-member-array-bad.exit => args-int.exit}  |   0
 tests/qapi-schema/{data-int.json => args-int.json} |   0
 .../{data-member-array.err => args-int.out}        |   0
 ...ber-array-bad.err => args-member-array-bad.err} |   2 +-
 .../{data-int.exit => args-member-array-bad.exit}  |   0
 ...r-array-bad.json => args-member-array-bad.json} |   0
 ...ber-array-bad.out => args-member-array-bad.out} |   0
 .../{data-int.out => args-member-array.err}        |   0
 ...ta-member-array.exit => args-member-array.exit} |   0
 ...ta-member-array.json => args-member-array.json} |   0
 ...data-member-array.out => args-member-array.out} |   0
 tests/qapi-schema/args-member-unknown.err          |   1 +
 ...array-unknown.exit => args-member-unknown.exit} |   0
 ...ember-unknown.json => args-member-unknown.json} |   0
 ...a-array-unknown.out => args-member-unknown.out} |   0
 tests/qapi-schema/args-union.err                   |   1 +
 .../{data-array-empty.exit => args-union.exit}     |   0
 tests/qapi-schema/args-union.json                  |   4 +
 .../{data-array-empty.out => args-union.out}       |   0
 tests/qapi-schema/args-unknown.err                 |   1 +
 tests/qapi-schema/args-unknown.exit                |   1 +
 .../{data-unknown.json => args-unknown.json}       |   0
 tests/qapi-schema/args-unknown.out                 |   0
 tests/qapi-schema/data-int.err                     |   1 -
 tests/qapi-schema/data-member-unknown.err          |   1 -
 tests/qapi-schema/data-unknown.err                 |   1 -
 tests/qapi-schema/nested-struct-returns.err        |   1 -
 tests/qapi-schema/nested-struct-returns.json       |   3 -
 tests/qapi-schema/qapi-schema-test.json            |  39 +++++---
 tests/qapi-schema/qapi-schema-test.out             |  14 +--
 tests/qapi-schema/returns-dict.err                 |   1 +
 tests/qapi-schema/returns-dict.exit                |   1 +
 tests/qapi-schema/returns-dict.json                |   2 +
 tests/qapi-schema/returns-dict.out                 |   0
 tests/test-qmp-event.c                             |   2 +-
 tests/test-qmp-input-visitor.c                     |   4 +-
 tests/test-qmp-output-visitor.c                    |   2 +-
 57 files changed, 271 insertions(+), 234 deletions(-)
 create mode 100644 tests/qapi-schema/args-alternate.err
 rename tests/qapi-schema/{nested-struct-returns.exit => args-alternate.exit} (100%)
 create mode 100644 tests/qapi-schema/args-alternate.json
 rename tests/qapi-schema/{nested-struct-returns.out => args-alternate.out} (100%)
 rename tests/qapi-schema/{data-array-empty.err => args-array-empty.err} (50%)
 rename tests/qapi-schema/{data-unknown.exit => args-array-empty.exit} (100%)
 rename tests/qapi-schema/{data-array-empty.json => args-array-empty.json} (100%)
 rename tests/qapi-schema/{data-unknown.out => args-array-empty.out} (100%)
 rename tests/qapi-schema/{data-array-unknown.err => args-array-unknown.err} (50%)
 rename tests/qapi-schema/{data-member-unknown.exit => args-array-unknown.exit} (100%)
 rename tests/qapi-schema/{data-array-unknown.json => args-array-unknown.json} (100%)
 rename tests/qapi-schema/{data-member-unknown.out => args-array-unknown.out} (100%)
 create mode 100644 tests/qapi-schema/args-int.err
 rename tests/qapi-schema/{data-member-array-bad.exit => args-int.exit} (100%)
 rename tests/qapi-schema/{data-int.json => args-int.json} (100%)
 rename tests/qapi-schema/{data-member-array.err => args-int.out} (100%)
 rename tests/qapi-schema/{data-member-array-bad.err => args-member-array-bad.err} (52%)
 rename tests/qapi-schema/{data-int.exit => args-member-array-bad.exit} (100%)
 rename tests/qapi-schema/{data-member-array-bad.json => args-member-array-bad.json} (100%)
 rename tests/qapi-schema/{data-member-array-bad.out => args-member-array-bad.out} (100%)
 rename tests/qapi-schema/{data-int.out => args-member-array.err} (100%)
 rename tests/qapi-schema/{data-member-array.exit => args-member-array.exit} (100%)
 rename tests/qapi-schema/{data-member-array.json => args-member-array.json} (100%)
 rename tests/qapi-schema/{data-member-array.out => args-member-array.out} (100%)
 create mode 100644 tests/qapi-schema/args-member-unknown.err
 rename tests/qapi-schema/{data-array-unknown.exit => args-member-unknown.exit} (100%)
 rename tests/qapi-schema/{data-member-unknown.json => args-member-unknown.json} (100%)
 rename tests/qapi-schema/{data-array-unknown.out => args-member-unknown.out} (100%)
 create mode 100644 tests/qapi-schema/args-union.err
 rename tests/qapi-schema/{data-array-empty.exit => args-union.exit} (100%)
 create mode 100644 tests/qapi-schema/args-union.json
 rename tests/qapi-schema/{data-array-empty.out => args-union.out} (100%)
 create mode 100644 tests/qapi-schema/args-unknown.err
 create mode 100644 tests/qapi-schema/args-unknown.exit
 rename tests/qapi-schema/{data-unknown.json => args-unknown.json} (100%)
 create mode 100644 tests/qapi-schema/args-unknown.out
 delete mode 100644 tests/qapi-schema/data-int.err
 delete mode 100644 tests/qapi-schema/data-member-unknown.err
 delete mode 100644 tests/qapi-schema/data-unknown.err
 delete mode 100644 tests/qapi-schema/nested-struct-returns.err
 delete mode 100644 tests/qapi-schema/nested-struct-returns.json
 create mode 100644 tests/qapi-schema/returns-dict.err
 create mode 100644 tests/qapi-schema/returns-dict.exit
 create mode 100644 tests/qapi-schema/returns-dict.json
 create mode 100644 tests/qapi-schema/returns-dict.out

-- 
2.4.3

[Qemu-devel] [PATCH 01/26] qapi: Clarify docs on including the same file multiple times

[Markus Armbruster]

It's idempotent.

While there, update examples to current code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 docs/qapi-code-gen.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 61b5be4..e7e7281 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -163,7 +163,7 @@ The QAPI schema definitions can be modularized using the 'include' directive:
 
 The directive is evaluated recursively, and include paths are relative to the
 file using the directive. Multiple includes of the same file are
-safe.  No other keys should appear in the expression, and the include
+idempotent.  No other keys should appear in the expression, and the include
 value should be a string.
 
 As a matter of style, it is a good idea to have all files be
@@ -555,6 +555,7 @@ Example:
         qapi_dealloc_visitor_cleanup(md);
     }
 
+
     void qapi_free_UserDefOne(UserDefOne *obj)
     {
         QapiDeallocVisitor *md;
@@ -769,7 +770,6 @@ Example:
         v = qapi_dealloc_get_visitor(md);
         visit_type_UserDefOne(v, &arg1, "arg1", NULL);
         qapi_dealloc_visitor_cleanup(md);
-        return;
     }
 
     static void qmp_init_marshal(void)
-- 
2.4.3

[Qemu-devel] [PATCH 02/26] qapi: Clean up cgen() and mcgen()

[Markus Armbruster]

Commit 05dfb26 added eatspace stripping to mcgen().  Move it to
cgen(), just in case somebody gets tempted to use cgen() directly
instead of via mcgen().

cgen() indents blank lines.  No such lines get generated right now,
but fix it anyway.

We use triple-quoted strings for program text, like this:

    '''
    Program text
    any number of lines
    '''

Keeps the program text relatively readable, but puts an extra newline
at either end.  mcgen() "fixes" that by dropping the first and last
line outright.  Drop only the newlines.

This unmasks a bug in qapi-commands.py: four quotes instead of three.
Fix it up.

Output doesn't change

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py |  2 +-
 scripts/qapi.py          | 19 ++++++++++++-------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index ca22acc..ce51408 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -56,7 +56,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
                 name=c_name(name), args=arglist, retval=retval).rstrip()
     if ret_type:
         ret += "\n" + gen_err_check('local_err')
-        ret += "\n" + mcgen(''''
+        ret += "\n" + mcgen('''
 %(marshal_output_call)s
 ''',
                             marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip()
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 06d7fc2..20383ef 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -943,16 +943,21 @@ def pop_indent(indent_amount=4):
     global indent_level
     indent_level -= indent_amount
 
+# Generate @code with @kwds interpolated.
+# Obey indent_level, and strip eatspace.
 def cgen(code, **kwds):
-    indent = genindent(indent_level)
-    lines = code.split('\n')
-    lines = map(lambda x: indent + x, lines)
-    return '\n'.join(lines) % kwds + '\n'
-
-def mcgen(code, **kwds):
-    raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
+    raw = code % kwds
+    if indent_level:
+        indent = genindent(indent_level)
+        raw = re.subn("^.", indent + '\g<0>', raw, 0, re.MULTILINE)
+        raw = raw[0]
     return re.sub(re.escape(eatspace) + ' *', '', raw)
 
+def mcgen(code, **kwds):
+    if code[0] == '\n':
+        code = code[1:]
+    return cgen(code, **kwds)
+
 def basename(filename):
     return filename.split("/")[-1]
 
-- 
2.4.3

[Qemu-devel] [PATCH 03/26] qapi: Simplify guardname()

[Markus Armbruster]

The guards around built-in declarations lose their _H.  It never made
much sense anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 20383ef..2bbc8ff 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -958,14 +958,9 @@ def mcgen(code, **kwds):
         code = code[1:]
     return cgen(code, **kwds)
 
-def basename(filename):
-    return filename.split("/")[-1]
 
 def guardname(filename):
-    guard = basename(filename).rsplit(".", 1)[0]
-    for substr in [".", " ", "-"]:
-        guard = guard.replace(substr, "_")
-    return guard.upper() + '_H'
+    return c_name(filename, protect=False).upper()
 
 def guardstart(name):
     return mcgen('''
@@ -1035,6 +1030,7 @@ def parse_command_line(extra_options = "", extra_long_options = []):
 
 def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
                 c_comment, h_comment):
+    guard = guardname(prefix + h_file)
     c_file = output_dir + prefix + c_file
     h_file = output_dir + prefix + h_file
 
@@ -1067,7 +1063,7 @@ def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
 #define %(guard)s
 
 ''',
-                      comment = h_comment, guard = guardname(h_file)))
+                      comment = h_comment, guard = guard))
 
     return (fdef, fdecl)
 
-- 
2.4.3

[Qemu-devel] [PATCH 04/26] qapi-event: Clean up how name of enum QAPIEvent is made

[Markus Armbruster]

Use c_name() instead of ad hoc code.  Doesn't upcase the -p prefix,
which is an improvement in my book.  Unbreaks prefix containing '.',
but other funny characters remain broken.  To be fixed next.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 docs/qapi-code-gen.txt | 8 ++++----
 scripts/qapi-event.py  | 2 +-
 tests/test-qmp-event.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index e7e7281..c2ac21c 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -826,7 +826,7 @@ Example:
         QDECREF(qmp);
     }
 
-    const char *EXAMPLE_QAPIEvent_lookup[] = {
+    const char *example_QAPIEvent_lookup[] = {
         "MY_EVENT",
         NULL,
     };
@@ -843,11 +843,11 @@ Example:
 
     void qapi_event_send_my_event(Error **errp);
 
-    extern const char *EXAMPLE_QAPIEvent_lookup[];
-    typedef enum EXAMPLE_QAPIEvent
+    extern const char *example_QAPIEvent_lookup[];
+    typedef enum example_QAPIEvent
     {
         EXAMPLE_QAPI_EVENT_MY_EVENT = 0,
         EXAMPLE_QAPI_EVENT_MAX = 1,
-    } EXAMPLE_QAPIEvent;
+    } example_QAPIEvent;
 
     #endif
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 56bc602..cc74f4d 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -267,7 +267,7 @@ fdecl.write(mcgen('''
 
 exprs = parse_schema(input_file)
 
-event_enum_name = prefix.upper().replace('-', '_') + "QAPIEvent"
+event_enum_name = c_name(prefix + "QAPIEvent", protect=False)
 event_enum_values = []
 event_enum_strings = []
 
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 1ee40e1..28f146d 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -94,7 +94,7 @@ static bool qdict_cmp_simple(QDict *a, QDict *b)
 
 /* This function is hooked as final emit function, which can verify the
    correctness. */
-static void event_test_emit(TEST_QAPIEvent event, QDict *d, Error **errp)
+static void event_test_emit(test_QAPIEvent event, QDict *d, Error **errp)
 {
     QObject *obj;
     QDict *t;
-- 
2.4.3

[Qemu-devel] [PATCH 05/26] qapi: Reject -p arguments that break qapi-event.py

[Markus Armbruster]

qapi-event.py breaks when you ask for a funny prefix like '@'.
Protect it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2bbc8ff..04c1d96 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1003,6 +1003,12 @@ def parse_command_line(extra_options = "", extra_long_options = []):
     for oa in opts:
         o, a = oa
         if o in ("-p", "--prefix"):
+            match = re.match('([A-Za-z_.-][A-Za-z0-9_.-]*)?', a)
+            if match.end() != len(a):
+                print >>sys.stderr, \
+                    "%s: 'funny character '%s' in argument of --prefix" \
+                    % (sys.argv[0], a[match.end()])
+                sys.exit(1)
             prefix = a
         elif o in ("-o", "--output-dir"):
             output_dir = a + "/"
-- 
2.4.3

[Qemu-devel] [PATCH 06/26] qapi: Drop unused and useless parameters and variables

[Markus Armbruster]

gen_sync_call()'s parameter indent is useless: gen_sync_call() uses it
only as optional argument for push_indent() and pop_indent(), their
default is four, and gen_sync_call()'s only caller passes four.  Drop
the parameter.

gen_visitor_input_containers_decl()'s parameter obj is always
"QOBJECT(args)".  Use that, and drop the parameter.

Drop unused parameters of gen_marshal_output(),
gen_marshal_input_decl(), generate_visit_struct_body(),
generate_visit_list(), generate_visit_enum(), generate_declaration(),
generate_enum_declaration(), generate_decl_enum().

Drop unused variables in generate_event_enum_lookup(),
generate_enum_lookup(), generate_visit_struct_fields(), check_event().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py | 27 +++++++++++++--------------
 scripts/qapi-event.py    |  1 -
 scripts/qapi-types.py    |  1 -
 scripts/qapi-visit.py    | 47 ++++++++++++++++++++++-------------------------
 scripts/qapi.py          |  1 -
 5 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index ce51408..69029f5 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -38,7 +38,7 @@ if (local_err) {
 ''')
     return ''
 
-def gen_sync_call(name, args, ret_type, indent=0):
+def gen_sync_call(name, args, ret_type):
     ret = ""
     arglist=""
     retval=""
@@ -48,7 +48,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
         if optional:
             arglist += "has_%s, " % c_name(argname)
         arglist += "%s, " % (c_name(argname))
-    push_indent(indent)
+    push_indent()
     ret = mcgen('''
 %(retval)sqmp_%(name)s(%(args)s&local_err);
 
@@ -60,7 +60,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
 %(marshal_output_call)s
 ''',
                             marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip()
-    pop_indent(indent)
+    pop_indent()
     return ret.rstrip()
 
 
@@ -69,17 +69,16 @@ def gen_marshal_output_call(name, ret_type):
         return ""
     return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_name(name)
 
-def gen_visitor_input_containers_decl(args, obj):
+def gen_visitor_input_containers_decl(args):
     ret = ""
 
     push_indent()
     if len(args) > 0:
         ret += mcgen('''
-QmpInputVisitor *mi = qmp_input_visitor_new_strict(%(obj)s);
+QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
 QapiDeallocVisitor *md;
 Visitor *v;
-''',
-                     obj=obj)
+''')
     pop_indent()
 
     return ret.rstrip()
@@ -161,7 +160,7 @@ qapi_dealloc_visitor_cleanup(md);
     pop_indent()
     return ret.rstrip()
 
-def gen_marshal_output(name, args, ret_type, middle_mode):
+def gen_marshal_output(name, ret_type):
     if not ret_type:
         return ""
 
@@ -194,14 +193,14 @@ out:
 
     return ret
 
-def gen_marshal_input_decl(name, args, ret_type, middle_mode):
+def gen_marshal_input_decl(name, middle_mode):
     ret = 'void qmp_marshal_input_%s(QDict *args, QObject **ret, Error **errp)' % c_name(name)
     if not middle_mode:
         ret = "static " + ret
     return ret
 
 def gen_marshal_input(name, args, ret_type, middle_mode):
-    hdr = gen_marshal_input_decl(name, args, ret_type, middle_mode)
+    hdr = gen_marshal_input_decl(name, middle_mode)
 
     ret = mcgen('''
 %(header)s
@@ -228,7 +227,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
 %(visitor_input_block)s
 
 ''',
-                     visitor_input_containers_decl=gen_visitor_input_containers_decl(args, "QOBJECT(args)"),
+                     visitor_input_containers_decl=gen_visitor_input_containers_decl(args),
                      visitor_input_vars_decl=gen_visitor_input_vars_decl(args),
                      visitor_input_block=gen_visitor_input_block(args))
     else:
@@ -240,7 +239,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
     ret += mcgen('''
 %(sync_call)s
 ''',
-                 sync_call=gen_sync_call(name, args, ret_type, indent=4))
+                 sync_call=gen_sync_call(name, args, ret_type))
     if re.search('^ *goto out\\;', ret, re.MULTILINE):
         ret += mcgen('''
 
@@ -360,11 +359,11 @@ for cmd in commands:
     ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
     fdecl.write(ret)
     if ret_type:
-        ret = gen_marshal_output(cmd['command'], arglist, ret_type, middle_mode) + "\n"
+        ret = gen_marshal_output(cmd['command'], ret_type) + "\n"
         fdef.write(ret)
 
     if middle_mode:
-        fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], arglist, ret_type, middle_mode))
+        fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], middle_mode))
 
     ret = gen_marshal_input(cmd['command'], arglist, ret_type, middle_mode) + "\n"
     fdef.write(ret)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index cc74f4d..88b0620 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -199,7 +199,6 @@ const char *%(event_enum_name)s_lookup[] = {
 ''',
                 event_enum_name = event_enum_name)
 
-    i = 0
     for string in event_enum_strings:
         ret += mcgen('''
     "%(string)s",
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index e6eb4b6..4902440 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -108,7 +108,6 @@ def generate_enum_lookup(name, values):
 const char * const %(name)s_lookup[] = {
 ''',
                 name=c_name(name))
-    i = 0
     for value in values:
         index = c_enum_const(name, value)
         ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 5b99336..e8ee268 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -40,7 +40,6 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
                  c_type=type_name(type))
 
 def generate_visit_struct_fields(name, members, base = None):
-    substructs = []
     ret = ''
 
     if base:
@@ -103,7 +102,7 @@ out:
     return ret
 
 
-def generate_visit_struct_body(name, members):
+def generate_visit_struct_body(name):
     ret = mcgen('''
     Error *err = NULL;
 
@@ -135,14 +134,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
 ''',
                  name=c_name(name))
 
-    ret += generate_visit_struct_body(name, members)
+    ret += generate_visit_struct_body(name)
 
     ret += mcgen('''
 }
 ''')
     return ret
 
-def generate_visit_list(name, members):
+def generate_visit_list(name):
     return mcgen('''
 
 void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp)
@@ -171,7 +170,7 @@ out:
 ''',
                 name=type_name(name))
 
-def generate_visit_enum(name, members):
+def generate_visit_enum(name):
     return mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp)
@@ -252,7 +251,7 @@ def generate_visit_union(expr):
     else:
         # There will always be a discriminator in the C switch code, by default
         # it is an enum type generated silently
-        ret = generate_visit_enum(name + 'Kind', members.keys())
+        ret = generate_visit_enum(name + 'Kind')
         disc_type = c_name(name) + 'Kind'
 
     if base:
@@ -340,7 +339,7 @@ out:
 
     return ret
 
-def generate_declaration(name, members, builtin_type=False):
+def generate_declaration(name, builtin_type=False):
     ret = ""
     if not builtin_type:
         name = c_name(name)
@@ -357,7 +356,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, E
 
     return ret
 
-def generate_enum_declaration(name, members):
+def generate_enum_declaration(name):
     ret = mcgen('''
 void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, Error **errp);
 ''',
@@ -365,7 +364,7 @@ void visit_type_%(name)sList(Visitor *m, %(name)sList **obj, const char *name, E
 
     return ret
 
-def generate_decl_enum(name, members):
+def generate_decl_enum(name):
     return mcgen('''
 
 void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp);
@@ -433,7 +432,7 @@ exprs = parse_schema(input_file)
 # for built-in types in our header files and simply guard them
 fdecl.write(guardstart("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
 for typename in builtin_types.keys():
-    fdecl.write(generate_declaration(typename, None, builtin_type=True))
+    fdecl.write(generate_declaration(typename, builtin_type=True))
 fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
 
 # ...this doesn't work for cases where we link in multiple objects that
@@ -441,44 +440,42 @@ fdecl.write(guardend("QAPI_VISIT_BUILTIN_VISITOR_DECL"))
 # over these cases
 if do_builtins:
     for typename in builtin_types.keys():
-        fdef.write(generate_visit_list(typename, None))
+        fdef.write(generate_visit_list(typename))
 
 for expr in exprs:
     if expr.has_key('struct'):
         ret = generate_visit_struct(expr)
-        ret += generate_visit_list(expr['struct'], expr['data'])
+        ret += generate_visit_list(expr['struct'])
         fdef.write(ret)
 
-        ret = generate_declaration(expr['struct'], expr['data'])
+        ret = generate_declaration(expr['struct'])
         fdecl.write(ret)
     elif expr.has_key('union'):
         ret = generate_visit_union(expr)
-        ret += generate_visit_list(expr['union'], expr['data'])
+        ret += generate_visit_list(expr['union'])
         fdef.write(ret)
 
         enum_define = discriminator_find_enum_define(expr)
         ret = ""
         if not enum_define:
-            ret = generate_decl_enum('%sKind' % expr['union'],
-                                     expr['data'].keys())
-        ret += generate_declaration(expr['union'], expr['data'])
+            ret = generate_decl_enum('%sKind' % expr['union'])
+        ret += generate_declaration(expr['union'])
         fdecl.write(ret)
     elif expr.has_key('alternate'):
         ret = generate_visit_alternate(expr['alternate'], expr['data'])
-        ret += generate_visit_list(expr['alternate'], expr['data'])
+        ret += generate_visit_list(expr['alternate'])
         fdef.write(ret)
 
-        ret = generate_decl_enum('%sKind' % expr['alternate'],
-                                 expr['data'].keys())
-        ret += generate_declaration(expr['alternate'], expr['data'])
+        ret = generate_decl_enum('%sKind' % expr['alternate'])
+        ret += generate_declaration(expr['alternate'])
         fdecl.write(ret)
     elif expr.has_key('enum'):
-        ret = generate_visit_list(expr['enum'], expr['data'])
-        ret += generate_visit_enum(expr['enum'], expr['data'])
+        ret = generate_visit_list(expr['enum'])
+        ret += generate_visit_enum(expr['enum'])
         fdef.write(ret)
 
-        ret = generate_decl_enum(expr['enum'], expr['data'])
-        ret += generate_enum_declaration(expr['enum'], expr['data'])
+        ret = generate_decl_enum(expr['enum'])
+        ret += generate_enum_declaration(expr['enum'])
         fdecl.write(ret)
 
 close_output(fdef, fdecl)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 04c1d96..4af47ef 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -507,7 +507,6 @@ def check_command(expr, expr_info):
 def check_event(expr, expr_info):
     global events
     name = expr['event']
-    params = expr.get('data')
 
     if name.upper() == 'MAX':
         raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
-- 
2.4.3

[Qemu-devel] [PATCH 07/26] qapi: Fix generated code when flat union has member 'kind'

[Markus Armbruster]

A flat union's tag member gets renamed to 'kind' in the generated
code.  Breaks when another member is named 'kind' exists.

Example, adapted from qapi-schema-test.json:

    { 'struct': 'UserDefUnionBase',
      'data': { 'kind': 'str', 'enum1': 'EnumOne' } }

We generate:

    struct UserDefFlatUnion
    {
        EnumOne kind;
        union {
            void *data;
            UserDefA *value1;
            UserDefB *value2;
            UserDefB *value3;
        };
        char *kind;
    };

Kill the silly rename.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py           | 3 ++-
 scripts/qapi-visit.py           | 7 +++++--
 tests/test-qmp-input-visitor.c  | 2 +-
 tests/test-qmp-output-visitor.c | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4902440..ac8dad3 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -200,11 +200,12 @@ def generate_union(expr, meta):
     ret = mcgen('''
 struct %(name)s
 {
-    %(discriminator_type_name)s kind;
+    %(discriminator_type_name)s %(discriminator)s;
     union {
         void *data;
 ''',
                 name=name,
+                discriminator=c_name(discriminator or 'kind'),
                 discriminator_type_name=c_name(discriminator_type_name))
 
     for key in typeinfo:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index e8ee268..4ec79a6 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -288,20 +288,23 @@ void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **e
                      name=c_name(name))
 
     if not discriminator:
+        tag = 'kind'
         disc_key = "type"
     else:
+        tag = discriminator
         disc_key = discriminator
     ret += mcgen('''
-        visit_type_%(disc_type)s(m, &(*obj)->kind, "%(disc_key)s", &err);
+        visit_type_%(disc_type)s(m, &(*obj)->%(c_tag)s, "%(disc_key)s", &err);
         if (err) {
             goto out_obj;
         }
         if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
             goto out_obj;
         }
-        switch ((*obj)->kind) {
+        switch ((*obj)->%(c_tag)s) {
 ''',
                  disc_type = disc_type,
+                 c_tag=c_name(tag),
                  disc_key = disc_key)
 
     for key in members:
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index b961953..b7a87ee 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -313,7 +313,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
 
     visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
     g_assert(err == NULL);
-    g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1);
     g_assert_cmpstr(tmp->string, ==, "str");
     /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
     g_assert_cmpint(tmp->value1->boolean, ==, true);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 87ba350..338ada0 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -437,7 +437,7 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
     Error *err = NULL;
 
     UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
-    tmp->kind = ENUM_ONE_VALUE1;
+    tmp->enum1 = ENUM_ONE_VALUE1;
     tmp->string = g_strdup("str");
     tmp->value1 = g_malloc0(sizeof(UserDefA));
     /* TODO when generator bug is fixed: tmp->integer = 41; */
-- 
2.4.3

[Qemu-devel] [PATCH 08/26] qapi: Generate a nicer struct for flat unions

[Markus Armbruster]

The struct generated for a flat union is weird: the members of its
base are at the end, except for the union tag, which is at the
beginning.

Example: qapi-schema-test.json has

    { 'struct': 'UserDefUnionBase',
      'data': { 'string': 'str', 'enum1': 'EnumOne' } }

    { 'union': 'UserDefFlatUnion',
      'base': 'UserDefUnionBase',
      'discriminator': 'enum1',
      'data': { 'value1' : 'UserDefA',
                'value2' : 'UserDefB',
                'value3' : 'UserDefB' } }

We generate:

    struct UserDefFlatUnion
    {
        EnumOne enum1;
        union {
            void *data;
            UserDefA *value1;
            UserDefB *value2;
            UserDefB *value3;
        };
        char *string;
    };

Change to put all base members at the beginning, unadulterated.  Not
only is this easier to understand, it also permits casting the flat
union to its base, if that should become useful.

We now generate:

    struct UserDefFlatUnion
    {
        /* Members inherited from UserDefUnionBase: */
        char *string;
        EnumOne enum1;
        /* Own members: */
        union { /* union tag is @enum1 */
            void *data;
            UserDefA *value1;
            UserDefB *value2;
            UserDefB *value3;
        };
    };

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index ac8dad3..82141cd 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -200,13 +200,30 @@ def generate_union(expr, meta):
     ret = mcgen('''
 struct %(name)s
 {
-    %(discriminator_type_name)s %(discriminator)s;
-    union {
+''',
+                name=name)
+    if base:
+        ret += mcgen('''
+    /* Members inherited from %(c_name)s: */
+''',
+                     c_name=c_name(base))
+        base_fields = find_struct(base)['data']
+        ret += generate_struct_fields(base_fields)
+        ret += mcgen('''
+    /* Own members: */
+''')
+    else:
+        assert not discriminator
+        ret += mcgen('''
+    %(discriminator_type_name)s kind;
+''',
+                     discriminator_type_name=c_name(discriminator_type_name))
+
+    ret += mcgen('''
+    union { /* union tag is @%(c_name)s */
         void *data;
 ''',
-                name=name,
-                discriminator=c_name(discriminator or 'kind'),
-                discriminator_type_name=c_name(discriminator_type_name))
+                 c_name=c_name(discriminator or 'kind'))
 
     for key in typeinfo:
         ret += mcgen('''
@@ -217,17 +234,6 @@ struct %(name)s
 
     ret += mcgen('''
     };
-''')
-
-    if base:
-        assert discriminator
-        base_fields = find_struct(base)['data'].copy()
-        del base_fields[discriminator]
-        ret += generate_struct_fields(base_fields)
-    else:
-        assert not discriminator
-
-    ret += mcgen('''
 };
 ''')
     if meta == 'alternate':
-- 
2.4.3

[Qemu-devel] [PATCH 09/26] qapi-visit: Fix generated code when schema has forward refs

[Markus Armbruster]

The visit_type_implicit_FOO() are generated on demand, right before
their first use.  Used by visit_type_STRUCT_fields() when STRUCT has
base FOO, and by visit_type_UNION() when flat UNION has member a FOO.

If the schema defines FOO after its first use as struct base or flat
union member, visit_type_implicit_FOO() calls
visit_type_implicit_FOO() before its definition, which doesn't
compile.

Rearrange qapi-schema-test.json to demonstrate the bug.

Fix by generating the necessary forward declaration.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-visit.py                   | 15 ++++++++++++++-
 tests/qapi-schema/qapi-schema-test.json | 30 +++++++++++++++++-------------
 tests/qapi-schema/qapi-schema-test.out  | 10 +++++-----
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4ec79a6..b3a308f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,13 +17,23 @@ from qapi import *
 import re
 
 implicit_structs = []
+struct_fields_seen = set()
 
 def generate_visit_implicit_struct(type):
     global implicit_structs
     if type in implicit_structs:
         return ''
     implicit_structs.append(type)
-    return mcgen('''
+    ret = ''
+    if type not in struct_fields_seen:
+        # Need a forward declaration
+        ret += mcgen('''
+
+static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **errp);
+''',
+                     c_type=type_name(type))
+
+    ret += mcgen('''
 
 static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
 {
@@ -38,8 +48,11 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error *
 }
 ''',
                  c_type=type_name(type))
+    return ret
 
 def generate_visit_struct_fields(name, members, base = None):
+    struct_fields_seen.add(name)
+
     ret = ''
 
     if base:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c7eaa86..ccadb13 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -7,13 +7,13 @@
   'data': { 'enum1': 'EnumOne', '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
 
 # for testing nested structs
-{ 'struct': 'UserDefZero',
-  'data': { 'integer': 'int' } }
-
 { 'struct': 'UserDefOne',
-  'base': 'UserDefZero',
+  'base': 'UserDefZero',        # intentional forward reference
   'data': { 'string': 'str', '*enum1': 'EnumOne' } }
 
+{ 'struct': 'UserDefZero',
+  'data': { 'integer': 'int' } }
+
 { 'struct': 'UserDefTwoDictDict',
   'data': { 'userdef': 'UserDefOne', 'string': 'str' } }
 
@@ -33,29 +33,33 @@
 { 'struct': 'UserDefB',
   'data': { 'integer': 'int' } }
 
-{ 'struct': 'UserDefC',
-  'data': { 'string1': 'str', 'string2': 'str' } }
-
-{ 'struct': 'UserDefUnionBase',
-  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
-
 { 'union': 'UserDefFlatUnion',
-  'base': 'UserDefUnionBase',
+  'base': 'UserDefUnionBase',   # intentional forward reference
   'discriminator': 'enum1',
-  'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
+  'data': { 'value1' : 'UserDefA',
+            'value2' : 'UserDefB',
+            'value3' : 'UserDefB' } }
 # FIXME generated struct UserDefFlatUnion has members for direct base
 # UserDefOne, but lacks members for indirect base UserDefZero
 
+{ 'struct': 'UserDefUnionBase',
+  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
+
 # this variant of UserDefFlatUnion defaults to a union that uses fields with
 # allocated types to test corner cases in the cleanup/dealloc visitor
 { 'union': 'UserDefFlatUnion2',
   'base': 'UserDefUnionBase',
   'discriminator': 'enum1',
-  'data': { 'value1' : 'UserDefC', 'value2' : 'UserDefB', 'value3' : 'UserDefA' } }
+  'data': { 'value1' : 'UserDefC', # intentional forward reference
+            'value2' : 'UserDefB',
+            'value3' : 'UserDefA' } }
 
 { 'alternate': 'UserDefAlternate',
   'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
 
+{ 'struct': 'UserDefC',
+  'data': { 'string1': 'str', 'string2': 'str' } }
+
 # for testing native lists
 { 'union': 'UserDefNativeListUnion',
   'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index cf0ccc4..a4291db 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,17 +1,17 @@
 [OrderedDict([('enum', 'EnumOne'), ('data', ['value1', 'value2', 'value3'])]),
  OrderedDict([('struct', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
- OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]),
  OrderedDict([('struct', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
  OrderedDict([('struct', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('struct', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
- OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
+ OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
  OrderedDict([('alternate', 'UserDefAlternate'), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
+ OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  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']), ('sizes', ['size'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
@@ -39,15 +39,15 @@
  {'enum_name': '__org.qemu_x-Union1Kind', 'enum_values': None},
  {'enum_name': '__org.qemu_x-AltKind', 'enum_values': None}]
 [OrderedDict([('struct', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
- OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('struct', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]),
  OrderedDict([('struct', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
  OrderedDict([('struct', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('struct', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
  OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))]),
  OrderedDict([('struct', '__org.qemu_x-Base'), ('data', OrderedDict([('__org.qemu_x-member1', '__org.qemu_x-Enum')]))]),
-- 
2.4.3

[Qemu-devel] [PATCH 10/26] qapi-visit: Replace list implicit_structs by set

[Markus Armbruster]

Use set because that's what it is.  While there, rename to
implicit_structs_seen.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-visit.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b3a308f..9fc040e 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -16,14 +16,13 @@ from ordereddict import OrderedDict
 from qapi import *
 import re
 
-implicit_structs = []
+implicit_structs_seen = set()
 struct_fields_seen = set()
 
 def generate_visit_implicit_struct(type):
-    global implicit_structs
-    if type in implicit_structs:
+    if type in implicit_structs_seen:
         return ''
-    implicit_structs.append(type)
+    implicit_structs_seen.add(type)
     ret = ''
     if type not in struct_fields_seen:
         # Need a forward declaration
-- 
2.4.3

[Qemu-devel] [PATCH 11/26] qapi-visit: Fix two name arguments passed to visitors

[Markus Armbruster]

The generated code passes mangled schema names to visit_type_enum()
and union's visit_start_struct().  Fix it to pass the names
unadulterated, like we do everywhere else.

Only qapi-schema-test.json actually has names where this makes a
difference: enum __org.qemu_x-Enum, flat union __org.qemu_x-Union2,
simple union __org.qemu_x-Union1 and its implicit enum
__org.qemu_x-Union1Kind.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-visit.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 9fc040e..73f136f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -185,12 +185,12 @@ out:
 def generate_visit_enum(name):
     return mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error **errp)
 {
-    visit_type_enum(m, (int *)obj, %(name)s_lookup, "%(name)s", name, errp);
+    visit_type_enum(m, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
 }
 ''',
-                 name=c_name(name))
+                 c_name=c_name(name), name=name)
 
 def generate_visit_alternate(name, members):
     ret = mcgen('''
@@ -278,17 +278,17 @@ def generate_visit_union(expr):
 
     ret += mcgen('''
 
-void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
 
-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
+    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
     if (*obj) {
 ''',
-                 name=c_name(name))
+                 c_name=c_name(name), name=name)
 
     if base:
         ret += mcgen('''
-- 
2.4.3

[Qemu-devel] [PATCH 12/26] tests/qapi-schema: Document alternate's enum lacks visit function

[Markus Armbruster]

We generate a declaration, but no definition.

The QMP schema has two: Qcow2OverlapChecks and BlockdevRef.  Neither
visit_type_Qcow2OverlapChecksKind() nor visit_type_BlockdevRefKind()
is actually used.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index ccadb13..8337ba9 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -56,6 +56,7 @@
 
 { 'alternate': 'UserDefAlternate',
   'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } }
+# FIXME only a declaration of visit_type_UserDefAlternateKind() generated
 
 { 'struct': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
-- 
2.4.3

[Qemu-devel] [PATCH 13/26] tests/qapi-schema: Document events with base don't work

[Markus Armbruster]

When event FOO's 'data' is a struct with a base, we consider only the
struct's direct members, and ignore its base.  The generated
qapi_event_send_foo() doesn't take arguments for base members.

No such events currently exist in the QMP schema.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 8337ba9..829dd30 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -128,6 +128,9 @@
 { 'alternate': '__org.qemu_x-Alt',
   'data': { '__org.qemu_x-branch': 'str', 'b': '__org.qemu_x-Base' } }
 { 'event': '__ORG.QEMU_X-EVENT', 'data': '__org.qemu_x-Struct' }
+# FIXME generated qapi_event_send___org_qemu_x_event() has only a
+# parameter for data's member __org_qemu_x_member2, none for its base
+# __org.qemu_x-Base's member __org_qemu_x_member1
 { 'command': '__org.qemu_x-command',
   'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'],
             'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' },
-- 
2.4.3

[Qemu-devel] [PATCH 14/26] qapi: Document that input visitor semantics are prone to leaks

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

Most functions that can return a pointer or set an Error ** value
are decent enough to guarantee a NULL return when reporting an error.
Not so with our generated qapi visitor functions.  If the caller
is not careful to clean up partially-allocated objects on error,
then the caller suffers a memory leak.

Properly fixing it is probably complex enough to save for a later
day, so merely document it for now.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1438295587-19069-1-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-visit.py          | 4 ++++
 tests/test-qmp-input-visitor.c | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 73f136f..eec5f1f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -115,6 +115,10 @@ out:
 
 
 def generate_visit_struct_body(name):
+    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
+    # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
+    # rather than leaving it non-NULL. As currently written, the caller must
+    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret = mcgen('''
     Error *err = NULL;
 
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index b7a87ee..a5cfefa 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -636,6 +636,8 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
 
     visit_type_TestStruct(v, &p, NULL, &err);
     g_assert(err);
+    /* FIXME - a failed parse should not leave a partially-allocated p
+     * for us to clean up; this could cause callers to leak memory. */
     g_assert(p->string == NULL);
 
     error_free(err);
-- 
2.4.3

[Qemu-devel] [PATCH 15/26] qapi: Document shortcoming with union 'data' branch

[Markus Armbruster]

From: Eric Blake <eblake@redhat.com>

Add a FIXME to remind us to fully audit whether removing the
'void *data' branch of each qapi union type can be done safely.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <1438297637-26789-1-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-types.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 82141cd..8444f98 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -219,6 +219,14 @@ struct %(name)s
 ''',
                      discriminator_type_name=c_name(discriminator_type_name))
 
+    # FIXME: What purpose does data serve, besides preventing a union that
+    # has a branch named 'data'? We use it in qapi-visit.py to decide
+    # whether to bypass the switch statement if visiting the discriminator
+    # failed; but since we 0-initialize structs, and cannot tell what
+    # branch of the union is in use if the discriminator is invalid, there
+    # should not be any data leaks even without a data pointer.  Or, if
+    # 'data' is merely added to guarantee we don't have an empty union,
+    # shouldn't we enforce that at .json parse time?
     ret += mcgen('''
     union { /* union tag is @%(c_name)s */
         void *data;
-- 
2.4.3

[Qemu-devel] [PATCH 16/26] qapi: Document flaws in checking of names

[Markus Armbruster]

We don't actually enforce our "other than downstream extensions [...],
all names should begin with a letter" rule.  Add a FIXME.

We should reject names that differ only in '_' vs. '.'  vs. '-',
because they're liable to clash in generated C.  Add a FIXME.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4af47ef..e61db30 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -341,6 +341,8 @@ def discriminator_find_enum_define(expr):
 
     return find_enum(discriminator_type)
 
+# FIXME should enforce "other than downstream extensions [...], all
+# names should begin with a letter".
 valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
 def check_name(expr_info, source, name, allow_optional = False,
                enum_member = False):
@@ -367,6 +369,8 @@ def check_name(expr_info, source, name, allow_optional = False,
 def add_name(name, info, meta, implicit = False):
     global all_names
     check_name(info, "'%s'" % meta, name)
+    # FIXME should reject names that differ only in '_' vs. '.'
+    # vs. '-', because they're liable to clash in generated C.
     if name in all_names:
         raise QAPIExprError(info,
                             "%s '%s' is already defined"
-- 
2.4.3

[Qemu-devel] [PATCH 17/26] tests/qapi-schema: Restore test case for flat union base bug

[Markus Armbruster]

Test case added in commit 2fc0043, and messed up in commit 5223070.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 5 +++--
 tests/qapi-schema/qapi-schema-test.out  | 8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 829dd30..a9e5aab 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -31,7 +31,7 @@
   'data': { 'boolean': 'bool' } }
 
 { 'struct': 'UserDefB',
-  'data': { 'integer': 'int' } }
+  'data': { 'intb': 'int' } }
 
 { 'union': 'UserDefFlatUnion',
   'base': 'UserDefUnionBase',   # intentional forward reference
@@ -40,9 +40,10 @@
             'value2' : 'UserDefB',
             'value3' : 'UserDefB' } }
 # FIXME generated struct UserDefFlatUnion has members for direct base
-# UserDefOne, but lacks members for indirect base UserDefZero
+# UserDefUnionBase, but lacks members for indirect base UserDefZero
 
 { 'struct': 'UserDefUnionBase',
+  'base': 'UserDefZero',
   'data': { 'string': 'str', 'enum1': 'EnumOne' } }
 
 # this variant of UserDefFlatUnion defaults to a union that uses fields with
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index a4291db..b0b7187 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -6,9 +6,9 @@
  OrderedDict([('struct', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
  OrderedDict([('struct', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('struct', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
- OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('intb', 'int')]))]),
  OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
- OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefUnionBase'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]),
  OrderedDict([('alternate', 'UserDefAlternate'), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
@@ -45,8 +45,8 @@
  OrderedDict([('struct', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
  OrderedDict([('struct', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('struct', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
- OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
- OrderedDict([('struct', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('struct', 'UserDefB'), ('data', OrderedDict([('intb', 'int')]))]),
+ OrderedDict([('struct', 'UserDefUnionBase'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('struct', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
  OrderedDict([('struct', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))]),
  OrderedDict([('struct', 'EventStructOne'), ('data', OrderedDict([('struct1', 'UserDefOne'), ('string', 'str'), ('*enum2', 'EnumOne')]))]),
-- 
2.4.3

[Qemu-devel] [PATCH 18/26] tests/qapi-schema: Rename tests from data- to args-

[Markus Armbruster]

Since every schema entity has 'data', the data- prefix conveys no
information.  These tests actually exercise commands.  Only commands
have arguments, so change the prefix to to args-.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile                                                      | 6 +++---
 tests/qapi-schema/{data-array-empty.err => args-array-empty.err}    | 2 +-
 tests/qapi-schema/{data-unknown.exit => args-array-empty.exit}      | 0
 tests/qapi-schema/{data-array-empty.json => args-array-empty.json}  | 0
 tests/qapi-schema/{data-unknown.out => args-array-empty.out}        | 0
 .../qapi-schema/{data-array-unknown.err => args-array-unknown.err}  | 2 +-
 .../{data-member-unknown.exit => args-array-unknown.exit}           | 0
 .../{data-array-unknown.json => args-array-unknown.json}            | 0
 .../qapi-schema/{data-member-unknown.out => args-array-unknown.out} | 0
 tests/qapi-schema/args-int.err                                      | 1 +
 tests/qapi-schema/{data-member-array-bad.exit => args-int.exit}     | 0
 tests/qapi-schema/{data-int.json => args-int.json}                  | 0
 tests/qapi-schema/{data-member-array.err => args-int.out}           | 0
 .../{data-member-array-bad.err => args-member-array-bad.err}        | 2 +-
 tests/qapi-schema/{data-int.exit => args-member-array-bad.exit}     | 0
 .../{data-member-array-bad.json => args-member-array-bad.json}      | 0
 .../{data-member-array-bad.out => args-member-array-bad.out}        | 0
 tests/qapi-schema/{data-int.out => args-member-array.err}           | 0
 .../qapi-schema/{data-member-array.exit => args-member-array.exit}  | 0
 .../qapi-schema/{data-member-array.json => args-member-array.json}  | 0
 tests/qapi-schema/{data-member-array.out => args-member-array.out}  | 0
 tests/qapi-schema/args-member-unknown.err                           | 1 +
 .../{data-array-unknown.exit => args-member-unknown.exit}           | 0
 .../{data-member-unknown.json => args-member-unknown.json}          | 0
 .../qapi-schema/{data-array-unknown.out => args-member-unknown.out} | 0
 tests/qapi-schema/args-unknown.err                                  | 1 +
 tests/qapi-schema/{data-array-empty.exit => args-unknown.exit}      | 0
 tests/qapi-schema/{data-unknown.json => args-unknown.json}          | 0
 tests/qapi-schema/{data-array-empty.out => args-unknown.out}        | 0
 tests/qapi-schema/data-int.err                                      | 1 -
 tests/qapi-schema/data-member-unknown.err                           | 1 -
 tests/qapi-schema/data-unknown.err                                  | 1 -
 32 files changed, 9 insertions(+), 9 deletions(-)
 rename tests/qapi-schema/{data-array-empty.err => args-array-empty.err} (50%)
 rename tests/qapi-schema/{data-unknown.exit => args-array-empty.exit} (100%)
 rename tests/qapi-schema/{data-array-empty.json => args-array-empty.json} (100%)
 rename tests/qapi-schema/{data-unknown.out => args-array-empty.out} (100%)
 rename tests/qapi-schema/{data-array-unknown.err => args-array-unknown.err} (50%)
 rename tests/qapi-schema/{data-member-unknown.exit => args-array-unknown.exit} (100%)
 rename tests/qapi-schema/{data-array-unknown.json => args-array-unknown.json} (100%)
 rename tests/qapi-schema/{data-member-unknown.out => args-array-unknown.out} (100%)
 create mode 100644 tests/qapi-schema/args-int.err
 rename tests/qapi-schema/{data-member-array-bad.exit => args-int.exit} (100%)
 rename tests/qapi-schema/{data-int.json => args-int.json} (100%)
 rename tests/qapi-schema/{data-member-array.err => args-int.out} (100%)
 rename tests/qapi-schema/{data-member-array-bad.err => args-member-array-bad.err} (52%)
 rename tests/qapi-schema/{data-int.exit => args-member-array-bad.exit} (100%)
 rename tests/qapi-schema/{data-member-array-bad.json => args-member-array-bad.json} (100%)
 rename tests/qapi-schema/{data-member-array-bad.out => args-member-array-bad.out} (100%)
 rename tests/qapi-schema/{data-int.out => args-member-array.err} (100%)
 rename tests/qapi-schema/{data-member-array.exit => args-member-array.exit} (100%)
 rename tests/qapi-schema/{data-member-array.json => args-member-array.json} (100%)
 rename tests/qapi-schema/{data-member-array.out => args-member-array.out} (100%)
 create mode 100644 tests/qapi-schema/args-member-unknown.err
 rename tests/qapi-schema/{data-array-unknown.exit => args-member-unknown.exit} (100%)
 rename tests/qapi-schema/{data-member-unknown.json => args-member-unknown.json} (100%)
 rename tests/qapi-schema/{data-array-unknown.out => args-member-unknown.out} (100%)
 create mode 100644 tests/qapi-schema/args-unknown.err
 rename tests/qapi-schema/{data-array-empty.exit => args-unknown.exit} (100%)
 rename tests/qapi-schema/{data-unknown.json => args-unknown.json} (100%)
 rename tests/qapi-schema/{data-array-empty.out => args-unknown.out} (100%)
 delete mode 100644 tests/qapi-schema/data-int.err
 delete mode 100644 tests/qapi-schema/data-member-unknown.err
 delete mode 100644 tests/qapi-schema/data-unknown.err

diff --git a/tests/Makefile b/tests/Makefile
index 7494582..0f01b5c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -229,9 +229,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
 	redefined-type.json redefined-command.json redefined-builtin.json \
 	redefined-event.json command-int.json bad-data.json event-max.json \
 	type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
-	data-array-empty.json data-array-unknown.json data-int.json \
-	data-unknown.json data-member-unknown.json data-member-array.json \
-	data-member-array-bad.json returns-array-bad.json returns-int.json \
+	args-array-empty.json args-array-unknown.json args-int.json \
+	args-unknown.json args-member-unknown.json args-member-array.json \
+	args-member-array-bad.json returns-array-bad.json returns-int.json \
 	returns-unknown.json returns-alternate.json returns-whitelist.json \
 	missing-colon.json missing-comma-list.json missing-comma-object.json \
 	nested-struct-data.json nested-struct-returns.json non-objects.json \
diff --git a/tests/qapi-schema/data-array-empty.err b/tests/qapi-schema/args-array-empty.err
similarity index 50%
rename from tests/qapi-schema/data-array-empty.err
rename to tests/qapi-schema/args-array-empty.err
index f713f14..cb7ed33 100644
--- a/tests/qapi-schema/data-array-empty.err
+++ b/tests/qapi-schema/args-array-empty.err
@@ -1 +1 @@
-tests/qapi-schema/data-array-empty.json:2: Member 'empty' of 'data' for command 'oops': array type must contain single type name
+tests/qapi-schema/args-array-empty.json:2: Member 'empty' of 'data' for command 'oops': array type must contain single type name
diff --git a/tests/qapi-schema/data-unknown.exit b/tests/qapi-schema/args-array-empty.exit
similarity index 100%
rename from tests/qapi-schema/data-unknown.exit
rename to tests/qapi-schema/args-array-empty.exit
diff --git a/tests/qapi-schema/data-array-empty.json b/tests/qapi-schema/args-array-empty.json
similarity index 100%
rename from tests/qapi-schema/data-array-empty.json
rename to tests/qapi-schema/args-array-empty.json
diff --git a/tests/qapi-schema/data-unknown.out b/tests/qapi-schema/args-array-empty.out
similarity index 100%
rename from tests/qapi-schema/data-unknown.out
rename to tests/qapi-schema/args-array-empty.out
diff --git a/tests/qapi-schema/data-array-unknown.err b/tests/qapi-schema/args-array-unknown.err
similarity index 50%
rename from tests/qapi-schema/data-array-unknown.err
rename to tests/qapi-schema/args-array-unknown.err
index 8b731bb..7834d11 100644
--- a/tests/qapi-schema/data-array-unknown.err
+++ b/tests/qapi-schema/args-array-unknown.err
@@ -1 +1 @@
-tests/qapi-schema/data-array-unknown.json:2: Member 'array' of 'data' for command 'oops' uses unknown type 'array of NoSuchType'
+tests/qapi-schema/args-array-unknown.json:2: Member 'array' of 'data' for command 'oops' uses unknown type 'array of NoSuchType'
diff --git a/tests/qapi-schema/data-member-unknown.exit b/tests/qapi-schema/args-array-unknown.exit
similarity index 100%
rename from tests/qapi-schema/data-member-unknown.exit
rename to tests/qapi-schema/args-array-unknown.exit
diff --git a/tests/qapi-schema/data-array-unknown.json b/tests/qapi-schema/args-array-unknown.json
similarity index 100%
rename from tests/qapi-schema/data-array-unknown.json
rename to tests/qapi-schema/args-array-unknown.json
diff --git a/tests/qapi-schema/data-member-unknown.out b/tests/qapi-schema/args-array-unknown.out
similarity index 100%
rename from tests/qapi-schema/data-member-unknown.out
rename to tests/qapi-schema/args-array-unknown.out
diff --git a/tests/qapi-schema/args-int.err b/tests/qapi-schema/args-int.err
new file mode 100644
index 0000000..dc1d250
--- /dev/null
+++ b/tests/qapi-schema/args-int.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-int.json:2: 'data' for command 'oops' cannot use built-in type 'int'
diff --git a/tests/qapi-schema/data-member-array-bad.exit b/tests/qapi-schema/args-int.exit
similarity index 100%
rename from tests/qapi-schema/data-member-array-bad.exit
rename to tests/qapi-schema/args-int.exit
diff --git a/tests/qapi-schema/data-int.json b/tests/qapi-schema/args-int.json
similarity index 100%
rename from tests/qapi-schema/data-int.json
rename to tests/qapi-schema/args-int.json
diff --git a/tests/qapi-schema/data-member-array.err b/tests/qapi-schema/args-int.out
similarity index 100%
rename from tests/qapi-schema/data-member-array.err
rename to tests/qapi-schema/args-int.out
diff --git a/tests/qapi-schema/data-member-array-bad.err b/tests/qapi-schema/args-member-array-bad.err
similarity index 52%
rename from tests/qapi-schema/data-member-array-bad.err
rename to tests/qapi-schema/args-member-array-bad.err
index 2c072d5..881b4d9 100644
--- a/tests/qapi-schema/data-member-array-bad.err
+++ b/tests/qapi-schema/args-member-array-bad.err
@@ -1 +1 @@
-tests/qapi-schema/data-member-array-bad.json:2: Member 'member' of 'data' for command 'oops': array type must contain single type name
+tests/qapi-schema/args-member-array-bad.json:2: Member 'member' of 'data' for command 'oops': array type must contain single type name
diff --git a/tests/qapi-schema/data-int.exit b/tests/qapi-schema/args-member-array-bad.exit
similarity index 100%
rename from tests/qapi-schema/data-int.exit
rename to tests/qapi-schema/args-member-array-bad.exit
diff --git a/tests/qapi-schema/data-member-array-bad.json b/tests/qapi-schema/args-member-array-bad.json
similarity index 100%
rename from tests/qapi-schema/data-member-array-bad.json
rename to tests/qapi-schema/args-member-array-bad.json
diff --git a/tests/qapi-schema/data-member-array-bad.out b/tests/qapi-schema/args-member-array-bad.out
similarity index 100%
rename from tests/qapi-schema/data-member-array-bad.out
rename to tests/qapi-schema/args-member-array-bad.out
diff --git a/tests/qapi-schema/data-int.out b/tests/qapi-schema/args-member-array.err
similarity index 100%
rename from tests/qapi-schema/data-int.out
rename to tests/qapi-schema/args-member-array.err
diff --git a/tests/qapi-schema/data-member-array.exit b/tests/qapi-schema/args-member-array.exit
similarity index 100%
rename from tests/qapi-schema/data-member-array.exit
rename to tests/qapi-schema/args-member-array.exit
diff --git a/tests/qapi-schema/data-member-array.json b/tests/qapi-schema/args-member-array.json
similarity index 100%
rename from tests/qapi-schema/data-member-array.json
rename to tests/qapi-schema/args-member-array.json
diff --git a/tests/qapi-schema/data-member-array.out b/tests/qapi-schema/args-member-array.out
similarity index 100%
rename from tests/qapi-schema/data-member-array.out
rename to tests/qapi-schema/args-member-array.out
diff --git a/tests/qapi-schema/args-member-unknown.err b/tests/qapi-schema/args-member-unknown.err
new file mode 100644
index 0000000..f6f8282
--- /dev/null
+++ b/tests/qapi-schema/args-member-unknown.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-member-unknown.json:2: Member 'member' of 'data' for command 'oops' uses unknown type 'NoSuchType'
diff --git a/tests/qapi-schema/data-array-unknown.exit b/tests/qapi-schema/args-member-unknown.exit
similarity index 100%
rename from tests/qapi-schema/data-array-unknown.exit
rename to tests/qapi-schema/args-member-unknown.exit
diff --git a/tests/qapi-schema/data-member-unknown.json b/tests/qapi-schema/args-member-unknown.json
similarity index 100%
rename from tests/qapi-schema/data-member-unknown.json
rename to tests/qapi-schema/args-member-unknown.json
diff --git a/tests/qapi-schema/data-array-unknown.out b/tests/qapi-schema/args-member-unknown.out
similarity index 100%
rename from tests/qapi-schema/data-array-unknown.out
rename to tests/qapi-schema/args-member-unknown.out
diff --git a/tests/qapi-schema/args-unknown.err b/tests/qapi-schema/args-unknown.err
new file mode 100644
index 0000000..4d91ec8
--- /dev/null
+++ b/tests/qapi-schema/args-unknown.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-unknown.json:2: 'data' for command 'oops' uses unknown type 'NoSuchType'
diff --git a/tests/qapi-schema/data-array-empty.exit b/tests/qapi-schema/args-unknown.exit
similarity index 100%
rename from tests/qapi-schema/data-array-empty.exit
rename to tests/qapi-schema/args-unknown.exit
diff --git a/tests/qapi-schema/data-unknown.json b/tests/qapi-schema/args-unknown.json
similarity index 100%
rename from tests/qapi-schema/data-unknown.json
rename to tests/qapi-schema/args-unknown.json
diff --git a/tests/qapi-schema/data-array-empty.out b/tests/qapi-schema/args-unknown.out
similarity index 100%
rename from tests/qapi-schema/data-array-empty.out
rename to tests/qapi-schema/args-unknown.out
diff --git a/tests/qapi-schema/data-int.err b/tests/qapi-schema/data-int.err
deleted file mode 100644
index 1a9b077..0000000
--- a/tests/qapi-schema/data-int.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/data-int.json:2: 'data' for command 'oops' cannot use built-in type 'int'
diff --git a/tests/qapi-schema/data-member-unknown.err b/tests/qapi-schema/data-member-unknown.err
deleted file mode 100644
index ab905db..0000000
--- a/tests/qapi-schema/data-member-unknown.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/data-member-unknown.json:2: Member 'member' of 'data' for command 'oops' uses unknown type 'NoSuchType'
diff --git a/tests/qapi-schema/data-unknown.err b/tests/qapi-schema/data-unknown.err
deleted file mode 100644
index 5b07277..0000000
--- a/tests/qapi-schema/data-unknown.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/data-unknown.json:2: 'data' for command 'oops' uses unknown type 'NoSuchType'
-- 
2.4.3

[Qemu-devel] [PATCH 19/26] qapi-tests: New tests for union, alternate command arguments

[Markus Armbruster]

A command's 'data' must be a struct type, given either as a
dictionary, or as struct type name.

Existing test case data-int.json covers simple type 'int'.  Add test
cases for type names referring to union and alternate types.

The latter is caught (good), but the former is not (bug).

Events have the same problem, but since they get checked by the same
code, we don't bother to duplicate the tests.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/Makefile                        | 3 ++-
 tests/qapi-schema/args-alternate.err  | 1 +
 tests/qapi-schema/args-alternate.exit | 1 +
 tests/qapi-schema/args-alternate.json | 3 +++
 tests/qapi-schema/args-alternate.out  | 0
 tests/qapi-schema/args-union.err      | 0
 tests/qapi-schema/args-union.exit     | 1 +
 tests/qapi-schema/args-union.json     | 4 ++++
 tests/qapi-schema/args-union.out      | 4 ++++
 9 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 tests/qapi-schema/args-alternate.err
 create mode 100644 tests/qapi-schema/args-alternate.exit
 create mode 100644 tests/qapi-schema/args-alternate.json
 create mode 100644 tests/qapi-schema/args-alternate.out
 create mode 100644 tests/qapi-schema/args-union.err
 create mode 100644 tests/qapi-schema/args-union.exit
 create mode 100644 tests/qapi-schema/args-union.json
 create mode 100644 tests/qapi-schema/args-union.out

diff --git a/tests/Makefile b/tests/Makefile
index 0f01b5c..fd0ae45 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -231,7 +231,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
 	type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
 	args-array-empty.json args-array-unknown.json args-int.json \
 	args-unknown.json args-member-unknown.json args-member-array.json \
-	args-member-array-bad.json returns-array-bad.json returns-int.json \
+	args-member-array-bad.json args-alternate.json args-union.json \
+	returns-array-bad.json returns-int.json \
 	returns-unknown.json returns-alternate.json returns-whitelist.json \
 	missing-colon.json missing-comma-list.json missing-comma-object.json \
 	nested-struct-data.json nested-struct-returns.json non-objects.json \
diff --git a/tests/qapi-schema/args-alternate.err b/tests/qapi-schema/args-alternate.err
new file mode 100644
index 0000000..3086eae
--- /dev/null
+++ b/tests/qapi-schema/args-alternate.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-alternate.json:3: 'data' for command 'oops' cannot use alternate type 'Alt'
diff --git a/tests/qapi-schema/args-alternate.exit b/tests/qapi-schema/args-alternate.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-alternate.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-alternate.json b/tests/qapi-schema/args-alternate.json
new file mode 100644
index 0000000..69e94d4
--- /dev/null
+++ b/tests/qapi-schema/args-alternate.json
@@ -0,0 +1,3 @@
+# we do not allow alternate arguments
+{ 'alternate': 'Alt', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Alt' }
diff --git a/tests/qapi-schema/args-alternate.out b/tests/qapi-schema/args-alternate.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-union.exit b/tests/qapi-schema/args-union.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/args-union.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/args-union.json b/tests/qapi-schema/args-union.json
new file mode 100644
index 0000000..db97ef6
--- /dev/null
+++ b/tests/qapi-schema/args-union.json
@@ -0,0 +1,4 @@
+# FIXME we should reject union arguments
+# TODO should we support this?
+{ 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'command': 'oops', 'data': 'Uni' }
diff --git a/tests/qapi-schema/args-union.out b/tests/qapi-schema/args-union.out
new file mode 100644
index 0000000..907080c
--- /dev/null
+++ b/tests/qapi-schema/args-union.out
@@ -0,0 +1,4 @@
+[OrderedDict([('union', 'Uni'), ('data', OrderedDict([('case1', 'int'), ('case2', 'str')]))]),
+ OrderedDict([('command', 'oops'), ('data', 'Uni')])]
+[{'enum_name': 'UniKind', 'enum_values': None}]
+[]
-- 
2.4.3

[Qemu-devel] [PATCH 20/26] qapi: Fix to reject union command and event arguments

[Markus Armbruster]

A command's or event's 'data' must be a struct type, given either as a
dictionary, or as struct type name.

Commit dd883c6 tightened the checking there, but not enough: we still
accept 'union'.  Fix to reject it.

We may want to support union types there, but we'll have to extend
qapi-commands.py and qapi-events.py for it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py                   | 4 ++--
 tests/qapi-schema/args-union.err  | 1 +
 tests/qapi-schema/args-union.exit | 2 +-
 tests/qapi-schema/args-union.json | 2 +-
 tests/qapi-schema/args-union.out  | 4 ----
 5 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index e61db30..ec7923a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -499,7 +499,7 @@ def check_command(expr, expr_info):
 
     check_type(expr_info, "'data' for command '%s'" % name,
                expr.get('data'), allow_dict=True, allow_optional=True,
-               allow_metas=['union', 'struct'], allow_star=allow_star)
+               allow_metas=['struct'], allow_star=allow_star)
     returns_meta = ['union', 'struct']
     if name in returns_whitelist:
         returns_meta += ['built-in', 'alternate', 'enum']
@@ -517,7 +517,7 @@ def check_event(expr, expr_info):
     events.append(name)
     check_type(expr_info, "'data' for event '%s'" % name,
                expr.get('data'), allow_dict=True, allow_optional=True,
-               allow_metas=['union', 'struct'])
+               allow_metas=['struct'])
 
 def check_union(expr, expr_info):
     name = expr['union']
diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err
index e69de29..1d693d7 100644
--- a/tests/qapi-schema/args-union.err
+++ b/tests/qapi-schema/args-union.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-union.json:4: 'data' for command 'oops' cannot use union type 'Uni'
diff --git a/tests/qapi-schema/args-union.exit b/tests/qapi-schema/args-union.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-union.exit
+++ b/tests/qapi-schema/args-union.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-union.json b/tests/qapi-schema/args-union.json
index db97ef6..7bdcbb7 100644
--- a/tests/qapi-schema/args-union.json
+++ b/tests/qapi-schema/args-union.json
@@ -1,4 +1,4 @@
-# FIXME we should reject union arguments
+# we do not allow union arguments
 # TODO should we support this?
 { 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
 { 'command': 'oops', 'data': 'Uni' }
diff --git a/tests/qapi-schema/args-union.out b/tests/qapi-schema/args-union.out
index 907080c..e69de29 100644
--- a/tests/qapi-schema/args-union.out
+++ b/tests/qapi-schema/args-union.out
@@ -1,4 +0,0 @@
-[OrderedDict([('union', 'Uni'), ('data', OrderedDict([('case1', 'int'), ('case2', 'str')]))]),
- OrderedDict([('command', 'oops'), ('data', 'Uni')])]
-[{'enum_name': 'UniKind', 'enum_values': None}]
-[]
-- 
2.4.3

[Qemu-devel] [PATCH 21/26] qapi: Command returning anonymous type doesn't work, outlaw

[Markus Armbruster]

Reproducer: with

    { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }

added to qapi-schema-test.json, qapi-commands.py dies when it tries to
generate the command handler function

    Traceback (most recent call last):
      File "/work/armbru/qemu/scripts/qapi-commands.py", line 359, in <module>
        ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
      File "/work/armbru/qemu/scripts/qapi-commands.py", line 29, in generate_command_decl
        ret_type=c_type(ret_type), name=c_name(name),
      File "/work/armbru/qemu/scripts/qapi.py", line 927, in c_type
        assert isinstance(value, str) and value != ""
    AssertionError

because the return type doesn't exist.

Simply outlaw this usage.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt                                  | 17 ++++++++---------
 scripts/qapi.py                                         |  2 +-
 tests/Makefile                                          |  4 ++--
 tests/qapi-schema/nested-struct-returns.err             |  1 -
 tests/qapi-schema/nested-struct-returns.json            |  3 ---
 tests/qapi-schema/returns-dict.err                      |  1 +
 .../{nested-struct-returns.exit => returns-dict.exit}   |  0
 tests/qapi-schema/returns-dict.json                     |  2 ++
 .../{nested-struct-returns.out => returns-dict.out}     |  0
 9 files changed, 14 insertions(+), 16 deletions(-)
 delete mode 100644 tests/qapi-schema/nested-struct-returns.err
 delete mode 100644 tests/qapi-schema/nested-struct-returns.json
 create mode 100644 tests/qapi-schema/returns-dict.err
 rename tests/qapi-schema/{nested-struct-returns.exit => returns-dict.exit} (100%)
 create mode 100644 tests/qapi-schema/returns-dict.json
 rename tests/qapi-schema/{nested-struct-returns.out => returns-dict.out} (100%)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c2ac21c..279c681 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -394,7 +394,7 @@ following example objects:
 === Commands ===
 
 Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
-         '*returns': TYPE-NAME-OR-DICT,
+         '*returns': TYPE-NAME,
          '*gen': false, '*success-response': false }
 
 Commands are defined by using a dictionary containing several members,
@@ -416,14 +416,13 @@ The member is optional from the command declaration; if absent, the
 "return" field will be an empty dictionary.  If 'returns' is present,
 it must be the string name of a complex or built-in type, a
 one-element array containing the name of a complex or built-in type,
-or a dictionary that declares an anonymous type with the same
-semantics as a 'struct' expression, with one exception noted below
-when 'gen' is used.  Although it is permitted to have the 'returns'
-member name a built-in type or an array of built-in types, any command
-that does this cannot be extended to return additional information in
-the future; thus, new commands should strongly consider returning a
-dictionary-based type or an array of dictionaries, even if the
-dictionary only contains one field at the present.
+with one exception noted below when 'gen' is used.  Although it is
+permitted to have the 'returns' member name a built-in type or an
+array of built-in types, any command that does this cannot be extended
+to return additional information in the future; thus, new commands
+should strongly consider returning a dictionary-based type or an array
+of dictionaries, even if the dictionary only contains one field at the
+present.
 
 All commands in Client JSON Protocol use a dictionary to report
 failure, with no way to specify that in QAPI.  Where the error return
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ec7923a..56efb18 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -504,7 +504,7 @@ def check_command(expr, expr_info):
     if name in returns_whitelist:
         returns_meta += ['built-in', 'alternate', 'enum']
     check_type(expr_info, "'returns' for command '%s'" % name,
-               expr.get('returns'), allow_array=True, allow_dict=True,
+               expr.get('returns'), allow_array=True,
                allow_optional=True, allow_metas=returns_meta,
                allow_star=allow_star)
 
diff --git a/tests/Makefile b/tests/Makefile
index fd0ae45..2d6ca9d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -232,10 +232,10 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
 	args-array-empty.json args-array-unknown.json args-int.json \
 	args-unknown.json args-member-unknown.json args-member-array.json \
 	args-member-array-bad.json args-alternate.json args-union.json \
-	returns-array-bad.json returns-int.json \
+	returns-array-bad.json returns-int.json returns-dict.json \
 	returns-unknown.json returns-alternate.json returns-whitelist.json \
 	missing-colon.json missing-comma-list.json missing-comma-object.json \
-	nested-struct-data.json nested-struct-returns.json non-objects.json \
+	nested-struct-data.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 \
diff --git a/tests/qapi-schema/nested-struct-returns.err b/tests/qapi-schema/nested-struct-returns.err
deleted file mode 100644
index 5238d07..0000000
--- a/tests/qapi-schema/nested-struct-returns.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/nested-struct-returns.json:2: Member 'a' of 'returns' for command 'foo' should be a type name
diff --git a/tests/qapi-schema/nested-struct-returns.json b/tests/qapi-schema/nested-struct-returns.json
deleted file mode 100644
index d2cd047..0000000
--- a/tests/qapi-schema/nested-struct-returns.json
+++ /dev/null
@@ -1,3 +0,0 @@
-# inline subtypes collide with our desired future use of defaults
-{ 'command': 'foo',
-  'returns': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }
diff --git a/tests/qapi-schema/returns-dict.err b/tests/qapi-schema/returns-dict.err
new file mode 100644
index 0000000..eb2d0c4
--- /dev/null
+++ b/tests/qapi-schema/returns-dict.err
@@ -0,0 +1 @@
+tests/qapi-schema/returns-dict.json:2: 'returns' for command 'oops' should be a type name
diff --git a/tests/qapi-schema/nested-struct-returns.exit b/tests/qapi-schema/returns-dict.exit
similarity index 100%
rename from tests/qapi-schema/nested-struct-returns.exit
rename to tests/qapi-schema/returns-dict.exit
diff --git a/tests/qapi-schema/returns-dict.json b/tests/qapi-schema/returns-dict.json
new file mode 100644
index 0000000..1cfef3e
--- /dev/null
+++ b/tests/qapi-schema/returns-dict.json
@@ -0,0 +1,2 @@
+# we reject inline struct return type
+{ 'command': 'oops', 'returns': { 'a': 'str' } }
diff --git a/tests/qapi-schema/nested-struct-returns.out b/tests/qapi-schema/returns-dict.out
similarity index 100%
rename from tests/qapi-schema/nested-struct-returns.out
rename to tests/qapi-schema/returns-dict.out
-- 
2.4.3

[Qemu-devel] [PATCH 22/26] qapi-commands: Fix gen_err_check(e) for e and e != 'local_err'

[Markus Armbruster]

gen_err_check() hard-codes 'local_err' instead of substituting the
argument.  Currently harmless, since all callers pass either None or
'local_err'.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 69029f5..3965ca8 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -29,14 +29,15 @@ def generate_command_decl(name, args, ret_type):
                  ret_type=c_type(ret_type), name=c_name(name),
                  args=arglist).strip()
 
-def gen_err_check(errvar):
-    if errvar:
-        return mcgen('''
-if (local_err) {
+def gen_err_check(err):
+    if not err:
+        return ''
+    return mcgen('''
+if (%(err)s) {
     goto out;
 }
-''')
-    return ''
+''',
+                 err=err)
 
 def gen_sync_call(name, args, ret_type):
     ret = ""
-- 
2.4.3

[Qemu-devel] [PATCH 23/26] qapi-commands: Inline gen_marshal_output_call()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 3965ca8..6de5229 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -57,19 +57,15 @@ def gen_sync_call(name, args, ret_type):
                 name=c_name(name), args=arglist, retval=retval).rstrip()
     if ret_type:
         ret += "\n" + gen_err_check('local_err')
-        ret += "\n" + mcgen('''
-%(marshal_output_call)s
+        ret += mcgen('''
+
+qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
 ''',
-                            marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip()
+                            c_name=c_name(name))
     pop_indent()
     return ret.rstrip()
 
 
-def gen_marshal_output_call(name, ret_type):
-    if not ret_type:
-        return ""
-    return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_name(name)
-
 def gen_visitor_input_containers_decl(args):
     ret = ""
 
-- 
2.4.3

[Qemu-devel] [PATCH 24/26] qapi-commands: Don't feed output of mcgen() to mcgen() again

[Markus Armbruster]

Multiple passes through mcgen() is prone to produce unwanted blank
lines, which we then combat by sprinkling .rstrip() on top.  Just
don't do it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py | 52 +++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 6de5229..cfbd59c 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -27,7 +27,7 @@ def generate_command_decl(name, args, ret_type):
 %(ret_type)s qmp_%(name)s(%(args)sError **errp);
 ''',
                  ret_type=c_type(ret_type), name=c_name(name),
-                 args=arglist).strip()
+                 args=arglist)
 
 def gen_err_check(err):
     if not err:
@@ -52,19 +52,17 @@ def gen_sync_call(name, args, ret_type):
     push_indent()
     ret = mcgen('''
 %(retval)sqmp_%(name)s(%(args)s&local_err);
-
 ''',
-                name=c_name(name), args=arglist, retval=retval).rstrip()
+                name=c_name(name), args=arglist, retval=retval)
     if ret_type:
-        ret += "\n" + gen_err_check('local_err')
+        ret += gen_err_check('local_err')
         ret += mcgen('''
 
 qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
 ''',
                             c_name=c_name(name))
     pop_indent()
-    return ret.rstrip()
-
+    return ret
 
 def gen_visitor_input_containers_decl(args):
     ret = ""
@@ -78,7 +76,7 @@ Visitor *v;
 ''')
     pop_indent()
 
-    return ret.rstrip()
+    return ret
 
 def gen_visitor_input_vars_decl(args):
     ret = ""
@@ -101,7 +99,7 @@ bool has_%(argname)s = false;
                          argname=c_name(argname), argtype=c_type(argtype))
 
     pop_indent()
-    return ret.rstrip()
+    return ret
 
 def gen_visitor_input_block(args, dealloc=False):
     ret = ""
@@ -155,7 +153,7 @@ visit_type_%(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s);
 qapi_dealloc_visitor_cleanup(md);
 ''')
     pop_indent()
-    return ret.rstrip()
+    return ret
 
 def gen_marshal_output(name, ret_type):
     if not ret_type:
@@ -217,26 +215,17 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
                      retval=retval)
 
     if len(args) > 0:
-        ret += mcgen('''
-%(visitor_input_containers_decl)s
-%(visitor_input_vars_decl)s
-
-%(visitor_input_block)s
-
-''',
-                     visitor_input_containers_decl=gen_visitor_input_containers_decl(args),
-                     visitor_input_vars_decl=gen_visitor_input_vars_decl(args),
-                     visitor_input_block=gen_visitor_input_block(args))
+        ret += gen_visitor_input_containers_decl(args)
+        ret += gen_visitor_input_vars_decl(args) + '\n'
+        ret += gen_visitor_input_block(args) + '\n'
     else:
         ret += mcgen('''
 
     (void)args;
 ''')
 
-    ret += mcgen('''
-%(sync_call)s
-''',
-                 sync_call=gen_sync_call(name, args, ret_type))
+    ret += gen_sync_call(name, args, ret_type)
+
     if re.search('^ *goto out\\;', ret, re.MULTILINE):
         ret += mcgen('''
 
@@ -244,11 +233,11 @@ out:
 ''')
     ret += mcgen('''
     error_propagate(errp, local_err);
-%(visitor_input_block_cleanup)s
+''')
+    ret += gen_visitor_input_block(args, dealloc=True)
+    ret += mcgen('''
 }
-''',
-                 visitor_input_block_cleanup=gen_visitor_input_block(args,
-                                                                     dealloc=True))
+''')
     return ret
 
 def gen_registry(commands):
@@ -268,12 +257,13 @@ qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s);
     ret = mcgen('''
 static void qmp_init_marshal(void)
 {
-%(registry)s
+''')
+    ret += registry
+    ret += mcgen('''
 }
 
 qapi_init(qmp_init_marshal);
-''',
-                registry=registry.rstrip())
+''')
     return ret
 
 middle_mode = False
@@ -353,7 +343,7 @@ for cmd in commands:
         arglist = cmd['data']
     if cmd.has_key('returns'):
         ret_type = cmd['returns']
-    ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
+    ret = generate_command_decl(cmd['command'], arglist, ret_type)
     fdecl.write(ret)
     if ret_type:
         ret = gen_marshal_output(cmd['command'], ret_type) + "\n"
-- 
2.4.3

[Qemu-devel] [PATCH 25/26] qapi-commands: Drop useless initialization

[Markus Armbruster]

In generated command handlers, the assignment to retval dominates its
only use.  Therefore, its initialization is useless.  Drop it.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt   | 2 +-
 scripts/qapi-commands.py | 8 ++------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 279c681..6e09fed 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -743,7 +743,7 @@ Example:
     static void qmp_marshal_input_my_command(QDict *args, QObject **ret, Error **errp)
     {
         Error *local_err = NULL;
-        UserDefOne *retval = NULL;
+        UserDefOne *retval;
         QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
         QapiDeallocVisitor *md;
         Visitor *v;
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index cfbd59c..8bf84a7 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -205,14 +205,10 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
                 header=hdr)
 
     if ret_type:
-        if is_c_ptr(ret_type):
-            retval = "    %s retval = NULL;" % c_type(ret_type)
-        else:
-            retval = "    %s retval;" % c_type(ret_type)
         ret += mcgen('''
-%(retval)s
+    %(c_type)s retval;
 ''',
-                     retval=retval)
+                     c_type=c_type(ret_type))
 
     if len(args) > 0:
         ret += gen_visitor_input_containers_decl(args)
-- 
2.4.3

[Qemu-devel] [PATCH 26/26] qapi: Generated code cleanup

[Markus Armbruster]

Clean up white-space, brace placement, and superfluous #ifdef
QAPI_TYPES_BUILTIN_CLEANUP_DEF.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qapi-code-gen.txt   | 12 ++++-----
 scripts/qapi-commands.py |  1 +
 scripts/qapi-event.py    |  3 +--
 scripts/qapi-types.py    | 66 +++++++++++++++++++++++-------------------------
 scripts/qapi-visit.py    |  1 +
 5 files changed, 39 insertions(+), 44 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 6e09fed..da6c6b7 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -569,7 +569,6 @@ Example:
         visit_type_UserDefOne(v, &obj, NULL, NULL);
         qapi_dealloc_visitor_cleanup(md);
     }
-
     $ cat qapi-generated/example-qapi-types.h
 [Uninteresting stuff omitted...]
 
@@ -580,8 +579,7 @@ Example:
 
     typedef struct UserDefOne UserDefOne;
 
-    typedef struct UserDefOneList
-    {
+    typedef struct UserDefOneList {
         union {
             UserDefOne *value;
             uint64_t padding;
@@ -589,10 +587,10 @@ Example:
         struct UserDefOneList *next;
     } UserDefOneList;
 
+
 [Functions on built-in types omitted...]
 
-    struct UserDefOne
-    {
+    struct UserDefOne {
         int64_t integer;
         char *string;
     };
@@ -630,6 +628,7 @@ Example:
     static void visit_type_UserDefOne_fields(Visitor *m, UserDefOne **obj, Error **errp)
     {
         Error *err = NULL;
+
         visit_type_int(m, &(*obj)->integer, "integer", &err);
         if (err) {
             goto out;
@@ -843,8 +842,7 @@ Example:
     void qapi_event_send_my_event(Error **errp);
 
     extern const char *example_QAPIEvent_lookup[];
-    typedef enum example_QAPIEvent
-    {
+    typedef enum example_QAPIEvent {
         EXAMPLE_QAPI_EVENT_MY_EVENT = 0,
         EXAMPLE_QAPI_EVENT_MAX = 1,
     } example_QAPIEvent;
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 8bf84a7..890ce5d 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -218,6 +218,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
         ret += mcgen('''
 
     (void)args;
+
 ''')
 
     ret += gen_sync_call(name, args, ret_type)
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 88b0620..7f238df 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -167,8 +167,7 @@ extern const char *%(event_enum_name)s_lookup[];
                         event_enum_name = event_enum_name)
 
     enum_decl = mcgen('''
-typedef enum %(event_enum_name)s
-{
+typedef enum %(event_enum_name)s {
 ''',
                       event_enum_name = event_enum_name)
 
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 8444f98..f2428f3 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -15,8 +15,7 @@ from qapi import *
 def generate_fwd_builtin(name):
     return mcgen('''
 
-typedef struct %(name)sList
-{
+typedef struct %(name)sList {
     union {
         %(type)s value;
         uint64_t padding;
@@ -32,8 +31,7 @@ def generate_fwd_struct(name):
 
 typedef struct %(name)s %(name)s;
 
-typedef struct %(name)sList
-{
+typedef struct %(name)sList {
     union {
         %(name)s *value;
         uint64_t padding;
@@ -45,8 +43,8 @@ typedef struct %(name)sList
 
 def generate_fwd_enum_struct(name):
     return mcgen('''
-typedef struct %(name)sList
-{
+
+typedef struct %(name)sList {
     union {
         %(name)s value;
         uint64_t padding;
@@ -79,8 +77,8 @@ def generate_struct(expr):
     base = expr.get('base')
 
     ret = mcgen('''
-struct %(name)s
-{
+
+struct %(name)s {
 ''',
           name=c_name(structname))
 
@@ -105,7 +103,8 @@ struct %(name)s
 
 def generate_enum_lookup(name, values):
     ret = mcgen('''
-const char * const %(name)s_lookup[] = {
+
+const char *const %(name)s_lookup[] = {
 ''',
                 name=c_name(name))
     for value in values:
@@ -119,7 +118,6 @@ const char * const %(name)s_lookup[] = {
     ret += mcgen('''
     [%(max_index)s] = NULL,
 };
-
 ''',
         max_index=max_index)
     return ret
@@ -127,13 +125,14 @@ const char * const %(name)s_lookup[] = {
 def generate_enum(name, values):
     name = c_name(name)
     lookup_decl = mcgen('''
-extern const char * const %(name)s_lookup[];
+
+extern const char *const %(name)s_lookup[];
 ''',
                 name=name)
 
     enum_decl = mcgen('''
-typedef enum %(name)s
-{
+
+typedef enum %(name)s {
 ''',
                 name=name)
 
@@ -155,7 +154,7 @@ typedef enum %(name)s
 ''',
                  name=name)
 
-    return lookup_decl + enum_decl
+    return enum_decl + lookup_decl
 
 def generate_alternate_qtypes(expr):
 
@@ -163,6 +162,7 @@ def generate_alternate_qtypes(expr):
     members = expr['data']
 
     ret = mcgen('''
+
 const int %(name)s_qtypes[QTYPE_MAX] = {
 ''',
                 name=c_name(name))
@@ -198,8 +198,8 @@ def generate_union(expr, meta):
         discriminator_type_name = '%sKind' % (name)
 
     ret = mcgen('''
-struct %(name)s
-{
+
+struct %(name)s {
 ''',
                 name=name)
     if base:
@@ -328,14 +328,12 @@ fdef.write(mcgen('''
 #include "qapi/dealloc-visitor.h"
 #include "%(prefix)sqapi-types.h"
 #include "%(prefix)sqapi-visit.h"
-
 ''',
                  prefix=prefix))
 
 fdecl.write(mcgen('''
 #include <stdbool.h>
 #include <stdint.h>
-
 '''))
 
 exprs = parse_schema(input_file)
@@ -346,22 +344,22 @@ for typename in builtin_types.keys():
 fdecl.write(guardend("QAPI_TYPES_BUILTIN_STRUCT_DECL"))
 
 for expr in exprs:
-    ret = "\n"
+    ret = ""
     if expr.has_key('struct'):
         ret += generate_fwd_struct(expr['struct'])
     elif expr.has_key('enum'):
-        ret += generate_enum(expr['enum'], expr['data']) + "\n"
+        ret += generate_enum(expr['enum'], expr['data'])
         ret += generate_fwd_enum_struct(expr['enum'])
         fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
     elif expr.has_key('union'):
-        ret += generate_fwd_struct(expr['union']) + "\n"
+        ret += generate_fwd_struct(expr['union'])
         enum_define = discriminator_find_enum_define(expr)
         if not enum_define:
             ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
             fdef.write(generate_enum_lookup('%sKind' % expr['union'],
                                             expr['data'].keys()))
     elif expr.has_key('alternate'):
-        ret += generate_fwd_struct(expr['alternate']) + "\n"
+        ret += generate_fwd_struct(expr['alternate'])
         ret += generate_enum('%sKind' % expr['alternate'], expr['data'].keys())
         fdef.write(generate_enum_lookup('%sKind' % expr['alternate'],
                                         expr['data'].keys()))
@@ -381,34 +379,32 @@ fdecl.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DECL"))
 # have the functions defined, so we use -b option to provide control
 # over these cases
 if do_builtins:
-    fdef.write(guardstart("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
     for typename in builtin_types.keys():
         fdef.write(generate_type_cleanup(typename + "List"))
-    fdef.write(guardend("QAPI_TYPES_BUILTIN_CLEANUP_DEF"))
 
 for expr in exprs:
-    ret = "\n"
+    ret = ""
     if expr.has_key('struct'):
         ret += generate_struct(expr) + "\n"
         ret += generate_type_cleanup_decl(expr['struct'] + "List")
-        fdef.write(generate_type_cleanup(expr['struct'] + "List") + "\n")
+        fdef.write(generate_type_cleanup(expr['struct'] + "List"))
         ret += generate_type_cleanup_decl(expr['struct'])
-        fdef.write(generate_type_cleanup(expr['struct']) + "\n")
+        fdef.write(generate_type_cleanup(expr['struct']))
     elif expr.has_key('union'):
-        ret += generate_union(expr, 'union')
+        ret += generate_union(expr, 'union') + "\n"
         ret += generate_type_cleanup_decl(expr['union'] + "List")
-        fdef.write(generate_type_cleanup(expr['union'] + "List") + "\n")
+        fdef.write(generate_type_cleanup(expr['union'] + "List"))
         ret += generate_type_cleanup_decl(expr['union'])
-        fdef.write(generate_type_cleanup(expr['union']) + "\n")
+        fdef.write(generate_type_cleanup(expr['union']))
     elif expr.has_key('alternate'):
-        ret += generate_union(expr, 'alternate')
+        ret += generate_union(expr, 'alternate') + "\n"
         ret += generate_type_cleanup_decl(expr['alternate'] + "List")
-        fdef.write(generate_type_cleanup(expr['alternate'] + "List") + "\n")
+        fdef.write(generate_type_cleanup(expr['alternate'] + "List"))
         ret += generate_type_cleanup_decl(expr['alternate'])
-        fdef.write(generate_type_cleanup(expr['alternate']) + "\n")
+        fdef.write(generate_type_cleanup(expr['alternate']))
     elif expr.has_key('enum'):
-        ret += generate_type_cleanup_decl(expr['enum'] + "List")
-        fdef.write(generate_type_cleanup(expr['enum'] + "List") + "\n")
+        ret += "\n" + generate_type_cleanup_decl(expr['enum'] + "List")
+        fdef.write(generate_type_cleanup(expr['enum'] + "List"))
     else:
         continue
     fdecl.write(ret)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index eec5f1f..3cd662b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -62,6 +62,7 @@ def generate_visit_struct_fields(name, members, base = None):
 static void visit_type_%(name)s_fields(Visitor *m, %(name)s **obj, Error **errp)
 {
     Error *err = NULL;
+
 ''',
                  name=c_name(name))
     push_indent()
-- 
2.4.3

Re: [Qemu-devel] [PATCH 02/26] qapi: Clean up cgen() and mcgen()

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Commit 05dfb26 added eatspace stripping to mcgen().  Move it to
> cgen(), just in case somebody gets tempted to use cgen() directly
> instead of via mcgen().
>
> cgen() indents blank lines.  No such lines get generated right now,
> but fix it anyway.
>
> We use triple-quoted strings for program text, like this:
>
>     '''
>     Program text
>     any number of lines
>     '''
>
> Keeps the program text relatively readable, but puts an extra newline
> at either end.  mcgen() "fixes" that by dropping the first and last
> line outright.  Drop only the newlines.
>
> This unmasks a bug in qapi-commands.py: four quotes instead of three.
> Fix it up.
>
> Output doesn't change
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi-commands.py |  2 +-
>  scripts/qapi.py          | 19 ++++++++++++-------
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index ca22acc..ce51408 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -56,7 +56,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
>                  name=c_name(name), args=arglist, retval=retval).rstrip()
>      if ret_type:
>          ret += "\n" + gen_err_check('local_err')
> -        ret += "\n" + mcgen(''''
> +        ret += "\n" + mcgen('''
>  %(marshal_output_call)s
>  ''',
>                              marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip()
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 06d7fc2..20383ef 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -943,16 +943,21 @@ def pop_indent(indent_amount=4):
>      global indent_level
>      indent_level -= indent_amount
>  
> +# Generate @code with @kwds interpolated.
> +# Obey indent_level, and strip eatspace.
>  def cgen(code, **kwds):
> -    indent = genindent(indent_level)
> -    lines = code.split('\n')
> -    lines = map(lambda x: indent + x, lines)
> -    return '\n'.join(lines) % kwds + '\n'
> -
> -def mcgen(code, **kwds):
> -    raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
> +    raw = code % kwds
> +    if indent_level:
> +        indent = genindent(indent_level)
> +        raw = re.subn("^.", indent + '\g<0>', raw, 0, re.MULTILINE)

pylint wants us to use r'\g<0>'.  If nothing else comes up, I'll fix
this up for my pull.

> +        raw = raw[0]
>      return re.sub(re.escape(eatspace) + ' *', '', raw)
>  
> +def mcgen(code, **kwds):
> +    if code[0] == '\n':
> +        code = code[1:]
> +    return cgen(code, **kwds)
> +
>  def basename(filename):
>      return filename.split("/")[-1]

Re: [Qemu-devel] [PATCH 07/26] qapi: Fix generated code when flat union has member 'kind'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

On 08/04/2015 03:17 AM, Markus Armbruster wrote:
> A flat union's tag member gets renamed to 'kind' in the generated
> code.  Breaks when another member is named 'kind' exists.

Too many verbs. Drop either 'is' or 'exists'.

> 
> Example, adapted from qapi-schema-test.json:
> 
>     { 'struct': 'UserDefUnionBase',
>       'data': { 'kind': 'str', 'enum1': 'EnumOne' } }
> 
> We generate:
> 
>     struct UserDefFlatUnion
>     {
>         EnumOne kind;
>         union {
>             void *data;
>             UserDefA *value1;
>             UserDefB *value2;
>             UserDefB *value3;
>         };
>         char *kind;
>     };
> 
> Kill the silly rename.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-types.py           | 3 ++-
>  scripts/qapi-visit.py           | 7 +++++--
>  tests/test-qmp-input-visitor.c  | 2 +-
>  tests/test-qmp-output-visitor.c | 2 +-
>  4 files changed, 9 insertions(+), 5 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 --]

Re: [Qemu-devel] [PATCH 02/26] qapi: Clean up cgen() and mcgen()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On 08/04/2015 09:44 AM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Commit 05dfb26 added eatspace stripping to mcgen().  Move it to
>> cgen(), just in case somebody gets tempted to use cgen() directly
>> instead of via mcgen().
>>

>> Output doesn't change
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  scripts/qapi-commands.py |  2 +-
>>  scripts/qapi.py          | 19 ++++++++++++-------
>>  2 files changed, 13 insertions(+), 8 deletions(-)

>> -def mcgen(code, **kwds):
>> -    raw = cgen('\n'.join(code.split('\n')[1:-1]), **kwds)
>> +    raw = code % kwds
>> +    if indent_level:
>> +        indent = genindent(indent_level)
>> +        raw = re.subn("^.", indent + '\g<0>', raw, 0, re.MULTILINE)
> 
> pylint wants us to use r'\g<0>'.  If nothing else comes up, I'll fix
> this up for my pull.
> 

Goes to show that I've never (yet) used pylint. I probably ought to :)

Fine by me to make that tweak and keep my R-b.

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

Re: [Qemu-devel] [PATCH 08/26] qapi: Generate a nicer struct for flat unions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

On 08/04/2015 03:17 AM, Markus Armbruster wrote:
> The struct generated for a flat union is weird: the members of its
> base are at the end, except for the union tag, which is at the
> beginning.
> 

> Change to put all base members at the beginning, unadulterated.  Not
> only is this easier to understand, it also permits casting the flat
> union to its base, if that should become useful.

And I already have a followup patch to your RFCv3 where it IS useful :)

> 
> We now generate:
> 
>     struct UserDefFlatUnion
>     {
>         /* Members inherited from UserDefUnionBase: */
>         char *string;
>         EnumOne enum1;
>         /* Own members: */
>         union { /* union tag is @enum1 */
>             void *data;
>             UserDefA *value1;
>             UserDefB *value2;
>             UserDefB *value3;
>         };
>     };
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-types.py | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 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 --]

Re: [Qemu-devel] [PATCH 16/26] qapi: Document flaws in checking of names

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> We don't actually enforce our "other than downstream extensions [...],
> all names should begin with a letter" rule.  Add a FIXME.
> 
> We should reject names that differ only in '_' vs. '.'  vs. '-',
> because they're liable to clash in generated C.  Add a FIXME.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 4 ++++
>  1 file changed, 4 insertions(+)

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

Re: [Qemu-devel] [PATCH 18/26] tests/qapi-schema: Rename tests from data- to args-

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> Since every schema entity has 'data', the data- prefix conveys no
> information.  These tests actually exercise commands.  Only commands
> have arguments, so change the prefix to to args-.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile                                                      | 6 +++---
>  tests/qapi-schema/{data-array-empty.err => args-array-empty.err}    | 2 +-
>  tests/qapi-schema/{data-unknown.exit => args-array-empty.exit}      | 0
>  tests/qapi-schema/{data-array-empty.json => args-array-empty.json}  | 0

git rename detection doesn't work too well when renaming identical
contents :)  Thankfully, it all works out in the end, and the Makefile
changes are the easiest way to review this.  And thank goodness that git
compresses renames in patches; this email would have been a lot longer
by traditional diff.

> diff --git a/tests/Makefile b/tests/Makefile
> index 7494582..0f01b5c 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -229,9 +229,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>  	redefined-type.json redefined-command.json redefined-builtin.json \
>  	redefined-event.json command-int.json bad-data.json event-max.json \
>  	type-bypass.json type-bypass-no-gen.json type-bypass-bad-gen.json \
> -	data-array-empty.json data-array-unknown.json data-int.json \
> -	data-unknown.json data-member-unknown.json data-member-array.json \
> -	data-member-array-bad.json returns-array-bad.json returns-int.json \
> +	args-array-empty.json args-array-unknown.json args-int.json \
> +	args-unknown.json args-member-unknown.json args-member-array.json \
> +	args-member-array-bad.json returns-array-bad.json returns-int.json \
>  	returns-unknown.json returns-alternate.json returns-whitelist.json \

7 sets of files renamed, including fallout to the changed error messages.

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

Re: [Qemu-devel] [PATCH 20/26] qapi: Fix to reject union command and event arguments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> A command's or event's 'data' must be a struct type, given either as a
> dictionary, or as struct type name.
> 
> Commit dd883c6 tightened the checking there, but not enough: we still
> accept 'union'.  Fix to reject it.
> 
> We may want to support union types there, but we'll have to extend
> qapi-commands.py and qapi-events.py for it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py                   | 4 ++--
>  tests/qapi-schema/args-union.err  | 1 +
>  tests/qapi-schema/args-union.exit | 2 +-
>  tests/qapi-schema/args-union.json | 2 +-
>  tests/qapi-schema/args-union.out  | 4 ----
>  5 files changed, 5 insertions(+), 8 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 --]

Re: [Qemu-devel] [PATCH 21/26] qapi: Command returning anonymous type doesn't work, outlaw

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> Reproducer: with
> 
>     { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }
> 
> added to qapi-schema-test.json, qapi-commands.py dies when it tries to
> generate the command handler function
> 
>     Traceback (most recent call last):
>       File "/work/armbru/qemu/scripts/qapi-commands.py", line 359, in <module>
>         ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
>       File "/work/armbru/qemu/scripts/qapi-commands.py", line 29, in generate_command_decl
>         ret_type=c_type(ret_type), name=c_name(name),
>       File "/work/armbru/qemu/scripts/qapi.py", line 927, in c_type
>         assert isinstance(value, str) and value != ""
>     AssertionError
> 
> because the return type doesn't exist.
> 
> Simply outlaw this usage.

Might be worth allowing someday, but that would imply that we can come
up with a sane naming scheme for anonymous structs in the qapi schema
that won't risk collisions with explicit types.  Shame on me for not
thinking to test this in my earlier testsuite additions, but I
definitely agree with your solution of outlawing it for now.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt                                  | 17 ++++++++---------
>  scripts/qapi.py                                         |  2 +-
>  tests/Makefile                                          |  4 ++--
>  tests/qapi-schema/nested-struct-returns.err             |  1 -
>  tests/qapi-schema/nested-struct-returns.json            |  3 ---
>  tests/qapi-schema/returns-dict.err                      |  1 +
>  .../{nested-struct-returns.exit => returns-dict.exit}   |  0
>  tests/qapi-schema/returns-dict.json                     |  2 ++
>  .../{nested-struct-returns.out => returns-dict.out}     |  0
>  9 files changed, 14 insertions(+), 16 deletions(-)

Once again, git rename detection didn't accurately capture what you did :)

>  delete mode 100644 tests/qapi-schema/nested-struct-returns.err
>  delete mode 100644 tests/qapi-schema/nested-struct-returns.json
>  create mode 100644 tests/qapi-schema/returns-dict.err
>  rename tests/qapi-schema/{nested-struct-returns.exit => returns-dict.exit} (100%)
>  create mode 100644 tests/qapi-schema/returns-dict.json
>  rename tests/qapi-schema/{nested-struct-returns.out => returns-dict.out} (100%)
> 

git grep "'returns'.*{"

found a couple more culprits (tests that fail elsewhere prior to warning
about this, but where an anonymous return does not add to the negative
test).  Please squash this in:

diff --git i/tests/qapi-schema/command-int.json
w/tests/qapi-schema/command-int.json
index c90d408..40a6ae3 100644
--- i/tests/qapi-schema/command-int.json
+++ w/tests/qapi-schema/command-int.json
@@ -1,3 +1,4 @@
 # we reject collisions between commands and types
 { 'command': 'int', 'data': { 'character': 'str' },
-  'returns': { 'value': 'int' } }
+  'returns': 'Foo' }
+{ 'struct': 'Foo', 'data': { 'value': 'int' } }
diff --git i/tests/qapi-schema/nested-struct-data.json
w/tests/qapi-schema/nested-struct-data.json
index 3d52d2b..efbe773 100644
--- i/tests/qapi-schema/nested-struct-data.json
+++ w/tests/qapi-schema/nested-struct-data.json
@@ -1,4 +1,3 @@
 # inline subtypes collide with our desired future use of defaults
 { 'command': 'foo',
-  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' },
-  'returns': {} }
+  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }


at which point you may add:

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

Re: [Qemu-devel] [PATCH 25/26] qapi-commands: Drop useless initialization

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> In generated command handlers, the assignment to retval dominates its
> only use.  Therefore, its initialization is useless.  Drop it.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt   | 2 +-
>  scripts/qapi-commands.py | 8 ++------
>  2 files changed, 3 insertions(+), 7 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 --]

Re: [Qemu-devel] [PATCH 26/26] qapi: Generated code cleanup

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On 08/04/2015 03:18 AM, Markus Armbruster wrote:
> Clean up white-space, brace placement, and superfluous #ifdef
> QAPI_TYPES_BUILTIN_CLEANUP_DEF.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qapi-code-gen.txt   | 12 ++++-----
>  scripts/qapi-commands.py |  1 +
>  scripts/qapi-event.py    |  3 +--
>  scripts/qapi-types.py    | 66 +++++++++++++++++++++++-------------------------
>  scripts/qapi-visit.py    |  1 +
>  5 files changed, 39 insertions(+), 44 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 --]

Re: [Qemu-devel] [PATCH 07/26] qapi: Fix generated code when flat union has member 'kind'

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 08/04/2015 03:17 AM, Markus Armbruster wrote:
>> A flat union's tag member gets renamed to 'kind' in the generated
>> code.  Breaks when another member is named 'kind' exists.
>
> Too many verbs. Drop either 'is' or 'exists'.

Will fix.

>> 
>> Example, adapted from qapi-schema-test.json:
>> 
>>     { 'struct': 'UserDefUnionBase',
>>       'data': { 'kind': 'str', 'enum1': 'EnumOne' } }
>> 
>> We generate:
>> 
>>     struct UserDefFlatUnion
>>     {
>>         EnumOne kind;
>>         union {
>>             void *data;
>>             UserDefA *value1;
>>             UserDefB *value2;
>>             UserDefB *value3;
>>         };
>>         char *kind;
>>     };
>> 
>> Kill the silly rename.
>> 
>> Reported-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-types.py           | 3 ++-
>>  scripts/qapi-visit.py           | 7 +++++--
>>  tests/test-qmp-input-visitor.c  | 2 +-
>>  tests/test-qmp-output-visitor.c | 2 +-
>>  4 files changed, 9 insertions(+), 5 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 21/26] qapi: Command returning anonymous type doesn't work, outlaw

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 08/04/2015 03:18 AM, Markus Armbruster wrote:
>> Reproducer: with
>> 
>>     { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }
>> 
>> added to qapi-schema-test.json, qapi-commands.py dies when it tries to
>> generate the command handler function
>> 
>>     Traceback (most recent call last):
>>       File "/work/armbru/qemu/scripts/qapi-commands.py", line 359, in <module>
>>         ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
>>       File "/work/armbru/qemu/scripts/qapi-commands.py", line 29, in generate_command_decl
>>         ret_type=c_type(ret_type), name=c_name(name),
>>       File "/work/armbru/qemu/scripts/qapi.py", line 927, in c_type
>>         assert isinstance(value, str) and value != ""
>>     AssertionError
>> 
>> because the return type doesn't exist.
>> 
>> Simply outlaw this usage.
>
> Might be worth allowing someday, but that would imply that we can come
> up with a sane naming scheme for anonymous structs in the qapi schema
> that won't risk collisions with explicit types.  Shame on me for not
> thinking to test this in my earlier testsuite additions, but I
> definitely agree with your solution of outlawing it for now.
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qapi-code-gen.txt | 17 ++++++++---------
>>  scripts/qapi.py                                         |  2 +-
>>  tests/Makefile                                          |  4 ++--
>>  tests/qapi-schema/nested-struct-returns.err             |  1 -
>>  tests/qapi-schema/nested-struct-returns.json            |  3 ---
>>  tests/qapi-schema/returns-dict.err                      |  1 +
>>  .../{nested-struct-returns.exit => returns-dict.exit}   |  0
>>  tests/qapi-schema/returns-dict.json                     |  2 ++
>>  .../{nested-struct-returns.out => returns-dict.out}     |  0
>>  9 files changed, 14 insertions(+), 16 deletions(-)
>
> Once again, git rename detection didn't accurately capture what you did :)
>
>>  delete mode 100644 tests/qapi-schema/nested-struct-returns.err
>>  delete mode 100644 tests/qapi-schema/nested-struct-returns.json
>>  create mode 100644 tests/qapi-schema/returns-dict.err
>>  rename tests/qapi-schema/{nested-struct-returns.exit =>
>> returns-dict.exit} (100%)
>>  create mode 100644 tests/qapi-schema/returns-dict.json
>>  rename tests/qapi-schema/{nested-struct-returns.out =>
>> returns-dict.out} (100%)
>> 
>
> git grep "'returns'.*{"
>
> found a couple more culprits (tests that fail elsewhere prior to warning
> about this, but where an anonymous return does not add to the negative
> test).  Please squash this in:
>
> diff --git i/tests/qapi-schema/command-int.json
> w/tests/qapi-schema/command-int.json
> index c90d408..40a6ae3 100644
> --- i/tests/qapi-schema/command-int.json
> +++ w/tests/qapi-schema/command-int.json
> @@ -1,3 +1,4 @@
>  # we reject collisions between commands and types
>  { 'command': 'int', 'data': { 'character': 'str' },
> -  'returns': { 'value': 'int' } }
> +  'returns': 'Foo' }
> +{ 'struct': 'Foo', 'data': { 'value': 'int' } }

Okay to simply drop the 'returns' instead?

> diff --git i/tests/qapi-schema/nested-struct-data.json
> w/tests/qapi-schema/nested-struct-data.json
> index 3d52d2b..efbe773 100644
> --- i/tests/qapi-schema/nested-struct-data.json
> +++ w/tests/qapi-schema/nested-struct-data.json
> @@ -1,4 +1,3 @@
>  # inline subtypes collide with our desired future use of defaults
>  { 'command': 'foo',
> -  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' },
> -  'returns': {} }
> +  'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } }
>
>
> at which point you may add:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 21/26] qapi: Command returning anonymous type doesn't work, outlaw

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On 08/04/2015 11:29 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 08/04/2015 03:18 AM, Markus Armbruster wrote:
>>> Reproducer: with
>>>
>>>     { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }
>>>
>>> added to qapi-schema-test.json, qapi-commands.py dies when it tries to
>>> generate the command handler function
>>>

>> +++ w/tests/qapi-schema/command-int.json
>> @@ -1,3 +1,4 @@
>>  # we reject collisions between commands and types
>>  { 'command': 'int', 'data': { 'character': 'str' },
>> -  'returns': { 'value': 'int' } }
>> +  'returns': 'Foo' }
>> +{ 'struct': 'Foo', 'data': { 'value': 'int' } }
> 
> Okay to simply drop the 'returns' instead?
> 

Sure, that works too.

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

Re: [Qemu-devel] [PATCH 21/26] qapi: Command returning anonymous type doesn't work, outlaw

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 08/04/2015 11:29 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 08/04/2015 03:18 AM, Markus Armbruster wrote:
>>>> Reproducer: with
>>>>
>>>>     { 'command': 'user_def_cmd4', 'returns': { 'a': 'int' } }
>>>>
>>>> added to qapi-schema-test.json, qapi-commands.py dies when it tries to
>>>> generate the command handler function
>>>>
>
>>> +++ w/tests/qapi-schema/command-int.json
>>> @@ -1,3 +1,4 @@
>>>  # we reject collisions between commands and types
>>>  { 'command': 'int', 'data': { 'character': 'str' },
>>> -  'returns': { 'value': 'int' } }
>>> +  'returns': 'Foo' }
>>> +{ 'struct': 'Foo', 'data': { 'value': 'int' } }
>> 
>> Okay to simply drop the 'returns' instead?
>> 
>
> Sure, that works too.

Done, with commit message amended:

    Simply outlaw this usage, and drop or dumb down test cases accordingly.

Re: [Qemu-devel] [PATCH 20/26] qapi: Fix to reject union command and event arguments

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1939 bytes --]

On 08/04/2015 02:18 AM, Markus Armbruster wrote:
> A command's or event's 'data' must be a struct type, given either as a
> dictionary, or as struct type name.
> 
> Commit dd883c6 tightened the checking there, but not enough: we still
> accept 'union'.  Fix to reject it.
> 
> We may want to support union types there, but we'll have to extend
> qapi-commands.py and qapi-events.py for it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py                   | 4 ++--
>  tests/qapi-schema/args-union.err  | 1 +
>  tests/qapi-schema/args-union.exit | 2 +-
>  tests/qapi-schema/args-union.json | 2 +-
>  tests/qapi-schema/args-union.out  | 4 ----
>  5 files changed, 5 insertions(+), 8 deletions(-)

Our documentation is also wrong; you may want to squash this in:

diff --git i/docs/qapi-code-gen.txt w/docs/qapi-code-gen.txt
index 688a60c..4652a75 100644
--- i/docs/qapi-code-gen.txt
+++ w/docs/qapi-code-gen.txt
@@ -408,10 +408,9 @@ Client JSON Protocol command exchange.
 The 'data' argument maps to the "arguments" dictionary passed in as
 part of a Client JSON Protocol command.  The 'data' member is optional
 and defaults to {} (an empty dictionary).  If present, it must be the
-string name of a complex type, a one-element array containing the name
-of a complex type, or a dictionary that declares an anonymous type
-with the same semantics as a 'struct' expression, with one exception
-noted below when 'gen' is used.
+string name of a complex type, or a dictionary that declares an
+anonymous type with the same semantics as a 'struct' expression, with
+one exception noted below when 'gen' is used.

 The 'returns' member describes what will appear in the "return" field
 of a Client JSON Protocol reply on successful completion of a command.


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

Re: [Qemu-devel] [PATCH 20/26] qapi: Fix to reject union command and event arguments

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 08/04/2015 02:18 AM, Markus Armbruster wrote:
>> A command's or event's 'data' must be a struct type, given either as a
>> dictionary, or as struct type name.
>> 
>> Commit dd883c6 tightened the checking there, but not enough: we still
>> accept 'union'.  Fix to reject it.
>> 
>> We may want to support union types there, but we'll have to extend
>> qapi-commands.py and qapi-events.py for it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi.py                   | 4 ++--
>>  tests/qapi-schema/args-union.err  | 1 +
>>  tests/qapi-schema/args-union.exit | 2 +-
>>  tests/qapi-schema/args-union.json | 2 +-
>>  tests/qapi-schema/args-union.out  | 4 ----
>>  5 files changed, 5 insertions(+), 8 deletions(-)
>
> Our documentation is also wrong; you may want to squash this in:
>
> diff --git i/docs/qapi-code-gen.txt w/docs/qapi-code-gen.txt
> index 688a60c..4652a75 100644
> --- i/docs/qapi-code-gen.txt
> +++ w/docs/qapi-code-gen.txt
> @@ -408,10 +408,9 @@ Client JSON Protocol command exchange.
>  The 'data' argument maps to the "arguments" dictionary passed in as
>  part of a Client JSON Protocol command.  The 'data' member is optional
>  and defaults to {} (an empty dictionary).  If present, it must be the
> -string name of a complex type, a one-element array containing the name
> -of a complex type, or a dictionary that declares an anonymous type
> -with the same semantics as a 'struct' expression, with one exception
> -noted below when 'gen' is used.
> +string name of a complex type, or a dictionary that declares an
> +anonymous type with the same semantics as a 'struct' expression, with
> +one exception noted below when 'gen' is used.
>
>  The 'returns' member describes what will appear in the "return" field
>  of a Client JSON Protocol reply on successful completion of a command.

Will do, thanks!