Re: [Qemu-devel] [PATCH v2 1/2] qapi: Allow decimal values

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Allow decimal values

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/29/2014 03:44 AM, Fam Zheng wrote:
>> This allows giving decimal constants in the schema as expr.
>> 
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  scripts/qapi.py | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>> 
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index b474c39..6022de5 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -108,6 +108,14 @@ class QAPISchema:
>>                          return
>>                      else:
>>                          string += ch
>> +            elif self.tok in "123456789":
>> +                self.val = int(self.tok)
>> +                while True:
>> +                    ch = self.src[self.cursor]
>> +                    if ch not in "1234567890":
>> +                        return
>> +                    self.val = self.val * 10 + int(ch)
>> +                    self.cursor += 1
>
> What does this do on integer overflow?  Should you validate that the
> result fits in [u]int64_t?  Should you allow for a leading '-' sign for
> a default of a negative number?  You have forbidden '0' as a valid
> number (although you correctly forbid '00' and any non-zero number with
> a leading 0, which matches JSON restrictions).

If every valid JSON number is also a valid Python number, then you can
accumulate the token, then convert it with built-in function int().
Just like our C JSON lexer uses strtoll(), in parse_literal().

>> -        if not self.tok in [ '{', '[', "'" ]:
>> -            raise QAPISchemaError(self, 'Expected "{", "[", "]" or string')
>> +        if not self.tok in "{['123456789":
>> + raise QAPISchemaError(self, 'Expected "{", "[", "]", string or
>> number')
>
> Again, this forbids the use of '0' as a default.

The spec for our lexical analysis comes from RFC 4627.  We take some
liberties with it, but we should do so only deliberately, and with a
sensible reason.  We should also keep the Python version (qapi.py)
consistent with the C version (qobject/json*.[ch]).

In particular see how parse_literal() deals with integer overflow.

Grammar for numbers, straight from RFC 4627:

     number = [ minus ] int [ frac ] [ exp ]

         decimal-point = %x2E       ; .

         digit1-9 = %x31-39         ; 1-9

         e = %x65 / %x45            ; e E

         exp = e [ minus / plus ] 1*DIGIT

         frac = decimal-point 1*DIGIT

         int = zero / ( digit1-9 *DIGIT )

         minus = %x2D               ; -

         plus = %x2B                ; +

         zero = %x30                ; 0

Since we distinguish between integral and fractional numbers everywhere
in our usage of JSON, it makes sense to do so here as well.

This means we want to accept this sub-grammar:

    inum = [ minus ] int

         minus = %x2D               ; -

         digit1-9 = %x31-39         ; 1-9

         zero = %x30                ; 0

         int = zero / ( digit1-9 *DIGIT )

json-lexer.c's state machine implements this faithfully (as far as I can
tell).

Fractional numbers could be left for later here, since your PATCH 2/2,
the first and so far only user of numbers, arbitrarily restricts them to
integer.  Just implementing them might be easier than documenting the
restriction, though...

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

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

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

Currently, an optional FOO should only be used when has_FOO.  We
initialize it anyway, to suppress complaints from static analyzers.

Do we want to relax the rule "officially", so that code can rely on
zero-initialization instead of checking has_FOO?

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

s/can simplify/should simplify/!  Defining a default value for FOO
renders anything guarded by !has_FOO dead code.  Not generating has_FOO
is good, because it forces us to bury the dead code properly.

Losely related: I want to get rid of has_FOO for pointer-valued FOOs,
too.

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

We should use common sense.

Changing the schema in a way that alters the meaning of existing usage
is a no-no, and that applies to defaults as much as to anything else.
But not every change of a default necessarily does that.  Contrieved
example: if the value defines a buffer size, and it's specified to
default to a sensible size, then we can and should let the default value
evolve along with our idea of what a sensible size is.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

On 05/05/2014 03:51 AM, Markus Armbruster wrote:

