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

[Fam Zheng <famz@redhat.com>]

On Mon, 05/05 13:06, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> >  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.

Thanks! That's very constructive.

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

Good idea. We have a problem to solve though.

In data definition, we allow inline sub-structure:

{ 'type': 'VersionInfo',
  'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
           'package': 'str'} }

If we allow properties as a dict, we need to disambiguate properly from the
above case. Proposing:

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

Where MAGIC should NOT be something that is a valid NAME from current schema.
Some ideas:

 - Special string, that has invalid chars as a identifier, like '*', '%', '&',
   etc, or simply an empty str ''.
   Of course we need to enforce the checking and distinguishing in
   scripts/qapi-types.py.

 - Non-string: current NAME must be a string, any type non-string could be
   distinguished from NAME, like number, bool, null, []. But its meaning could
   be confusing to reviewer.

 - Special symbol: we can define a dedicate symbol for this, like the literal
   TYPE, in the schema. But this way we deviate more from JSON.

Personally, I think empty str '' makes more sense for me. What do you think?

Anyway we only add things, so we will keep the '*name' suger.

Thanks,
Fam