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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

I strongly suspect the "input" visitors are assignment-like: they store
something, and don't care what value they overwrite.  Let's keep that in
mind when we retrofit a contract.

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

I suspect it's just as dead as clearing an assignment's LHS before the
actual assignment is :)

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

Supports my idea that these assignments are useless.

If they are, let's just drop them.

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

Then nothing is wrong with this test case, and the FIXME doesn't belong
here.

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