[Qemu-devel] [PATCH v6 07/16] qapi: Implement boxed types for commands/events

[Eric Blake]

Turn on the ability to pass command and event arguments in
a single boxed parameter.  For structs, it makes it possible
to pass a single qapi type instead of a breakout of all
struct members; for unions, it is now possible to use a
union as the data for a command or event.

Generated code is unchanged, as long as no client uses the
new feature.

Signed-off-by: Eric Blake <address@hidden>

---
v6: retitle, rebase, and merge v5 40/46 and 41/46 into one patch
---
 docs/qapi-code-gen.txt                  | 24 ++++++++++++++++++++++--
 scripts/qapi-commands.py                | 18 +++++++++++++++---
 scripts/qapi-event.py                   | 11 +++++++++--
 scripts/qapi.py                         | 27 ++++++++++++++++++++-------
 tests/Makefile                          |  2 ++
 tests/qapi-schema/args-bad-box.err      |  1 +
 tests/qapi-schema/args-bad-box.exit     |  1 +
 tests/qapi-schema/args-bad-box.json     |  2 ++
 tests/qapi-schema/args-bad-box.out      |  0
 tests/qapi-schema/args-box-anon.err     |  1 +
 tests/qapi-schema/args-box-anon.exit    |  1 +
 tests/qapi-schema/args-box-anon.json    |  2 ++
 tests/qapi-schema/args-box-anon.out     |  0
 tests/qapi-schema/args-union.err        |  2 +-
 tests/qapi-schema/args-union.json       |  3 +--
 tests/qapi-schema/qapi-schema-test.json |  6 +++++-
 tests/qapi-schema/qapi-schema-test.out  | 10 +++++++++-
 tests/test-qmp-commands.c               | 12 ++++++++++++
 18 files changed, 104 insertions(+), 19 deletions(-)
 create mode 100644 tests/qapi-schema/args-bad-box.err
 create mode 100644 tests/qapi-schema/args-bad-box.exit
 create mode 100644 tests/qapi-schema/args-bad-box.json
 create mode 100644 tests/qapi-schema/args-bad-box.out
 create mode 100644 tests/qapi-schema/args-box-anon.err
 create mode 100644 tests/qapi-schema/args-box-anon.exit
 create mode 100644 tests/qapi-schema/args-box-anon.json
 create mode 100644 tests/qapi-schema/args-box-anon.out

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 999f3b9..bd437c8 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -409,7 +409,7 @@ following example objects:
 === Commands ===

 Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
-         '*returns': TYPE-NAME,
+         '*returns': TYPE-NAME, '*box': true,
          '*gen': false, '*success-response': false }

 Commands are defined by using a dictionary containing several members,
@@ -460,6 +460,17 @@ which would validate this Client JSON Protocol transaction:
  => { "execute": "my-second-command" }
  <= { "return": [ { "value": "one" }, { } ] }

+By default, the generator creates a marshalling function that converts
+an input QDict into a function call implemented by the user, and
+declares a prototype for the user's function which has a parameter for
+each member of the argument struct, including boolean arguments that
+describe whether optional arguments were provided.  But if the QAPI
+description includes the key 'box' with the boolean value true, the
+user call prototype will have only a single parameter for the overall
+generated C structure.  The 'box' key is required in order to use a
+union as an input argument, since it is not possible to list all
+members of the union as separate parameters.
+
 In rare cases, QAPI cannot express a type-safe representation of a
 corresponding Client JSON Protocol command.  You then have to suppress
 generation of a marshalling function by including a key 'gen' with
@@ -483,7 +494,8 @@ use of this field.

 === Events ===

-Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT }
+Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
+         '*box': true }

 Events are defined with the keyword 'event'.  It is not allowed to
 name an event 'MAX', since the generator also produces a C enumeration
@@ -504,6 +516,14 @@ Resulting in this JSON object:
   "data": { "b": "test string" },
   "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }

