Eric Blake <ebl...@redhat.com> writes:
> Previously, working with alternates required two enums, and
> some indirection: for type Foo, we created Foo_qtypes[] which
> maps each qtype to a member of FooKind_lookup[], then use
member of FooKind, actually.
> FooKind_lookup[] like we do for other union types.
You probably mean FooKind here as well.
> This has a subtle bug: since the values of FooKind_lookup
> start at zero, all entries of Foo_qtypes that were not
> explicitly initialized map to the same branch of the union as
> the first member of the alternate, rather than triggering a
> 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
> 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).
Yes, I remember us discussing this bug.
While reading code to double-check your description, I stumbled over
this beauty in generated qapi-visit.c:
visit_get_next_type(v, (int*) &(*obj)->type, BlockdevRef_qtypes, name,
&err);
This casts enum BlockdevRefKind * to int *, which assumes the compiler
represents the enum BlockdevRefKind as int or unsigned. It is free to
use any integer type, though. Common mistake of programmers with
insufficiently developed wariness of C's subtleties.
visit_get_next_type() passes the fishy int * on to v->get_next_type().
Only implementation is qmp_input_get_next_type(), which uses it so:
*kind = qobjects[qobject_type(qobj)];
Latent death trap.
Does your patch clean this up?
> However, it 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, 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 it is still marked FIXME.
>
> This patch fixes the validation bug by deleting the indirection,
> and modifying get_next_type() to directly return a qtype code.
get_next_type() doesn't return anything. Do you mean "store a qtype
code"?
> There is no longer a need to generate an implicit FooKind array
FooKind is an enum, not an array.
> 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'. Next, the generated visitor is
> fixed to properly detect unexpected qtypes in the switch
> statement. This is done via the use of a new
> QAPISchemaAlternateTypeTag subclass and the use of a new
> member.c_type() method when producing qapi-types. The new
> subtype also allows us to clean up a TODO left in the previous
> commit.
>
> 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. If that gets too confusing, we
> could reintroduce FooKind, but initialize it 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 without a current client, I
> didn't see the point of doing it now.
>
> 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"}}
> Now it fails with:
> {"error": {"class": "GenericError",
> "desc": "Invalid parameter type for 'file', expected: BlockdevRef"}}
I wonder how that happens. Perhaps it's obvious in the patch.
QMP introspection isn't affected, because we carefully minimized the
information to expose there.
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> 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 | 12 +++++----
> scripts/qapi.py | 43
> ++++++++++++++++++++++-----------
> tests/qapi-schema/alternate-empty.out | 1 -
> tests/qapi-schema/qapi-schema-test.json | 2 +-
> tests/qapi-schema/qapi-schema-test.out | 10 +-------
> tests/test-qmp-input-visitor.c | 33 +++++++++++++------------
> tests/test-qmp-output-visitor.c | 21 ++++++++++++----
> 13 files changed, 86 insertions(+), 94 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 163f547..0c1bd36 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.
> + */
> +void visit_get_next_type(Visitor *v, qtype_code *type,
> const char *name, Error **errp);
Naive question: what makes a visitor an input visitor?
> 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 5dd9ed5..803ffad 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 403768b..3fd07fd 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -47,7 +47,7 @@ def gen_struct_field(member):
> ret += mcgen('''
> %(c_type)s %(c_name)s;
> ''',
> - c_type=member.type.c_type(), c_name=c_name(member.name))
> + c_type=member.c_type(), c_name=c_name(member.name))
This uses the new member.c_type() defined further down in the patch.
No change if member is an instance of QAPISchemaObjectTypeMember.
> return ret
>
>
> @@ -111,38 +111,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
> @@ -267,9 +235,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 318b8e6..2091a0f 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -189,7 +189,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;
> }
Yes, your patch disarms the latent death trap: no more pointer casting.
> @@ -203,14 +203,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s
> **obj, const char *name, Error
> 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");
Okay, this is where the new error message comes from.
Before, default is unreachable, because (*obj)->type got erroneously set
the enum's first member when none of the alternate's variants matches
the qtype.
After, (*obj)->type *is* the qtype, and we do reach default when no
variant matches.
How can name be null?
I really need to finish the QERR_ killing job.
> }
> out_obj:
> error_propagate(errp, err);
> @@ -219,7 +219,8 @@ out_obj:
> out:
> error_propagate(errp, err);
> }
> -''')
> +''',
> + name=name)
>
> return ret
>
> @@ -425,6 +426,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 a0639e4..bd74470 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)
Why c_name()?
> 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.type
> 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.
A member without a type? Ugh! I wouldn't dare breaking invariants like
that.
Of course, an alternate's tag member still has a type: qtype_code. It's
just not declared in the schema. Should it be a built-in type then?
> assert bool(tag_member) != bool(tag_name)
> assert (isinstance(tag_name, str) or
> isinstance(tag_member, QAPISchemaObjectTypeMember))
> @@ -1045,8 +1049,7 @@ class QAPISchemaObjectTypeVariants(object):
> self.tag_member = tag_member
> self.variants = variants
>
> - # TODO drop union once alternates can be distinguished by tag_member
> - def check(self, schema, seen, union=True):
> + def check(self, schema, seen):
> if self.tag_name: # flat union
> self.tag_member = seen[self.tag_name]
> assert self.tag_member
> @@ -1054,25 +1057,25 @@ class QAPISchemaObjectTypeVariants(object):
> assert self.tag_member in seen.itervalues()
> else: # alternate
> self.tag_member.check(schema, seen)
> - assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> + if not isinstance(self.tag_member, QAPISchemaAlternateTypeTag):
> + assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> cases = OrderedDict()
> for v in self.variants:
> # Reset seen array for each variant, since QMP names from one
> # branch do not affect another branch, nor add to all_members
> - v.check(schema, self.tag_member.type, dict(seen), cases, union)
> + v.check(schema, self.tag_member.type, dict(seen), cases)
I expect some rebase churn around here, so I'm not reviewing closely.
>
>
> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> def __init__(self, name, typ):
> QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> - # TODO drop union once alternates can be distinguished by tag_type
> - def check(self, schema, tag_type, seen, cases, union):
> + def check(self, schema, tag_type, seen, cases):
> # cases is case names we must not collide with
> QAPISchemaObjectTypeMember.check(self, schema, cases)
> - assert self.name in tag_type.values
> - if union:
> + if tag_type:
> # seen is QMP names our members must not collide with
> + assert self.name in tag_type.values
> self.type.check_qmp(schema, seen)
>
My patches move the member name collision checking to
QAPISchemaObjectType.check().
I suspect alternate branch name collision checking should similarly move
to QAPISchemaAlternateType.check().
Union branch name collision checking is superfluous, because they
already get checked by enum member name collision checking.
> # This function exists to support ugly simple union special cases
> @@ -1094,7 +1097,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
> self.variants = variants
>
> def check(self, schema):
> - self.variants.check(schema, {}, False)
> + self.variants.check(schema, {})
>
> def json_type(self):
> return 'value'
> @@ -1103,6 +1106,18 @@ class QAPISchemaAlternateType(QAPISchemaType):
> visitor.visit_alternate_type(self.name, self.info, self.variants)
>
>
> +class QAPISchemaAlternateTypeTag(QAPISchemaObjectTypeMember):
> + def __init__(self):
> + QAPISchemaObjectTypeMember.__init__(self, 'type', '', False)
> +
> + def check(self, schema, seen):
> + assert len(seen) == 0
> + seen[self.name] = self
> +
> + def c_type(self):
> + return 'qtype_code'
> +
> +
This is a hack to work around the lack of a qtype_code type. I suspect
creating such a type would be simpler in the end. Safer, too, because
it would avoid having members without a type, which scares me.
> class QAPISchemaCommand(QAPISchemaEntity):
> def __init__(self, name, info, arg_type, ret_type, gen,
> success_response):
> QAPISchemaEntity.__init__(self, name, info)
> @@ -1296,7 +1311,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,
> diff --git a/tests/qapi-schema/alternate-empty.out
> b/tests/qapi-schema/alternate-empty.out
> index 0f153b6..9b010d8 100644
> --- a/tests/qapi-schema/alternate-empty.out
> +++ b/tests/qapi-schema/alternate-empty.out
> @@ -1,4 +1,3 @@
> object :empty
> alternate Alt
> case i: int
> -enum AltKind ['i']
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index 4354604..4513d94 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -131,7 +131,7 @@
> 'data': { 'value1': 'UserDefZero', 'has_a': 'UserDefZero',
> 'u': 'UserDefZero', 'type': 'UserDefZero' } }
> { 'alternate': 'AltName', 'data': { 'type': 'int', 'u': 'bool',
> - 'myKind': 'has_a' } }
> + 'myKind': 'has_a', 'max': 'str' } }
Here, you add the positive test that alternate name 'max' works.
One, not mentioned in the commit message.
Two, the commit message says we may reintroduce FooKind if working with
qtype_code turns out to be too confusing. If we ever do that, alternate
name 'max' breaks, doesn't it? Shouldn't we keep it reserved then, just
in case?
>
> # testing commands
> { 'command': 'user_def_cmd', 'data': {} }
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index 29c46da..eda71b9 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -60,32 +60,26 @@ object :obj-user_def_cmd2-arg
> alternate AltIntNum
> case i: int
> case n: number
> -enum AltIntNumKind ['i', 'n']
> alternate AltName
> case type: int
> case u: bool
> case myKind: has_a
> -enum AltNameKind ['type', 'u', 'myKind']
> + case max: str
> alternate AltNumInt
> case n: number
> case i: int
> -enum AltNumIntKind ['n', 'i']
> alternate AltNumStr
> case n: number
> case s: str
> -enum AltNumStrKind ['n', 's']
> alternate AltStrBool
> case s: str
> case b: bool
> -enum AltStrBoolKind ['s', 'b']
> alternate AltStrInt
> case s: str
> case i: int
> -enum AltStrIntKind ['s', 'i']
> alternate AltStrNum
> case s: str
> case n: number
> -enum AltStrNumKind ['s', 'n']
> event EVENT_A None
> event EVENT_B None
> event EVENT_C :obj-EVENT_C-arg
> @@ -129,7 +123,6 @@ alternate UserDefAlternate
> case uda: UserDefA
> case s: str
> case i: int
> -enum UserDefAlternateKind ['uda', 's', 'i']
> object UserDefB
> member intb: int optional=False
> member a-b: bool optional=True
> @@ -198,7 +191,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
> alternate __org.qemu_x-Alt
> case __org.qemu_x-branch: str
> case b: __org.qemu_x-Base
> -enum __org.qemu_x-AltKind ['__org.qemu_x-branch', 'b']
> object __org.qemu_x-Base
> member __org.qemu_x-member1: __org.qemu_x-Enum optional=False
> enum __org.qemu_x-Enum ['__org.qemu_x-value']
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 3f6bc4d..552fc74 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -343,14 +343,14 @@ static void
> test_visitor_in_alternate(TestInputVisitorData *data,
>
> v = visitor_input_test_init(data, "42");
> visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
> - g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
> + g_assert_cmpint(tmp->type, ==, QTYPE_QINT);
> g_assert_cmpint(tmp->u.i, ==, 42);
> qapi_free_UserDefAlternate(tmp);
> visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "'string'");
> visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
> - g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
> + g_assert_cmpint(tmp->type, ==, QTYPE_QSTRING);
> g_assert_cmpstr(tmp->u.s, ==, "string");
> qapi_free_UserDefAlternate(tmp);
> visitor_input_teardown(data, NULL);
> @@ -386,11 +386,10 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
> qapi_free_AltStrBool(asb);
> visitor_input_teardown(data, NULL);
>
> - /* FIXME: Order of alternate should not affect semantics; asn should
> - * parse the same as ans */
> + /* FIXME: integer should parse as number */
> v = visitor_input_test_init(data, "42");
> visit_type_AltStrNum(v, &asn, NULL, &err);
> - /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
> + /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */
> /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
> g_assert(err);
> error_free(err);
> @@ -398,30 +397,34 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
> qapi_free_AltStrNum(asn);
> visitor_input_teardown(data, NULL);
>
> + /* FIXME: integer should parse as number */
> v = visitor_input_test_init(data, "42");
> - visit_type_AltNumStr(v, &ans, NULL, &error_abort);
> - g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
> - g_assert_cmpfloat(ans->u.n, ==, 42);
> + visit_type_AltNumStr(v, &ans, NULL, &err);
> + /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */
> + /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */
> + g_assert(err);
> + error_free(err);
> + err = NULL;
What's happening here? Whatever it is, the commit message didn't
prepare me for it...
> qapi_free_AltNumStr(ans);
> visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltStrInt(v, &asi, NULL, &error_abort);
> - g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
> + g_assert_cmpint(asi->type, ==, QTYPE_QINT);
> g_assert_cmpint(asi->u.i, ==, 42);
> qapi_free_AltStrInt(asi);
> visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltIntNum(v, &ain, NULL, &error_abort);
> - g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
> + g_assert_cmpint(ain->type, ==, QTYPE_QINT);
> g_assert_cmpint(ain->u.i, ==, 42);
> qapi_free_AltIntNum(ain);
> visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42");
> visit_type_AltNumInt(v, &ani, NULL, &error_abort);
> - g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
> + g_assert_cmpint(ani->type, ==, QTYPE_QINT);
> g_assert_cmpint(ani->u.i, ==, 42);
> qapi_free_AltNumInt(ani);
> visitor_input_teardown(data, NULL);
> @@ -438,14 +441,14 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltStrNum(v, &asn, NULL, &error_abort);
> - g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N);
> + g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
> g_assert_cmpfloat(asn->u.n, ==, 42.5);
> qapi_free_AltStrNum(asn);
> visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltNumStr(v, &ans, NULL, &error_abort);
> - g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
> + g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
> g_assert_cmpfloat(ans->u.n, ==, 42.5);
> qapi_free_AltNumStr(ans);
> visitor_input_teardown(data, NULL);
> @@ -460,14 +463,14 @@ static void
> test_visitor_in_alternate_number(TestInputVisitorData *data,
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltIntNum(v, &ain, NULL, &error_abort);
> - g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N);
> + g_assert_cmpint(ain->type, ==, QTYPE_QFLOAT);
> g_assert_cmpfloat(ain->u.n, ==, 42.5);
> qapi_free_AltIntNum(ain);
> visitor_input_teardown(data, NULL);
>
> v = visitor_input_test_init(data, "42.5");
> visit_type_AltNumInt(v, &ani, NULL, &error_abort);
> - g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N);
> + g_assert_cmpint(ani->type, ==, QTYPE_QFLOAT);
> g_assert_cmpfloat(ani->u.n, ==, 42.5);
> qapi_free_AltNumInt(ani);
> visitor_input_teardown(data, NULL);
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 9364843..695db32 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -449,20 +449,31 @@ static void
> test_visitor_out_alternate(TestOutputVisitorData *data,
> const void *unused)
> {
> QObject *arg;
> - Error *err = NULL;
> + UserDefAlternate *tmp;
>
> - UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
> - tmp->type = USER_DEF_ALTERNATE_KIND_I;
> + tmp = g_new0(UserDefAlternate, 1);
> + tmp->type = QTYPE_QINT;
> tmp->u.i = 42;
Coding style touched up. Okay.
>
> - visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
> - g_assert(err == NULL);
> + visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
We need to make up our mind whether to use g_assert(err == NULL) or
&error_abort in tests. Wholesale conversion could be in order. I like
&error_abort, because it's more concise.
> arg = qmp_output_get_qobject(data->qov);
>
> g_assert(qobject_type(arg) == QTYPE_QINT);
> g_assert_cmpint(qint_get_int(qobject_to_qint(arg)), ==, 42);
>
> qapi_free_UserDefAlternate(tmp);
> +
> + tmp = g_malloc0(sizeof(UserDefAlternate));
g_new0(UserDefAlternate, 1), please.
> + tmp->type = QTYPE_QSTRING;
> + tmp->u.s = g_strdup("hello");
> +
> + visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
> + arg = qmp_output_get_qobject(data->qov);
> +
> + g_assert(qobject_type(arg) == QTYPE_QSTRING);
> + g_assert_cmpstr(qstring_get_str(qobject_to_qstring(arg)), ==, "hello");
> +
> + qapi_free_UserDefAlternate(tmp);
New test, not mentioned in commit message. Separate patch, perhaps,
along with the nearby coding style touch ups?
> }
>
> static void test_visitor_out_empty(TestOutputVisitorData *data,
