On 11/03/2015 11:30 AM, Markus Armbruster wrote: > 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.
Or entry in the FooKind_lookup[] array.
>
>> FooKind_lookup[] like we do for other union types.
>
> You probably mean FooKind here as well.
I'll play with the wording.
>
>> 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?
Yes, and I need to also document that this is an additional bug fix.
>
>> 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.
See [1] below.
>>
>> 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"?
Yes.
>
>> There is no longer a need to generate an implicit FooKind array
>
> FooKind is an enum, not an array.
...to generate an implicit FooKind enum, nor FooKind_lookup[] 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.
You have a point below that we either need to reserve MAX and require no
case-insensitive clashes, or that we will never want to add it. I'm
leaning towards never going back, because the new way feels so much nicer.
>>
>> 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.
I think you found it below.
>
> QMP introspection isn't affected, because we carefully minimized the
> information to expose there.
Ooh, nice tidbit to add.
>> +/**
>> + * 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?
I've got a later patch in my queue that adds a lot more documentation:
http://repo.or.cz/qemu/ericb.git/commitdiff/f7674a87e72
+/* This file describes the client view for visiting a map between
+ * generated QAPI C structs and another representation (command line
+ * options, strings, or QObjects). An input visitor converts from
+ * some other form into QAPI representation; an output visitor
+ * converts from QAPI back into another form. In the descriptions
+ * below, an object or dictionary refers to a JSON '{}', and an array
+ * or list refers to a JSON '[]'. These functions seldom need to be
+ * called directly, but are instead used by code generated by
+ * scripts/qapi-visit.py. For the visitor callback contracts, see
+ * visitor-impl.h. */
>> +++ 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.
Indeed, I noticed the cleanup as well (I'm quite familiar with the
unsafe nature of casting enum* because you cannot guarantee its size),
but failed to call out the trap in my commit message.
>
>> @@ -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?
When you have the qapi representation ['MyAlternate'], you will have
qapi_visit_type_MyAlternateList() which passes NULL for the name of each
list member (because names are only present for objects, not lists).
>
> I really need to finish the QERR_ killing job.
Agreed. But shouldn't stall this patch, though.
>> # 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()?
So that 'a-b' and 'a_b' are properly flagged as conflicting (they map to
the same c_name).
>> 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?
It's not a builtin that can ever be referenced in the .json files. But
I could probably come up with something, if it would make you feel better.
>> 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.
>
Indeed. All the more reason for me to post a v9 spin (and maybe defer
the question of a non-None type for tag_member until after that post).
> My patches move the member name collision checking to
> QAPISchemaObjectType.check().
>
> I suspect alternate branch name collision checking should similarly move
> to QAPISchemaAlternateType.check().
>
Yep, already that way in my pending v9 series after incorporating your
patches.
>> +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.
I can play with dropping c_type() here in favor of adding a qtype_code
special class, but I may still need to keep this
QAPISchemaAlternateTypeTag subclass.
>> +++ 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.
D'oh.
>
> 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?
>
See my comment above; at this point, I doubt we'll ever want to go back,
so maybe I just need to be more definitive in stating that.
>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -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...
See [1] above. 'asn' is now parsing the same as 'ans' (we are no longer
sensitive to whether 'number' was the first member of the alternate),
but it isn't until patch 11/17 that we fix things that 'ans' and 'asn'
both properly parse '1' as a number instead of rejecting it as an integer.
>> +++ 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.
Wholesale conversion dead-ahead, in 14/17 of this subset.
>
>> 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?
Yes, I will split this portion of the test changes out to a separate commit.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