>> 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.
> 
> We should use common sense.
> 
> Changing the schema in a way that alters the meaning of existing usage
> is a no-no, and that applies to defaults as much as to anything else.
> But not every change of a default necessarily does that.  Contrieved
> example: if the value defines a buffer size, and it's specified to
> default to a sensible size, then we can and should let the default value
> evolve along with our idea of what a sensible size is.

I agree - it's more important to declare that we are maintaining
documented semantics (omitting the parameter will always pick the best
size, even if that size differs over time) than precise values (omitting
the parameter will always pick 512 bytes, even if that is not the ideal
size for newer versions) - so it is somewhat a documentation game of
whether we document default values well enough to let them be flexible :)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Fam Zheng <famz@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 <famz@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.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On 05/05/2014 05:06 AM, Markus Armbruster wrote:

>>
>>     { 'command': 'block-commit',
>>       'data': { 'device': 'str', '*base': 'str', 'top': 'str',
>>                 '*speed': 'int' },
>>       'defaults': {'base': 'earthquake', 'speed': 100 } }
>>

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

I like it.  It also means:

data: { '*foo': { 'type': 'str', 'default': 'hello' } }

is invalid - a defaulted argument is NOT spelled '*foo' but merely 'foo'.

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

Or, putting the question in reverse, you are asking if:

data: { '*foo': 'str' }

can blindly be rewritten into:

data: { 'foo': { 'type': 'str', 'default': null } }

and the rest of the introspection use the fact that 'default':null
implies that the argument is optional but has no specified default, and
therefore still needs the has_foo magic.

As you say, it's a bit more of a stretch, but does make introspection
nice (introspection already has to deal with leading '*' to turn it into
something nicer to pass over the wire - if we ever get introspection
working).  I'd have to see it actually coded up to decide for sure if it
turned out to be a net win.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/05/2014 05:06 AM, Markus Armbruster wrote:
>
>>>
>>>     { 'command': 'block-commit',
>>>       'data': { 'device': 'str', '*base': 'str', 'top': 'str',
>>>                 '*speed': 'int' },
>>>       'defaults': {'base': 'earthquake', 'speed': 100 } }
>>>
>
>> 
>> 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
>
> I like it.  It also means:
>
> data: { '*foo': { 'type': 'str', 'default': 'hello' } }
>
> is invalid - a defaulted argument is NOT spelled '*foo' but merely 'foo'.
>
>> 
>> 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.
>
> Or, putting the question in reverse, you are asking if:
>
> data: { '*foo': 'str' }
>
> can blindly be rewritten into:
>
> data: { 'foo': { 'type': 'str', 'default': null } }
>
> and the rest of the introspection use the fact that 'default':null
> implies that the argument is optional but has no specified default, and
> therefore still needs the has_foo magic.
>
> As you say, it's a bit more of a stretch, but does make introspection
> nice (introspection already has to deal with leading '*' to turn it into
> something nicer to pass over the wire - if we ever get introspection
> working).  I'd have to see it actually coded up to decide for sure if it
> turned out to be a net win.

Glad you mention introspection; I didn't think of it.

The "an asterisk at the beginning of a name is not part of the name, but
means the parameter is optional" thing is a deviation from our usual
design rule against parsing strings in addition to JSON.  My proposal to
make the "is optional" property an ordinary JSON member straightens this
out.

The bit I'm not sure about is whether we want to keep the
NAME-PREFIXED-BY-ASKTERISK: TYPE form as syntactic sugar.

Keeping the TYPE: NAME form as sugar makes sense to me, because it cuts
noise in the schema while adding no syntax beyond JSON.

The schema parser desugars its input.  Sugaring schema introspection
output makes no sense to me, because all that accomplishes is
introspection users have to duplicate the schema parser's desugaring.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2443 bytes --]

