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...]
