[Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys

[Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys

[Eric Blake]

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

[Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Eric Blake]

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

[Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input

[Eric Blake]

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

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Michael Roth]

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
> 

Re: [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input

[Michael Roth]

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
> 

Re: [Qemu-devel] [PATCH 2/2] qapi: Fix QemuOpts visitor regression on unvisited input

[Laurent Vivier]

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
> 

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Laurent Vivier]

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;
>  }
> 

Re: [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH for-2.9 0/2] Fix QemuOpts regression on bogus keys

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Laurent Vivier]

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

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Eric Blake]

[-- 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 --]

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Laurent Vivier]

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