Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates

[Eric Blake]

On 09/24/2015 08:36 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> Add some testsuite exposure for use of a 'number' as part of
>> an alternate.  The current state of the tree has a few bugs
>> exposed by this: our input parser depends on the ordering of
>> how the qapi schema declared the alternate, and the parser
>> does not accept integers for a 'number' in an alternate even
>> though it does for numbers outside of an alternate.
>>
>> Mixing 'int' and 'number' in the same alternate is unusual,
>> since both are supplied by json-numbers, but there does not
>> seem to be a technical reason to forbid it given that our
>> json lexer distinguishes between json-numbers that can be
>> represented as an int vs. those that cannot.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>  tests/qapi-schema/qapi-schema-test.json |   8 ++
>>  tests/qapi-schema/qapi-schema-test.out  |  24 ++++++
>>  tests/test-qmp-input-visitor.c          | 129 
>> +++++++++++++++++++++++++++++++-
>>  3 files changed, 158 insertions(+), 3 deletions(-)
>>

>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -368,15 +368,136 @@ static void 
>> test_visitor_in_alternate(TestInputVisitorData *data,
>>  {
>>      Visitor *v;
>>      Error *err = NULL;
>> -    UserDefAlternate *tmp;
>> +    UserDefAlternate *tmp = NULL;
> 
> Any particular reason for adding the initializer?
> 
>>
>>      v = visitor_input_test_init(data, "42");
>>
>> -    visit_type_UserDefAlternate(v, &tmp, NULL, &err);
>> -    g_assert(err == NULL);
>> +    visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);

Hmm - I don't know if we have a clear contract for what happens if you
call visit_type_FOO on an uninitialized pointer.  It may have been
succeeding by mere luck.

> 
> The pattern
> 
>        foo(..., &err);
>        g_assert(err == NULL);
> 
> is pretty common in tests.  Can't see what it buys us over straight
> &error_abort.  Perhaps I'll spatch it away.
> 
>>      g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>>      g_assert_cmpint(tmp->i, ==, 42);
>>      qapi_free_UserDefAlternate(tmp);
>> +    tmp = NULL;
> 
> Why do you need to clear tmp?

If we were succeeding on a single call by mere luck where tmp started
life as all 0 due to stack contents, but the second call has tmp
pointing to stale memory, then that would be an obvious reason.  I'll
have to revisit what happens, because I don't recall any specific reason
for why I did this other than the symmetry of making sure each parse had
clean state (that is, I don't recall a crash happening if I didn't do
it, and haven't yet tested under valgrind to see if we are provably
using memory incorrectly if we don't initialize).


>> +
>> +    /* FIXME: Integers should parse as numbers */
> 
> Suggest to augment or replace this comment...
> 
>> +    v = visitor_input_test_init(data, "42");
>> +    visit_type_AltTwo(v, &two, NULL, &err);
> 
> ... with
> 
>        /* FIXME g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); */
>        /* FIXME g_assert_cmpfloat(two->n, ==, 42); */

Ah, to better document what the test will look like in the future when
the bugs are fixed. Sure, I can do that.

> 
>> +    g_assert(err);
>> +    error_free(err);
>> +    err = NULL;
>> +    qapi_free_AltTwo(two);
>> +    one = NULL;
> 
> *chuckle*  Why do you clear one here?  More of the same below.

Too much copy-and-paste.  Will fix.

> 
>> +
>> +    /* FIXME: Order of alternate should not affect semantics */
> 
> Inhowfar does it affect semantics?  Or asked differently: what exactly
> is wrong with this test now?
> 
>> +    v = visitor_input_test_init(data, "42");
>> +    visit_type_AltThree(v, &three, NULL, &error_abort);
>> +    g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N);
>> +    g_assert_cmpfloat(three->n, ==, 42);
>> +    qapi_free_AltThree(three);
>> +    one = NULL;


AltTwo and AltThree are ostensibly the same struct (two branches, one
for 'str' and one for 'number', just in a different order), but they
parsed differently (AltTwo failed, AltThree succeeded).  The bug is
fixed later when the order of the branch declaration no longer impacts
the result of the parse.

> 
> Reading this, I had to refer back to the definition of AltOne, ...,
> AltSix all the time.  Let's rename them to AltStrBool, AltStrNum, ...,
> AltNumInt.

Good idea, will do.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature