| Message ID | 1455582057-27565-11-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > There's no reason to do two malloc's for an alternate type visiting > a QAPI struct; let's just inline the struct directly as the C union > branch of the struct. > > Surprisingly, no clients were actually using the struct member prior > to this patch; some testsuite coverage is added to avoid future > regressions. > > Ultimately, we want to do the same treatment for QAPI unions, but > as that touches a lot more client code, it is better as a separate > patch. So in the meantime, I had to hack in a way to test if we > are visiting an alternate type, within qapi-types:gen_variants(); > the hack is possible because an earlier patch guaranteed that all > alternates have at least two branches, with at most one object > branch; the hack will go away in a later patch. Suggest: Ultimately, we want to do the same treatment for QAPI unions, but as that touches a lot more client code, it is better as a separate patch. The two share gen_variants(), and I had to hack in a way to test if we are visiting an alternate type: look for a non-object branch. This works because alternates have at least two branches, with at most one object branch, and unions have only object branches. The hack will go away in a later patch. > The generated code difference to qapi-types.h is relatively small, > made possible by a new c_type(is_member) parameter in qapi.py: Let's drop the "made possible" clause here. > > | struct BlockdevRef { > | QType type; > | union { /* union tag is @type */ > | void *data; > |- BlockdevOptions *definition; > |+ BlockdevOptions definition; > | char *reference; > | } u; > | }; > > meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(), > comparable to visit_type_implicit_Foo(): > > |+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions *obj, Error **errp) > |+{ > |+ Error *err = NULL; > |+ > |+ visit_start_struct(v, name, NULL, 0, &err); > |+ if (err) { > |+ goto out; > |+ } > |+ visit_type_BlockdevOptions_fields(v, obj, &err); > |+ error_propagate(errp, err); > |+ err = NULL; > |+ visit_end_struct(v, &err); > |+out: > |+ error_propagate(errp, err); > |+} This is in addition to visit_type_BlockdevOptions(), so we need another name. I can't quite see how the function is tied to alternates, though. > > and use it like this: > > | switch ((*obj)->type) { > | case QTYPE_QDICT: > |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); > |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err); Let's compare the two functions. First visit_type_BlockdevOptions(): void visit_type_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions **obj, Error **errp) { Error *err = NULL; visit_start_struct(v, name, (void **)obj, sizeof(BlockdevOptions), &err); if (err) { goto out; } if (!*obj) { goto out_obj; } visit_type_BlockdevOptions_fields(v, *obj, &err); if (err) { goto out_obj; } if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { goto out_obj; } switch ((*obj)->driver) { case BLOCKDEV_DRIVER_ARCHIPELAGO: visit_type_implicit_BlockdevOptionsArchipelago(v, &(*obj)->u.archipelago, &err); break; [All the other cases...] default: abort(); } out_obj: error_propagate(errp, err); err = NULL; visit_end_struct(v, &err); out: error_propagate(errp, err); } Now visit_type_alternate_BlockdevOptions(), with differences annotated with //: static void visit_type_alternate_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions *obj, // one * less Error **errp) { Error *err = NULL; visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj // suppresses malloc if (err) { goto out; } // null check dropped (obj can't be null) visit_type_BlockdevOptions_fields(v, obj, &err); // visit_start_union() + switch dropped error_propagate(errp, err); err = NULL; visit_end_struct(v, &err); out: error_propagate(errp, err); } Why can we drop visit_start_union() + switch? > | break; > | case QTYPE_QSTRING: > | visit_type_str(v, name, &(*obj)->u.reference, &err); > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v10: new patch > --- > scripts/qapi-types.py | 10 ++++++- > scripts/qapi-visit.py | 49 +++++++++++++++++++++++++++++---- > scripts/qapi.py | 10 ++++--- > tests/test-qmp-input-visitor.c | 29 ++++++++++++++++++- > tests/qapi-schema/qapi-schema-test.json | 2 ++ > tests/qapi-schema/qapi-schema-test.out | 2 ++ > 6 files changed, 91 insertions(+), 11 deletions(-) > > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 2f23432..aba2847 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) > > > def gen_variants(variants): > + # HACK: Determine if this is an alternate (at least one variant > + # is not an object); unions have all branches as objects. > + inline = False > + for v in variants.variants: > + if not isinstance(v.type, QAPISchemaObjectType): > + inline = True > + break > + > # FIXME: What purpose does data serve, besides preventing a union that > # has a branch named 'data'? We use it in qapi-visit.py to decide > # whether to bypass the switch statement if visiting the discriminator > @@ -136,7 +144,7 @@ def gen_variants(variants): > ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > - c_type=typ.c_type(), > + c_type=typ.c_type(is_member=inline), > c_name=c_name(var.name)) > > ret += mcgen(''' > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 9ff0337..948bde4 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -15,10 +15,14 @@ > from qapi import * > import re > > -# visit_type_FOO_implicit() is emitted as needed; track if it has already > +# visit_type_implicit_FOO() is emitted as needed; track if it has already > # been output. > implicit_structs_seen = set() > > +# visit_type_alternate_FOO() is emitted as needed; track if it has already > +# been output. > +alternate_structs_seen = set() > + > # visit_type_FOO_fields() is always emitted; track if a forward declaration > # or implementation has already been output. > struct_fields_seen = set() > @@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * > return ret > > > +def gen_visit_alternate_struct(typ): > + if typ in alternate_structs_seen: > + return '' > + alternate_structs_seen.add(typ) > + > + ret = gen_visit_fields_decl(typ) > + > + ret += mcgen(''' > + > +static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, %(c_type)s *obj, Error **errp) > +{ > + Error *err = NULL; > + > + visit_start_struct(v, name, NULL, 0, &err); > + if (err) { > + goto out; > + } > + visit_type_%(c_type)s_fields(v, obj, &err); > + error_propagate(errp, err); > + err = NULL; > + visit_end_struct(v, &err); > +out: > + error_propagate(errp, err); > +} > +''', > + c_type=typ.c_name()) > + return ret > + > + > def gen_visit_struct_fields(name, base, members): > ret = '' > > @@ -158,11 +191,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error > > def gen_visit_alternate(name, variants): > promote_int = 'true' > + ret = '' > for var in variants.variants: > if var.type.alternate_qtype() == 'QTYPE_QINT': > promote_int = 'false' > + if isinstance(var.type, QAPISchemaObjectType): > + ret += gen_visit_alternate_struct(var.type) > > - ret = mcgen(''' > + ret += mcgen(''' > > void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) > { > @@ -178,15 +214,18 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > } > switch ((*obj)->type) { > ''', > - c_name=c_name(name), promote_int=promote_int) > + c_name=c_name(name), promote_int=promote_int) > > for var in variants.variants: > + prefix = 'visit_type_' > + if isinstance(var.type, QAPISchemaObjectType): > + prefix += 'alternate_' > ret += mcgen(''' > case %(case)s: > - visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); > + %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); > break; > ''', > - case=var.type.alternate_qtype(), > + prefix=prefix, case=var.type.alternate_qtype(), > c_type=var.type.c_name(), > c_name=c_name(var.name)) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 4d75d75..dbb58da 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -821,7 +821,7 @@ class QAPISchemaVisitor(object): > > > class QAPISchemaType(QAPISchemaEntity): > - def c_type(self, is_param=False): > + def c_type(self, is_param=False, is_member=False): > return c_name(self.name) + pointer_suffix > > def c_null(self): > @@ -854,7 +854,7 @@ class QAPISchemaBuiltinType(QAPISchemaType): > def c_name(self): > return self.name > > - def c_type(self, is_param=False): > + def c_type(self, is_param=False, is_member=False): > if is_param and self.name == 'str': > return 'const ' + self._c_type_name > return self._c_type_name > @@ -888,7 +888,7 @@ class QAPISchemaEnumType(QAPISchemaType): > # See QAPISchema._make_implicit_enum_type() > return self.name.endswith('Kind') > > - def c_type(self, is_param=False): > + def c_type(self, is_param=False, is_member=False): > return c_name(self.name) > > def member_names(self): > @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): > assert not self.is_implicit() > return QAPISchemaType.c_name(self) > > - def c_type(self, is_param=False): > + def c_type(self, is_param=False, is_member=False): > assert not self.is_implicit() > + if is_member: > + return c_name(self.name) > return QAPISchemaType.c_type(self) To understand this, you have to peel off OO abstraction: QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix. I think we should keep it peeled off in the source code. > > def json_type(self): [tests/ diff looks good]
On 02/16/2016 12:07 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> There's no reason to do two malloc's for an alternate type visiting >> a QAPI struct; let's just inline the struct directly as the C union >> branch of the struct. >> >> Surprisingly, no clients were actually using the struct member prior >> to this patch; some testsuite coverage is added to avoid future >> regressions. >> >> Ultimately, we want to do the same treatment for QAPI unions, but >> as that touches a lot more client code, it is better as a separate >> patch. So in the meantime, I had to hack in a way to test if we >> are visiting an alternate type, within qapi-types:gen_variants(); >> the hack is possible because an earlier patch guaranteed that all >> alternates have at least two branches, with at most one object >> branch; the hack will go away in a later patch. > > Suggest: > > Ultimately, we want to do the same treatment for QAPI unions, but as > that touches a lot more client code, it is better as a separate patch. > The two share gen_variants(), and I had to hack in a way to test if we > are visiting an alternate type: look for a non-object branch. This > works because alternates have at least two branches, with at most one > object branch, and unions have only object branches. The hack will go > away in a later patch. Nicer. > >> The generated code difference to qapi-types.h is relatively small, >> made possible by a new c_type(is_member) parameter in qapi.py: > > Let's drop the "made possible" clause here. I was trying to document the new is_member parameter somewhere in the commit message, but I can agree that it could be its own paragraph rather than a clause here. > > This is in addition to visit_type_BlockdevOptions(), so we need another > name. > > I can't quite see how the function is tied to alternates, though. > I'm open to naming suggestions. Also, I noticed that we have 'visit_type_FOO_fields' and 'visit_type_implicit_FOO'; so I didn't know whether to name it 'visit_type_alternate_FOO' or 'visit_type_FOO_alternate'; it gets more interesting later in the series when I completely delete 'visit_type_implicit_FOO'. >> >> and use it like this: >> >> | switch ((*obj)->type) { >> | case QTYPE_QDICT: >> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); >> |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err); > > Let's compare the two functions. First visit_type_BlockdevOptions(): > > > Now visit_type_alternate_BlockdevOptions(), with differences annotated > with //: > > static void visit_type_alternate_BlockdevOptions(Visitor *v, > const char *name, > BlockdevOptions *obj, // one * less Yep, because we no longer need to malloc a second object, so we no longer need to propagate a change to obj back to the caller. > Error **errp) > { > Error *err = NULL; > > visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj > // suppresses malloc > if (err) { > goto out; > } > // null check dropped (obj can't be null) > visit_type_BlockdevOptions_fields(v, obj, &err); Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again, because we have one less level of indirection, and 7/13 reduced the indirection required in visit_type_FOO_fields()). > // visit_start_union() + switch dropped > error_propagate(errp, err); > err = NULL; > visit_end_struct(v, &err); > out: > error_propagate(errp, err); > } > > Why can we drop visit_start_union() + switch? visit_start_union() is dropped because its only purpose was to determine if the dealloc visitor needs to visit the default branch. When we had a separate allocation, we did not want to visit the branch if the discriminator was not successfully parsed, because otherwise we would dereference NULL. But now that we don't have a second pointer allocation, we no longer have anything to dealloc, and we can no longer dereference NULL. Explained better in 12/13, where I delete visit_start_union() altogether. But maybe I could keep it in this patch in the meantime, to minimize confusion. Dropped switch, on the other hand, looks to be a genuine bug. Eeek. That should absolutely be present, and it proves that our testsuite is not strong enough for not catching me on it. And now that you've made me think about it, maybe I have yet another idea. Right now, we've split the visit of local members into visit_type_FOO_fields(), while leaving the variant members to be visited in visit_type_FOO() visit_type_FOO_fields() is static, so we can change it without impacting the entire tree; I could add a bool parameter to that function, and write: visit_type_FOO() { visit_start_struct(obj) visit_type_FOO_fields(, false) visit_end_struct() } visit_type_FOO_fields(, wrap) { if (wrap) { visit_start_struct(NULL) } visit local fields visit variant fields if (wrap) { visit_end_struct() } } and for the variant type that includes FOO as one of its options, visit_type_VARIANT() { allocate (currently spelled visit_start_implicit_struct, but see 13/13) switch (type) { case QTYPE_QINT: visit_type_int(); break case QTYPE_QDICT: visit_type_FOO_fields(, true); break } visit_end } or not even create a new function, and just have: visit_type_FOO() { visit_start_struct(obj) visit_type_FOO_fields() visit_end_struct() } visit_type_FOO_fields() { visit local fields visit variant fields } and for the variant type, visit_type_VARIANT() { allocate (currently spelled visit_start_implicit_struct, but see 13/13) switch (type) { case QTYPE_QINT: visit_type_int(); break case QTYPE_QDICT: visit_start_struct(NULL) visit_type_FOO_fields() visit_end_struct() break } visit_end_implicit } where the logic gets a bit more complicated to do correct error handling. >> @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): >> assert not self.is_implicit() >> return QAPISchemaType.c_name(self) >> >> - def c_type(self, is_param=False): >> + def c_type(self, is_param=False, is_member=False): >> assert not self.is_implicit() >> + if is_member: >> + return c_name(self.name) >> return QAPISchemaType.c_type(self) > > To understand this, you have to peel off OO abstraction: > QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix. > > I think we should keep it peeled off in the source code. By this, you mean I should do: def c_type(self, is_param=False, is_member=False): assert not self.is_implicit() if is_member: return c_name(self.name) return c_name(self.name) + pointer_suffix ? or that I should hoist the suppression of pointer_suffix into the parent class? > >> >> def json_type(self): > [tests/ diff looks good] good for what they caught, but they didn't catch enough :)
Eric Blake <eblake@redhat.com> writes: > On 02/16/2016 12:07 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> There's no reason to do two malloc's for an alternate type visiting >>> a QAPI struct; let's just inline the struct directly as the C union >>> branch of the struct. >>> >>> Surprisingly, no clients were actually using the struct member prior >>> to this patch; some testsuite coverage is added to avoid future >>> regressions. >>> >>> Ultimately, we want to do the same treatment for QAPI unions, but >>> as that touches a lot more client code, it is better as a separate >>> patch. So in the meantime, I had to hack in a way to test if we >>> are visiting an alternate type, within qapi-types:gen_variants(); >>> the hack is possible because an earlier patch guaranteed that all >>> alternates have at least two branches, with at most one object >>> branch; the hack will go away in a later patch. >> >> Suggest: >> >> Ultimately, we want to do the same treatment for QAPI unions, but as >> that touches a lot more client code, it is better as a separate patch. >> The two share gen_variants(), and I had to hack in a way to test if we >> are visiting an alternate type: look for a non-object branch. This >> works because alternates have at least two branches, with at most one >> object branch, and unions have only object branches. The hack will go >> away in a later patch. > > Nicer. > >> >>> The generated code difference to qapi-types.h is relatively small, >>> made possible by a new c_type(is_member) parameter in qapi.py: >> >> Let's drop the "made possible" clause here. > > I was trying to document the new is_member parameter somewhere in the > commit message, but I can agree that it could be its own paragraph > rather than a clause here. That could work. >> This is in addition to visit_type_BlockdevOptions(), so we need another >> name. >> >> I can't quite see how the function is tied to alternates, though. >> > > I'm open to naming suggestions. Also, I noticed that we have > 'visit_type_FOO_fields' and 'visit_type_implicit_FOO'; so I didn't know > whether to name it 'visit_type_alternate_FOO' or > 'visit_type_FOO_alternate'; it gets more interesting later in the series > when I completely delete 'visit_type_implicit_FOO'. > >>> >>> and use it like this: >>> >>> | switch ((*obj)->type) { >>> | case QTYPE_QDICT: >>> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); >>> |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err); >> >> Let's compare the two functions. First visit_type_BlockdevOptions(): >> > >> >> Now visit_type_alternate_BlockdevOptions(), with differences annotated >> with //: >> >> static void visit_type_alternate_BlockdevOptions(Visitor *v, >> const char *name, >> BlockdevOptions *obj, // one * less > > Yep, because we no longer need to malloc a second object, so we no > longer need to propagate a change to obj back to the caller. > >> Error **errp) >> { >> Error *err = NULL; >> >> visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj >> // suppresses malloc >> if (err) { >> goto out; >> } >> // null check dropped (obj can't be null) >> visit_type_BlockdevOptions_fields(v, obj, &err); > > Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again, > because we have one less level of indirection, and 7/13 reduced the > indirection required in visit_type_FOO_fields()). > >> // visit_start_union() + switch dropped >> error_propagate(errp, err); >> err = NULL; >> visit_end_struct(v, &err); >> out: >> error_propagate(errp, err); >> } >> >> Why can we drop visit_start_union() + switch? > > visit_start_union() is dropped because its only purpose was to determine > if the dealloc visitor needs to visit the default branch. When we had a > separate allocation, we did not want to visit the branch if the > discriminator was not successfully parsed, because otherwise we would > dereference NULL. But now that we don't have a second pointer > allocation, we no longer have anything to dealloc, and we can no longer > dereference NULL. Explained better in 12/13, where I delete > visit_start_union() altogether. But maybe I could keep it in this patch > in the meantime, to minimize confusion. Perhaps squashing the two patches together could be less confusing. > Dropped switch, on the other hand, looks to be a genuine bug. Eeek. > That should absolutely be present, and it proves that our testsuite is > not strong enough for not catching me on it. > > And now that you've made me think about it, maybe I have yet another > idea. Right now, we've split the visit of local members into > visit_type_FOO_fields(), while leaving the variant members to be visited > in visit_type_FOO() Yes. I guess that's to support visiting "implicit" structs. > visit_type_FOO_fields() is static, so we can change it without impacting > the entire tree; I could add a bool parameter to that function, and write: > > visit_type_FOO() { > visit_start_struct(obj) > visit_type_FOO_fields(, false) > visit_end_struct() > } > > visit_type_FOO_fields(, wrap) { > if (wrap) { > visit_start_struct(NULL) > } > visit local fields > visit variant fields > if (wrap) { > visit_end_struct() > } > } > > and for the variant type that includes FOO as one of its options, > > visit_type_VARIANT() { > allocate (currently spelled visit_start_implicit_struct, but see 13/13) > switch (type) { > case QTYPE_QINT: > visit_type_int(); break > case QTYPE_QDICT: > visit_type_FOO_fields(, true); break > } > visit_end > } > > or not even create a new function, and just have: > > > visit_type_FOO() { > visit_start_struct(obj) > visit_type_FOO_fields() > visit_end_struct() > } > > visit_type_FOO_fields() { > visit local fields > visit variant fields > } > > and for the variant type, > > visit_type_VARIANT() { > allocate (currently spelled visit_start_implicit_struct, but see 13/13) > switch (type) { > case QTYPE_QINT: > visit_type_int(); break > case QTYPE_QDICT: > visit_start_struct(NULL) > visit_type_FOO_fields() > visit_end_struct() > break > } > visit_end_implicit > } > > where the logic gets a bit more complicated to do correct error handling. Let me get to this result from a different angle. A "boxed" visit is allocate hook visit the members (common and variant) cleanup hook An "unboxed" visit is basically the same without the two hooks. "Implicit" is like unboxed, except the members are inlined rather than wrapped in a JSON object. So the common code to factor out is "visit the members". >>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py >>> index 2f23432..aba2847 100644 >>> --- a/scripts/qapi-types.py >>> +++ b/scripts/qapi-types.py >>> @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) >>> >>> >>> def gen_variants(variants): >>> + # HACK: Determine if this is an alternate (at least one variant >>> + # is not an object); unions have all branches as objects. >>> + inline = False >>> + for v in variants.variants: >>> + if not isinstance(v.type, QAPISchemaObjectType): >>> + inline = True >>> + break >>> + >>> # FIXME: What purpose does data serve, besides preventing a union that >>> # has a branch named 'data'? We use it in qapi-visit.py to decide >>> # whether to bypass the switch statement if visiting the discriminator >>> @@ -136,7 +144,7 @@ def gen_variants(variants): >>> ret += mcgen(''' >>> %(c_type)s %(c_name)s; >>> ''', >>> - c_type=typ.c_type(), >>> + c_type=typ.c_type(is_member=inline), I don't like the name is_member. The thing we're dealing with here is a member, regardless of the value of inline. I think it's boxed vs. unboxed. >>> c_name=c_name(var.name)) >>> >>> ret += mcgen(''' [...] >>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>> index 4d75d75..dbb58da 100644 >>> --- a/scripts/qapi.py >>> +++ b/scripts/qapi.py [...] >>> @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): >>> assert not self.is_implicit() >>> return QAPISchemaType.c_name(self) >>> >>> - def c_type(self, is_param=False): >>> + def c_type(self, is_param=False, is_member=False): >>> assert not self.is_implicit() >>> + if is_member: >>> + return c_name(self.name) >>> return QAPISchemaType.c_type(self) >> >> To understand this, you have to peel off OO abstraction: >> QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix. >> >> I think we should keep it peeled off in the source code. > > By this, you mean I should do: > > def c_type(self, is_param=False, is_member=False): > assert not self.is_implicit() > if is_member: > return c_name(self.name) > return c_name(self.name) + pointer_suffix > > ? Yes, that's what I had in mind. > or that I should hoist the suppression of pointer_suffix into the parent > class? c_type()'s is_param is ugly, and is_member adds to the ugliness. If you can come up with a clean way to avoid one or both, go for it. >>> def json_type(self): >> [tests/ diff looks good] > > good for what they caught, but they didn't catch enough :)
On 02/17/2016 07:40 AM, Markus Armbruster wrote: >>>> There's no reason to do two malloc's for an alternate type visiting >>>> a QAPI struct; let's just inline the struct directly as the C union >>>> branch of the struct. >>>> >> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again, >> because we have one less level of indirection, and 7/13 reduced the >> indirection required in visit_type_FOO_fields()). >> >>> // visit_start_union() + switch dropped >>> error_propagate(errp, err); >>> err = NULL; >>> visit_end_struct(v, &err); >>> out: >>> error_propagate(errp, err); >>> } >>> >>> Why can we drop visit_start_union() + switch? >> >> visit_start_union() is dropped because its only purpose was to determine >> if the dealloc visitor needs to visit the default branch. When we had a >> separate allocation, we did not want to visit the branch if the >> discriminator was not successfully parsed, because otherwise we would >> dereference NULL. But now that we don't have a second pointer >> allocation, we no longer have anything to dealloc, and we can no longer >> dereference NULL. Explained better in 12/13, where I delete >> visit_start_union() altogether. But maybe I could keep it in this patch >> in the meantime, to minimize confusion. > > Perhaps squashing the two patches together could be less confusing. Yes, I'm closer to posting v11, and in that version, visit_start_union is only dropped in a single patch; and this patch just inlines the visit_start_struct() allocation directly into the visit_type_ALT() rather than creating a new visit_type_alternate_ALT(). > >> Dropped switch, on the other hand, looks to be a genuine bug. Eeek. >> That should absolutely be present, and it proves that our testsuite is >> not strong enough for not catching me on it. >> >> And now that you've made me think about it, maybe I have yet another >> idea. Right now, we've split the visit of local members into >> visit_type_FOO_fields(), while leaving the variant members to be visited >> in visit_type_FOO() > > Yes. I guess that's to support visiting "implicit" structs. Up to now, we've forbidden the use of one union as a branch of another (but allowed a union as a branch of an alternate), so the types passed to visit_struct_FOO_fields() never had variants. As part of our generic "object" work, I _want_ to support one union as a branch of another (as long as we can prove there will be no QMP name collisions); and that's another reason why visit_struct_FOO_fields() would need to always visit variants if present. > Let me get to this result from a different angle. A "boxed" visit is > > allocate hook > visit the members (common and variant) > cleanup hook Correct, and we have two choices of allocate hook: visit_start_struct() [allocate and consume {}, for visit_type_FOO() in general], and visit_start_implicit_struct() [allocate, but don't consume {}, for flat union branches prior to this series]. > > An "unboxed" visit is basically the same without the two hooks. Done anywhere we don't have a separate C struct [base classes prior to this series; and then this series is adding unboxed variant visits within flat unions and alternates]. > > "Implicit" is like unboxed, except the members are inlined rather than > wrapped in a JSON object. > > So the common code to factor out is "visit the members". Yep, we're on the same wavelength, and it is looking fairly nice for what I'm about to post as v11. And I like 'unboxed' better than 'is_member': >>>> - c_type=typ.c_type(), >>>> + c_type=typ.c_type(is_member=inline), > > I don't like the name is_member. The thing we're dealing with here is a > member, regardless of the value of inline. I think it's boxed > vs. unboxed. Hmm. I have later patches that add a 'box':true QAPI designation to commands and events, to let us do qmp_command(Foo *arg, Error **errp) instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error **errp) (that is, instead of breaking out each parameter, we pass them all boxed behind a single pointer). What we are doing here is in the opposite direction - we are taking code that has boxed all the sub-type fields behind a single pointer, and unboxing it so that they occur inline in the parent type's storage. Works for me; I'm switching to 'is_unboxed' as the case for when we want to omit the pointer designation during the member declaration.
Eric Blake <eblake@redhat.com> writes: > On 02/17/2016 07:40 AM, Markus Armbruster wrote: > >>>>> There's no reason to do two malloc's for an alternate type visiting >>>>> a QAPI struct; let's just inline the struct directly as the C union >>>>> branch of the struct. >>>>> > >>> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again, >>> because we have one less level of indirection, and 7/13 reduced the >>> indirection required in visit_type_FOO_fields()). >>> >>>> // visit_start_union() + switch dropped >>>> error_propagate(errp, err); >>>> err = NULL; >>>> visit_end_struct(v, &err); >>>> out: >>>> error_propagate(errp, err); >>>> } >>>> >>>> Why can we drop visit_start_union() + switch? >>> >>> visit_start_union() is dropped because its only purpose was to determine >>> if the dealloc visitor needs to visit the default branch. When we had a >>> separate allocation, we did not want to visit the branch if the >>> discriminator was not successfully parsed, because otherwise we would >>> dereference NULL. But now that we don't have a second pointer >>> allocation, we no longer have anything to dealloc, and we can no longer >>> dereference NULL. Explained better in 12/13, where I delete >>> visit_start_union() altogether. But maybe I could keep it in this patch >>> in the meantime, to minimize confusion. >> >> Perhaps squashing the two patches together could be less confusing. > > Yes, I'm closer to posting v11, and in that version, visit_start_union > is only dropped in a single patch; and this patch just inlines the > visit_start_struct() allocation directly into the visit_type_ALT() > rather than creating a new visit_type_alternate_ALT(). > >> >>> Dropped switch, on the other hand, looks to be a genuine bug. Eeek. >>> That should absolutely be present, and it proves that our testsuite is >>> not strong enough for not catching me on it. >>> >>> And now that you've made me think about it, maybe I have yet another >>> idea. Right now, we've split the visit of local members into >>> visit_type_FOO_fields(), while leaving the variant members to be visited >>> in visit_type_FOO() >> >> Yes. I guess that's to support visiting "implicit" structs. > > Up to now, we've forbidden the use of one union as a branch of another > (but allowed a union as a branch of an alternate), so the types passed > to visit_struct_FOO_fields() never had variants. As part of our generic > "object" work, I _want_ to support one union as a branch of another (as > long as we can prove there will be no QMP name collisions); and that's > another reason why visit_struct_FOO_fields() would need to always visit > variants if present. > > >> Let me get to this result from a different angle. A "boxed" visit is >> >> allocate hook >> visit the members (common and variant) >> cleanup hook > > Correct, and we have two choices of allocate hook: visit_start_struct() > [allocate and consume {}, for visit_type_FOO() in general], and > visit_start_implicit_struct() [allocate, but don't consume {}, for flat > union branches prior to this series]. > >> >> An "unboxed" visit is basically the same without the two hooks. > > Done anywhere we don't have a separate C struct [base classes prior to > this series; and then this series is adding unboxed variant visits > within flat unions and alternates]. Should work for visiting both "inlined" and "unboxed" members, shouldn't it? struct { A a; B b } S; struct { S *ps; // boxed member of type S S s; // unboxed member of type S A a; B b; // inlined member of type S } >> >> "Implicit" is like unboxed, except the members are inlined rather than >> wrapped in a JSON object. >> >> So the common code to factor out is "visit the members". > > Yep, we're on the same wavelength, and it is looking fairly nice for > what I'm about to post as v11. And I like 'unboxed' better than > 'is_member': > >>>>> - c_type=typ.c_type(), >>>>> + c_type=typ.c_type(is_member=inline), >> >> I don't like the name is_member. The thing we're dealing with here is a >> member, regardless of the value of inline. I think it's boxed >> vs. unboxed. > > Hmm. I have later patches that add a 'box':true QAPI designation to > commands and events, to let us do qmp_command(Foo *arg, Error **errp) > instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error > **errp) (that is, instead of breaking out each parameter, we pass them > all boxed behind a single pointer). What we are doing here is in the > opposite direction - we are taking code that has boxed all the sub-type > fields behind a single pointer, and unboxing it so that they occur > inline in the parent type's storage. Works for me; I'm switching to > 'is_unboxed' as the case for when we want to omit the pointer > designation during the member declaration. Better. "Unboxed" isn't tied to "member"; we could choose to unbox elsewhere, too.
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 2f23432..aba2847 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj) def gen_variants(variants): + # HACK: Determine if this is an alternate (at least one variant + # is not an object); unions have all branches as objects. + inline = False + for v in variants.variants: + if not isinstance(v.type, QAPISchemaObjectType): + inline = True + break + # FIXME: What purpose does data serve, besides preventing a union that # has a branch named 'data'? We use it in qapi-visit.py to decide # whether to bypass the switch statement if visiting the discriminator @@ -136,7 +144,7 @@ def gen_variants(variants): ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(), + c_type=typ.c_type(is_member=inline), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 9ff0337..948bde4 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -15,10 +15,14 @@ from qapi import * import re -# visit_type_FOO_implicit() is emitted as needed; track if it has already +# visit_type_implicit_FOO() is emitted as needed; track if it has already # been output. implicit_structs_seen = set() +# visit_type_alternate_FOO() is emitted as needed; track if it has already +# been output. +alternate_structs_seen = set() + # visit_type_FOO_fields() is always emitted; track if a forward declaration # or implementation has already been output. struct_fields_seen = set() @@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * return ret +def gen_visit_alternate_struct(typ): + if typ in alternate_structs_seen: + return '' + alternate_structs_seen.add(typ) + + ret = gen_visit_fields_decl(typ) + + ret += mcgen(''' + +static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, %(c_type)s *obj, Error **errp) +{ + Error *err = NULL; + + visit_start_struct(v, name, NULL, 0, &err); + if (err) { + goto out; + } + visit_type_%(c_type)s_fields(v, obj, &err); + error_propagate(errp, err); + err = NULL; + visit_end_struct(v, &err); +out: + error_propagate(errp, err); +} +''', + c_type=typ.c_name()) + return ret + + def gen_visit_struct_fields(name, base, members): ret = '' @@ -158,11 +191,14 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, Error def gen_visit_alternate(name, variants): promote_int = 'true' + ret = '' for var in variants.variants: if var.type.alternate_qtype() == 'QTYPE_QINT': promote_int = 'false' + if isinstance(var.type, QAPISchemaObjectType): + ret += gen_visit_alternate_struct(var.type) - ret = mcgen(''' + ret += mcgen(''' void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { @@ -178,15 +214,18 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } switch ((*obj)->type) { ''', - c_name=c_name(name), promote_int=promote_int) + c_name=c_name(name), promote_int=promote_int) for var in variants.variants: + prefix = 'visit_type_' + if isinstance(var.type, QAPISchemaObjectType): + prefix += 'alternate_' ret += mcgen(''' case %(case)s: - visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); + %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err); break; ''', - case=var.type.alternate_qtype(), + prefix=prefix, case=var.type.alternate_qtype(), c_type=var.type.c_name(), c_name=c_name(var.name)) diff --git a/scripts/qapi.py b/scripts/qapi.py index 4d75d75..dbb58da 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -821,7 +821,7 @@ class QAPISchemaVisitor(object): class QAPISchemaType(QAPISchemaEntity): - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): return c_name(self.name) + pointer_suffix def c_null(self): @@ -854,7 +854,7 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_name(self): return self.name - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): if is_param and self.name == 'str': return 'const ' + self._c_type_name return self._c_type_name @@ -888,7 +888,7 @@ class QAPISchemaEnumType(QAPISchemaType): # See QAPISchema._make_implicit_enum_type() return self.name.endswith('Kind') - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): return c_name(self.name) def member_names(self): @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.is_implicit() return QAPISchemaType.c_name(self) - def c_type(self, is_param=False): + def c_type(self, is_param=False, is_member=False): assert not self.is_implicit() + if is_member: + return c_name(self.name) return QAPISchemaType.c_type(self) def json_type(self): diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index c72cdad..139ff7d 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -1,7 +1,7 @@ /* * QMP Input Visitor unit-tests. * - * Copyright (C) 2011, 2015 Red Hat Inc. + * Copyright (C) 2011-2016 Red Hat Inc. * * Authors: * Luiz Capitulino <lcapitulino@redhat.com> @@ -309,6 +309,7 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, Visitor *v; Error *err = NULL; UserDefAlternate *tmp; + WrapAlternate *wrap; v = visitor_input_test_init(data, "42"); visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); @@ -322,10 +323,36 @@ static void test_visitor_in_alternate(TestInputVisitorData *data, g_assert_cmpstr(tmp->u.s, ==, "string"); qapi_free_UserDefAlternate(tmp); + v = visitor_input_test_init(data, "{'boolean':true}"); + visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort); + g_assert_cmpint(tmp->type, ==, QTYPE_QDICT); + g_assert_cmpint(tmp->u.uda.boolean, ==, true); + g_assert_cmpint(tmp->u.uda.has_a_b, ==, false); + qapi_free_UserDefAlternate(tmp); + v = visitor_input_test_init(data, "false"); visit_type_UserDefAlternate(v, NULL, &tmp, &err); error_free_or_abort(&err); qapi_free_UserDefAlternate(tmp); + + v = visitor_input_test_init(data, "{ 'alt': 42 }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QINT); + g_assert_cmpint(wrap->alt->u.i, ==, 42); + qapi_free_WrapAlternate(wrap); + + v = visitor_input_test_init(data, "{ 'alt': 'string' }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QSTRING); + g_assert_cmpstr(wrap->alt->u.s, ==, "string"); + qapi_free_WrapAlternate(wrap); + + v = visitor_input_test_init(data, "{ 'alt': {'boolean':true} }"); + visit_type_WrapAlternate(v, NULL, &wrap, &error_abort); + g_assert_cmpint(wrap->alt->type, ==, QTYPE_QDICT); + g_assert_cmpint(wrap->alt->u.uda.boolean, ==, true); + g_assert_cmpint(wrap->alt->u.uda.has_a_b, ==, false); + qapi_free_WrapAlternate(wrap); } static void test_visitor_in_alternate_number(TestInputVisitorData *data, diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 4b89527..b393572 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -82,6 +82,8 @@ 'value2' : 'UserDefB', 'value3' : 'UserDefA' } } +{ 'struct': 'WrapAlternate', + 'data': { 'alt': 'UserDefAlternate' } } { '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..11cdc9a 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -169,6 +169,8 @@ object UserDefUnionBase member enum1: EnumOne optional=False object UserDefZero member integer: int optional=False +object WrapAlternate + member alt: UserDefAlternate optional=False event __ORG.QEMU_X-EVENT __org.qemu_x-Struct alternate __org.qemu_x-Alt case __org.qemu_x-branch: str
There's no reason to do two malloc's for an alternate type visiting a QAPI struct; let's just inline the struct directly as the C union branch of the struct. Surprisingly, no clients were actually using the struct member prior to this patch; some testsuite coverage is added to avoid future regressions. Ultimately, we want to do the same treatment for QAPI unions, but as that touches a lot more client code, it is better as a separate patch. So in the meantime, I had to hack in a way to test if we are visiting an alternate type, within qapi-types:gen_variants(); the hack is possible because an earlier patch guaranteed that all alternates have at least two branches, with at most one object branch; the hack will go away in a later patch. The generated code difference to qapi-types.h is relatively small, made possible by a new c_type(is_member) parameter in qapi.py: | struct BlockdevRef { | QType type; | union { /* union tag is @type */ | void *data; |- BlockdevOptions *definition; |+ BlockdevOptions definition; | char *reference; | } u; | }; meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(), comparable to visit_type_implicit_Foo(): |+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char *name, BlockdevOptions *obj, Error **errp) |+{ |+ Error *err = NULL; |+ |+ visit_start_struct(v, name, NULL, 0, &err); |+ if (err) { |+ goto out; |+ } |+ visit_type_BlockdevOptions_fields(v, obj, &err); |+ error_propagate(errp, err); |+ err = NULL; |+ visit_end_struct(v, &err); |+out: |+ error_propagate(errp, err); |+} and use it like this: | switch ((*obj)->type) { | case QTYPE_QDICT: |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); |+ visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, &err); | break; | case QTYPE_QSTRING: | visit_type_str(v, name, &(*obj)->u.reference, &err); Signed-off-by: Eric Blake <eblake@redhat.com> --- v10: new patch --- scripts/qapi-types.py | 10 ++++++- scripts/qapi-visit.py | 49 +++++++++++++++++++++++++++++---- scripts/qapi.py | 10 ++++--- tests/test-qmp-input-visitor.c | 29 ++++++++++++++++++- tests/qapi-schema/qapi-schema-test.json | 2 ++ tests/qapi-schema/qapi-schema-test.out | 2 ++ 6 files changed, 91 insertions(+), 11 deletions(-)