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

[Luiz Capitulino]

On Fri, 11 Jul 2014 18:01:50 +0200
Markus Armbruster <address@hidden> wrote:

> Eric Blake <address@hidden> 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?