scripts: qapi-event.py: support vendor extension

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 10 Jul 2014 16:31:38 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > The event code generator barfs when it sees a dot in an event
>> > argument, this makes it impossible to support vendor extensions
>> > in event arguments as they always contain dots. Fix this by
>> > replacing dots by hyphens in the generated code.
>> 
>> Code replaces by underbar, not hyphen.
>> 
>> > PS: Event names and QMP command arguments may suffer from the
>> > same issue, but I'm not checking/fixing them today.
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  scripts/qapi-event.py | 8 ++++----
>> >  scripts/qapi.py       | 4 ++++
>> >  2 files changed, 8 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> > index 601e307..485694b 100644
>> > --- a/scripts/qapi-event.py
>> > +++ b/scripts/qapi-event.py
>> > @@ -23,11 +23,11 @@ def _generate_event_api_name(event_name, params):
>> >      if params:
>> >          for argname, argentry, optional, structured in parse_args(params):
>> >              if optional:
>> > -                api_name += "bool has_%s,\n" % c_var(argname)
>> > +                api_name += "bool has_%s,\n" % c_arg(argname)
>> >                  api_name += "".ljust(l)
>> >  
>> >              api_name += "%s %s,\n" % (c_type(argentry, is_param=True),
>> > -                                      c_var(argname))
>> > +                                      c_arg(argname))
>> >              api_name += "".ljust(l)
>> >  
>> >      api_name += "Error **errp)"
>> > @@ -98,7 +98,7 @@ def generate_event_implement(api_name, event_name, params):
>> >                  ret += mcgen("""
>> >      if (has_%(var)s) {
>> >  """,
>> > -                             var = c_var(argname))
>> > +                             var = c_arg(argname))
>> >                  push_indent()
>> >  
>> >              if argentry == "str":
>> > @@ -113,7 +113,7 @@ def generate_event_implement(api_name, event_name, params):
>> >      }
>> >  """,
>> >                           var_type = var_type,
>> > -                         var = c_var(argname),
>> > +                         var = c_arg(argname),
>> >                           type = type_name(argentry),
>> >                           name = argname)
>> >  
>> > diff --git a/scripts/qapi.py b/scripts/qapi.py
>> > index f2c6d1f..ddab14d 100644
>> > --- a/scripts/qapi.py
>> > +++ b/scripts/qapi.py
>> > @@ -434,6 +434,10 @@ def c_var(name, protect=True):
>> >  def c_fun(name, protect=True):
>> >      return c_var(name, protect).replace('.', '_')
>> >  
>> > +# Should be used where vendor extensions are supported
>> > +def c_arg(name):
>> > +	return c_var(name).replace('.', '_')
>> > +
>> >  def c_list_type(name):
>> >      return '%sList' % name
>> 
>> Can anybody think of a use of c_var() that needs '.' preserved?
>
> Doing the replace in c_var() breaks some struct accesses in the generated
> code. I didn't look deeper to determine the users though.

Feels like a misuse of c_var() to me.

Dig, dig...  aha.  generate_visit_struct_fields() joins QAPI names
separated by '.', and passes the result to c_var().  I expect such code
to break when one of the names contains '.'.

It does indeed; try the appended patch to see it yourself.  It generates

struct VersionInfo
{
    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } qemu;
    struct 
    {
        int64_t major;
        int64_t minor;
        int64_t micro;
    } __com.redhat.crap;
    char *package;
};

Conclusion: this is simply a bug that needs fixing.

On 07/11/2014 08:42 AM, Markus Armbruster wrote:

>>> Can anybody think of a use of c_var() that needs '.' preserved?
>>
>> Doing the replace in c_var() breaks some struct accesses in the generated
>> code. I didn't look deeper to determine the users though.
> 
> Feels like a misuse of c_var() to me.
> 
> Dig, dig...  aha.  generate_visit_struct_fields() joins QAPI names
> separated by '.', and passes the result to c_var().  I expect such code
> to break when one of the names contains '.'.
> 
> It does indeed; try the appended patch to see it yourself.  It generates
> 
> struct VersionInfo
> {
>     struct 
>     {
>         int64_t major;
>         int64_t minor;
>         int64_t micro;
>     } qemu;

Wait a minute.  Isn't this one of the three cases of nested structs,
where we were already arguing that nested structs are evil if we are
going to introduce a fuller syntax for optional argument defaults?

>     struct 
>     {
>         int64_t major;
>         int64_t minor;
>         int64_t micro;
>     } __com.redhat.crap;
>     char *package;
> };
> 
> Conclusion: this is simply a bug that needs fixing.
> 
> 
> diff --git a/qapi/common.json b/qapi/common.json
> index 4e9a21f..74ccde3 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -52,6 +52,7 @@
>  ##
>  { 'type': 'VersionInfo',
>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
>             'package': 'str'} }

And the fix may be as simple as ditching support for nested structs in
the first place, and rewriting this as:

{ 'type': 'VersionDetails',
  'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
{ 'type': 'VersionInfo',
  'data': {'qemu': 'VersionDetails',
           '__com.redhat.crap': 'VersionDetails',
           'package': 'str' } }

But the fact that we are still discussing makes it obvious - this is 2.2
material.

Eric Blake <eblake@redhat.com> writes:

> On 07/11/2014 08:42 AM, Markus Armbruster wrote:
>
>>>> Can anybody think of a use of c_var() that needs '.' preserved?
>>>
>>> Doing the replace in c_var() breaks some struct accesses in the generated
>>> code. I didn't look deeper to determine the users though.
>> 
>> Feels like a misuse of c_var() to me.
>> 
>> Dig, dig...  aha.  generate_visit_struct_fields() joins QAPI names
>> separated by '.', and passes the result to c_var().  I expect such code
>> to break when one of the names contains '.'.
>> 
>> It does indeed; try the appended patch to see it yourself.  It generates
>> 
>> struct VersionInfo
>> {
>>     struct 
>>     {
>>         int64_t major;
>>         int64_t minor;
>>         int64_t micro;
>>     } qemu;
>
> Wait a minute.  Isn't this one of the three cases of nested structs,
> where we were already arguing that nested structs are evil if we are
> going to introduce a fuller syntax for optional argument defaults?

Yes.

>>     struct 
>>     {
>>         int64_t major;
>>         int64_t minor;
>>         int64_t micro;
>>     } __com.redhat.crap;
>>     char *package;
>> };
>> 
>> Conclusion: this is simply a bug that needs fixing.
>> 
>> 
>> diff --git a/qapi/common.json b/qapi/common.json
>> index 4e9a21f..74ccde3 100644
>> --- a/qapi/common.json
>> +++ b/qapi/common.json
>> @@ -52,6 +52,7 @@
>>  ##
>>  { 'type': 'VersionInfo',
>>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
>> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
>>             'package': 'str'} }
>
> And the fix may be as simple as ditching support for nested structs in
> the first place, and rewriting this as:
>
> { 'type': 'VersionDetails',
>   'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
> { 'type': 'VersionInfo',
>   'data': {'qemu': 'VersionDetails',
>            '__com.redhat.crap': 'VersionDetails',
>            'package': 'str' } }
>
> But the fact that we are still discussing makes it obvious - this is 2.2
> material.

Agree.  Let's ditch nested structs and see whether there are any misuses
of c_var() left.

On Fri, 11 Jul 2014 18:01:50 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Eric Blake <eblake@redhat.com> writes:
> 
> > On 07/11/2014 08:42 AM, Markus Armbruster wrote:
> >
> >>>> Can anybody think of a use of c_var() that needs '.' preserved?
> >>>
> >>> Doing the replace in c_var() breaks some struct accesses in the generated
> >>> code. I didn't look deeper to determine the users though.
> >> 
> >> Feels like a misuse of c_var() to me.
> >> 
> >> Dig, dig...  aha.  generate_visit_struct_fields() joins QAPI names
> >> separated by '.', and passes the result to c_var().  I expect such code
> >> to break when one of the names contains '.'.
> >> 
> >> It does indeed; try the appended patch to see it yourself.  It generates
> >> 
> >> struct VersionInfo
> >> {
> >>     struct 
> >>     {
> >>         int64_t major;
> >>         int64_t minor;
> >>         int64_t micro;
> >>     } qemu;
> >
> > Wait a minute.  Isn't this one of the three cases of nested structs,
> > where we were already arguing that nested structs are evil if we are
> > going to introduce a fuller syntax for optional argument defaults?
> 
> Yes.
> 
> >>     struct 
> >>     {
> >>         int64_t major;
> >>         int64_t minor;
> >>         int64_t micro;
> >>     } __com.redhat.crap;
> >>     char *package;
> >> };
> >> 
> >> Conclusion: this is simply a bug that needs fixing.
> >> 
> >> 
> >> diff --git a/qapi/common.json b/qapi/common.json
> >> index 4e9a21f..74ccde3 100644
> >> --- a/qapi/common.json
> >> +++ b/qapi/common.json
> >> @@ -52,6 +52,7 @@
> >>  ##
> >>  { 'type': 'VersionInfo',
> >>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> >> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> >>             'package': 'str'} }
> >
> > And the fix may be as simple as ditching support for nested structs in
> > the first place, and rewriting this as:
> >
> > { 'type': 'VersionDetails',
> >   'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
> > { 'type': 'VersionInfo',
> >   'data': {'qemu': 'VersionDetails',
> >            '__com.redhat.crap': 'VersionDetails',
> >            'package': 'str' } }
> >
> > But the fact that we are still discussing makes it obvious - this is 2.2
> > material.
> 
> Agree.  Let's ditch nested structs and see whether there are any misuses
> of c_var() left.

This is an honest question: do we really want to drop nested struct support,
wasn't it added by the block layer or am I just confused?

On 07/11/2014 12:51 PM, Luiz Capitulino wrote:

>>>>  { 'type': 'VersionInfo',
>>>>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
>>>> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
>>>>             'package': 'str'} }
>>>
>>> And the fix may be as simple as ditching support for nested structs in
>>> the first place, and rewriting this as:
>>>
>>> { 'type': 'VersionDetails',
>>>   'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
>>> { 'type': 'VersionInfo',
>>>   'data': {'qemu': 'VersionDetails',
>>>            '__com.redhat.crap': 'VersionDetails',
>>>            'package': 'str' } }
>>>
>>> But the fact that we are still discussing makes it obvious - this is 2.2
>>> material.
>>
>> Agree.  Let's ditch nested structs and see whether there are any misuses
>> of c_var() left.
> 
> This is an honest question: do we really want to drop nested struct support,
> wasn't it added by the block layer or am I just confused?

We're talking about raw inline structs - there's only 3 impacted QAPI
typesMP commands (if I counted correctly), and they have nothing to do
with block layer complex structs.  The idea is that we want to outlaw
'foo':{...} implicit structs, and instead require 'foo':'type', where
'type' was earlier defined with the {...} guts.  The QMP wire format
would be unchanged; it is just a change to the QAPI template that the
generators read.  Removing inline structs would also simplify the
generators.  Then, with that gone, we are free to to repurpose
'foo':{...} for default values of optional arguments.  Here's a link to
some of the earlier conversation:

https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html

On Fri, 11 Jul 2014 15:22:24 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/11/2014 12:51 PM, Luiz Capitulino wrote:
> 
> >>>>  { 'type': 'VersionInfo',
> >>>>    'data': {'qemu': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> >>>> +           '__com.redhat.crap': {'major': 'int', 'minor': 'int', 'micro': 'int'},
> >>>>             'package': 'str'} }
> >>>
> >>> And the fix may be as simple as ditching support for nested structs in
> >>> the first place, and rewriting this as:
> >>>
> >>> { 'type': 'VersionDetails',
> >>>   'data': { major': 'int', 'minor': 'int', 'micro': 'int'} }
> >>> { 'type': 'VersionInfo',
> >>>   'data': {'qemu': 'VersionDetails',
> >>>            '__com.redhat.crap': 'VersionDetails',
> >>>            'package': 'str' } }
> >>>
> >>> But the fact that we are still discussing makes it obvious - this is 2.2
> >>> material.
> >>
> >> Agree.  Let's ditch nested structs and see whether there are any misuses
> >> of c_var() left.
> > 
> > This is an honest question: do we really want to drop nested struct support,
> > wasn't it added by the block layer or am I just confused?
> 
> We're talking about raw inline structs - there's only 3 impacted QAPI
> typesMP commands (if I counted correctly), and they have nothing to do
> with block layer complex structs.  The idea is that we want to outlaw
> 'foo':{...} implicit structs, and instead require 'foo':'type', where
> 'type' was earlier defined with the {...} guts.  The QMP wire format
> would be unchanged; it is just a change to the QAPI template that the
> generators read.  Removing inline structs would also simplify the
> generators.  Then, with that gone, we are free to to repurpose
> 'foo':{...} for default values of optional arguments.  Here's a link to
> some of the earlier conversation:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html

Right, this makes sense. Thanks for the URLs.

Is there anyone planning on doing this? I don't think I'll have cycles
anytime soon... I'd be willing to merge my fix if nobody steps up.

On 07/14/2014 12:12 PM, Luiz Capitulino wrote:
>>>> Agree.  Let's ditch nested structs and see whether there are any misuses
>>>> of c_var() left.
>>>
>>> This is an honest question: do we really want to drop nested struct support,
>>> wasn't it added by the block layer or am I just confused?
>>
>> We're talking about raw inline structs - there's only 3 impacted QAPI
>> typesMP commands (if I counted correctly), and they have nothing to do
>> with block layer complex structs.  The idea is that we want to outlaw
>> 'foo':{...} implicit structs, and instead require 'foo':'type', where
>> 'type' was earlier defined with the {...} guts.  The QMP wire format
>> would be unchanged; it is just a change to the QAPI template that the
>> generators read.  Removing inline structs would also simplify the
>> generators.  Then, with that gone, we are free to to repurpose
>> 'foo':{...} for default values of optional arguments.  Here's a link to
>> some of the earlier conversation:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
>> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html
> 
> Right, this makes sense. Thanks for the URLs.
> 
> Is there anyone planning on doing this? I don't think I'll have cycles
> anytime soon... I'd be willing to merge my fix if nobody steps up.

I may take a stab at removing raw inline structs after 2.1 is released,
if no one beats me to it.

On Mon, 14 Jul 2014 12:31:32 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/14/2014 12:12 PM, Luiz Capitulino wrote:
> >>>> Agree.  Let's ditch nested structs and see whether there are any misuses
> >>>> of c_var() left.
> >>>
> >>> This is an honest question: do we really want to drop nested struct support,
> >>> wasn't it added by the block layer or am I just confused?
> >>
> >> We're talking about raw inline structs - there's only 3 impacted QAPI
> >> typesMP commands (if I counted correctly), and they have nothing to do
> >> with block layer complex structs.  The idea is that we want to outlaw
> >> 'foo':{...} implicit structs, and instead require 'foo':'type', where
> >> 'type' was earlier defined with the {...} guts.  The QMP wire format
> >> would be unchanged; it is just a change to the QAPI template that the
> >> generators read.  Removing inline structs would also simplify the
> >> generators.  Then, with that gone, we are free to to repurpose
> >> 'foo':{...} for default values of optional arguments.  Here's a link to
> >> some of the earlier conversation:
> >>
> >> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00708.html
> >> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg04268.html
> > 
> > Right, this makes sense. Thanks for the URLs.
> > 
> > Is there anyone planning on doing this? I don't think I'll have cycles
> > anytime soon... I'd be willing to merge my fix if nobody steps up.
> 
> I may take a stab at removing raw inline structs after 2.1 is released,
> if no one beats me to it.

Great! Thanks a lot.