* [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys @ 2017-03-21 3:17 Eric Blake 2017-03-21 3:17 ` [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor Eric Blake ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Eric Blake @ 2017-03-21 3:17 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier Reported to me off-list by Laurent Vivier, who found the problem while working on https://bugzilla.redhat.com/1433193 Broken since 2.7, but the fix is a one-liner (pointing out my embarrassing mistake of mis-converting a pre-decrement operator); as a bug fix, it still qualifies for 2.9 in spite of hard freeze, on the other hand, as the regression was not introduced in 2.9, I also understand if it is postponed. Eric Blake (2): tests: Expose regression in QemuOpts visitor qapi: Fix QemuOpts visitor regression on unvisited input qapi/opts-visitor.c | 6 +++--- tests/test-opts-visitor.c | 29 ++++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 6 deletions(-) -- 2.9.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 3:17 [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Eric Blake @ 2017-03-21 3:17 ` Eric Blake 2017-03-21 4:41 ` Michael Roth 2017-03-21 9:01 ` Laurent Vivier 2017-03-21 3:17 ` [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake 2017-03-21 9:28 ` [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Markus Armbruster 2 siblings, 2 replies; 14+ messages in thread From: Eric Blake @ 2017-03-21 3:17 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier, Michael Roth Commit 15c2f669e broke the ability of the QemuOpts visitor to flag extra input parameters, but the regression went unnoticed because of missing testsuite coverage. Add a test to cover this. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-opts-visitor.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c index 2238f8e..a47c344 100644 --- a/tests/test-opts-visitor.c +++ b/tests/test-opts-visitor.c @@ -247,6 +247,24 @@ test_opts_range_beyond(void) qemu_opts_del(opts); } +static void +test_opts_dict_unvisited(void) +{ + QemuOpts *opts; + Visitor *v; + UserDefOptions *userdef; + + opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false, + &error_abort); + + v = opts_visitor_new(opts); + /* FIXME: bogus should be diagnosed */ + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); + visit_free(v); + qemu_opts_del(opts); + qapi_free_UserDefOptions(userdef); +} + int main(int argc, char **argv) { @@ -343,6 +361,8 @@ main(int argc, char **argv) g_test_add_func("/visitor/opts/range/beyond", test_opts_range_beyond); + g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited); + g_test_run(); return 0; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 3:17 ` [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor Eric Blake @ 2017-03-21 4:41 ` Michael Roth 2017-03-21 9:01 ` Laurent Vivier 1 sibling, 0 replies; 14+ messages in thread From: Michael Roth @ 2017-03-21 4:41 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: armbru, lvivier Quoting Eric Blake (2017-03-20 22:17:04) > Commit 15c2f669e broke the ability of the QemuOpts visitor to > flag extra input parameters, but the regression went unnoticed > because of missing testsuite coverage. Add a test to cover this. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > tests/test-opts-visitor.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index 2238f8e..a47c344 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -247,6 +247,24 @@ test_opts_range_beyond(void) > qemu_opts_del(opts); > } > > +static void > +test_opts_dict_unvisited(void) > +{ > + QemuOpts *opts; > + Visitor *v; > + UserDefOptions *userdef; > + > + opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false, > + &error_abort); > + > + v = opts_visitor_new(opts); > + /* FIXME: bogus should be diagnosed */ > + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > + visit_free(v); > + qemu_opts_del(opts); > + qapi_free_UserDefOptions(userdef); > +} > + > int > main(int argc, char **argv) > { > @@ -343,6 +361,8 @@ main(int argc, char **argv) > g_test_add_func("/visitor/opts/range/beyond", > test_opts_range_beyond); > > + g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited); > + > g_test_run(); > return 0; > } > -- > 2.9.3 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 3:17 ` [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor Eric Blake 2017-03-21 4:41 ` Michael Roth @ 2017-03-21 9:01 ` Laurent Vivier 2017-03-21 13:21 ` Eric Blake 1 sibling, 1 reply; 14+ messages in thread From: Laurent Vivier @ 2017-03-21 9:01 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: armbru, Michael Roth On 21/03/2017 04:17, Eric Blake wrote: > Commit 15c2f669e broke the ability of the QemuOpts visitor to > flag extra input parameters, but the regression went unnoticed > because of missing testsuite coverage. Add a test to cover this. I don't know where I'm wrong, but when I run this test without the fix it never fails. Laurent > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/test-opts-visitor.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index 2238f8e..a47c344 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -247,6 +247,24 @@ test_opts_range_beyond(void) > qemu_opts_del(opts); > } > > +static void > +test_opts_dict_unvisited(void) > +{ > + QemuOpts *opts; > + Visitor *v; > + UserDefOptions *userdef; > + > + opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false, > + &error_abort); > + > + v = opts_visitor_new(opts); > + /* FIXME: bogus should be diagnosed */ > + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > + visit_free(v); > + qemu_opts_del(opts); > + qapi_free_UserDefOptions(userdef); > +} > + > int > main(int argc, char **argv) > { > @@ -343,6 +361,8 @@ main(int argc, char **argv) > g_test_add_func("/visitor/opts/range/beyond", > test_opts_range_beyond); > > + g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited); > + > g_test_run(); > return 0; > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 9:01 ` Laurent Vivier @ 2017-03-21 13:21 ` Eric Blake 2017-03-21 13:33 ` Laurent Vivier 0 siblings, 1 reply; 14+ messages in thread From: Eric Blake @ 2017-03-21 13:21 UTC (permalink / raw) To: Laurent Vivier, qemu-devel; +Cc: armbru, Michael Roth [-- Attachment #1: Type: text/plain, Size: 1131 bytes --] On 03/21/2017 04:01 AM, Laurent Vivier wrote: > On 21/03/2017 04:17, Eric Blake wrote: >> Commit 15c2f669e broke the ability of the QemuOpts visitor to >> flag extra input parameters, but the regression went unnoticed >> because of missing testsuite coverage. Add a test to cover this. > > I don't know where I'm wrong, but when I run this test without the fix > it never fails. Intentional: >> + v = opts_visitor_new(opts); >> + /* FIXME: bogus should be diagnosed */ >> + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); The test is written with a FIXME here, then updated in the next patch to remove the fixme and adjust the condition to what we really want, so that 'make check-unit' is not broken in the meantime. A similar approach was taken by Markus in commit 9cb8ef3 (add a test that passes but shows undesirable behavior with a BUG: note), which also gets fixed up by my 2/2. Maybe I should use BUG: instead of FIXME; but it all goes away in the next patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 13:21 ` Eric Blake @ 2017-03-21 13:33 ` Laurent Vivier 2017-03-21 15:36 ` Eric Blake 0 siblings, 1 reply; 14+ messages in thread From: Laurent Vivier @ 2017-03-21 13:33 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: armbru, Michael Roth On 21/03/2017 14:21, Eric Blake wrote: > On 03/21/2017 04:01 AM, Laurent Vivier wrote: >> On 21/03/2017 04:17, Eric Blake wrote: >>> Commit 15c2f669e broke the ability of the QemuOpts visitor to >>> flag extra input parameters, but the regression went unnoticed >>> because of missing testsuite coverage. Add a test to cover this. >> >> I don't know where I'm wrong, but when I run this test without the fix >> it never fails. > > Intentional: > > >>> + v = opts_visitor_new(opts); >>> + /* FIXME: bogus should be diagnosed */ >>> + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > > The test is written with a FIXME here, then updated in the next patch to > remove the fixme and adjust the condition to what we really want, so > that 'make check-unit' is not broken in the meantime. OK. Why don't you reverse the patch order to have a commit to apply the fix and a commit to apply the test (fully)? Like this, it easy to not apply the fix and only the test to check the test really detects the problem and the fix really fix it (it's what I've tried to do)... and the "make check" is never broken. Thanks, Laurent ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 13:33 ` Laurent Vivier @ 2017-03-21 15:36 ` Eric Blake 2017-03-21 16:01 ` Markus Armbruster 0 siblings, 1 reply; 14+ messages in thread From: Eric Blake @ 2017-03-21 15:36 UTC (permalink / raw) To: Laurent Vivier, qemu-devel; +Cc: armbru, Michael Roth [-- Attachment #1: Type: text/plain, Size: 1884 bytes --] On 03/21/2017 08:33 AM, Laurent Vivier wrote: > On 21/03/2017 14:21, Eric Blake wrote: >> On 03/21/2017 04:01 AM, Laurent Vivier wrote: >>> On 21/03/2017 04:17, Eric Blake wrote: >>>> Commit 15c2f669e broke the ability of the QemuOpts visitor to >>>> flag extra input parameters, but the regression went unnoticed >>>> because of missing testsuite coverage. Add a test to cover this. >>> >>> I don't know where I'm wrong, but when I run this test without the fix >>> it never fails. >> >> Intentional: >> >> >>>> + v = opts_visitor_new(opts); >>>> + /* FIXME: bogus should be diagnosed */ >>>> + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); >> >> The test is written with a FIXME here, then updated in the next patch to >> remove the fixme and adjust the condition to what we really want, so >> that 'make check-unit' is not broken in the meantime. > > OK. > > Why don't you reverse the patch order to have a commit to apply the fix > and a commit to apply the test (fully)? > > Like this, it easy to not apply the fix and only the test to check the > test really detects the problem and the fix really fix it (it's what > I've tried to do)... and the "make check" is never broken. Applying just the one-liner fix to qapi/opts-visitor.c in isolation already causes a 'make check' failure; a careful read of 2/2 shows that I was adjusting the expected output of two separate tests, added over two separate commits, but both with a BUG/FIXME tag. I'm not opposed to reworking the series to apply the testsuite coverage after the bug fix, if that is deemed necessary, but would like an opinion from Markus which way he would prefer (as this is the code he maintains) before causing myself artificial churn. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 15:36 ` Eric Blake @ 2017-03-21 16:01 ` Markus Armbruster 2017-03-21 16:10 ` Laurent Vivier 0 siblings, 1 reply; 14+ messages in thread From: Markus Armbruster @ 2017-03-21 16:01 UTC (permalink / raw) To: Eric Blake; +Cc: Laurent Vivier, qemu-devel, Michael Roth Eric Blake <eblake@redhat.com> writes: > On 03/21/2017 08:33 AM, Laurent Vivier wrote: >> On 21/03/2017 14:21, Eric Blake wrote: >>> On 03/21/2017 04:01 AM, Laurent Vivier wrote: >>>> On 21/03/2017 04:17, Eric Blake wrote: >>>>> Commit 15c2f669e broke the ability of the QemuOpts visitor to >>>>> flag extra input parameters, but the regression went unnoticed >>>>> because of missing testsuite coverage. Add a test to cover this. >>>> >>>> I don't know where I'm wrong, but when I run this test without the fix >>>> it never fails. >>> >>> Intentional: >>> >>> >>>>> + v = opts_visitor_new(opts); >>>>> + /* FIXME: bogus should be diagnosed */ >>>>> + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); >>> >>> The test is written with a FIXME here, then updated in the next patch to >>> remove the fixme and adjust the condition to what we really want, so >>> that 'make check-unit' is not broken in the meantime. >> >> OK. >> >> Why don't you reverse the patch order to have a commit to apply the fix >> and a commit to apply the test (fully)? >> >> Like this, it easy to not apply the fix and only the test to check the >> test really detects the problem and the fix really fix it (it's what >> I've tried to do)... and the "make check" is never broken. > > Applying just the one-liner fix to qapi/opts-visitor.c in isolation > already causes a 'make check' failure; a careful read of 2/2 shows that > I was adjusting the expected output of two separate tests, added over > two separate commits, but both with a BUG/FIXME tag. I'm not opposed to > reworking the series to apply the testsuite coverage after the bug fix, > if that is deemed necessary, but would like an opinion from Markus which > way he would prefer (as this is the code he maintains) before causing > myself artificial churn. I really, really like to start with the problem statement (test case), not the solution. I also like see the solution's effect in the update to the test case. Since "make check" must not fail, and our (rickety) testing framework doesn't let us express "this is expected to fail", the problem statement can't be a failing test case, but has to be a test case expecting the broken behavior. If that's not good enough to convince you that it detects the problem, I recommend to git-checkout tests/ after the fix into the tree before the fix. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor 2017-03-21 16:01 ` Markus Armbruster @ 2017-03-21 16:10 ` Laurent Vivier 0 siblings, 0 replies; 14+ messages in thread From: Laurent Vivier @ 2017-03-21 16:10 UTC (permalink / raw) To: Markus Armbruster, Eric Blake; +Cc: qemu-devel, Michael Roth On 21/03/2017 17:01, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 03/21/2017 08:33 AM, Laurent Vivier wrote: >>> On 21/03/2017 14:21, Eric Blake wrote: >>>> On 03/21/2017 04:01 AM, Laurent Vivier wrote: >>>>> On 21/03/2017 04:17, Eric Blake wrote: >>>>>> Commit 15c2f669e broke the ability of the QemuOpts visitor to >>>>>> flag extra input parameters, but the regression went unnoticed >>>>>> because of missing testsuite coverage. Add a test to cover this. >>>>> >>>>> I don't know where I'm wrong, but when I run this test without the fix >>>>> it never fails. >>>> >>>> Intentional: >>>> >>>> >>>>>> + v = opts_visitor_new(opts); >>>>>> + /* FIXME: bogus should be diagnosed */ >>>>>> + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); >>>> >>>> The test is written with a FIXME here, then updated in the next patch to >>>> remove the fixme and adjust the condition to what we really want, so >>>> that 'make check-unit' is not broken in the meantime. >>> >>> OK. >>> >>> Why don't you reverse the patch order to have a commit to apply the fix >>> and a commit to apply the test (fully)? >>> >>> Like this, it easy to not apply the fix and only the test to check the >>> test really detects the problem and the fix really fix it (it's what >>> I've tried to do)... and the "make check" is never broken. >> >> Applying just the one-liner fix to qapi/opts-visitor.c in isolation >> already causes a 'make check' failure; a careful read of 2/2 shows that >> I was adjusting the expected output of two separate tests, added over >> two separate commits, but both with a BUG/FIXME tag. I'm not opposed to >> reworking the series to apply the testsuite coverage after the bug fix, >> if that is deemed necessary, but would like an opinion from Markus which >> way he would prefer (as this is the code he maintains) before causing >> myself artificial churn. > > I really, really like to start with the problem statement (test case), > not the solution. I also like see the solution's effect in the update > to the test case. > > Since "make check" must not fail, and our (rickety) testing framework > doesn't let us express "this is expected to fail", the problem statement > can't be a failing test case, but has to be a test case expecting the > broken behavior. > > If that's not good enough to convince you that it detects the problem, I > recommend to git-checkout tests/ after the fix into the tree before the > fix. > OK. I'm convinced :) Thanks, Laurent ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input 2017-03-21 3:17 [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Eric Blake 2017-03-21 3:17 ` [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor Eric Blake @ 2017-03-21 3:17 ` Eric Blake 2017-03-21 4:42 ` Michael Roth 2017-03-21 8:19 ` Laurent Vivier 2017-03-21 9:28 ` [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Markus Armbruster 2 siblings, 2 replies; 14+ messages in thread From: Eric Blake @ 2017-03-21 3:17 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier, qemu-stable, Michael Roth An off-by-one in commit 15c2f669e meant that we were failing to check for unparsed input in all QemuOpts visitors. Recent testsuite additions show that fixing the obvious bug with bogus fields will also fix the case of an incomplete list visit; update the tests to match the new behavior. Simple testcase: ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -numa node,size=1g failed to diagnose that 'size' is not a valid argument to -numa, and now once again reports: qemu-system-x86_64: -numa node,size=1g: Invalid parameter 'size' CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> --- qapi/opts-visitor.c | 6 +++--- tests/test-opts-visitor.c | 15 +++++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 026d25b..b54da81 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -164,7 +164,7 @@ opts_check_struct(Visitor *v, Error **errp) GHashTableIter iter; GQueue *any; - if (ov->depth > 0) { + if (ov->depth > 1) { return; } @@ -276,8 +276,8 @@ static void opts_check_list(Visitor *v, Error **errp) { /* - * FIXME should set error when unvisited elements remain. Mostly - * harmless, as the generated visits always visit all elements. + * Unvisited list elements will be reported later when checking if + * unvisited struct members remain. */ } diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c index a47c344..1766919 100644 --- a/tests/test-opts-visitor.c +++ b/tests/test-opts-visitor.c @@ -175,6 +175,7 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data) static void test_opts_range_unvisited(void) { + Error *err = NULL; intList *list = NULL; intList *tail; QemuOpts *opts; @@ -199,10 +200,11 @@ test_opts_range_unvisited(void) g_assert_cmpint(tail->value, ==, 1); tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list)); g_assert(tail); - visit_check_list(v, &error_abort); /* BUG: unvisited tail not reported */ + visit_check_list(v, &error_abort); /* unvisited tail ignored until... */ visit_end_list(v, (void **)&list); - visit_check_struct(v, &error_abort); + visit_check_struct(v, &err); /* ...here */ + error_free_or_abort(&err); visit_end_struct(v, NULL); qapi_free_intList(list); @@ -239,7 +241,7 @@ test_opts_range_beyond(void) error_free_or_abort(&err); visit_end_list(v, (void **)&list); - visit_check_struct(v, &error_abort); + visit_check_struct(v, &err); visit_end_struct(v, NULL); qapi_free_intList(list); @@ -250,6 +252,7 @@ test_opts_range_beyond(void) static void test_opts_dict_unvisited(void) { + Error *err = NULL; QemuOpts *opts; Visitor *v; UserDefOptions *userdef; @@ -258,11 +261,11 @@ test_opts_dict_unvisited(void) &error_abort); v = opts_visitor_new(opts); - /* FIXME: bogus should be diagnosed */ - visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); + visit_type_UserDefOptions(v, NULL, &userdef, &err); + error_free_or_abort(&err); visit_free(v); qemu_opts_del(opts); - qapi_free_UserDefOptions(userdef); + g_assert(!userdef); } int -- 2.9.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input 2017-03-21 3:17 ` [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake @ 2017-03-21 4:42 ` Michael Roth 2017-03-21 8:19 ` Laurent Vivier 1 sibling, 0 replies; 14+ messages in thread From: Michael Roth @ 2017-03-21 4:42 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: armbru, lvivier, qemu-stable Quoting Eric Blake (2017-03-20 22:17:05) > An off-by-one in commit 15c2f669e meant that we were failing to > check for unparsed input in all QemuOpts visitors. Recent testsuite > additions show that fixing the obvious bug with bogus fields will > also fix the case of an incomplete list visit; update the tests to > match the new behavior. > > Simple testcase: > > ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -numa node,size=1g > > failed to diagnose that 'size' is not a valid argument to -numa, and > now once again reports: > > qemu-system-x86_64: -numa node,size=1g: Invalid parameter 'size' > > CC: qemu-stable@nongnu.org > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > qapi/opts-visitor.c | 6 +++--- > tests/test-opts-visitor.c | 15 +++++++++------ > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index 026d25b..b54da81 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -164,7 +164,7 @@ opts_check_struct(Visitor *v, Error **errp) > GHashTableIter iter; > GQueue *any; > > - if (ov->depth > 0) { > + if (ov->depth > 1) { > return; > } > > @@ -276,8 +276,8 @@ static void > opts_check_list(Visitor *v, Error **errp) > { > /* > - * FIXME should set error when unvisited elements remain. Mostly > - * harmless, as the generated visits always visit all elements. > + * Unvisited list elements will be reported later when checking if > + * unvisited struct members remain. > */ > } > > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index a47c344..1766919 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -175,6 +175,7 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data) > static void > test_opts_range_unvisited(void) > { > + Error *err = NULL; > intList *list = NULL; > intList *tail; > QemuOpts *opts; > @@ -199,10 +200,11 @@ test_opts_range_unvisited(void) > g_assert_cmpint(tail->value, ==, 1); > tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list)); > g_assert(tail); > - visit_check_list(v, &error_abort); /* BUG: unvisited tail not reported */ > + visit_check_list(v, &error_abort); /* unvisited tail ignored until... */ > visit_end_list(v, (void **)&list); > > - visit_check_struct(v, &error_abort); > + visit_check_struct(v, &err); /* ...here */ > + error_free_or_abort(&err); > visit_end_struct(v, NULL); > > qapi_free_intList(list); > @@ -239,7 +241,7 @@ test_opts_range_beyond(void) > error_free_or_abort(&err); > visit_end_list(v, (void **)&list); > > - visit_check_struct(v, &error_abort); > + visit_check_struct(v, &err); > visit_end_struct(v, NULL); > > qapi_free_intList(list); > @@ -250,6 +252,7 @@ test_opts_range_beyond(void) > static void > test_opts_dict_unvisited(void) > { > + Error *err = NULL; > QemuOpts *opts; > Visitor *v; > UserDefOptions *userdef; > @@ -258,11 +261,11 @@ test_opts_dict_unvisited(void) > &error_abort); > > v = opts_visitor_new(opts); > - /* FIXME: bogus should be diagnosed */ > - visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > + visit_type_UserDefOptions(v, NULL, &userdef, &err); > + error_free_or_abort(&err); > visit_free(v); > qemu_opts_del(opts); > - qapi_free_UserDefOptions(userdef); > + g_assert(!userdef); > } > > int > -- > 2.9.3 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input 2017-03-21 3:17 ` [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake 2017-03-21 4:42 ` Michael Roth @ 2017-03-21 8:19 ` Laurent Vivier 1 sibling, 0 replies; 14+ messages in thread From: Laurent Vivier @ 2017-03-21 8:19 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: armbru, qemu-stable, Michael Roth On 21/03/2017 04:17, Eric Blake wrote: > An off-by-one in commit 15c2f669e meant that we were failing to > check for unparsed input in all QemuOpts visitors. Recent testsuite > additions show that fixing the obvious bug with bogus fields will > also fix the case of an incomplete list visit; update the tests to > match the new behavior. > > Simple testcase: > > ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -numa node,size=1g > > failed to diagnose that 'size' is not a valid argument to -numa, and > now once again reports: > > qemu-system-x86_64: -numa node,size=1g: Invalid parameter 'size' > > CC: qemu-stable@nongnu.org > Signed-off-by: Eric Blake <eblake@redhat.com> Tested-by: Laurent Vivier <lvivier@redhat.com> > --- > qapi/opts-visitor.c | 6 +++--- > tests/test-opts-visitor.c | 15 +++++++++------ > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index 026d25b..b54da81 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -164,7 +164,7 @@ opts_check_struct(Visitor *v, Error **errp) > GHashTableIter iter; > GQueue *any; > > - if (ov->depth > 0) { > + if (ov->depth > 1) { > return; > } > > @@ -276,8 +276,8 @@ static void > opts_check_list(Visitor *v, Error **errp) > { > /* > - * FIXME should set error when unvisited elements remain. Mostly > - * harmless, as the generated visits always visit all elements. > + * Unvisited list elements will be reported later when checking if > + * unvisited struct members remain. > */ > } > > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index a47c344..1766919 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -175,6 +175,7 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data) > static void > test_opts_range_unvisited(void) > { > + Error *err = NULL; > intList *list = NULL; > intList *tail; > QemuOpts *opts; > @@ -199,10 +200,11 @@ test_opts_range_unvisited(void) > g_assert_cmpint(tail->value, ==, 1); > tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list)); > g_assert(tail); > - visit_check_list(v, &error_abort); /* BUG: unvisited tail not reported */ > + visit_check_list(v, &error_abort); /* unvisited tail ignored until... */ > visit_end_list(v, (void **)&list); > > - visit_check_struct(v, &error_abort); > + visit_check_struct(v, &err); /* ...here */ > + error_free_or_abort(&err); > visit_end_struct(v, NULL); > > qapi_free_intList(list); > @@ -239,7 +241,7 @@ test_opts_range_beyond(void) > error_free_or_abort(&err); > visit_end_list(v, (void **)&list); > > - visit_check_struct(v, &error_abort); > + visit_check_struct(v, &err); > visit_end_struct(v, NULL); > > qapi_free_intList(list); > @@ -250,6 +252,7 @@ test_opts_range_beyond(void) > static void > test_opts_dict_unvisited(void) > { > + Error *err = NULL; > QemuOpts *opts; > Visitor *v; > UserDefOptions *userdef; > @@ -258,11 +261,11 @@ test_opts_dict_unvisited(void) > &error_abort); > > v = opts_visitor_new(opts); > - /* FIXME: bogus should be diagnosed */ > - visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > + visit_type_UserDefOptions(v, NULL, &userdef, &err); > + error_free_or_abort(&err); > visit_free(v); > qemu_opts_del(opts); > - qapi_free_UserDefOptions(userdef); > + g_assert(!userdef); > } > > int > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys 2017-03-21 3:17 [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Eric Blake 2017-03-21 3:17 ` [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor Eric Blake 2017-03-21 3:17 ` [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake @ 2017-03-21 9:28 ` Markus Armbruster 2017-03-21 13:23 ` Eric Blake 2 siblings, 1 reply; 14+ messages in thread From: Markus Armbruster @ 2017-03-21 9:28 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel, lvivier, Igor Mammedov Eric Blake <eblake@redhat.com> writes: > Reported to me off-list by Laurent Vivier, who found the > problem while working on https://bugzilla.redhat.com/1433193 > Broken since 2.7, but the fix is a one-liner (pointing out my > embarrassing mistake of mis-converting a pre-decrement operator); > as a bug fix, it still qualifies for 2.9 in spite of hard freeze, > on the other hand, as the regression was not introduced in 2.9, > I also understand if it is postponed. > > Eric Blake (2): > tests: Expose regression in QemuOpts visitor > qapi: Fix QemuOpts visitor regression on unvisited input > > qapi/opts-visitor.c | 6 +++--- > tests/test-opts-visitor.c | 29 ++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 6 deletions(-) Regresses $ qemu-system-x86_64 -object memory-backend-ram,id=mem1,size=4k qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k: Invalid parameter 'id' I guess the culprit is commit 3a4641: pdict = qemu_opts_to_qdict(opts, NULL); qdict_del(pdict, "qom-type"); qdict_del(pdict, "id"); v = opts_visitor_new(opts); obj = user_creatable_add_type(type, id, pdict, v, errp); visit_free(v); This deletes "qom-type" and "id" from pdict, but not opts. The deletion makes user_creatable_add_type() skip visiting them as intended, but it also makes visit_check_struct() fail, because the opts visitor still expects the two to be visited. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys 2017-03-21 9:28 ` [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Markus Armbruster @ 2017-03-21 13:23 ` Eric Blake 0 siblings, 0 replies; 14+ messages in thread From: Eric Blake @ 2017-03-21 13:23 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, lvivier, Igor Mammedov [-- Attachment #1: Type: text/plain, Size: 1878 bytes --] On 03/21/2017 04:28 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Reported to me off-list by Laurent Vivier, who found the >> problem while working on https://bugzilla.redhat.com/1433193 >> Broken since 2.7, but the fix is a one-liner (pointing out my >> embarrassing mistake of mis-converting a pre-decrement operator); >> as a bug fix, it still qualifies for 2.9 in spite of hard freeze, >> on the other hand, as the regression was not introduced in 2.9, >> I also understand if it is postponed. >> >> Eric Blake (2): >> tests: Expose regression in QemuOpts visitor >> qapi: Fix QemuOpts visitor regression on unvisited input >> >> qapi/opts-visitor.c | 6 +++--- >> tests/test-opts-visitor.c | 29 ++++++++++++++++++++++++++--- >> 2 files changed, 29 insertions(+), 6 deletions(-) > > Regresses > > $ qemu-system-x86_64 -object memory-backend-ram,id=mem1,size=4k > qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k: Invalid parameter 'id' D'oh - you caught me running just 'make check-unit' instead of the longer 'make check' (which includes the failing 'make check-qtest'). > > I guess the culprit is commit 3a4641: > > pdict = qemu_opts_to_qdict(opts, NULL); > qdict_del(pdict, "qom-type"); > qdict_del(pdict, "id"); > > v = opts_visitor_new(opts); > obj = user_creatable_add_type(type, id, pdict, v, errp); > visit_free(v); > > This deletes "qom-type" and "id" from pdict, but not opts. The deletion > makes user_creatable_add_type() skip visiting them as intended, but it > also makes visit_check_struct() fail, because the opts visitor still > expects the two to be visited. I'll come up with a solution, and post v2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-21 16:10 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-21 3:17 [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Eric Blake 2017-03-21 3:17 ` [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor Eric Blake 2017-03-21 4:41 ` Michael Roth 2017-03-21 9:01 ` Laurent Vivier 2017-03-21 13:21 ` Eric Blake 2017-03-21 13:33 ` Laurent Vivier 2017-03-21 15:36 ` Eric Blake 2017-03-21 16:01 ` Markus Armbruster 2017-03-21 16:10 ` Laurent Vivier 2017-03-21 3:17 ` [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake 2017-03-21 4:42 ` Michael Roth 2017-03-21 8:19 ` Laurent Vivier 2017-03-21 9:28 ` [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys Markus Armbruster 2017-03-21 13:23 ` Eric Blake
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.