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

[Eric Blake]

On 04/29/2014 03:44 AM, Fam Zheng wrote:
> 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.

In addition to Kevin's review...

> +++ 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.

This is a dictionary; order shouldn't be important; and since things can
be omitted, 'defaults' is not always the fourth entry.  That is,

{ 'command': 'my-command1', 'returns': 'str' }
{ 'returns': 'str', 'command': 'my-command2' }

should be a valid file, even though the second line does not list the
command name as the first member.

Can a 'str' parameter explicitly default to NULL, as in:

{ 'command': 'foo', 'data': { '*s': 'str' }, 'defaults': { 's': null } }

for the case where the code really wants NULL (and not "") as the
default value for an omitted string?

> +++ b/tests/test-qmp-commands.c
> @@ -48,6 +48,13 @@ int64_t qmp_user_def_cmd3(int64_t a, bool has_b, int64_t 
> b, Error **errp)
>      return a + (has_b ? b : 0);
>  }
>  
> +int64_t qmp_user_def_cmd4(int64_t a, bool has_b, int64_t b,
> +                          bool has_c, const char *c,
> +                          Error **errp)
> +{
> +    return has_b && b == 100 && has_c && !strcmp(c, "A string");
> +}
> +

Missing (at least) the following semantic tests:

Supplying a defaults for a non-optional parameter should be an error
Supplying a defaults for an optional parameter not of integer or string
type should be an error
Supplying a string default for an integer parameter, or an integer
default for a string parameter, should be an error
Test for integer overflow of an integer default
Test for an integer default of 0 and/or negative

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature