Re: [Qemu-devel] [PATCH v9 28/37] qapi: Fix command with named empty argument type

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> The generator special-cased
>  { 'command':'foo', 'data': {} }
> to avoid emitting a visitor variable, but failed to see that
>  { 'struct':'NamedEmptyType, 'data': {} }
>  { 'command':'foo', 'data':'NamedEmptyType' }
> needs the same treatment.  Without a fix to the generator, the
> change to qapi-schema-test.json fails to compile with:
>
> tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
> tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable]
>      Visitor *v;
>               ^
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: no change
> v7: no change
> v6: new patch
> ---
>  scripts/qapi-commands.py                | 6 +++---
>  tests/qapi-schema/qapi-schema-test.json | 2 ++
>  tests/qapi-schema/qapi-schema-test.out  | 2 ++
>  tests/test-qmp-commands.c               | 5 +++++
>  4 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 91c5a4e..00ee565 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -65,7 +65,7 @@ def gen_marshal_vars(arg_type, ret_type):
>  ''',
>                       c_type=ret_type.c_type())
>
> -    if arg_type:
> +    if arg_type and not arg_type.is_empty():

This guards generating the variables for getting arguments, first a few
common ones, then one or two per argument.

The patch changes it to effectively

       if arg_type and (arg_type.members or arg_type.variants)

.variants is None (asserted in QAPISchemaCommand.check()).  Thus, the
patch effectively just adds and arg_type.members.

Impact: when arg_type has no members, we still generate the common
variables before the patch.  The patch suppresses them.  Whether that
makes sense depends on the next hunk, which generates the code using
these variables.

>          ret += mcgen('''
>      QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
>      QapiDeallocVisitor *qdv;
> @@ -97,7 +97,7 @@ def gen_marshal_vars(arg_type, ret_type):
>  def gen_marshal_input_visit(arg_type, dealloc=False):
>      ret = ''
>
> -    if not arg_type:
> +    if not arg_type or arg_type.is_empty():
>          return ret

This guards generating the code for getting arguments: common code, and
per-argument code.

Impact: when arg_type has no members, we still generate the common code
before the patch.  The patch suppresses it.  Is that okay?  We'll see
below.

>
>      if dealloc:
> @@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type):
>
>      # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
>      # for each arg_type member, and by gen_call() for ret_type
> -    if (arg_type and arg_type.members) or ret_type:
> +    if (arg_type and not arg_type.is_empty()) or ret_type:

This guards generating label out.  or's left operand is about the
arguments.  Unlike the guards above, it checks .members even before the
patch.

>          ret += mcgen('''
>
>  out:
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 4b89527..a0fdb88 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -18,6 +18,8 @@
>  { 'struct': 'Empty1', 'data': { } }
>  { 'struct': 'Empty2', 'base': 'Empty1', 'data': { } }
>
> +{ 'command': 'user_def_cmd0', 'data': 'Empty2', 'returns': 'Empty2' }
> +
>  # for testing override of default naming heuristic
>  { 'enum': 'QEnumTwo',
>    'prefix': 'QENUM_TWO',
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 2c546b7..d8f9108 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -198,6 +198,8 @@ command guest-sync :obj-guest-sync-arg -> any
>     gen=True success_response=True
>  command user_def_cmd None -> None
>     gen=True success_response=True
> +command user_def_cmd0 Empty2 -> Empty2
> +   gen=True success_response=True
>  command user_def_cmd1 :obj-user_def_cmd1-arg -> None
>     gen=True success_response=True
>  command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 4d267b6..bc8085d 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -12,6 +12,11 @@ void qmp_user_def_cmd(Error **errp)
>  {
>  }
>
> +Empty2 *qmp_user_def_cmd0(Error **errp)
> +{
> +    return g_new0(Empty2, 1);
> +}
> +
>  void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
>  {
>  }

Patch's effect on the test case:

--- bld-x86/tests/test-qmp-marshal.c	2016-01-25 15:54:34.106265263 +0100
+++ bld-x86/tests/new-test-qmp-marshal.c	2016-01-25 15:54:08.312574713 +0100
@@ -254,11 +254,8 @@
 {
     Error *err = NULL;
     Empty2 *retval;
-    QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
-    QapiDeallocVisitor *qdv;
-    Visitor *v;
 
-    v = qmp_input_get_visitor(qiv);
+    (void)args;
 
     retval = qmp_user_def_cmd0(&err);
     if (err) {
@@ -269,10 +266,6 @@
 
 out:
     error_propagate(errp, err);
-    qmp_input_visitor_cleanup(qiv);
-    qdv = qapi_dealloc_visitor_new();
-    v = qapi_dealloc_get_visitor(qdv);
-    qapi_dealloc_visitor_cleanup(qdv);
 }
 
 static void qmp_marshal_user_def_cmd1(QDict *args, QObject **ret, Error **errp)

We drop a pair of "create a visitor, destroy it without having done
anything with it".  Okay.

Commit message might mislead readers into assuming the patch merely
drops unused variables.  It actually drops creating useless visitors.
Easy enough to clarify:

    The generator special-cased

        { 'command':'foo', 'data': {} }
    to avoid emitting a visitor variable, but failed to see that

        { 'struct':'NamedEmptyType, 'data': {} }
        { 'command':'foo', 'data':'NamedEmptyType' }

    needs the same treatment.  There, the generator happily generates a
    visitor to get no arguments, and a visitor to destroy no arguments.
    The compiler isn't happy with that, as demonstrated by the updated
    qapi-schema-test.json:

        tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’:
        tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable]
             Visitor *v;
                      ^