Re: [Qemu-devel] [PATCH v2 2/2] qapi: Allow setting default values for optional parameters

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> In command definition, 'defaults' is now parsed as a dict of default
> values. Only optional parameters will have effect in generated code.
>
> 'str' and 'int' are supported. In generated code, 'str' will be
> converted to g_strdup'ed string pointer, 'int' will be identically
> assigned.
>
> E.g.
>
>     { 'command': 'block-commit',
>       'data': { 'device': 'str', '*base': 'str', 'top': 'str',
>                 '*speed': 'int' },
>       'defaults': {'base': 'earthquake', 'speed': 100 } }
>
> will generate
>
>     int qmp_marshal_input_block_commit(Monitor *mon, const QDict *qdict, 
> QObject **ret)
>     {
>         ...
>         bool has_base = true;
>         char * base = g_strdup("earthquake");
>         ...
>         bool has_speed = true;
>         int64_t speed = 100;
>
> Updated docs/qapi-code-gen.txt and qapi-schema tests.
>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  docs/qapi-code-gen.txt                  |  8 ++++++--
>  scripts/qapi-commands.py                | 29 ++++++++++++++++++++++-------
>  scripts/qapi.py                         |  8 ++++++++
>  tests/qapi-schema/qapi-schema-test.json |  3 +++
>  tests/qapi-schema/qapi-schema-test.out  |  1 +
>  tests/test-qmp-commands.c               |  7 +++++++
>  6 files changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index d78921f..b4cc6ed 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -172,12 +172,16 @@ This example allows using both of the following example 
> objects:
>  
>  Commands are defined by using a list containing three members.  The first
>  member is the command name, the second member is a dictionary containing
> -arguments, and the third member is the return type.
> +arguments, the third member is optional to define default values for optional
> +arguments in 'data' dictionary, and the fourth member is the return type.
> +
> +Non-optional argument names are not allowed in the 'defaults' dictionary.
>  
>  An example command is:
>  
>   { 'command': 'my-command',
> -   'data': { 'arg1': 'str', '*arg2': 'str' },
> +   'data': { 'arg1': 'str', '*arg2': 'str', '*arg3': 'int' },
> +   'defaults': { 'arg2': 'default value for arg2', 'arg3': 12345 },
>     'returns': 'str' }
>  
>  

I'm only reviewing schema design here.

So far, a command parameter has three propagated: name, type, and
whether it's optional.  Undocumented hack: a type '**' means "who
knows", and suppresses marshalling function generation for the command.

The three properties are encoded in a single member of 'data': name
becomes the member name, and type becomes the value, except optional is
shoehorned into the name as well[*].

Your patch adds have a fourth property, namely the default value.  It is
only valid for optional parameters.  You put it into a separate member
'defaults', next to 'data.  This spreads parameter properties over two
separate objects.  I don't like that.  What if we come up with a fifth
one?  Then it'll get even worse.

Can we keep a parameter's properties together?  Perhaps like this:

    NAME: { 'type': TYPE, 'default': DEFAULT }

where

    NAME: { 'type': TYPE }

can be abbreviated to

    NAME: TYPE

and

    NAME: { 'type': TYPE, 'default': null }
to

    NAME-PREFIXED-BY_ASKTERISK: TYPE

if we think these abbreviations enhance schema readability enough to be
worthwhile.  The first one does, in my opinion. but I'm not so sure
about the second one.


[*] I'm sure that felt like a good idea at the time.