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

[Fam Zheng]

On Tue, 04/29 06:51, Eric Blake wrote:
> On 04/29/2014 05:24 AM, Kevin Wolf wrote:
> > 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.
> >>
> 
> >>   { '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).
> 
> That includes documenting that omitting a defaults entry for an optional
> parameter will now guarantee 0-initialization of that member.

OK.

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

Agreed.

> 
> Yes, that's a nice idea - anywhere we have a qapi-documented default, we
> can simplify the code to take advantage of the default.  It's backward
> compatible since none of the existing code has any contract on defaults.
> 
> Also, is the default value allowed to change between qemu versions?
> What are the back-compat ramifications if two different releases want to
> tweak an omitted variable to different defaults?  The documentation
> should mention the rule of thumb that we plan on enforcing during
> reviews.  I could go either way: the wire format is unimpacted by what
> default value is used when an argument is omitted, and management can
> always explicitly specify a parameter if they don't trust the default;
> on the other hand, if changing a default changes visible behavior, then
> we have not maintained ABI compatibility.
> 

This is a good question but not new here: omitting explict value for optional
parameter yields a "default" value chosen by QEMU, either specified in
qapi-schema.json as we are adding support in the syntax, or in the C code
surrounded by "if (!has_foo)" as before. That's not changed here.

So I agree as you said, the rule is always there, we shouldn't change visible
behavior.

Will send a separate patch on docs/qapi-code-gen.txt to document this
convention.

Fam