Re: [Qemu-devel] [PATCH v8 10/17] qapi: Simplify visiting of alternate types

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 16181 bytes --]

On 11/03/2015 11:30 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]