Message ID | 1442872682-6523-6-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > --- > 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(-) > > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json > index c904db4..7593ecb 100644 > --- a/tests/qapi-schema/qapi-schema-test.json > +++ b/tests/qapi-schema/qapi-schema-test.json > @@ -65,6 +65,14 @@ > { 'struct': 'UserDefC', > 'data': { 'string1': 'str', 'string2': 'str' } } > > +# for testing use of 'number' within alternates > +{ 'alternate': 'AltOne', 'data': { 's': 'str', 'b': 'bool' } } > +{ 'alternate': 'AltTwo', 'data': { 's': 'str', 'n': 'number' } } > +{ 'alternate': 'AltThree', 'data': { 'n': 'number', 's': 'str' } } > +{ 'alternate': 'AltFour', 'data': { 's': 'str', 'i': 'int' } } > +{ 'alternate': 'AltFive', 'data': { 'i': 'int', 'n': 'number' } } > +{ 'alternate': 'AltSix', 'data': { 'n': 'number', 'i': 'int' } } > + > # for testing native lists > { 'union': 'UserDefNativeListUnion', > 'data': { 'integer': ['int'], > diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out > index 28a0b3c..de29a45 100644 > --- a/tests/qapi-schema/qapi-schema-test.out > +++ b/tests/qapi-schema/qapi-schema-test.out > @@ -53,6 +53,30 @@ object :obj-user_def_cmd2-arg > object :obj-user_def_cmd3-arg > member a: int optional=False > member b: int optional=True > +alternate AltFive > + case i: int > + case n: number > +enum AltFiveKind ['i', 'n'] > +alternate AltFour > + case s: str > + case i: int > +enum AltFourKind ['s', 'i'] > +alternate AltOne > + case s: str > + case b: bool > +enum AltOneKind ['s', 'b'] > +alternate AltSix > + case n: number > + case i: int > +enum AltSixKind ['n', 'i'] > +alternate AltThree > + case n: number > + case s: str > +enum AltThreeKind ['n', 's'] > +alternate AltTwo > + case s: str > + case n: number > +enum AltTwoKind ['s', 'n'] > event EVENT_A None > event EVENT_B None > event EVENT_C :obj-EVENT_C-arg > diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c > index 61715b3..cd41847 100644 > --- a/tests/test-qmp-input-visitor.c > +++ 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); 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? > + > + v = visitor_input_test_init(data, "'string'"); > + > + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); > + g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S); > + g_assert_cmpstr(tmp->s, ==, "string"); > + qapi_free_UserDefAlternate(tmp); > + tmp = NULL; > + > + v = visitor_input_test_init(data, "false"); > + > + visit_type_UserDefAlternate(v, &tmp, NULL, &err); > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_UserDefAlternate(tmp); Okay, here you merely make the test of existing UserDefAlternate more thorough. Could be a separate patch, but keeping it in this one is fine with me, too. > +} > + > +static void test_visitor_in_alternate_number(TestInputVisitorData *data, > + const void *unused) > +{ > + Visitor *v; > + Error *err = NULL; > + AltOne *one = NULL; > + AltTwo *two = NULL; > + AltThree *three = NULL; > + AltFour *four = NULL; > + AltFive *five = NULL; > + AltSix *six = NULL; > + > + /* Parsing an int */ > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltOne(v, &one, NULL, &err); > + g_assert(err); > + qapi_free_AltOne(one); > + one = NULL; Similar to the new last test in test_visitor_in_alternate(). Not sure that one's needed. > + > + /* 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); */ > + 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. > + > + /* 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; > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltFour(v, &four, NULL, &error_abort); > + g_assert_cmpint(four->kind, ==, ALT_FOUR_KIND_I); > + g_assert_cmpint(four->i, ==, 42); > + qapi_free_AltFour(four); > + one = NULL; > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltFive(v, &five, NULL, &error_abort); > + g_assert_cmpint(five->kind, ==, ALT_FIVE_KIND_I); > + g_assert_cmpint(five->i, ==, 42); > + qapi_free_AltFive(five); > + one = NULL; > + > + v = visitor_input_test_init(data, "42"); > + visit_type_AltSix(v, &six, NULL, &error_abort); > + g_assert_cmpint(six->kind, ==, ALT_SIX_KIND_I); > + g_assert_cmpint(six->i, ==, 42); > + qapi_free_AltSix(six); > + one = NULL; > + > + /* Parsing a double */ > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltOne(v, &one, NULL, &err); > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_AltOne(one); > + one = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltTwo(v, &two, NULL, &error_abort); > + g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); > + g_assert_cmpfloat(two->n, ==, 42.5); > + qapi_free_AltTwo(two); > + two = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltThree(v, &three, NULL, &error_abort); > + g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N); > + g_assert_cmpfloat(three->n, ==, 42.5); > + qapi_free_AltThree(three); > + three = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltFour(v, &four, NULL, &err); > + g_assert(err); > + error_free(err); > + err = NULL; > + qapi_free_AltFour(four); > + four = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltFive(v, &five, NULL, &error_abort); > + g_assert_cmpint(five->kind, ==, ALT_FIVE_KIND_N); > + g_assert_cmpfloat(five->n, ==, 42.5); > + qapi_free_AltFive(five); > + five = NULL; > + > + v = visitor_input_test_init(data, "42.5"); > + visit_type_AltSix(v, &six, NULL, &error_abort); > + g_assert_cmpint(six->kind, ==, ALT_SIX_KIND_N); > + g_assert_cmpint(six->n, ==, 42.5); > + qapi_free_AltSix(six); > + six = NULL; Reading this, I had to refer back to the definition of AltOne, ..., AltSix all the time. Let's rename them to AltStrBool, AltStrNum, ..., AltNumInt. > } > > static void test_native_list_integer_helper(TestInputVisitorData *data, > @@ -720,6 +841,8 @@ int main(int argc, char **argv) > &in_visitor_data, test_visitor_in_alternate); > input_visitor_test_add("/visitor/input/errors", > &in_visitor_data, test_visitor_in_errors); > + input_visitor_test_add("/visitor/input/alternate-number", > + &in_visitor_data, test_visitor_in_alternate_number); > input_visitor_test_add("/visitor/input/native_list/int", > &in_visitor_data, > test_visitor_in_native_list_int);
On 09/24/2015 08:36 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >> --- >> 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> writes: > On 09/24/2015 08:36 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >>> --- >>> 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.
On 09/24/2015 10:29 AM, Markus Armbruster wrote: >>>> + >>>> + /* 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. Actually, the test for AltThree succeeds only by accident. There are two bugs at play; when I fix the first bug (order shouldn't matter: AltTwo and AltThree should parse identically), then the second bug is finally exposed (integers aren't being parsed as numbers, in either AltTwo or AltThree). But I can certainly rework the FIXMEs both here and on the first fix (19/46) to make it more obvious what the second fix (20/46) is good for.
On 09/24/2015 10:29 AM, Markus Armbruster wrote: >>> 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. > Hmm. When I delete the initializer, valgrind starts warning at a later point in my series: /visitor/input/errors: OK /visitor/input/alternate-number: ==16451== Conditional jump or move depends on uninitialised value(s) ==16451== at 0x14AA7E: visit_start_implicit_struct (qapi-visit-core.c:36) ==16451== by 0x111D80: visit_type_AltStrBool (test-qapi-visit.c:207) ==16451== by 0x10EC16: test_visitor_in_alternate_number (test-qmp-input-visitor.c:426) ==16451== by 0x4EBBB92: test_case_run (gtestutils.c:2124) ==16451== by 0x4EBBB92: g_test_run_suite_internal (gtestutils.c:2185) ==16451== by 0x4EBBD5A: g_test_run_suite_internal (gtestutils.c:2196) ==16451== by 0x4EBBD5A: g_test_run_suite_internal (gtestutils.c:2196) ==16451== by 0x4EBC0DA: g_test_run_suite (gtestutils.c:2249) ==16451== by 0x4EBC110: g_test_run (gtestutils.c:1553) ==16451== by 0x111406: main (test-qmp-input-visitor.c:901) ==16451== Uninitialised value was created by a stack allocation ==16451== at 0x10EBA1: test_visitor_in_alternate_number (test-qmp-input-visitor.c:413) I don't know if that means my changes are introducing the problems, or if we really DO want to require users to pass in an initial NULL pointer. I've run out of time to investigate today, but it's not turning out to be as trivial as I had hoped.
Eric Blake <eblake@redhat.com> writes: > On 09/24/2015 10:29 AM, Markus Armbruster wrote: > >>>>> + >>>>> + /* 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. > > Actually, the test for AltThree succeeds only by accident. There are two > bugs at play; when I fix the first bug (order shouldn't matter: AltTwo > and AltThree should parse identically), then the second bug is finally > exposed (integers aren't being parsed as numbers, in either AltTwo or > AltThree). But I can certainly rework the FIXMEs both here and on the > first fix (19/46) to make it more obvious what the second fix (20/46) is > good for. Yes, please. I find the FIXME quoted above confusing, because it makes me look for problems exposed by this test when there are none.
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index c904db4..7593ecb 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -65,6 +65,14 @@ { 'struct': 'UserDefC', 'data': { 'string1': 'str', 'string2': 'str' } } +# for testing use of 'number' within alternates +{ 'alternate': 'AltOne', 'data': { 's': 'str', 'b': 'bool' } } +{ 'alternate': 'AltTwo', 'data': { 's': 'str', 'n': 'number' } } +{ 'alternate': 'AltThree', 'data': { 'n': 'number', 's': 'str' } } +{ 'alternate': 'AltFour', 'data': { 's': 'str', 'i': 'int' } } +{ 'alternate': 'AltFive', 'data': { 'i': 'int', 'n': 'number' } } +{ 'alternate': 'AltSix', 'data': { 'n': 'number', 'i': 'int' } } + # for testing native lists { 'union': 'UserDefNativeListUnion', 'data': { 'integer': ['int'], diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 28a0b3c..de29a45 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -53,6 +53,30 @@ object :obj-user_def_cmd2-arg object :obj-user_def_cmd3-arg member a: int optional=False member b: int optional=True +alternate AltFive + case i: int + case n: number +enum AltFiveKind ['i', 'n'] +alternate AltFour + case s: str + case i: int +enum AltFourKind ['s', 'i'] +alternate AltOne + case s: str + case b: bool +enum AltOneKind ['s', 'b'] +alternate AltSix + case n: number + case i: int +enum AltSixKind ['n', 'i'] +alternate AltThree + case n: number + case s: str +enum AltThreeKind ['n', 's'] +alternate AltTwo + case s: str + case n: number +enum AltTwoKind ['s', 'n'] event EVENT_A None event EVENT_B None event EVENT_C :obj-EVENT_C-arg diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 61715b3..cd41847 100644 --- a/tests/test-qmp-input-visitor.c +++ 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; 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); g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I); g_assert_cmpint(tmp->i, ==, 42); qapi_free_UserDefAlternate(tmp); + tmp = NULL; + + v = visitor_input_test_init(data, "'string'"); + + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort); + g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S); + g_assert_cmpstr(tmp->s, ==, "string"); + qapi_free_UserDefAlternate(tmp); + tmp = NULL; + + v = visitor_input_test_init(data, "false"); + + visit_type_UserDefAlternate(v, &tmp, NULL, &err); + g_assert(err); + error_free(err); + err = NULL; + qapi_free_UserDefAlternate(tmp); +} + +static void test_visitor_in_alternate_number(TestInputVisitorData *data, + const void *unused) +{ + Visitor *v; + Error *err = NULL; + AltOne *one = NULL; + AltTwo *two = NULL; + AltThree *three = NULL; + AltFour *four = NULL; + AltFive *five = NULL; + AltSix *six = NULL; + + /* Parsing an int */ + + v = visitor_input_test_init(data, "42"); + visit_type_AltOne(v, &one, NULL, &err); + g_assert(err); + qapi_free_AltOne(one); + one = NULL; + + /* FIXME: Integers should parse as numbers */ + v = visitor_input_test_init(data, "42"); + visit_type_AltTwo(v, &two, NULL, &err); + g_assert(err); + error_free(err); + err = NULL; + qapi_free_AltTwo(two); + one = NULL; + + /* FIXME: Order of alternate should not affect semantics */ + 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; + + v = visitor_input_test_init(data, "42"); + visit_type_AltFour(v, &four, NULL, &error_abort); + g_assert_cmpint(four->kind, ==, ALT_FOUR_KIND_I); + g_assert_cmpint(four->i, ==, 42); + qapi_free_AltFour(four); + one = NULL; + + v = visitor_input_test_init(data, "42"); + visit_type_AltFive(v, &five, NULL, &error_abort); + g_assert_cmpint(five->kind, ==, ALT_FIVE_KIND_I); + g_assert_cmpint(five->i, ==, 42); + qapi_free_AltFive(five); + one = NULL; + + v = visitor_input_test_init(data, "42"); + visit_type_AltSix(v, &six, NULL, &error_abort); + g_assert_cmpint(six->kind, ==, ALT_SIX_KIND_I); + g_assert_cmpint(six->i, ==, 42); + qapi_free_AltSix(six); + one = NULL; + + /* Parsing a double */ + + v = visitor_input_test_init(data, "42.5"); + visit_type_AltOne(v, &one, NULL, &err); + g_assert(err); + error_free(err); + err = NULL; + qapi_free_AltOne(one); + one = NULL; + + v = visitor_input_test_init(data, "42.5"); + visit_type_AltTwo(v, &two, NULL, &error_abort); + g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); + g_assert_cmpfloat(two->n, ==, 42.5); + qapi_free_AltTwo(two); + two = NULL; + + v = visitor_input_test_init(data, "42.5"); + visit_type_AltThree(v, &three, NULL, &error_abort); + g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N); + g_assert_cmpfloat(three->n, ==, 42.5); + qapi_free_AltThree(three); + three = NULL; + + v = visitor_input_test_init(data, "42.5"); + visit_type_AltFour(v, &four, NULL, &err); + g_assert(err); + error_free(err); + err = NULL; + qapi_free_AltFour(four); + four = NULL; + + v = visitor_input_test_init(data, "42.5"); + visit_type_AltFive(v, &five, NULL, &error_abort); + g_assert_cmpint(five->kind, ==, ALT_FIVE_KIND_N); + g_assert_cmpfloat(five->n, ==, 42.5); + qapi_free_AltFive(five); + five = NULL; + + v = visitor_input_test_init(data, "42.5"); + visit_type_AltSix(v, &six, NULL, &error_abort); + g_assert_cmpint(six->kind, ==, ALT_SIX_KIND_N); + g_assert_cmpint(six->n, ==, 42.5); + qapi_free_AltSix(six); + six = NULL; } static void test_native_list_integer_helper(TestInputVisitorData *data, @@ -720,6 +841,8 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_alternate); input_visitor_test_add("/visitor/input/errors", &in_visitor_data, test_visitor_in_errors); + input_visitor_test_add("/visitor/input/alternate-number", + &in_visitor_data, test_visitor_in_alternate_number); input_visitor_test_add("/visitor/input/native_list/int", &in_visitor_data, test_visitor_in_native_list_int);
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 <eblake@redhat.com> --- 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(-)