+By default, the generator creates a C function that takes as
+parameters each member of the argument struct and turns it into the
+appropriate JSON Client event.  But if the QAPI description includes
+the key 'box' with the boolean value true, the event function will
+have only a single parameter for the overall generated C structure.
+The 'box' key is required in order to use a union as the data key,
+since it is not possible to list all members of the union as separate
+parameters.

 == Client JSON Protocol introspection ==

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index efdeb70..d47ee10 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -31,7 +31,7 @@ def gen_call(name, arg_type, box, ret_type):
     argstr = ''
     assert arg_type
     if box and not arg_type.is_empty():
-        assert False    # not implemented
+        argstr = 'arg, '
     else:
         for memb in arg_type.members:
             if memb.optional:
@@ -77,7 +77,10 @@ def gen_marshal_vars(arg_type, box, ret_type):
 ''')

         if box:
-            assert False    # not implemented
+            ret += mcgen('''
+    %(c_type)s arg = %(c_null)s;
+''',
+                         c_type=arg_type.c_type(), c_null=arg_type.c_null())
         else:
             for memb in arg_type.members:
                 if memb.optional:
@@ -119,7 +122,16 @@ def gen_marshal_input_visit(arg_type, box, dealloc=False):
 ''')

     if box:
-        assert False    # not implemented
+        if dealloc:
+            errp = 'NULL'
+        else:
+            errp = '&err'
+        ret += mcgen('''
+    visit_type_%(c_name)s(v, NULL, &arg, %(errp)s);
+
+''',
+                     c_name=arg_type.c_name(), errp=errp)
+        ret += gen_err_check(skiperr=dealloc)
     else:
         ret += gen_visit_fields(arg_type.members, skiperr=dealloc)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 451d077..bae200d 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -41,8 +41,11 @@ def gen_event_send(name, arg_type, box):

     assert arg_type
     if not arg_type.is_empty():
-        ret += mcgen('''
+        if not box:
+            ret += mcgen('''
     QObject *obj;
+''')
+        ret += mcgen('''
     QmpOutputVisitor *qov;
     Visitor *v;

@@ -67,7 +70,11 @@ def gen_event_send(name, arg_type, box):
 ''')

         if box:
-            assert False     # not implemented
+            ret += mcgen('''
+    visit_type_%(c_name)s(v, NULL, &arg, &err);
+''',
+                         c_name=arg_type.c_name(), name=arg_type.name)
+            ret += gen_err_check()
         else:
             ret += mcgen('''
     visit_start_struct(v, "%(name)s", NULL, 0, &err);
diff --git a/scripts/qapi.py b/scripts/qapi.py
index b408a82..84c0bd0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -521,10 +521,14 @@ def check_type(expr_info, source, value, 
allow_array=False,

 def check_command(expr, expr_info):
     name = expr['command']
+    box = expr.get('box', False)

+    args_meta = ['struct']
+    if box:
+        args_meta += ['union']
     check_type(expr_info, "'data' for command '%s'" % name,
-               expr.get('data'), allow_dict=True, allow_optional=True,
-               allow_metas=['struct'])
+               expr.get('data'), allow_dict=not box, allow_optional=True,
+               allow_metas=args_meta)
     returns_meta = ['union', 'struct']
     if name in returns_whitelist:
         returns_meta += ['built-in', 'alternate', 'enum']
@@ -536,11 +540,15 @@ def check_command(expr, expr_info):
 def check_event(expr, expr_info):
     global events
     name = expr['event']
+    box = expr.get('box', False)
+    meta = ['struct']

+    if box:
+        meta += ['union']
     events.append(name)
     check_type(expr_info, "'data' for event '%s'" % name,
-               expr.get('data'), allow_dict=True, allow_optional=True,
-               allow_metas=['struct'])
+               expr.get('data'), allow_dict=not box, allow_optional=True,
+               allow_metas=meta)


 def check_union(expr, expr_info):
