Fam Zheng <f...@redhat.com> 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 <f...@redhat.com>
> ---
> 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.
