Message ID | 87vbr4vtwp.fsf@blackfin.pond.sub.org |
---|---|
State | New |
Headers | show |
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.
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'} } ##