On 05/05/2014 11:34 AM, Markus Armbruster wrote:
>>
>> Or, putting the question in reverse, you are asking if:
>>
>> data: { '*foo': 'str' }
>>
>> can blindly be rewritten into:
>>
>> data: { 'foo': { 'type': 'str', 'default': null } }
>>
>> and the rest of the introspection use the fact that 'default':null
>> implies that the argument is optional but has no specified default, and
>> therefore still needs the has_foo magic.
>>
>> As you say, it's a bit more of a stretch, but does make introspection
>> nice (introspection already has to deal with leading '*' to turn it into
>> something nicer to pass over the wire - if we ever get introspection
>> working).  I'd have to see it actually coded up to decide for sure if it
>> turned out to be a net win.
> 
> Glad you mention introspection; I didn't think of it.
> 
> The "an asterisk at the beginning of a name is not part of the name, but
> means the parameter is optional" thing is a deviation from our usual
> design rule against parsing strings in addition to JSON.  My proposal to
> make the "is optional" property an ordinary JSON member straightens this
> out.
> 
> The bit I'm not sure about is whether we want to keep the
> NAME-PREFIXED-BY-ASKTERISK: TYPE form as syntactic sugar.

Keeping it as sugar in the input .json files seems reasonable.

Exposing it as sugar in introspection is a bad idea.

Keeping the input file easy to write, and more compact than what
introspection will output, is a fine tradeoff in my book (easier to
maintain if there is less to type; while still having a well-defined
conversion to the formal output form).

> 
> Keeping the TYPE: NAME form as sugar makes sense to me, because it cuts
> noise in the schema while adding no syntax beyond JSON.

Exactly - the point of syntactic sugar is to have a short form for
common usage, while having the full form when full expressiveness is
needed.  The .json schema files are internal use only; the introspection
QMP command is not yet written but can adapt to the ideas in this thread.

> 
> The schema parser desugars its input.  Sugaring schema introspection
> output makes no sense to me, because all that accomplishes is
> introspection users have to duplicate the schema parser's desugaring.

I think we're on the same page, then.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/05/2014 11:34 AM, Markus Armbruster wrote:
>>>
>>> Or, putting the question in reverse, you are asking if:
>>>
>>> data: { '*foo': 'str' }
>>>
>>> can blindly be rewritten into:
>>>
>>> data: { 'foo': { 'type': 'str', 'default': null } }
>>>
>>> and the rest of the introspection use the fact that 'default':null
>>> implies that the argument is optional but has no specified default, and
>>> therefore still needs the has_foo magic.
>>>
>>> As you say, it's a bit more of a stretch, but does make introspection
>>> nice (introspection already has to deal with leading '*' to turn it into
>>> something nicer to pass over the wire - if we ever get introspection
>>> working).  I'd have to see it actually coded up to decide for sure if it
>>> turned out to be a net win.
>> 
>> Glad you mention introspection; I didn't think of it.
>> 
>> The "an asterisk at the beginning of a name is not part of the name, but
>> means the parameter is optional" thing is a deviation from our usual
>> design rule against parsing strings in addition to JSON.  My proposal to
>> make the "is optional" property an ordinary JSON member straightens this
>> out.
>> 
>> The bit I'm not sure about is whether we want to keep the
>> NAME-PREFIXED-BY-ASKTERISK: TYPE form as syntactic sugar.
>
> Keeping it as sugar in the input .json files seems reasonable.
>
> Exposing it as sugar in introspection is a bad idea.
>
> Keeping the input file easy to write, and more compact than what
> introspection will output, is a fine tradeoff in my book (easier to
> maintain if there is less to type; while still having a well-defined
> conversion to the formal output form).

Unlike the other sugared form, this one adds syntax beyond JSON, namely
in some (but not all) member names, and that makes it a bit harder to
stomach for me.

Whether it's worthwhile depends on how common the case "optional, no
default" turns out to be.  Wait and see.

>> Keeping the TYPE: NAME form as sugar makes sense to me, because it cuts
>> noise in the schema while adding no syntax beyond JSON.
>
> Exactly - the point of syntactic sugar is to have a short form for
> common usage, while having the full form when full expressiveness is
> needed.  The .json schema files are internal use only; the introspection
> QMP command is not yet written but can adapt to the ideas in this thread.
>
>> 
>> The schema parser desugars its input.  Sugaring schema introspection
>> output makes no sense to me, because all that accomplishes is
>> introspection users have to duplicate the schema parser's desugaring.
>
> I think we're on the same page, then.

