On Fri, 11 Jul 2014 18:01:50 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Eric Blake <ebl...@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?
