OpenE2K
/
qemu-e2k
13
4
Fork 0
Code Issues 4 Pull Requests Projects 2 Releases Activity

qapi: Simplify visiting of alternate types 

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.

Related bug, not not fixed in this patch: qapi-visit.py's
gen_visit_enum() generates a cast of its enum * argument to
int *. Marked FIXME.

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: parsing an
integer causes unusual behavior in an alternate that contains
at least a 'number' member but no 'int' member, because the
'number' parser accepts QTYPE_QINT in addition to the expected
QTYPE_QFLOAT (that is, since 'int' is not a member, the type
QTYPE_QINT accidentally maps to FooKind 0; if this enum value
is the 'number' branch the integer parses successfully, but if
the 'number' branch is not first, some other branch tries to
parse the integer and rejects it).  A later patch will worry
about fixing alternates to always parse all inputs that a
non-alternate 'number' would accept, for now this is still
marked FIXME in the updated test-qmp-input-visitor.c, to
merely point out that new undesired behavior of 'ans' matches
the existing undesired behavior of 'asn'.

This patch fixes the default-initialization bug by deleting the
indirection, and modifying get_next_type() to directly assign a
QTypeCode 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).  Since the updated 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.

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.

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 <eblake@redhat.com>
Message-Id: <1449033659-25497-5-git-send-email-eblake@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
This commit is contained in:
Eric Blake 2015-12-01 22:20:48 -07:00 committed by Markus Armbruster
parent 7264f5c50c
commit 0426d53c65
12 changed files with 54 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
docs/qapi-code-gen.txt
View File

@ -384,9 +384,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

3
include/qapi/visitor-impl.h
View File

@ -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; only needed for input visitors. */
void (*get_next_type)(Visitor *v, QType *type,
const char *name, Error **errp);
void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error **errp);

8
include/qapi/visitor.h
View File

@ -38,7 +38,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 *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);

4
qapi/qapi-visit-core.c
View File

@ -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 *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);
}
}

4
qapi/qmp-input-visitor.c
View File

@ -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 *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,

34
scripts/qapi-types.py
View File

@ -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
@ -264,9 +232,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

15
scripts/qapi-visit.py
View File

@ -172,6 +172,7 @@ out:
def gen_visit_enum(name):
# FIXME cast from enum *obj to int * invalidly assumes enum is int
return mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
@ -193,7 +194,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 +202,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");
}
out_obj:
error_propagate(errp, err);
@ -223,7 +226,8 @@ out_obj:
out:
error_propagate(errp, err);
}
''')
''',
name=name)
return ret
@ -437,6 +441,7 @@ fdef.write(mcgen('''
fdecl.write(mcgen('''
#include "qapi/visitor.h"
#include "qapi/qmp/qerror.h"
#include "%(prefix)sqapi-types.h"
''',

18
scripts/qapi.py
View File

@ -635,8 +635,8 @@ def check_alternate(expr, expr_info):
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'"
@ -1092,8 +1092,11 @@ class QAPISchemaObjectTypeVariants(object):
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
v.check(schema)
assert v.name in self.tag_member.type.values
if isinstance(v.type, QAPISchemaObjectType):
# Union names must match enum values; alternate names are
# checked separately. Use 'seen' to tell the two apart.
if seen:
assert v.name in self.tag_member.type.values
assert isinstance(v.type, QAPISchemaObjectType)
v.type.check(schema)
def check_clash(self, schema, info, seen):
@ -1135,6 +1138,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
# Not calling self.variants.check_clash(), because there's nothing
# to clash with
self.variants.check(schema, {})
# Alternate branch names have no relation to the tag enum values;
# so we have to check for potential name collisions ourselves.
seen = {}
for v in self.variants.variants:
v.check_clash(self.info, seen)
def json_type(self):
return 'value'
@ -1342,7 +1350,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 = QAPISchemaObjectTypeMember('type', 'QType', False)
self._def_entity(
QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,

1
tests/qapi-schema/alternate-empty.out
View File

@ -1,6 +1,5 @@
object :empty
alternate Alt
case i: int
enum AltKind ['i']
enum QType ['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', 'qfloat', 'qbool']
prefix QTYPE

8
tests/qapi-schema/qapi-schema-test.out
View File

@ -56,27 +56,21 @@ object :obj-user_def_cmd2-arg
alternate AltIntNum
case i: int
case n: number
enum AltIntNumKind ['i', 'n']
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
@ -114,7 +108,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
@ -180,7 +173,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']

31
tests/test-qmp-input-visitor.c
View File

@ -312,13 +312,13 @@ 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);
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);
@ -347,36 +347,37 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
error_free_or_abort(&err);
qapi_free_AltStrBool(asb);
/* 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); */
error_free_or_abort(&err);
qapi_free_AltStrNum(asn);
/* 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); */
error_free_or_abort(&err);
qapi_free_AltNumStr(ans);
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);
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);
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);
@ -389,13 +390,13 @@ 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);
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);
@ -406,13 +407,13 @@ 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);
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);
}

4
tests/test-qmp-output-visitor.c
View File

@ -428,7 +428,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
UserDefAlternate *tmp;
tmp = g_new0(UserDefAlternate, 1);
tmp->type = USER_DEF_ALTERNATE_KIND_I;
tmp->type = QTYPE_QINT;
tmp->u.i = 42;
visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);
@ -441,7 +441,7 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
qobject_decref(arg);
tmp = g_new0(UserDefAlternate, 1);
tmp->type = USER_DEF_ALTERNATE_KIND_S;
tmp->type = QTYPE_QSTRING;
tmp->u.s = g_strdup("hello");
visit_type_UserDefAlternate(data->ov, &tmp, NULL, &error_abort);