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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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.

The generated

    const int BlockdevRef_qtypes[QTYPE_MAX] = {
        [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION,
        [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE,
    };

maps from qtype_code to BlockdevRefKind, except it uses int instead of
BlockdevRefKind, so it can be passed to visit_get_next_type().

>>> 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.

Yes.

>>> 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.

Perhaps un-reserving MAX and making the collision checking more lenient
should be a separate follow-up patch we could easily revert.  Probably
useful only if this patch doesn't make 2.5.

>>>
>>> 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. */

That's a useful step towards a visitors contract.

>>> +++ 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).

Examining the occurences of visit_type_BlockdevRef* in generated
qapi-visit.c shows that parameter name is commonly the member name.  It
appears to be used only for error messages.

Works fine when the object containing the member is obvious.  With
multiple objects, the recipient of the error message has to guess the
object containing the member.  If the member name isn't unique, the
error message is ambiguous.  Crap, but not crap we can afford to improve
right now.

Array elements are anonymous, and I guess that's what led to NULL.
Stupid choice.  Make it "array element" or something.  Code becomes
slightly simpler, error message becomes slightly less cruel.  This could
be simple enough to improve now.

>> I really need to finish the QERR_ killing job.
>
> Agreed. But shouldn't stall this patch, though.

Certainly not.

>>>      # 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.

Let's give it a try and see.

>>> +++ 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.

Recommend to stick a brief note into the commit message that this
temporarily breaks test so-and-so, marked FIXME.

>>> +++ 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.