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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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>
> Tested-by: Laurent Vivier <lvivier@redhat.com>
>
> ---
> v2: trivial rebase to comment tweak in patch 1
> ---
>  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.

Non-native speaker question: if or whether?

>       */
>  }
>
> diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
> index 8e0dda5..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);

How come unvisited tails are diagnosed late?

> @@ -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);

This looks wrong.  Either you expect an error or not.  If you do,
error_free_or_abort() seems missing.  If you don't, the hunk needs to be
dropped.

>      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);
> -    /* BUG: 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