[v2,3/3] qapi: Fix QemuOpts visitor regression on unvisited input

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(-)

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

On 03/22/2017 01:47 AM, Markus Armbruster wrote:
> 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.
>>

>> @@ -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?
> 

Both work to my ear, but whether sounds a bit more formal. I can switch,
since...


>> -    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?

Because opts_check_list() is still a no-op, and I didn't want to muck
with how to make it work to catch things earlier.  The late catch is by
virtue of the fact that we track complete coverage by whether the clone
of the QemuOpts still has the key, and the key is not removed until the
list is fully parsed.

> 
>> @@ -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.
> 

...you are correct that this is a spurious hunk, and removing it does
not change the testsuite. v3 coming up.