:)

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

On 05/06/2014 03:55 AM, Markus Armbruster wrote:

>> Keeping the input file easy to write, and more compact than what
>> introspection will output, is a fine tradeoff in my book (easier to
>> maintain if there is less to type; while still having a well-defined
>> conversion to the formal output form).
> 
> Unlike the other sugared form, this one adds syntax beyond JSON, namely
> in some (but not all) member names, and that makes it a bit harder to
> stomach for me.

JSON has no requirement that a 'name':'value' object limit the 'name'
portion to just valid identifier characters.  But I do see your point
about the fact that we are parsing a sigil of '*' as the first character
of 'name' as sugar.

> 
> Whether it's worthwhile depends on how common the case "optional, no
> default" turns out to be.  Wait and see.

It's already VERY common - every optional variable already uses the
syntax.  The question is rather: how many optional variables will be
rewritten to express a default value, vs. those that can be left alone
because the default value is good enough.  A global search-and-replace
could rewrite the entire file to the new syntax for optional variables,
and we could enforce the new more verbose style going forward, but is
the churn worth it?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/06/2014 03:55 AM, Markus Armbruster wrote:
>
>>> Keeping the input file easy to write, and more compact than what
>>> introspection will output, is a fine tradeoff in my book (easier to
>>> maintain if there is less to type; while still having a well-defined
>>> conversion to the formal output form).
>> 
>> Unlike the other sugared form, this one adds syntax beyond JSON, namely
>> in some (but not all) member names, and that makes it a bit harder to
>> stomach for me.
>
> JSON has no requirement that a 'name':'value' object limit the 'name'
> portion to just valid identifier characters.  But I do see your point
> about the fact that we are parsing a sigil of '*' as the first character
> of 'name' as sugar.
>
>> 
>> Whether it's worthwhile depends on how common the case "optional, no
>> default" turns out to be.  Wait and see.
>
> It's already VERY common - every optional variable already uses the
> syntax.  The question is rather: how many optional variables will be
> rewritten to express a default value, vs. those that can be left alone
> because the default value is good enough.

That's precisely the question.

I like explicit defaults in the schema, they're fine documentation.
They can also simplify code.

>                                            A global search-and-replace
> could rewrite the entire file to the new syntax for optional variables,
> and we could enforce the new more verbose style going forward, but is
> the churn worth it?

Wait and see how many optionals pick up defaults.  The more do, the less
churn.

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

[Fam Zheng]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2921 bytes --]

On 05/05/2014 07:30 PM, Fam Zheng wrote:

>>     NAME: { 'type': TYPE, 'default': DEFAULT }
>>
>> where
>>
>>     NAME: { 'type': TYPE }
>>
>> can be abbreviated to
>>
>>     NAME: TYPE

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

Oh, good catch.  The alternative is to drop all instances of inline
sub-structs.  Searching for 'data.*{.*{', I found only VersionInfo and
PciBridgeInfo; I then did a full manual search of qapi-schema.json and
only found the additional instance of PciDeviceInfo where the sub-struct
is not on the same line as the initial { of the 'data'.  Just getting
rid of inlined sub-structs may be quite feasible.

On a related vein, there are a number of types that aren't merely a
string name.  For example:

{ 'command': 'migrate-set-capabilities',
  'data': { 'capabilities': ['MigrationCapabilityStatus'] } }

and similar, where we have an array type rather than a raw string type
name.  But at least with that, NAME: { 'type': [ 'array' ] } is still a
reasonable parse.

The problem with MAGIC:{'name'...} is that you need to express more than
one parameter, but as a dict, you can't reuse the same MAGIC more than
once.  That is:

'data': { MAGIC : { 'name': 'qemu', 'type': { 'major'...} },
          MAGIC : { 'name': 'package', 'type', 'str' } } }

proves that you have to have two distinct MAGIC in the same 'data', so
'' for both is out.

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

Also, while we aren't strict JSON, it's nice that we're still fairly
close to JSON5, and worth keeping that property.

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