@@ -681,6 +689,10 @@ def check_keys(expr_elem, meta, required, optional=[]):
             raise QAPIExprError(info,
                                 "'%s' of %s '%s' should only use false value"
                                 % (key, meta, name))
+        if key == 'box' and value is not True:
+            raise QAPIExprError(info,
+                                "'%s' of %s '%s' should only use true value"
+                                % (key, meta, name))
     for key in required:
         if key not in expr:
             raise QAPIExprError(info,
@@ -712,10 +724,10 @@ def check_exprs(exprs):
             add_struct(expr, info)
         elif 'command' in expr:
             check_keys(expr_elem, 'command', [],
-                       ['data', 'returns', 'gen', 'success-response'])
+                       ['data', 'returns', 'gen', 'success-response', 'box'])
             add_name(expr['command'], info, 'command')
         elif 'event' in expr:
-            check_keys(expr_elem, 'event', [], ['data'])
+            check_keys(expr_elem, 'event', [], ['data', 'box'])
             add_name(expr['event'], info, 'event')
         else:
             raise QAPIExprError(expr_elem['info'],
@@ -1631,7 +1643,8 @@ def gen_params(arg_type, box, extra):
     ret = ''
     sep = ''
     if box and not arg_type.is_empty():
-        assert False     # not implemented
+        ret += '%s arg' % arg_type.c_type(is_param=True)
+        sep = ', '
     else:
         for memb in arg_type.members:
             ret += sep
diff --git a/tests/Makefile b/tests/Makefile
index 84caf98..3e5d17d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -253,6 +253,8 @@ qapi-schema += args-alternate.json
 qapi-schema += args-any.json
 qapi-schema += args-array-empty.json
 qapi-schema += args-array-unknown.json
+qapi-schema += args-bad-box.json
+qapi-schema += args-box-anon.json
 qapi-schema += args-int.json
 qapi-schema += args-invalid.json
 qapi-schema += args-member-array-bad.json
diff --git a/tests/qapi-schema/args-bad-box.err 
b/tests/qapi-schema/args-bad-box.err
new file mode 100644
index 0000000..16afe3c
--- /dev/null
+++ b/tests/qapi-schema/args-bad-box.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-bad-box.json:2: 'box' of command 'foo' should only use 
true value
diff --git a/tests/qapi-schema/args-bad-box.exit 
b/tests/qapi-schema/args-bad-box.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-bad-box.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-bad-box.json 
b/tests/qapi-schema/args-bad-box.json
new file mode 100644
index 0000000..8d5737a
--- /dev/null
+++ b/tests/qapi-schema/args-bad-box.json
@@ -0,0 +1,2 @@
+# 'box' should only appear with value true
+{ 'command': 'foo', 'box': false }
diff --git a/tests/qapi-schema/args-bad-box.out 
b/tests/qapi-schema/args-bad-box.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-box-anon.err 
b/tests/qapi-schema/args-box-anon.err
new file mode 100644
index 0000000..11eaefc
--- /dev/null
+++ b/tests/qapi-schema/args-box-anon.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-box-anon.json:2: 'data' for command 'foo' should be a 
type name
diff --git a/tests/qapi-schema/args-box-anon.exit 
b/tests/qapi-schema/args-box-anon.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/args-box-anon.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/args-box-anon.json 
b/tests/qapi-schema/args-box-anon.json
new file mode 100644
index 0000000..947e3c6
--- /dev/null
+++ b/tests/qapi-schema/args-box-anon.json
@@ -0,0 +1,2 @@
+# 'box' can only be used with named types
+{ 'command': 'foo', 'box': true, 'data': { 'string': 'str' } }
diff --git a/tests/qapi-schema/args-box-anon.out 
b/tests/qapi-schema/args-box-anon.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/args-union.err b/tests/qapi-schema/args-union.err
index 1d693d7..f8ad223 100644
--- a/tests/qapi-schema/args-union.err
+++ b/tests/qapi-schema/args-union.err
@@ -1 +1 @@
-tests/qapi-schema/args-union.json:4: 'data' for command 'oops' cannot use 
union type 'Uni'
+tests/qapi-schema/args-union.json:3: 'data' for command 'oops' cannot use 
union type 'Uni'
diff --git a/tests/qapi-schema/args-union.json 
b/tests/qapi-schema/args-union.json
index 7bdcbb7..c0ce091 100644
--- a/tests/qapi-schema/args-union.json
+++ b/tests/qapi-schema/args-union.json
@@ -1,4 +1,3 @@
-# we do not allow union arguments
-# TODO should we support this?
+# use of union arguments requires 'box':true
 { 'union': 'Uni', 'data': { 'case1': 'int', 'case2': 'str' } }
 { 'command': 'oops', 'data': 'Uni' }
diff --git a/tests/qapi-schema/qapi-schema-test.json 
b/tests/qapi-schema/qapi-schema-test.json
index 44c1965..d0bc3d2 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -126,6 +126,9 @@
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
   'returns': 'int' }
 { 'command': 'guest-sync', 'data': { 'arg': 'any' }, 'returns': 'any' }
