| Message ID | 55B7E15D.50902@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > On 07/01/2015 02:21 PM, Markus Armbruster wrote: >> The struct generated for a flat union is weird: the members of its >> base are at the end, except for the union tag, which is renamed to >> 'kind' and put at the beginning. > > The renaming to 'kind' was a bug waiting to happen. Consider this > example, which is broken before your series: > > diff --git i/tests/qapi-schema/qapi-schema-test.json > w/tests/qapi-schema/qapi-schema-test.json > index c7eaa86..12c09e3 100644 > --- i/tests/qapi-schema/qapi-schema-test.json > +++ w/tests/qapi-schema/qapi-schema-test.json > @@ -37,7 +37,7 @@ > 'data': { 'string1': 'str', 'string2': 'str' } } > > { 'struct': 'UserDefUnionBase', > - 'data': { 'string': 'str', 'enum1': 'EnumOne' } } > + 'data': { 'kind': 'str', 'enum1': 'EnumOne' } } > > { 'union': 'UserDefFlatUnion', > 'base': 'UserDefUnionBase', > > > leading to this compilation error during 'make check-unit': > > In file included from tests/test-qmp-output-visitor.c:17:0: > tests/test-qapi-types.h:617:11: error: duplicate member ‘kind’ > char *kind; > ^ > tests/test-qapi-types.h:631:11: error: duplicate member ‘kind’ > char *kind; > ^ > > Therefore, it might be worth mentioning that avoiding the rename to > 'kind' is a bug fix, not just a nicer struct :) Cool! I'll work (a variation of) this test case into my series.
On 07/29/2015 01:33 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 07/01/2015 02:21 PM, Markus Armbruster wrote: >>> The struct generated for a flat union is weird: the members of its >>> base are at the end, except for the union tag, which is renamed to >>> 'kind' and put at the beginning. >> >> Therefore, it might be worth mentioning that avoiding the rename to >> 'kind' is a bug fix, not just a nicer struct :) > > Cool! I'll work (a variation of) this test case into my series. Another name collision bug: our code generates flat unions as: struct BlockdevOptions { BlockdevDriver driver; ... /* End fields inherited from BlockdevOptionsBase. */ /* union tag is BlockdevDriver driver */ union { void *data; BlockdevOptionsArchipelago *archipelago; ... which means that if we name any of the branches 'data' (that is, if 'data' is a member of the enum discriminator), things fail to compile. We could probably fix that by naming our dummy branch '_data'.
Eric Blake <eblake@redhat.com> writes: > On 07/29/2015 01:33 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 07/01/2015 02:21 PM, Markus Armbruster wrote: >>>> The struct generated for a flat union is weird: the members of its >>>> base are at the end, except for the union tag, which is renamed to >>>> 'kind' and put at the beginning. >>> > >>> Therefore, it might be worth mentioning that avoiding the rename to >>> 'kind' is a bug fix, not just a nicer struct :) >> >> Cool! I'll work (a variation of) this test case into my series. > > Another name collision bug: our code generates flat unions as: > > struct BlockdevOptions { > BlockdevDriver driver; > ... > /* End fields inherited from BlockdevOptionsBase. */ > /* union tag is BlockdevDriver driver */ > union { > void *data; > BlockdevOptionsArchipelago *archipelago; > ... > > which means that if we name any of the branches 'data' (that is, if > 'data' is a member of the enum discriminator), things fail to compile. > We could probably fix that by naming our dummy branch '_data'. I wonder whether member data is actually used. I'll find out.
On 07/30/2015 01:11 AM, Markus Armbruster wrote: >> Another name collision bug: our code generates flat unions as: >> >> struct BlockdevOptions { >> BlockdevDriver driver; >> ... >> /* End fields inherited from BlockdevOptionsBase. */ >> /* union tag is BlockdevDriver driver */ >> union { >> void *data; >> BlockdevOptionsArchipelago *archipelago; >> ... >> >> which means that if we name any of the branches 'data' (that is, if >> 'data' is a member of the enum discriminator), things fail to compile. >> We could probably fix that by naming our dummy branch '_data'. > > I wonder whether member data is actually used. I'll find out. The dealloc visitor uses 'data' being non-null as a flag on whether to deallocate the union even if the tag was invalid for some reason; or more importantly, if parsing consumed the tag but then detected an error while parsing the union, leaving the union branch partially allocated. To avoid a leak, we have to deallocate the branch. But if the tag was invalid, then why did we ever allocate the union in the first place, and how do we prove we are calling the correct free-ing function? And if the tag is valid, why can't we just guarantee that the union is 0-initialized and that deleting the branch will work through the correct branch type instead of worrying about 'data'? We still need a dummy member if it is valid to do { 'union':'Foo', 'data':{} } since C doesn't like empty unions, but an empty union seems like something we may want to reject, at which point you are probably right that deleting the data member altogether should work and still let us recover from bad partial parses without a leak.
Eric Blake <eblake@redhat.com> writes: > On 07/30/2015 01:11 AM, Markus Armbruster wrote: > >>> Another name collision bug: our code generates flat unions as: >>> >>> struct BlockdevOptions { >>> BlockdevDriver driver; >>> ... >>> /* End fields inherited from BlockdevOptionsBase. */ >>> /* union tag is BlockdevDriver driver */ >>> union { >>> void *data; >>> BlockdevOptionsArchipelago *archipelago; >>> ... >>> >>> which means that if we name any of the branches 'data' (that is, if >>> 'data' is a member of the enum discriminator), things fail to compile. >>> We could probably fix that by naming our dummy branch '_data'. >> >> I wonder whether member data is actually used. I'll find out. > > The dealloc visitor uses 'data' being non-null as a flag on whether to > deallocate the union even if the tag was invalid for some reason; or > more importantly, if parsing consumed the tag but then detected an error > while parsing the union, leaving the union branch partially allocated. > To avoid a leak, we have to deallocate the branch. > > But if the tag was invalid, then why did we ever allocate the union in > the first place, and how do we prove we are calling the correct free-ing > function? And if the tag is valid, why can't we just guarantee that the > union is 0-initialized and that deleting the branch will work through > the correct branch type instead of worrying about 'data'? Good questions. Someone will have to review and fix this code. Let's add a FIXME so we don't forget. Care to propose one? > We still need a dummy member if it is valid to do { 'union':'Foo', > 'data':{} } since C doesn't like empty unions, but an empty union seems > like something we may want to reject, at which point you are probably > right that deleting the data member altogether should work and still let > us recover from bad partial parses without a leak. Either we reject empty unions, or we detect them and add a dummy, similar to what we do for structs since commit 83ecb22. I figure simply rejecting them is easier.
On 07/30/2015 09:44 AM, Markus Armbruster wrote: >>>> which means that if we name any of the branches 'data' (that is, if >>>> 'data' is a member of the enum discriminator), things fail to compile. >>>> We could probably fix that by naming our dummy branch '_data'. >>> >>> I wonder whether member data is actually used. I'll find out. >> >> The dealloc visitor uses 'data' being non-null as a flag on whether to >> deallocate the union even if the tag was invalid for some reason; or >> more importantly, if parsing consumed the tag but then detected an error >> while parsing the union, leaving the union branch partially allocated. >> To avoid a leak, we have to deallocate the branch. >> >> But if the tag was invalid, then why did we ever allocate the union in >> the first place, and how do we prove we are calling the correct free-ing >> function? And if the tag is valid, why can't we just guarantee that the >> union is 0-initialized and that deleting the branch will work through >> the correct branch type instead of worrying about 'data'? > > Good questions. Someone will have to review and fix this code. Let's > add a FIXME so we don't forget. Care to propose one? Sure; see 12.6/47 (since that is close to several other patches adding FIXME comments).
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 07/01/2015 02:21 PM, Markus Armbruster wrote: >>> The struct generated for a flat union is weird: the members of its >>> base are at the end, except for the union tag, which is renamed to >>> 'kind' and put at the beginning. >> >> The renaming to 'kind' was a bug waiting to happen. Consider this >> example, which is broken before your series: >> >> diff --git i/tests/qapi-schema/qapi-schema-test.json >> w/tests/qapi-schema/qapi-schema-test.json >> index c7eaa86..12c09e3 100644 >> --- i/tests/qapi-schema/qapi-schema-test.json >> +++ w/tests/qapi-schema/qapi-schema-test.json >> @@ -37,7 +37,7 @@ >> 'data': { 'string1': 'str', 'string2': 'str' } } >> >> { 'struct': 'UserDefUnionBase', >> - 'data': { 'string': 'str', 'enum1': 'EnumOne' } } >> + 'data': { 'kind': 'str', 'enum1': 'EnumOne' } } >> >> { 'union': 'UserDefFlatUnion', >> 'base': 'UserDefUnionBase', >> >> >> leading to this compilation error during 'make check-unit': >> >> In file included from tests/test-qmp-output-visitor.c:17:0: >> tests/test-qapi-types.h:617:11: error: duplicate member ‘kind’ >> char *kind; >> ^ >> tests/test-qapi-types.h:631:11: error: duplicate member ‘kind’ >> char *kind; >> ^ >> >> Therefore, it might be worth mentioning that avoiding the rename to >> 'kind' is a bug fix, not just a nicer struct :) > > Cool! I'll work (a variation of) this test case into my series. I split the bug fix off this patch. I put the test in the commit message, because I feel it has little value as regression test going forward.
diff --git i/tests/qapi-schema/qapi-schema-test.json w/tests/qapi-schema/qapi-schema-test.json index c7eaa86..12c09e3 100644 --- i/tests/qapi-schema/qapi-schema-test.json +++ w/tests/qapi-schema/qapi-schema-test.json @@ -37,7 +37,7 @@ 'data': { 'string1': 'str', 'string2': 'str' } } { 'struct': 'UserDefUnionBase', - 'data': { 'string': 'str', 'enum1': 'EnumOne' } } + 'data': { 'kind': 'str', 'enum1': 'EnumOne' } } { 'union': 'UserDefFlatUnion', 'base': 'UserDefUnionBase',