| Message ID | 1455665965-27638-1-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > When we added support for a user-specified prefix for an enum > type (commit 351d36e), we forgot to teach the qapi-visit code > to honor that prefix in the case of using a prefixed enum as > the discriminator for a flat union. While there is still some > on-list debate on whether we want to keep prefixes, we should > at least make it work as long as it is still part of the code > base. > > Reported-by: Daniel P. Berrange <berrange@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > scripts/qapi-visit.py | 3 ++- > tests/qapi-schema/qapi-schema-test.json | 9 ++++++--- > tests/qapi-schema/qapi-schema-test.out | 7 +++++-- > 3 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 0cc9b08..2bdb5a1 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -293,7 +293,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > case %(case)s: > ''', > case=c_enum_const(variants.tag_member.type.name, > - var.name)) > + var.name, > + variants.tag_member.type.prefix)) > if simple_union_type: > ret += mcgen(''' > visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err); Yup. Let's check for completeness. Calls of c_enum_const(): * QAPISchemaEnumType.c_null() and (with your patch) gen_visit_union() call it like c_enum_const(TYPE.name, MEMBER, TYPE.prefix) where MEMBER is a member of enumeration type TYPE. As your patch shows, the prefix is easy to forget. A safer function would take just TYPE and MEMBER: TYPE.c_member(MEMBER) * gen_event_send() calls c_enum_const(event_enum_name, name) where name is member of the enum type named event_enum_name. That's okay because the type is auto-generated without a prefix. Regardless, we could do something like schema.lookup_type(event_enum_name).c_member(name) Requires actually constructing the type, which is probably a good idea anyway, because it gets us the necessary collision checks. Replacing global event_enum_name by event_enum_type would be nice then. Out of scope for this patch. * gen_enum_lookup() and gen_enum() work on name, values, prefix instead of the type. I figure they do to support qapi-event.py. If we clean it up to create the type, these functions could use the type as well, and then c_enum_const() could be dropped. Bottom line for this patch: the fix is complete. > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index 4b89527..353a34e 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -73,14 +73,17 @@ > 'base': 'UserDefZero', > 'data': { 'string': 'str', 'enum1': 'EnumOne' } } > > +{ 'struct': 'UserDefUnionBase2', > + 'base': 'UserDefZero', > + 'data': { 'string': 'str', 'enum1': 'QEnumTwo' } } > + > # this variant of UserDefFlatUnion defaults to a union that uses fields with > # allocated types to test corner cases in the cleanup/dealloc visitor > { 'union': 'UserDefFlatUnion2', > - 'base': 'UserDefUnionBase', > + 'base': 'UserDefUnionBase2', > 'discriminator': 'enum1', > 'data': { 'value1' : 'UserDefC', # intentional forward reference > - 'value2' : 'UserDefB', > - 'value3' : 'UserDefA' } } > + 'value2' : 'UserDefB' } } > > { 'alternate': 'UserDefAlternate', > 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } } > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 2c546b7..241aadb 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -121,11 +121,10 @@ object UserDefFlatUnion > case value2: UserDefB > case value3: UserDefB > object UserDefFlatUnion2 > - base UserDefUnionBase > + base UserDefUnionBase2 > tag enum1 > case value1: UserDefC > case value2: UserDefB > - case value3: UserDefA > object UserDefNativeListUnion > member type: UserDefNativeListUnionKind optional=False > case integer: :obj-intList-wrapper > @@ -167,6 +166,10 @@ object UserDefUnionBase > base UserDefZero > member string: str optional=False > member enum1: EnumOne optional=False > +object UserDefUnionBase2 > + base UserDefZero > + member string: str optional=False > + member enum1: QEnumTwo optional=False > object UserDefZero > member integer: int optional=False > event __ORG.QEMU_X-EVENT __org.qemu_x-Struct Applied to qapi-next, thanks!
On 02/17/2016 05:05 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> When we added support for a user-specified prefix for an enum >> type (commit 351d36e), we forgot to teach the qapi-visit code >> to honor that prefix in the case of using a prefixed enum as >> the discriminator for a flat union. While there is still some >> on-list debate on whether we want to keep prefixes, we should >> at least make it work as long as it is still part of the code >> base. >> > Let's check for completeness. Calls of c_enum_const(): > > * QAPISchemaEnumType.c_null() and (with your patch) gen_visit_union() > call it like > > c_enum_const(TYPE.name, MEMBER, TYPE.prefix) > > where MEMBER is a member of enumeration type TYPE. > > As your patch shows, the prefix is easy to forget. A safer function > would take just TYPE and MEMBER: > > TYPE.c_member(MEMBER) Yes, that would be nicer, but more invasive... > > * gen_event_send() calls > > c_enum_const(event_enum_name, name) > > where name is member of the enum type named event_enum_name. That's > okay because the type is auto-generated without a prefix. Regardless, > we could do something like > > schema.lookup_type(event_enum_name).c_member(name) > > Requires actually constructing the type, which is probably a good idea > anyway, because it gets us the necessary collision checks. Replacing > global event_enum_name by event_enum_type would be nice then. Out of > scope for this patch. ...and that's why the quick patch now rather than a more invasive refactor is reasonable. I've added this refactor to my list of things that might be worth doing some later day, but certainly not before the current queue is flushed. > > * gen_enum_lookup() and gen_enum() work on name, values, prefix instead > of the type. I figure they do to support qapi-event.py. If we clean > it up to create the type, these functions could use the type as well, > and then c_enum_const() could be dropped. > > Bottom line for this patch: the fix is complete. Good, your analysis matched mine.
On 02/17/2016 05:05 AM, Markus Armbruster wrote: > Let's check for completeness. Calls of c_enum_const(): > > * QAPISchemaEnumType.c_null() and (with your patch) gen_visit_union() > call it like > > c_enum_const(TYPE.name, MEMBER, TYPE.prefix) > > where MEMBER is a member of enumeration type TYPE. > > As your patch shows, the prefix is easy to forget. A safer function > would take just TYPE and MEMBER: > > TYPE.c_member(MEMBER) > I took a look at this email again today, and while it still sounds like a nice action, I wasn't able to get it done quickly (certainly not 2.6 material, at any rate). > * gen_event_send() calls > > c_enum_const(event_enum_name, name) > > where name is member of the enum type named event_enum_name. That's > okay because the type is auto-generated without a prefix. Regardless, > we could do something like > > schema.lookup_type(event_enum_name).c_member(name) > > Requires actually constructing the type, which is probably a good idea > anyway, because it gets us the necessary collision checks. Replacing > global event_enum_name by event_enum_type would be nice then. Out of > scope for this patch. > > * gen_enum_lookup() and gen_enum() work on name, values, prefix instead > of the type. I figure they do to support qapi-event.py. If we clean > it up to create the type, these functions could use the type as well, > and then c_enum_const() could be dropped. Well, they also do it to support qapi-types.py, where the visit_enum_type() visitor callback has already broken things out into name/values/prefix instead of providing a type, and where we don't have easy access to the schema to do schema.lookup_type(name). As I've worked more with the visitors, I'm really wondering if visit_enum_type(self, type, info) would be a saner callback interface than visit_enum_type(self, type.name, info, type.values, type.prefix)
Eric Blake <eblake@redhat.com> writes: > On 02/17/2016 05:05 AM, Markus Armbruster wrote: >> Let's check for completeness. Calls of c_enum_const(): >> >> * QAPISchemaEnumType.c_null() and (with your patch) gen_visit_union() >> call it like >> >> c_enum_const(TYPE.name, MEMBER, TYPE.prefix) >> >> where MEMBER is a member of enumeration type TYPE. >> >> As your patch shows, the prefix is easy to forget. A safer function >> would take just TYPE and MEMBER: >> >> TYPE.c_member(MEMBER) >> > > I took a look at this email again today, and while it still sounds like > a nice action, I wasn't able to get it done quickly (certainly not 2.6 > material, at any rate). > >> * gen_event_send() calls >> >> c_enum_const(event_enum_name, name) >> >> where name is member of the enum type named event_enum_name. That's >> okay because the type is auto-generated without a prefix. Regardless, >> we could do something like >> >> schema.lookup_type(event_enum_name).c_member(name) >> >> Requires actually constructing the type, which is probably a good idea >> anyway, because it gets us the necessary collision checks. Replacing >> global event_enum_name by event_enum_type would be nice then. Out of >> scope for this patch. >> >> * gen_enum_lookup() and gen_enum() work on name, values, prefix instead >> of the type. I figure they do to support qapi-event.py. If we clean >> it up to create the type, these functions could use the type as well, >> and then c_enum_const() could be dropped. > > Well, they also do it to support qapi-types.py, where the > visit_enum_type() visitor callback has already broken things out into > name/values/prefix instead of providing a type, and where we don't have > easy access to the schema to do schema.lookup_type(name). > > As I've worked more with the visitors, I'm really wondering if > visit_enum_type(self, type, info) would be a saner callback interface > than visit_enum_type(self, type.name, info, type.values, type.prefix) Maybe it is, but I'm not sure. The existing visit_enum_type() forces discipline: you can't just use whatever property of the type you like. Discipline is good until you overdo it and it becomes a straightjacket. Let's not worry about it now, but concentrate on keeping the existing, lengthy QAPI queue moving.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 0cc9b08..2bdb5a1 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -293,7 +293,8 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error case %(case)s: ''', case=c_enum_const(variants.tag_member.type.name, - var.name)) + var.name, + variants.tag_member.type.prefix)) if simple_union_type: ret += mcgen(''' visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, &err); diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 4b89527..353a34e 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -73,14 +73,17 @@ 'base': 'UserDefZero', 'data': { 'string': 'str', 'enum1': 'EnumOne' } } +{ 'struct': 'UserDefUnionBase2', + 'base': 'UserDefZero', + 'data': { 'string': 'str', 'enum1': 'QEnumTwo' } } + # this variant of UserDefFlatUnion defaults to a union that uses fields with # allocated types to test corner cases in the cleanup/dealloc visitor { 'union': 'UserDefFlatUnion2', - 'base': 'UserDefUnionBase', + 'base': 'UserDefUnionBase2', 'discriminator': 'enum1', 'data': { 'value1' : 'UserDefC', # intentional forward reference - 'value2' : 'UserDefB', - 'value3' : 'UserDefA' } } + 'value2' : 'UserDefB' } } { 'alternate': 'UserDefAlternate', 'data': { 'uda': 'UserDefA', 's': 'str', 'i': 'int' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 2c546b7..241aadb 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -121,11 +121,10 @@ object UserDefFlatUnion case value2: UserDefB case value3: UserDefB object UserDefFlatUnion2 - base UserDefUnionBase + base UserDefUnionBase2 tag enum1 case value1: UserDefC case value2: UserDefB - case value3: UserDefA object UserDefNativeListUnion member type: UserDefNativeListUnionKind optional=False case integer: :obj-intList-wrapper @@ -167,6 +166,10 @@ object UserDefUnionBase base UserDefZero member string: str optional=False member enum1: EnumOne optional=False +object UserDefUnionBase2 + base UserDefZero + member string: str optional=False + member enum1: QEnumTwo optional=False object UserDefZero member integer: int optional=False event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
When we added support for a user-specified prefix for an enum type (commit 351d36e), we forgot to teach the qapi-visit code to honor that prefix in the case of using a prefixed enum as the discriminator for a flat union. While there is still some on-list debate on whether we want to keep prefixes, we should at least make it work as long as it is still part of the code base. Reported-by: Daniel P. Berrange <berrange@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/qapi-visit.py | 3 ++- tests/qapi-schema/qapi-schema-test.json | 9 ++++++--- tests/qapi-schema/qapi-schema-test.out | 7 +++++-- 3 files changed, 13 insertions(+), 6 deletions(-)