Eric Blake <ebl...@redhat.com> writes: > Previously, working with alternates required two lookup arrays > and some indirection: for type Foo, we created Foo_qtypes[] > which maps each qtype to a value of the generated FooKind enum, > then look up that value in FooKind_lookup[] like we do for other > union types. > > This has a couple of subtle bugs. First, the generator was > creating a call with a parameter '(int *) &(*obj)->type' where > type is an enum type; this is unsafe if the compiler chooses > to store the enum type in a different size than int, where > assigning through the wrong size pointer can corrupt data or > cause a SIGBUS. > > Second, since the values of the FooKind enum start at zero, all > entries of the Foo_qtypes[] array that were not explicitly > initialized will map to the same branch of the union as the > first member of the alternate, rather than triggering a desired > failure in visit_get_next_type(). Fortunately, the bug seldom > bites; the very next thing the input visitor does is try to > parse the incoming JSON with the wrong parser, which normally > fails; the output visitor is not used with a C struct in that > state, and the dealloc visitor has nothing to clean up (so > there is no leak). > > However, the second bug IS observable in one case: the behavior > of an alternate that contains a 'number' member but no 'int' > member differs according to whether the 'number' was first in > the qapi definition,
This is confusing. If there is a 'number' but no 'int', and the 'number is first, what's second? > and when the input being parsed is an > integer; this is because the 'number' parser accepts QTYPE_QINT > in addition to the expected QTYPE_QFLOAT. A later patch will > worry about fixing alternates to parse all inputs that a > non-alternate 'number' would accept, for now this is still > marked FIXME, and the test merely updated to point out that and tests/test-qmp-input-visitor.c merely updated > new undesired behavior of 'ans' matches the existing undesired > behavior of 'asn'. > > This patch fixes the validation bug by deleting the indirection, What's the validation bug? I guess it's the one involving default-initialized FooKind_lookup[]. > and modifying get_next_type() to directly assign a qtype_code > parameter. This in turn fixes the type-casting bug, as we are > no longer casting a pointer to enum to a questionable size. > There is no longer a need to generate an implicit FooKind enum > associated with the alternate type (since the QMP wire format > never uses the stringized counterparts of the C union member > names); that also means we no longer have a collision with an > alternate branch named 'max'. Since visit_get_next_type() does > not know which qtypes are expected, the generated visitor is > modified to generate an error statement if an unexpected type > is encountered. > > The code relies on a new QAPISchemaAlternateTypeTag subclass > and the use of a new member.c_type() method for generating > qapi-types.h; this is because we don't want to expose > 'qtype_code' as a built-in type usable from .json files. I'd be totally cool with that, actually. > The new subtype happens to work by keeping tag_member.type > as None, although this feels a bit unclean, and may be touched > up further in a later patch. I hate special cases like this one. > Callers now have to know the QTYPE_* mapping when looking at the > discriminator; but so far, only the testsuite was even using the > C struct of an alternate types. I considered the possibility of > keeping the internal enum FooKind, but initialized differently > than most generated arrays, as in: > typedef enum FooKind { > FOO_KIND_A = QTYPE_QDICT, > FOO_KIND_B = QTYPE_QINT, > } FooKind; > to create nicer aliases for knowing when to use foo->a or foo->b > when inspecting foo->type; but it turned out to add too much > complexity, especially without a client. Having to use QTYPE_FOOs seems good enough. > There is a user-visible side effect to this change, but I > consider it to be an improvement. Previously, > the invalid QMP command: > {"execute":"blockdev-add", "arguments":{"options": > {"driver":"raw", "id":"a", "file":true}}} > failed with: > {"error": {"class": "GenericError", > "desc": "Invalid parameter type for 'file', expected: QDict"}} > (visit_get_next_type() succeeded, and the error comes from the > visit_type_BlockdevOptions() expecting {}; there is no mention of > the fact that a string would also work). Now it fails with: > {"error": {"class": "GenericError", > "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}} > (the error when the next type doesn't match any expected types for > the overall alternate). > > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v9: rebase to earlier changes, rework commit message to mention second > bug fix; move positive test in qapi-schema-test to later patch > v8: no change > v7: rebase onto earlier changes, rework how subtype makes things work > v6: rebase onto tag_member subclass, testsuite, gen_err_check(), > and info improvements > --- > docs/qapi-code-gen.txt | 3 --- > include/qapi/visitor-impl.h | 3 ++- > include/qapi/visitor.h | 8 ++++++- > qapi/qapi-visit-core.c | 4 ++-- > qapi/qmp-input-visitor.c | 4 ++-- > scripts/qapi-types.py | 36 +--------------------------- > scripts/qapi-visit.py | 14 +++++++---- > scripts/qapi.py | 44 > +++++++++++++++++++++++++--------- > tests/qapi-schema/alternate-empty.out | 1 - > tests/qapi-schema/qapi-schema-test.out | 8 ------- > tests/test-qmp-input-visitor.c | 31 ++++++++++++------------ > tests/test-qmp-output-visitor.c | 4 ++-- > 12 files changed, 74 insertions(+), 86 deletions(-) > > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > index 20e6907..9196e5c 100644 > --- a/docs/qapi-code-gen.txt > +++ b/docs/qapi-code-gen.txt > @@ -383,9 +383,6 @@ where each branch of the union names a QAPI type. For > example: > 'data': { 'definition': 'BlockdevOptions', > 'reference': 'str' } } > > -Just like for a simple union, an implicit C enum 'NameKind' is created > -to enumerate the branches for the alternate 'Name'. > - > Unlike a union, the discriminator string is never passed on the wire > for the Client JSON Protocol. Instead, the value's JSON type serves > as an implicit discriminator, which in turn means that an alternate > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index 8c0ba57..6d95b36 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -32,7 +32,8 @@ struct Visitor > > void (*type_enum)(Visitor *v, int *obj, const char * const strings[], > const char *kind, const char *name, Error **errp); > - void (*get_next_type)(Visitor *v, int *kind, const int *qobjects, > + /* May be NULL; most useful for input visitors. */ > + void (*get_next_type)(Visitor *v, qtype_code *type, > const char *name, Error **errp); > > void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error > **errp); > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index cfc19a6..b765993 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -41,7 +41,13 @@ GenericList *visit_next_list(Visitor *v, GenericList > **list, Error **errp); > void visit_end_list(Visitor *v, Error **errp); > void visit_optional(Visitor *v, bool *present, const char *name, > Error **errp); > -void visit_get_next_type(Visitor *v, int *obj, const int *qtypes, > + > +/** > + * Determine the qtype of the item @name in the current object visit. > + * For input visitors, set *@type to the correct qtype of a qapi > + * alternate type; for other visitors, leave *@type unchanged. Aside: I can't think of a reason for an output visitor to implement this method. > + */ > +void visit_get_next_type(Visitor *v, qtype_code *type, > const char *name, Error **errp); > void visit_type_enum(Visitor *v, int *obj, const char * const strings[], > const char *kind, const char *name, Error **errp); > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 59ed506..3f24daa 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char > *name, > } > } > > -void visit_get_next_type(Visitor *v, int *obj, const int *qtypes, > +void visit_get_next_type(Visitor *v, qtype_code *type, > const char *name, Error **errp) > { > if (v->get_next_type) { > - v->get_next_type(v, obj, qtypes, name, errp); > + v->get_next_type(v, type, name, errp); > } > } > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index eb6e110..c1e7ec8 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -208,7 +208,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp) > qmp_input_pop(qiv, errp); > } > > -static void qmp_input_get_next_type(Visitor *v, int *kind, const int > *qobjects, > +static void qmp_input_get_next_type(Visitor *v, qtype_code *type, > const char *name, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > @@ -218,7 +218,7 @@ static void qmp_input_get_next_type(Visitor *v, int > *kind, const int *qobjects, > error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > return; > } > - *kind = qobjects[qobject_type(qobj)]; > + *type = qobject_type(qobj); > } > > static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name, > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 2f2f7df..2ac1405 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -47,7 +47,7 @@ def gen_struct_fields(members): > ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > - c_type=memb.type.c_type(), c_name=c_name(memb.name)) > + c_type=memb.c_type(), c_name=c_name(memb.name)) > return ret > > > @@ -101,38 +101,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const > %(c_name)s *obj) > c_name=c_name(name), base=base.c_name()) > > > -def gen_alternate_qtypes_decl(name): > - return mcgen(''' > - > -extern const int %(c_name)s_qtypes[]; > -''', > - c_name=c_name(name)) > - > - > -def gen_alternate_qtypes(name, variants): > - ret = mcgen(''' > - > -const int %(c_name)s_qtypes[QTYPE_MAX] = { > -''', > - c_name=c_name(name)) > - > - for var in variants.variants: > - qtype = var.type.alternate_qtype() > - assert qtype > - > - ret += mcgen(''' > - [%(qtype)s] = %(enum_const)s, > -''', > - qtype=qtype, > - enum_const=c_enum_const(variants.tag_member.type.name, > - var.name)) > - > - ret += mcgen(''' > -}; > -''') > - return ret > - > - > def gen_variants(variants): > # 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 > @@ -257,9 +225,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > > def visit_alternate_type(self, name, info, variants): > self._fwdecl += gen_fwd_object_or_array(name) > - self._fwdefn += gen_alternate_qtypes(name, variants) > self.decl += gen_object(name, None, [variants.tag_member], variants) > - self.decl += gen_alternate_qtypes_decl(name) > self._gen_type_cleanup(name) > > # If you link code generated from multiple schemata, you want only one > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 94cd113..af80e6d 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -193,7 +193,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, > const char *name, Error > if (err) { > goto out; > } > - visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, > &err); > + visit_get_next_type(v, &(*obj)->type, name, &err); > if (err) { > goto out_obj; > } > @@ -201,20 +201,22 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s > **obj, const char *name, Error > ''', > c_name=c_name(name)) > > + # FIXME: When 'number' but not 'int' is present in the alternate, we > + # should allow QTYPE_INT to promote to QTYPE_FLOAT. > for var in variants.variants: > ret += mcgen(''' > case %(case)s: > visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err); > break; > ''', > - case=c_enum_const(variants.tag_member.type.name, > - var.name), > + case=var.type.alternate_qtype(), > c_type=var.type.c_name(), > c_name=c_name(var.name)) > > ret += mcgen(''' > default: > - abort(); > + error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > + "%(name)s"); Craptastic error reporting, but it's widely done elsewhere already, and cleaning it up is out of scope. > } > out_obj: > error_propagate(errp, err); > @@ -223,7 +225,8 @@ out_obj: > out: > error_propagate(errp, err); > } > -''') > +''', > + name=name) > > return ret > > @@ -430,6 +433,7 @@ fdef.write(mcgen(''' > > fdecl.write(mcgen(''' > #include "qapi/visitor.h" > +#include "qapi/qmp/qerror.h" > #include "%(prefix)sqapi-types.h" > > ''', > diff --git a/scripts/qapi.py b/scripts/qapi.py > index f5aa1d5..9a1f0ac 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -627,15 +627,15 @@ def check_union(expr, expr_info): > def check_alternate(expr, expr_info): > name = expr['alternate'] > members = expr['data'] > - values = {'MAX': '(automatic)'} > + values = {} > types_seen = {} > > # Check every branch > for (key, value) in members.items(): > check_name(expr_info, "Member of alternate '%s'" % name, key) > > - # Check for conflicts in the generated enum > - c_key = camel_to_upper(key) > + # Check for conflicts in the branch names > + c_key = c_name(key) > if c_key in values: > raise QAPIExprError(expr_info, > "Alternate '%s' member '%s' clashes with > '%s'" > @@ -1029,13 +1029,17 @@ class QAPISchemaObjectTypeMember(object): > assert self.name not in seen > seen[self.name] = self > > + def c_type(self): > + return self.type.c_type() > + > > class QAPISchemaObjectTypeVariants(object): > def __init__(self, tag_name, tag_member, variants): > # Flat unions pass tag_name but not tag_member. > # Simple unions and alternates pass tag_member but not tag_name. > # After check(), tag_member is always set, and tag_name remains > - # a reliable witness of being used by a flat union. > + # a reliable witness of being used by a flat union, and > + # tag_member.type being None is a reliable witness of an alternate. > assert bool(tag_member) != bool(tag_name) > assert (isinstance(tag_name, str) or > isinstance(tag_member, QAPISchemaObjectTypeMember)) > @@ -1049,7 +1053,11 @@ class QAPISchemaObjectTypeVariants(object): > # seen is non-empty for unions, empty for alternates > if self.tag_name: # flat union > self.tag_member = seen[self.tag_name] > - assert isinstance(self.tag_member.type, QAPISchemaEnumType) > + if seen: > + assert isinstance(self.tag_member.type, QAPISchemaEnumType) > + else: > + assert isinstance(self.tag_member, QAPISchemaAlternateTypeTag) > + assert not self.tag_member.type Awkward. > for v in self.variants: > # Reset seen array for each variant, since qapi names from one > # branch do not affect another branch > @@ -1062,10 +1070,8 @@ class > QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): > > def check(self, schema, tag_type, seen): > QAPISchemaObjectTypeMember.check(self, schema) > - assert self.name in tag_type.values > - if seen: > - # This variant is used within a union; ensure each qapi member > - # field does not collide with the union's non-variant members. > + if seen: # in a union > + assert self.name in tag_type.values > self.type.check_clash(schema, seen) > > # This function exists to support ugly simple union special cases > @@ -1087,8 +1093,12 @@ class QAPISchemaAlternateType(QAPISchemaType): > self.variants = variants > > def check(self, schema): > - self.variants.tag_member.check(schema) > self.variants.check(schema, {}) > + # Since we have no enum mapping, we have to check for potential > + # case name collisions ourselves. > + cases = {} > + for var in self.variants.variants: > + var.check_clash(cases) > > def json_type(self): > return 'value' > @@ -1097,6 +1107,18 @@ class QAPISchemaAlternateType(QAPISchemaType): > visitor.visit_alternate_type(self.name, self.info, self.variants) > > > +class QAPISchemaAlternateTypeTag(QAPISchemaObjectTypeMember): > + # TODO: This subclass intentionally leaves self.tag_type as None, > + # and intentionally has no check() method. It might be easier to > + # reason about the overall QAPISchema if we also subclassed > + # QAPISchemaBuiltinType to supply an internal-only 'qtype_code' type. Sure we need to subclass? I do hope an instance will do! Something like self.the_qtype_code_type = QAPISchemaEnumType(':qtype_code', None, ['NONE', 'QNULL', ...]) in _def_predefineds(). Also avoids the awkward conditional in QAPISchemaObjectTypeVariants.check(). Fewer special cases are worth a bit of scaffolding. > + def __init__(self): > + QAPISchemaObjectTypeMember.__init__(self, 'type', '', False) > + > + def c_type(self): > + return 'qtype_code' > + > + > class QAPISchemaCommand(QAPISchemaEntity): > def __init__(self, name, info, arg_type, ret_type, gen, > success_response): > QAPISchemaEntity.__init__(self, name, info) > @@ -1290,7 +1312,7 @@ class QAPISchema(object): > data = expr['data'] > variants = [self._make_variant(key, value) > for (key, value) in data.iteritems()] > - tag_member = self._make_implicit_tag(name, info, variants) > + tag_member = QAPISchemaAlternateTypeTag() > self._def_entity( > QAPISchemaAlternateType(name, info, > QAPISchemaObjectTypeVariants(None, [Tests snipped...]