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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Thu, 10 Jul 2014 16:31:38 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> 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 <address@hidden>
>> > ---
>> >  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'} }
 
 ##