At this point, I'm leaning towards dropping support for unnamed inlined
sub-structs.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Fam Zheng]

On Mon, 05/05 21:09, Eric Blake wrote:
> On 05/05/2014 07:30 PM, Fam Zheng wrote:
> 
> >>     NAME: { 'type': TYPE, 'default': DEFAULT }
> >>
> >> where
> >>
> >>     NAME: { 'type': TYPE }
> >>
> >> can be abbreviated to
> >>
> >>     NAME: TYPE
> 
> > 
> > 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 }
> 
> Oh, good catch.  The alternative is to drop all instances of inline
> sub-structs.  Searching for 'data.*{.*{', I found only VersionInfo and
> PciBridgeInfo; I then did a full manual search of qapi-schema.json and
> only found the additional instance of PciDeviceInfo where the sub-struct
> is not on the same line as the initial { of the 'data'.  Just getting
> rid of inlined sub-structs may be quite feasible.

Sounds good.

> 
> On a related vein, there are a number of types that aren't merely a
> string name.  For example:
> 
> { 'command': 'migrate-set-capabilities',
>   'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
> 
> and similar, where we have an array type rather than a raw string type
> name.  But at least with that, NAME: { 'type': [ 'array' ] } is still a
> reasonable parse.
> 
> The problem with MAGIC:{'name'...} is that you need to express more than
> one parameter, but as a dict, you can't reuse the same MAGIC more than
> once.  That is:
> 
> 'data': { MAGIC : { 'name': 'qemu', 'type': { 'major'...} },
>           MAGIC : { 'name': 'package', 'type', 'str' } } }

Right.

Not necessarily better than dropping inline structure, just another idea:

  'data' { '@arg_with_properties': { 'type': 'str' },
           '*simple_optional': 'str',
           '@optional_full': { 'type': 'int', 'default': 1 },
           'the_most_common_form': 'bool',
           'inline_structure': { 'major': ...} }

Where '@' is a prefix for property dictionary similar to, while exclusive with,
optional mark '*'.

> 
> proves that you have to have two distinct MAGIC in the same 'data', so
> '' for both is out.
> 
> > 
> > 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.
> 
> Also, while we aren't strict JSON, it's nice that we're still fairly
> close to JSON5, and worth keeping that property.
> 
> > 
> > Personally, I think empty str '' makes more sense for me. What do you think?
> > 
> 
> At this point, I'm leaning towards dropping support for unnamed inlined
> sub-structs.

Yes. That said, I'm also leaning towards dropping, that keeps things simple.

Fam

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 05/05 21:09, Eric Blake wrote:
>> On 05/05/2014 07:30 PM, Fam Zheng wrote:
>> 
>> >>     NAME: { 'type': TYPE, 'default': DEFAULT }
>> >>
>> >> where
>> >>
>> >>     NAME: { 'type': TYPE }
>> >>
>> >> can be abbreviated to
>> >>
>> >>     NAME: TYPE
>> 
>> > 
>> > 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 }
>> 
>> Oh, good catch.  The alternative is to drop all instances of inline
>> sub-structs.  Searching for 'data.*{.*{', I found only VersionInfo and
>> PciBridgeInfo; I then did a full manual search of qapi-schema.json and
>> only found the additional instance of PciDeviceInfo where the sub-struct
>> is not on the same line as the initial { of the 'data'.  Just getting
>> rid of inlined sub-structs may be quite feasible.
>
> Sounds good.
>
>> 
>> On a related vein, there are a number of types that aren't merely a
>> string name.  For example:
>> 
>> { 'command': 'migrate-set-capabilities',
>>   'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
>> 
>> and similar, where we have an array type rather than a raw string type
>> name.  But at least with that, NAME: { 'type': [ 'array' ] } is still a
>> reasonable parse.
>> 
>> The problem with MAGIC:{'name'...} is that you need to express more than
>> one parameter, but as a dict, you can't reuse the same MAGIC more than
>> once.  That is:
>> 
>> 'data': { MAGIC : { 'name': 'qemu', 'type': { 'major'...} },
>>           MAGIC : { 'name': 'package', 'type', 'str' } } }
>
> Right.
>
> Not necessarily better than dropping inline structure, just another idea:
>
>   'data' { '@arg_with_properties': { 'type': 'str' },
>            '*simple_optional': 'str',
>            '@optional_full': { 'type': 'int', 'default': 1 },
>            'the_most_common_form': 'bool',
>            'inline_structure': { 'major': ...} }
>
> Where '@' is a prefix for property dictionary similar to, while exclusive with,
> optional mark '*'.

If we decide to keep the '*' sigil, we can add more.  Two similar
violations of the "no syntax beyond JSON" design rule aren't really
worse than one.

That said, I dislike all sigils equally :)

