[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alte
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates |
Date: |
Thu, 24 Sep 2015 10:00:42 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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
- Re: [Qemu-devel] [PATCH v5 01/46] qapi: Sort qapi-schema tests, (continued)
[Qemu-devel] [PATCH v5 04/46] qapi: Add tests for empty unions, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 06/46] qapi: Improve 'include' error message, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 05/46] qapi: Test use of 'number' within alternates, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/21