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

[Qemu-devel] [PATCH v2 for-2.9 0/3] 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.

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04072.html

Since then:
- fix 'make check' failure, where we had accidentally been relying
on ignoring bogus keys in user_creatable_add_opts() (new patch 2) [Markus]
- commit message and one-word comment improvement, explaining why
test doesn't fail even though it flags problem (patch 1) [Laurent]
- add Reviewed/Tested-by as appropriate

Eric Blake (3):
  tests: Expose regression in QemuOpts visitor
  qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts
  qapi: Fix QemuOpts visitor regression on unvisited input

 qapi/opts-visitor.c       |  6 +++---
 qom/object_interfaces.c   |  7 ++++---
 tests/test-opts-visitor.c | 29 ++++++++++++++++++++++++++---
 3 files changed, 33 insertions(+), 9 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/3] 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;
take the approach already used in 9cb8ef3 of adding a test that
passes (to avoid breaking bisection) but marks with BUG the
behavior that we don't like, so that the actual impact of the
fix in a later patch is easier to see.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

---
v2: s/FIXME/BUG/ in comment, enhance commit message
---
 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..8e0dda5 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);
+    /* BUG: 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 v2 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts

[Eric Blake]

A regression in commit 15c2f669e caused us to silently ignore
excess input to the QemuOpts visitor.  Later, commit ea4641
accidentally abused that situation, by removing "qom-type" and
"id" from the corresponding QDict but leaving them defined in
the QemuOpts, when using the pair of containers to create a
user-defined object. Note that since we are already traversing
two separate items (a QDict and a QemuOpts), we are already
able to flag bogus arguments, as in:

$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found

So the only real concern is that when we re-enable strict checking
in the QemuOpts visitor, we do not want to start flagging the two
leftover keys as unvisited.  Rearrange the code to clean out the
QemuOpts listing in advance, rather than removing items from the
QDict.  Since "qom-type" is usually an automatic implicit default,
we don't have to restore it; but "id" has to be put back (requiring
us to cast away a const).

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: new patch
---
 qom/object_interfaces.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 03a95c3..cc9a694 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -114,7 +114,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
     QDict *pdict;
     Object *obj;
     const char *id = qemu_opts_id(opts);
-    const char *type = qemu_opt_get(opts, "qom-type");
+    char *type = qemu_opt_get_del(opts, "qom-type");

     if (!type) {
         error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
@@ -125,14 +125,15 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
         return NULL;
     }

+    qemu_opts_set_id(opts, NULL);
     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);

+    qemu_opts_set_id(opts, (char *) id);
+    g_free(type);
     QDECREF(pdict);
     return obj;
 }
-- 
2.9.3

[Qemu-devel] [PATCH v2 3/3] 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>
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.
      */
 }

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);
@@ -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);
-    /* 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
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> A regression in commit 15c2f669e caused us to silently ignore
> excess input to the QemuOpts visitor.  Later, commit ea4641
> accidentally abused that situation, by removing "qom-type" and
> "id" from the corresponding QDict but leaving them defined in
> the QemuOpts, when using the pair of containers to create a
> user-defined object. Note that since we are already traversing
> two separate items (a QDict and a QemuOpts), we are already
> able to flag bogus arguments, as in:
>
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
> qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found
>
> So the only real concern is that when we re-enable strict checking
> in the QemuOpts visitor, we do not want to start flagging the two
> leftover keys as unvisited.  Rearrange the code to clean out the
> QemuOpts listing in advance, rather than removing items from the
> QDict.  Since "qom-type" is usually an automatic implicit default,
> we don't have to restore it; but "id" has to be put back (requiring
> us to cast away a const).

This is a yet another example of how actual configuration can easily
diverge from the one in QemuOpts.  Discussed recently in:

Subject: Re: [PATCH 0/2] add writeconfig command on monitor
Message-ID: <87k28qlca9.fsf@dusky.pond.sub.org>
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03476.html

Not putting "qom-type" back here is okay.  Putting it back would also be
okay.  I guess what you prefer depends on your level of OCD.

> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v2: new patch
> ---
>  qom/object_interfaces.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 03a95c3..cc9a694 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -114,7 +114,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
>      QDict *pdict;
>      Object *obj;
>      const char *id = qemu_opts_id(opts);
> -    const char *type = qemu_opt_get(opts, "qom-type");
> +    char *type = qemu_opt_get_del(opts, "qom-type");
>
>      if (!type) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> @@ -125,14 +125,15 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
>          return NULL;
>      }
>
> +    qemu_opts_set_id(opts, NULL);
>      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);
>
> +    qemu_opts_set_id(opts, (char *) id);
> +    g_free(type);
>      QDECREF(pdict);
>      return obj;
>  }

Aside: I dislike how this converts QemuOpts to QDict so
user_creatable_add_type() can use the QDict to guide the visit.
Awkward, as so much code that uses QemuOpts in not entirely
straightforward ways.

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

[Markus Armbruster]

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]

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.

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