Re: [Qemu-devel] [PATCH v2 12/19] qapi: Fix command with named empty argument type

[Markus Armbruster]

Eric Blake <address@hidden> 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.  There, the generator happily generates a
> visitor to get no arguments, and a visitor to destroy no arguments;
> and 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;
>                 ^
>
> No change to generated code except for the testsuite addition.

Using a named empty type when you could just as well use {} is a rather
remote corner case, and I therefore don't care how we translate it as
long as it works.  We could make the generator suppress just the
variable, or suppress the warning, or suppress the whole "get the
arguments" thing.  Your patch shows that the latter is easy enough, so
why not.

> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> v2: no change
> v1: enhance commit message
> Previously posted as part of qapi cleanup subset E:
> v9: no change
> v8: no change
> v7: no change
> v6: new patch
> ---
>  scripts/qapi-commands.py                | 6 +++---
>  tests/test-qmp-commands.c               | 5 +++++
>  tests/qapi-schema/qapi-schema-test.json | 2 ++
>  tests/qapi-schema/qapi-schema-test.out  | 2 ++
>  4 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index f44e01f..7c9cfbf 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():

Since variants aren't possible here, arg_type and arg_type.members would
accomplish the same, wouldn't it?

Kills the idea to use the bug fix as motivation for .is_empty()...

>          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():

Likweise.

>          return ret
>
>      if dealloc:
> @@ -177,7 +177,7 @@ def gen_marshal(name, arg_type, ret_type):
>
>      # 'goto out' produced by gen_marshal_input_visit->gen_visit_members()
>      # 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 is just for consistency with the two previous hunks.

>          ret += mcgen('''
>
>  out:
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index d6171f2..650ba46 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -13,6 +13,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)
>  {
>  }
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index b5d0c53..33e8517 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 225e2db..6b97fa5 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -203,6 +203,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

Demonstrates another aspect of the arguments/results asymmetry in C and
QAPI.

At a sufficient level of abstraction, functions take one argument and
yield one result.  Multiple arguments and results are just notational
convenience.

Our schema language embraces this: both 'data' and 'returns' are the
name of the (single) argument/return type.  See the qapi-schema-test.out
snipped visible in the patch.

The schema also provides syntactic sugar for multiple arguments, so you
don't have to define the argument type yourself.  It doesn't do that for
results.

In C, functions take multiple arguments and yield one result.  The QAPI
code generator "unwraps" its single argument type into multiple C
arguments.  That's where the "no variants in argument type" restriction
comes from.  It can't unwrap the return type.  That's where the "no
sugar for return type" comes from: we really need a C type with a proper
name, and ':obj-frobnicate-res' wouldn't be one.

Unwrapping the empty argument type results in no arguments:
qmp_user_def_cmd0() takes only the additional Error ** argument.

Not unwrapping the empty result type results in returning nothing via
g_new0().  If that mattered, we could special-case it to return void
instead, but it doesn't.