[Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Luiz Capitulino]

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.

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

Re: [Qemu-devel] [PATCH for-2.1?] scripts: qapi-event.py: support vendor extension

[Eric Blake]

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

On 07/08/2014 12:17 PM, Luiz Capitulino wrote:
> 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.
> 
> 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(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

This is borderline on whether it is a bug fix worth applying in 2.1 - it
is fixing something that is new to this release (event-as-qapi) and
which affects downstream vendors; but at the same time, it is something
which cannot be triggered _except_ by downstream vendors, which are
perfectly capable of applying this patch even if it misses 2.1.  I'll
leave it up to you.

-- 
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 for-2.1?] scripts: qapi-event.py: support vendor extension

[Luiz Capitulino]

On Wed, 09 Jul 2014 09:43:53 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/08/2014 12:17 PM, Luiz Capitulino wrote:
> > 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.
> > 
> > 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(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> This is borderline on whether it is a bug fix worth applying in 2.1 - it
> is fixing something that is new to this release (event-as-qapi) and
> which affects downstream vendors; but at the same time, it is something
> which cannot be triggered _except_ by downstream vendors, which are
> perfectly capable of applying this patch even if it misses 2.1.  I'll
> leave it up to you.

I'm not sure it qualifies. On the one hand it's a bug in new code that
didn't exist before, on the other hand this is by far not blocker and
it doesn't cause any code to malfunction.

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Markus Armbruster]

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?

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Markus Armbruster]

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.


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

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Eric Blake]

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

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    +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] scripts: qapi-event.py: support vendor extension

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Luiz Capitulino]

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?

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Eric Blake]

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

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

-- 
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] scripts: qapi-event.py: support vendor extension

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Eric Blake]

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

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.

-- 
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] scripts: qapi-event.py: support vendor extension

[Luiz Capitulino]

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.

Re: [Qemu-devel] [PATCH] scripts: qapi-event.py: support vendor extension

[Eric Blake]

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

On 07/10/2014 08:31 AM, Markus Armbruster 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>
>> ---

>> +# 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?

If the generator spits out any comments, those comments should refer to
the QMP wire name, including the '.'. But right now, generated code
doesn't seem to do that.

-- 
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] scripts: qapi-event.py: support vendor extension

[Wenchao Xia]

Reviewed-by: Wenchao Xia <wenchaoqemu@gmail.com>

I didn't expect dot in schema before.