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
FooKind_lookup[] like we do for other union types.
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).
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.
This patch fixes the validation bug by deleting the indirection,
and modifying get_next_type() to directly return a qtype code.
There is no longer a need to generate an implicit FooKind array
associated with the alternate type (since the QMP wire format
never uses the stringized counterparts of the C union member
names). Next, the generated visitor is fixed to properly detect
unexpected qtypes in the switch statement. Things got a bit
tricky with validating QAPISchemaObjectTypeVariants, which now
has three different initialization paths; but I didn't think it
was confusing enough to need to create different sub-classes.
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->kind. 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"}}
Signed-off-by: Eric Blake <ebl...@redhat.com>
---
include/qapi/visitor-impl.h | 2 +-
include/qapi/visitor.h | 2 +-
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 | 25 ++++++++++++++++-------
tests/qapi-schema/alternate-good.out | 1 -
tests/qapi-schema/qapi-schema-test.out | 8 --------
tests/test-qmp-input-visitor.c | 26 ++++++++++++------------
tests/test-qmp-output-visitor.c | 21 +++++++++++++++-----
11 files changed, 64 insertions(+), 77 deletions(-)
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8c0ba57..7098b93 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -32,7 +32,7 @@ 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,
+ 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..f1ac5c4 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -41,7 +41,7 @@ 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,
+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 230d4b2..643e36b 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 f04b6cc..fd56659 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -87,36 +87,6 @@ struct %(c_name)s {
return ret
-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_union(name, base, variants):
ret = mcgen('''
@@ -130,6 +100,10 @@ struct %(c_name)s {
''',
c_type=c_name(variants.tag_member.type.name),
c_name=c_name(variants.tag_member.name))
+ elif not variants.tag_member:
+ ret += mcgen('''
+ qtype_code type;
+''')
else:
ret += mcgen('''
%(c_type)s type;
@@ -238,9 +212,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self._gen_type_cleanup(name)
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_union(name, None, variants)
- self.decl += gen_alternate_qtypes_decl(name)
self._gen_type_cleanup(name)
do_builtins = False
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 728abae..2907ce8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -197,7 +197,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj,
const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(m, (int*) &(*obj)->type, %(c_name)s_qtypes, name,
&err);
+ visit_get_next_type(m, &(*obj)->type, name, &err);
if (err) {
goto out_end;
}
@@ -211,14 +211,14 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj,
const char *name, Error
visit_type_%(c_type)s(m, &(*obj)->%(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");
}
out_end:
error_propagate(errp, err);
@@ -227,7 +227,8 @@ out_end:
out:
error_propagate(errp, err);
}
-''')
+''',
+ name=name)
return ret
@@ -417,6 +418,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 5fedfee..9e4e44a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -920,22 +920,27 @@ class QAPISchemaObjectTypeVariants(object):
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self.tag_name = tag_name
- if tag_name:
+ if tag_name: # flat union
assert not tag_enum
self.tag_member = None
- else:
+ elif tag_enum: # simple union
self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
False)
+ else: # alternate
+ self.tag_member = None
self.variants = variants
def check(self, schema, members, seen):
if self.tag_name:
self.tag_member = seen[self.tag_name]
- else:
+ elif self.tag_member:
self.tag_member.check(schema, members, seen)
- assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ typ = None
+ if self.tag_member:
+ assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+ typ = self.tag_member.type
for v in self.variants:
vseen = dict(seen)
- v.check(schema, self.tag_member.type, vseen)
+ v.check(schema, typ, vseen)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ, flat):
@@ -944,7 +949,8 @@ class
QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
self.flat = flat
def check(self, schema, tag_type, seen):
QAPISchemaObjectTypeMember.check(self, schema, [], seen)
- assert self.name in tag_type.values
+ if tag_type:
+ assert self.name in tag_type.values
if self.flat:
self.type.check(schema)
assert isinstance(self.type, QAPISchemaObjectType)
@@ -1115,6 +1121,11 @@ class QAPISchema(object):
[v.name for v in variants])
return QAPISchemaObjectTypeVariants(None, enum, variants)
+ def _make_alternate_variants(self, data):
+ variants = [self._make_simple_variant(key, data[key])
+ for key in data.keys()]
+ return QAPISchemaObjectTypeVariants(None, None, variants)
+
def _def_union_type(self, expr, info):
name = expr['union']
data = expr['data']
@@ -1134,7 +1145,7 @@ class QAPISchema(object):
name = expr['alternate']
data = expr['data']
self._def_entity(QAPISchemaAlternateType(name, info,
- self._make_simple_variants(name, data)))
+ self._make_alternate_variants(data)))
self._make_array_type(name) # TODO really needed?
def _def_command(self, expr, info):
diff --git a/tests/qapi-schema/alternate-good.out
b/tests/qapi-schema/alternate-good.out
index aede1ae..e2d26b0 100644
--- a/tests/qapi-schema/alternate-good.out
+++ b/tests/qapi-schema/alternate-good.out
@@ -3,7 +3,6 @@ alternate Alt
case value: int flat=False
case string: Enum flat=False
case struct: Data flat=False
-enum AltKind ['value', 'string', 'struct']
object Data
member number: int optional=True
member name: str optional=True
diff --git a/tests/qapi-schema/qapi-schema-test.out
b/tests/qapi-schema/qapi-schema-test.out
index 9d52ef3..1585bad 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -24,26 +24,20 @@ object :obj-user_def_cmd3-args
alternate AltFive
case i: int flat=False
case n: number flat=False
-enum AltFiveKind ['i', 'n']
alternate AltFour
case s: str flat=False
case i: int flat=False
-enum AltFourKind ['s', 'i']
alternate AltOne
case s: str flat=False
-enum AltOneKind ['s']
alternate AltSix
case n: number flat=False
case i: int flat=False
-enum AltSixKind ['n', 'i']
alternate AltThree
case n: number flat=False
case s: str flat=False
-enum AltThreeKind ['n', 's']
alternate AltTwo
case s: str flat=False
case n: number flat=False
-enum AltTwoKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-data
@@ -64,7 +58,6 @@ alternate UserDefAlternate
case uda: UserDefA flat=False
case s: str flat=False
case i: int flat=False
-enum UserDefAlternateKind ['uda', 's', 'i']
object UserDefB
member intb: int optional=False
object UserDefC
@@ -127,7 +120,6 @@ event __ORG.QEMU_X-EVENT __org.qemu_x-Struct
alternate __org.qemu_x-Alt
case __org.qemu_x-branch: str flat=False
case b: __org.qemu_x-Base flat=False
-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 56feedf..f89ede3 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -376,7 +376,7 @@ 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->i, ==, 42);
qapi_free_UserDefAlternate(tmp);
tmp = NULL;
@@ -384,7 +384,7 @@ static void test_visitor_in_alternate(TestInputVisitorData
*data,
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->s, ==, "string");
qapi_free_UserDefAlternate(tmp);
tmp = NULL;
@@ -427,31 +427,31 @@ static void
test_visitor_in_alternate_number(TestInputVisitorData *data,
qapi_free_AltTwo(two);
one = NULL;
- /* FIXME: Order of alternate should not affect semantics */
v = visitor_input_test_init(data, "42");
- visit_type_AltThree(v, &three, NULL, &error_abort);
- g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N);
- g_assert_cmpfloat(three->n, ==, 42);
+ visit_type_AltThree(v, &three, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
qapi_free_AltThree(three);
one = NULL;
v = visitor_input_test_init(data, "42");
visit_type_AltFour(v, &four, NULL, &error_abort);
- g_assert_cmpint(four->type, ==, ALT_FOUR_KIND_I);
+ g_assert_cmpint(four->type, ==, QTYPE_QINT);
g_assert_cmpint(four->i, ==, 42);
qapi_free_AltFour(four);
one = NULL;
v = visitor_input_test_init(data, "42");
visit_type_AltFive(v, &five, NULL, &error_abort);
- g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_I);
+ g_assert_cmpint(five->type, ==, QTYPE_QINT);
g_assert_cmpint(five->i, ==, 42);
qapi_free_AltFive(five);
one = NULL;
v = visitor_input_test_init(data, "42");
visit_type_AltSix(v, &six, NULL, &error_abort);
- g_assert_cmpint(six->type, ==, ALT_SIX_KIND_I);
+ g_assert_cmpint(six->type, ==, QTYPE_QINT);
g_assert_cmpint(six->i, ==, 42);
qapi_free_AltSix(six);
one = NULL;
@@ -468,14 +468,14 @@ static void
test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltTwo(v, &two, NULL, &error_abort);
- g_assert_cmpint(two->type, ==, ALT_TWO_KIND_N);
+ g_assert_cmpint(two->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(two->n, ==, 42.5);
qapi_free_AltTwo(two);
two = NULL;
v = visitor_input_test_init(data, "42.5");
visit_type_AltThree(v, &three, NULL, &error_abort);
- g_assert_cmpint(three->type, ==, ALT_THREE_KIND_N);
+ g_assert_cmpint(three->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(three->n, ==, 42.5);
qapi_free_AltThree(three);
three = NULL;
@@ -490,14 +490,14 @@ static void
test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltFive(v, &five, NULL, &error_abort);
- g_assert_cmpint(five->type, ==, ALT_FIVE_KIND_N);
+ g_assert_cmpint(five->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(five->n, ==, 42.5);
qapi_free_AltFive(five);
five = NULL;
v = visitor_input_test_init(data, "42.5");
visit_type_AltSix(v, &six, NULL, &error_abort);
- g_assert_cmpint(six->type, ==, ALT_SIX_KIND_N);
+ g_assert_cmpint(six->type, ==, QTYPE_QFLOAT);
g_assert_cmpint(six->n, ==, 42.5);
qapi_free_AltSix(six);
six = NULL;
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index ab4bc5d..2b5c8d5 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -510,20 +510,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_malloc0(sizeof(UserDefAlternate));
+ tmp->type = QTYPE_QINT;
tmp->i = 42;
- visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
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));
+ tmp->type = QTYPE_QSTRING;
+ tmp->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);
}
static void test_visitor_out_empty(TestOutputVisitorData *data,
--
2.4.3
