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

[Kevin Wolf]

Am 29.04.2014 um 11:44 hat Fam Zheng geschrieben:
> 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

(Up to) four now.

>  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 think we should document what effect setting a default has for the
code generation, and that it can be left out even for optional arguments
(because your example sets it for every optional member).

By the way, it seems that data and the return value are already optional,
too.

> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 9734ab0..aa4ce65 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -83,16 +83,28 @@ Visitor *v;
>  
>      return ret.rstrip()
>  
> -def gen_visitor_input_vars_decl(args):
> +def gen_visitor_input_vars_decl(cmd_name, args, defaults):
>      ret = ""
>      push_indent()
>      for argname, argtype, optional, structured in parse_args(args):
>          if optional:
>              ret += mcgen('''
> -bool has_%(argname)s = false;
> +bool has_%(argname)s = %(has_val)s;
>  ''',
> -                         argname=c_var(argname))
> -        if c_type(argtype).endswith("*"):
> +                         argname=c_var(argname),
> +                         has_val="true" if defaults.get(argname) else 
> "false")

So the local 'has_foo' variable of the marshaller is now initialised as
true. Later we have checks like:

    if (has_foo) {
        visit_type_int(v, &foo, "foo", errp);
    }

Doesn't this fail when the field isn't actually passed and the default
needs to be used?

Also, for the actual qmp_foo() handler, I would very much prefer if it
didn't get a has_foo argument which is meaningless because it is always
true. Instead, the has_foo argument shouldn't even be generated in the
first place if the schema has a default for foo.

> +        if defaults.get(argname):
> +            if optional:
> +                ret += mcgen('''
> +%(argtype)s %(argname)s = %(argval)s;
> +''',
> +                         argname=c_var(argname), argtype=c_type(argtype),
> +                         argval=c_val(argtype, defaults[argname]))
> +            else:
> +                raise Exception('Error while generating "%s": '
> +                                'default value given to non-optional value: 
> "%s"'
> +                                % (cmd_name, argname))
> +        elif c_type(argtype).endswith("*"):
>              ret += mcgen('''
>  %(argtype)s %(argname)s = NULL;
>  ''',
> @@ -194,7 +206,7 @@ def gen_marshal_input_decl(name, args, ret_type, 
> middle_mode):
>  
>  
>  
> -def gen_marshal_input(name, args, ret_type, middle_mode):
> +def gen_marshal_input(name, args, ret_type, middle_mode, defaults):
>      hdr = gen_marshal_input_decl(name, args, ret_type, middle_mode)
>  
>      ret = mcgen('''
> @@ -229,7 +241,7 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
>  
>  ''',
>                       
> visitor_input_containers_decl=gen_visitor_input_containers_decl(args),
> -                     
> visitor_input_vars_decl=gen_visitor_input_vars_decl(args),
> +                     
> visitor_input_vars_decl=gen_visitor_input_vars_decl(name, args, defaults),
>                       visitor_input_block=gen_visitor_input_block(args, 
> "QOBJECT(args)"))
>      else:
>          ret += mcgen('''
> @@ -434,9 +446,12 @@ if dispatch_type == "sync":
>  
>      for cmd in commands:
>          arglist = []
> +        defaults = {}
>          ret_type = None
>          if cmd.has_key('data'):
>              arglist = cmd['data']
> +        if cmd.has_key('defaults'):
> +            defaults = cmd['defaults']
>          if cmd.has_key('returns'):
>              ret_type = cmd['returns']
>          ret = generate_command_decl(cmd['command'], arglist, ret_type) + "\n"
> @@ -448,7 +463,7 @@ if dispatch_type == "sync":
>          if middle_mode:
>              fdecl.write('%s;\n' % gen_marshal_input_decl(cmd['command'], 
> arglist, ret_type, middle_mode))
>  
> -        ret = gen_marshal_input(cmd['command'], arglist, ret_type, 
> middle_mode) + "\n"
> +        ret = gen_marshal_input(cmd['command'], arglist, ret_type, 
> middle_mode, defaults) + "\n"
>          fdef.write(ret)
>  
>      fdecl.write("\n#endif\n");
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6022de5..e1ee232 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -432,6 +432,14 @@ def find_enum(name):
>  def is_enum(name):
>      return find_enum(name) != None
>  
> +def c_val(t, val):
> +    if t == 'str':
> +        return 'g_strdup("%s")' % val

Where is the corresponding g_free?

I think if the default is actually used, the deallocation visitor code
might take care of it even if the string wasn't originally created using
the string input visitor. However, if the default is overridden, it
looks like we leak the default.

> +    elif t == 'int':
> +        return val
> +    else:
> +        assert False, "Unknown type: %s" % t
> +

Kevin