[...]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/05/2014 07:30 PM, Fam Zheng wrote:
>
>>>     NAME: { 'type': TYPE, 'default': DEFAULT }
>>>
>>> where
>>>
>>>     NAME: { 'type': TYPE }
>>>
>>> can be abbreviated to
>>>
>>>     NAME: TYPE
>
>> 
>> 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 }
>
> Oh, good catch.

Indeed.

>                  The alternative is to drop all instances of inline
> sub-structs.  Searching for 'data.*{.*{', I found only VersionInfo and
> PciBridgeInfo; I then did a full manual search of qapi-schema.json and
> only found the additional instance of PciDeviceInfo where the sub-struct
> is not on the same line as the initial { of the 'data'.  Just getting
> rid of inlined sub-structs may be quite feasible.
>
> On a related vein, there are a number of types that aren't merely a
> string name.  For example:
>
> { 'command': 'migrate-set-capabilities',
>   'data': { 'capabilities': ['MigrationCapabilityStatus'] } }
>
> and similar, where we have an array type rather than a raw string type
> name.  But at least with that, NAME: { 'type': [ 'array' ] } is still a
> reasonable parse.
>
> The problem with MAGIC:{'name'...} is that you need to express more than
> one parameter, but as a dict, you can't reuse the same MAGIC more than
> once.  That is:
>
> 'data': { MAGIC : { 'name': 'qemu', 'type': { 'major'...} },
>           MAGIC : { 'name': 'package', 'type', 'str' } } }
>
> proves that you have to have two distinct MAGIC in the same 'data', so
> '' for both is out.
>
>> 
>> 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.

JSON requires member names to be strings.

>>  - 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.
>
> Also, while we aren't strict JSON, it's nice that we're still fairly
> close to JSON5, and worth keeping that property.

Both ideas move us further away from JSON and JSON5.

JSON5 allows unquoted identifiers as member names, but that's just
shorthand for the same name in quotes.

>> Personally, I think empty str '' makes more sense for me. What do you think?
>> 
>
> At this point, I'm leaning towards dropping support for unnamed inlined
> sub-structs.

For what it's worth, qapi-code-gen.txt has only strings (i.e. type
names) in the value position of a member declaration, never an object
(i.e. anonymous complex type, what you called unnamed inlined
sub-struct).

Another idea (I'm not sure I like it, though): in the unsugared NAME:
OBJECT form, OBJECT has a mandatory member 'type'.  This can be used to
decide whether we have an unsugared member definition or sugared member
definition with an anonymous complex type.  Examples:

* This can't be an unsugared member definition, because it lacks 'type':

    'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'}

* This could be either, but we prefer unsugared then:

    'foo': { 'type': 'str' }

  If you want an anonymous complex type, you need to desugar, like this:

    'foo': { 'type': { 'type': 'str' } }

Might want to outlaw member name 'type' to remove the ambiguity.

Yet another idea: drop anonymous complex types in the sugared form only.
Then an object in the value position always means unsugared.  We change
the existing members of anonymous complex type from NAME: OBJECT to
NAME: { 'type': OBJECT }.

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

The schema is still just an internal interface; we can change it as much
as we want.  With schema introspection, the desugared form of the schema
visible there will be ABI.  Which means we better use our chance to make
our schema language a bit more extensible while it lasts.