+{ 'command': 'boxed-empty', 'box': true }
+{ 'command': 'boxed-struct', 'box': true, 'data': 'UserDefZero' }
+{ 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'box': true }

 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
@@ -146,13 +149,14 @@
 { 'struct': 'EventStructOne',
   'data': { 'struct1': 'UserDefOne', 'string': 'str', '*enum2': 'EnumOne' } }

-{ 'event': 'EVENT_A' }
+{ 'event': 'EVENT_A', 'box': true }
 { 'event': 'EVENT_B',
   'data': { } }
 { 'event': 'EVENT_C',
   'data': { '*a': 'int', '*b': 'UserDefOne', 'c': 'str' } }
 { 'event': 'EVENT_D',
   'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 
'EnumOne' } }
+{ 'event': 'EVENT_E', 'box': true, 'data': 'UserDefZero' }

 # test that we correctly compile downstream extensions, as well as munge
 # ticklish names
diff --git a/tests/qapi-schema/qapi-schema-test.out 
b/tests/qapi-schema/qapi-schema-test.out
index 8731caa..3dc133e 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -100,13 +100,15 @@ alternate AltStrNum
     case s: str
     case n: number
 event EVENT_A :empty
-   box=False
+   box=True
 event EVENT_B :empty
    box=False
 event EVENT_C :obj-EVENT_C-arg
    box=False
 event EVENT_D :obj-EVENT_D-arg
    box=False
+event EVENT_E UserDefZero
+   box=True
 object Empty1
     base :empty
 object Empty2
@@ -246,6 +248,12 @@ object __org.qemu_x-Union2
     case __org.qemu_x-value: __org.qemu_x-Struct2
 command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> 
__org.qemu_x-Union1
    gen=True success_response=True box=False
+command boxed-empty :empty -> None
+   gen=True success_response=True box=True
+command boxed-struct UserDefZero -> None
+   gen=True success_response=True box=True
+command boxed-union UserDefNativeListUnion -> None
+   gen=True success_response=True box=True
 command guest-get-time :obj-guest-get-time-arg -> int
    gen=True success_response=True box=False
 command guest-sync :obj-guest-sync-arg -> any
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index d3466a4..a58a425 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -59,6 +59,18 @@ QObject *qmp_guest_sync(QObject *arg, Error **errp)
     return arg;
 }

+void qmp_boxed_empty(Error **errp)
+{
+}
+
+void qmp_boxed_struct(UserDefZero *arg, Error **errp)
+{
+}
+
+void qmp_boxed_union(UserDefNativeListUnion *arg, Error **errp)
+{
+}
+
 __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
                                               __org_qemu_x_StructList *b,
                                               __org_qemu_x_Union2 *c,
-- 
2.4.3