Re: [Qemu-devel] [PATCH RFC v3 11/32] qapi-visit: Convert to QAPISchemaVisitor, fixing bugs

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 08/04/2015 09:57 AM, Markus Armbruster wrote:
>> Fixes flat unions to visit the base's base members (the previous
>> commit merely added them to the struct).  Same test case.
>> 
>> Patch's effect on visit_type_UserDefFlatUnion():
>> 
>>      static void visit_type_UserDefFlatUnion_fields(Visitor *m, UserDefFlatUnion **obj, Error **errp)
>>      {
>>          Error *err = NULL;
>> 
>>     +    visit_type_int(m, &(*obj)->integer, "integer", &err);
>>     +    if (err) {
>>     +        goto out;
>>     +    }
>>          visit_type_str(m, &(*obj)->string, "string", &err);
>>          if (err) {
>>              goto out;
>> 
>> Test cases updated for the bug fix.
>
> Not quite right.
>
>> 
>> Fixes alternates to generate a visitor for their implicit enumeration
>> type.  None of them are currently used, obviously.  Example:
>> block-core.json's BlockdevRef now generates
>> visit_type_BlockdevRefKind().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-visit.py                   | 260 +++++++++++++-------------------
>>  tests/qapi-schema/qapi-schema-test.json |   3 -
>>  tests/test-qmp-input-strict.c           |   2 +-
>>  tests/test-qmp-input-visitor.c          |   4 +-
>>  4 files changed, 106 insertions(+), 163 deletions(-)
>
>> +++ b/tests/test-qmp-input-strict.c
>> @@ -167,9 +167,9 @@ static void test_validate_union_flat(TestInputVisitorData *data,
>>  
>>      v = validate_test_init(data,
>>                             "{ 'enum1': 'value1', "
>> +                           "'integer': 41, "
>>                             "'string': 'str', "
>>                             "'boolean': true }");
>> -    /* TODO when generator bug is fixed, add 'integer': 41 */
>>  
>>      visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
>>      g_assert(!err);
>
> Incomplete fix; you need to squash this in to test that cleanup of a
> partial struct is still correct (otherwise, the test sets err for the
> wrong reason of "missing 'integer'" rather than the intended reason of
> "missing 'enum1'"):
>
> diff --git i/tests/test-qmp-input-strict.c w/tests/test-qmp-input-strict.c
> index bfd9d04..4c18096 100644
> --- i/tests/test-qmp-input-strict.c
> +++ w/tests/test-qmp-input-strict.c
> @@ -299,7 +299,7 @@ static void
> test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
>      Visitor *v;
>
>      /* test situation where discriminator field ('enum1' here) is
> missing */
> -    v = validate_test_init(data, "{ 'string': 'c', 'string1': 'd',
> 'string2': 'e' }");
> +    v = validate_test_init(data, "{ 'integer': 42, 'string': 'c',
> 'string1': 'd', 'string2': 'e' }");
>
>      visit_type_UserDefFlatUnion2(v, &tmp, NULL, &err);
>      g_assert(err);

You're right.  I'll squash it in, thanks!