diff mbox

[2/2] qapi: Fix QemuOpts visitor regression on unvisited input

Message ID 20170321031705.22291-3-eblake@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake March 21, 2017, 3:17 a.m. UTC
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(-)

Comments

Michael Roth March 21, 2017, 4:42 a.m. UTC | #1
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
>
Laurent Vivier March 21, 2017, 8:19 a.m. UTC | #2
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
>
diff mbox

Patch

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