tests: trivial enhancements for OOB

PATCH In review Peter Xu 2 participants 10 messages Mar 21, 2018 – Mar 22, 2018

Share & tools

Peter Xu

author

2018-03-21 06:55 UTC #0 lore raw

cover tests: trivial enhancements for OOB

First two patches add OOB detection for current qapi-schema tests (which I missed in the OOB series but pointed out by Eric Blake). The 3rd patch addressed one suggestion from Eric too here:

http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03177.html

I tried to batch the commands in a single string buffer but it's not that easy - because currently qtest_async_qmp() (and finally, qmp_fd_sendv()) does not really support multiple qobjects in a single command buffer. Let's put that aside. After all even calling qtest_async_qmp() many times would be really fast, since we are basically filling things to the write buffer very quickly (I believe that's much faster than the IO really flushed to the receiver side).

But, adding the "id" field and check that would be far easier, that's what I did in that last patch.

It's fine even for 2.12, but I'll let people decide.

Please review, thanks.

Peter Xu (3):
tests: let qapi-schema tests detect oob
tests: add oob-test for qapi-schema
tests: more strict command batching test

 tests/Makefile.include                  |  1 +
 tests/qapi-schema/doc-good.out          |  4 ++--
 tests/qapi-schema/ident-with-escape.out |  2 +-
 tests/qapi-schema/indented-expr.out     |  4 ++--
 tests/qapi-schema/oob-test.err          |  0
 tests/qapi-schema/oob-test.exit         |  1 +
 tests/qapi-schema/oob-test.json         |  2 ++
 tests/qapi-schema/oob-test.out          |  6 ++++++
 tests/qapi-schema/qapi-schema-test.out  | 18 +++++++++---------
 tests/qapi-schema/test-qapi.py          |  4 ++--
 tests/qmp-test.c                        |  8 +++++++-
 11 files changed, 33 insertions(+), 17 deletions(-)
 create mode 100644 tests/qapi-schema/oob-test.err
 create mode 100644 tests/qapi-schema/oob-test.exit
 create mode 100644 tests/qapi-schema/oob-test.json
 create mode 100644 tests/qapi-schema/oob-test.out

Signature · 1 line

2.14.3

Peter Xu

author

2018-03-21 06:55 UTC #1 lore raw

PATCH 1/3 tests: let qapi-schema tests detect oob

The allow_oob parameter was passed in but not used in tests. Now reflect that in the tests, so we need to touch up other command testers with that new change.

Signed-off-by Peter Xu <[email protected]>

---
 tests/qapi-schema/doc-good.out          |  4 ++--
 tests/qapi-schema/ident-with-escape.out |  2 +-
 tests/qapi-schema/indented-expr.out     |  4 ++--
 tests/qapi-schema/qapi-schema-test.out  | 18 +++++++++---------
 tests/qapi-schema/test-qapi.py          |  4 ++--
 5 files changed, 16 insertions(+), 16 deletions(-)

tests/qapi-schema/doc-good.out +2 -2

index 430b5a87db..63058b1590 100644
@@ -28,9 +28,9 @@ object q_obj_cmd-arg
     member arg2: str optional=True
     member arg3: bool optional=False
 command cmd q_obj_cmd-arg -> Object
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 command cmd-boxed Object -> None
-   gen=True success_response=True boxed=True
+   gen=True success_response=True boxed=True oob=False
 doc freeform
     body=
 = Section

tests/qapi-schema/ident-with-escape.out +1 -1

index ee3b34e623..82213aa51d 100644
@@ -5,4 +5,4 @@ module ident-with-escape.json
 object q_obj_fooA-arg
     member bar1: str optional=False
 command fooA q_obj_fooA-arg -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False

tests/qapi-schema/indented-expr.out +2 -2

index a79935e8c3..862678f8f4 100644
@@ -3,6 +3,6 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
     prefix QTYPE
 module indented-expr.json
 command eins None -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 command zwei None -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False

tests/qapi-schema/qapi-schema-test.out +9 -9

index 012e7fc06a..4f43370017 100644
@@ -16,7 +16,7 @@ object Empty1
 object Empty2
     base Empty1
 command user_def_cmd0 Empty2 -> Empty2
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 enum QEnumTwo ['value1', 'value2']
     prefix QENUM_TWO
 object UserDefOne

@@ -143,29 +143,29 @@ object UserDefNativeListUnion
     case sizes: q_obj_sizeList-wrapper
     case any: q_obj_anyList-wrapper
 command user_def_cmd None -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_user_def_cmd1-arg
     member ud1a: UserDefOne optional=False
 command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_user_def_cmd2-arg
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
 command guest-get-time q_obj_guest-get-time-arg -> int
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 object q_obj_guest-sync-arg
     member arg: any optional=False
 command guest-sync q_obj_guest-sync-arg -> any
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False
 command boxed-struct UserDefZero -> None
-   gen=True success_response=True boxed=True
+   gen=True success_response=True boxed=True oob=False
 command boxed-union UserDefNativeListUnion -> None
-   gen=True success_response=True boxed=True
+   gen=True success_response=True boxed=True oob=False
 object UserDefOptions
     member i64: intList optional=True
     member u64: uint64List optional=True

@@ -229,4 +229,4 @@ object q_obj___org.qemu_x-command-arg
     member c: __org.qemu_x-Union2 optional=False
     member d: __org.qemu_x-Alt optional=False
 command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
-   gen=True success_response=True boxed=False
+   gen=True success_response=True boxed=False oob=False

tests/qapi-schema/test-qapi.py +2 -2

index 10e68b01d9..c1a144ba29 100644
@@ -45,8 +45,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
                       gen, success_response, boxed, allow_oob):
         print('command %s %s -> %s' % \
               (name, arg_type and arg_type.name, ret_type and ret_type.name))
-        print('   gen=%s success_response=%s boxed=%s' % \
-              (gen, success_response, boxed))
+        print('   gen=%s success_response=%s boxed=%s oob=%s' % \
+              (gen, success_response, boxed, allow_oob))
 
     def visit_event(self, name, info, arg_type, boxed):
         print('event %s %s' % (name, arg_type and arg_type.name))

Signature · 1 line

2.14.3

Eric Blake

Reviewed-by on v1 1/3

2018-03-21 12:47 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/3] tests: let qapi-schema tests detect oob

Quoted 16 lines · Peter Xu

The allow_oob parameter was passed in but not used in tests. Now reflect that in the tests, so we need to touch up other command testers with that new change.
Signed-off-by Peter Xu <[email protected]>
---
  tests/qapi-schema/doc-good.out          |  4 ++--
  tests/qapi-schema/ident-with-escape.out |  2 +-
  tests/qapi-schema/indented-expr.out     |  4 ++--
  tests/qapi-schema/qapi-schema-test.out  | 18 +++++++++---------
  tests/qapi-schema/test-qapi.py          |  4 ++--
  5 files changed, 16 insertions(+), 16 deletions(-)
+++ b/tests/qapi-schema/test-qapi.py

tests/qapi-schema/test-qapi.py class QAPISchemaTestVisitor(QAPISchemaVisitor): · line 49

Quoted 7 lines

tests/qapi-schema/test-qapi.py +2 -2

@@ -45,8 +45,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
                        gen, success_response, boxed, allow_oob):
          print('command %s %s -> %s' % \
                (name, arg_type and arg_type.name, ret_type and ret_type.name))
-        print('   gen=%s success_response=%s boxed=%s' % \
-              (gen, success_response, boxed))
+        print('   gen=%s success_response=%s boxed=%s oob=%s' % \
+              (gen, success_response, boxed, allow_oob))

Reviewed-by Eric Blake <[email protected]>

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Peter Xu

author

2018-03-21 06:55 UTC #3 lore raw

PATCH 2/3 tests: add oob-test for qapi-schema

In reply to Peter Xu

It simply tests the new OOB capability, and make sure the QAPISchema can parse it correctly.

Signed-off-by Peter Xu <[email protected]>

---
 tests/Makefile.include          | 1 +
 tests/qapi-schema/oob-test.err  | 0
 tests/qapi-schema/oob-test.exit | 1 +
 tests/qapi-schema/oob-test.json | 2 ++
 tests/qapi-schema/oob-test.out  | 6 ++++++
 5 files changed, 10 insertions(+)
 create mode 100644 tests/qapi-schema/oob-test.err
 create mode 100644 tests/qapi-schema/oob-test.exit
 create mode 100644 tests/qapi-schema/oob-test.json
 create mode 100644 tests/qapi-schema/oob-test.out

tests/Makefile.include +1 -0

index 0b277036df..059523e2d1 100644
@@ -523,6 +523,7 @@ qapi-schema += missing-comma-object.json
 qapi-schema += missing-type.json
 qapi-schema += nested-struct-data.json
 qapi-schema += non-objects.json
+qapi-schema += oob-test.json
 qapi-schema += pragma-doc-required-crap.json
 qapi-schema += pragma-extra-junk.json
 qapi-schema += pragma-name-case-whitelist-crap.json

tests/qapi-schema/oob-test.err new file +0 -0

new file mode 100644
index 0000000000..e69de29bb2

tests/qapi-schema/oob-test.exit new file +1 -0

new file mode 100644
index 0000000000..573541ac97
@@ -0,0 +1 @@
+0

tests/qapi-schema/oob-test.json new file +2 -0

new file mode 100644
index 0000000000..096c34b2fb
@@ -0,0 +1,2 @@
+# Some Out-Of-Band specific tests
+{ 'command': 'an-oob-command', 'allow-oob': true }

tests/qapi-schema/oob-test.out new file +6 -0

new file mode 100644
index 0000000000..ce5a130dcf
@@ -0,0 +1,6 @@
+object q_empty
+enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
+    prefix QTYPE
+module oob-test.json
+command an-oob-command None -> None
+   gen=True success_response=True boxed=False oob=True

Signature · 1 line

2.14.3

Eric Blake

2018-03-21 12:52 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 2/3] tests: add oob-test for qapi-schema

Quoted 2 lines · Peter Xu

It simply tests the new OOB capability, and make sure the QAPISchema can parse it correctly.

We also want negative tests that cover any new error messages in the qapi generator (such as 'allow-oob':'bad' diagnosing a non-bool, or 'allow-oob':false giving an error message that false is already the default such that only 'allow-oob':true makes sense).

Also, it's often easier to merge the positive test into the giant existing qapi-schema-test.json rather than creating a new positive test.

tests/qapi-schema/oob-test.out line 6

Quoted 8 lines

+++ b/tests/qapi-schema/oob-test.out

tests/qapi-schema/oob-test.out new file +6 -0

@@ -0,0 +1,6 @@
+object q_empty
+enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
+    prefix QTYPE
+module oob-test.json
+command an-oob-command None -> None
+   gen=True success_response=True boxed=False oob=True

At any rate, the positive test addition is good. I may beat you to submitting a v2 patch that covers the error messages that I'm thinking of.

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Peter Xu

author

2018-03-22 03:44 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 2/3] tests: add oob-test for qapi-schema

Quoted 11 lines · Eric Blake

Quoted 2 lines · Peter Xu
It simply tests the new OOB capability, and make sure the QAPISchema can parse it correctly.
We also want negative tests that cover any new error messages in the qapi generator (such as 'allow-oob':'bad' diagnosing a non-bool, or 'allow-oob':false giving an error message that false is already the default such that only 'allow-oob':true makes sense).
Also, it's often easier to merge the positive test into the giant existing qapi-schema-test.json rather than creating a new positive test.

It seems that for one QAPI schema negative test only the first error will be reported, then the script halts (so the 2nd negative test in the same .json won't be reported). To make it simple - I'll put the positive test into qapi-schema-test.json, and add one negative test in oob-test.json to check again strings (though in the code I'll only allow 'false').

(Actually I'll need one liner change to check that value when parsing
since it was not checked before...)

Quoted 12 lines

tests/qapi-schema/oob-test.out line 6
Quoted 9 lines
+++ b/tests/qapi-schema/oob-test.out
tests/qapi-schema/oob-test.out new file +6 -0
@@ -0,0 +1,6 @@
+object q_empty
+enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
+    prefix QTYPE
+module oob-test.json
+command an-oob-command None -> None
+   gen=True success_response=True boxed=False oob=True
At any rate, the positive test addition is good. I may beat you to submitting a v2 patch that covers the error messages that I'm thinking of.

Will post another version (and I'll see whether I should queue more to fix existing reported OOB problems). Thanks,

Signature · 1 line

Peter Xu

Peter Xu

author

2018-03-21 06:55 UTC #6 lore raw

PATCH 3/3 tests: more strict command batching test

In reply to Peter Xu

Add "id" fields to the commands, and check that the command returns are in order.

Signed-off-by Peter Xu <[email protected]>

---
 tests/qmp-test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

tests/qmp-test.c +7 -1

index 07c0b87e27..c861c3b550 100644
@@ -83,6 +83,7 @@ static void test_qmp_protocol(void)
     const QListEntry *entry;
     QString *qstr;
     int i;
+    char buf[128];
 
     qts = qtest_init_without_qmp_handshake(common_args);

@@ -150,7 +151,10 @@ static void test_qmp_protocol(void)
      * best-effort test.
      */
     for (i = 0; i < 16; i++) {
-        qtest_async_qmp(qts, "{ 'execute': 'query-version' }");
+        snprintf(buf, sizeof(buf) - 1, "{ 'execute': 'query-version', "
+                 "'id': %d }", i);
+        buf[sizeof(buf) - 1] = '\0';
+        qtest_async_qmp(qts, buf);
     }
     /* Verify the replies to make sure no command is dropped. */
     for (i = 0; i < 16; i++) {

@@ -158,6 +162,8 @@ static void test_qmp_protocol(void)
         /* It should never be dropped.  Each of them should be a reply. */
         g_assert(qdict_haskey(resp, "return"));
         g_assert(!qdict_haskey(resp, "event"));
+        g_assert(qdict_haskey(resp, "id"));
+        g_assert(qdict_get_int(resp, "id") == i);
         QDECREF(resp);
     }

Signature · 1 line

2.14.3

Eric Blake

2018-03-21 12:55 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 3/3] tests: more strict command batching test

Quoted 11 lines · Peter Xu

Add "id" fields to the commands, and check that the command returns are in order.
Signed-off-by Peter Xu <[email protected]>
---
  tests/qmp-test.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
+++ b/tests/qmp-test.c

tests/qmp-test.c static void test_qmp_protocol(void) · line 86

Quoted 4 lines

tests/qmp-test.c +1 -0

@@ -83,6 +83,7 @@ static void test_qmp_protocol(void)
      const QListEntry *entry;
      QString *qstr;
      int i;
+    char buf[128];

Eww, we shouldn't need this.

tests/qmp-test.c static void test_qmp_protocol(void) · line 157

Quoted 12 lines

tests/qmp-test.c +4 -1

      qts = qtest_init_without_qmp_handshake(common_args);

@@ -150,7 +151,10 @@ static void test_qmp_protocol(void)
       * best-effort test.
       */
      for (i = 0; i < 16; i++) {
-        qtest_async_qmp(qts, "{ 'execute': 'query-version' }");
+        snprintf(buf, sizeof(buf) - 1, "{ 'execute': 'query-version', "
+                 "'id': %d }", i);
+        buf[sizeof(buf) - 1] = '\0';
+        qtest_async_qmp(qts, buf);

snprintf() of JSON strings is prone to failures especially when the JSON string is reinterpreted as a printf argument in qtest_async_qmp. Better is to let qtest_async_qmp() directly do the formatting, as in:

qtest_async_qmp(qts, "{ 'execute':'query-version', 'id':%d}", i);

And then I was right - you don't need buf.

Otherwise the addition is good.

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Peter Xu

author

2018-03-22 03:48 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 3/3] tests: more strict command batching test

Quoted 34 lines · Eric Blake

Quoted 11 lines · Peter Xu
Add "id" fields to the commands, and check that the command returns are in order.
Signed-off-by Peter Xu <[email protected]>
---
  tests/qmp-test.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)
+++ b/tests/qmp-test.c
tests/qmp-test.c static void test_qmp_protocol(void) · line 86
Quoted 4 lines
tests/qmp-test.c +1 -0
@@ -83,6 +83,7 @@ static void test_qmp_protocol(void)
      const QListEntry *entry;
      QString *qstr;
      int i;
+    char buf[128];
Eww, we shouldn't need this.
tests/qmp-test.c static void test_qmp_protocol(void) · line 157
Quoted 10 lines
tests/qmp-test.c +4 -1
      qts = qtest_init_without_qmp_handshake(common_args);
@@ -150,7 +151,10 @@ static void test_qmp_protocol(void)
       * best-effort test.
       */
      for (i = 0; i < 16; i++) {
-        qtest_async_qmp(qts, "{ 'execute': 'query-version' }");
+        snprintf(buf, sizeof(buf) - 1, "{ 'execute': 'query-version', "
+                 "'id': %d }", i);
+        buf[sizeof(buf) - 1] = '\0';
+        qtest_async_qmp(qts, buf);
snprintf() of JSON strings is prone to failures especially when the JSON string is reinterpreted as a printf argument in qtest_async_qmp. Better is to let qtest_async_qmp() directly do the formatting, as in:

Quoted 3 lines

qtest_async_qmp(qts, "{ 'execute':'query-version', 'id':%d}", i);
And then I was right - you don't need buf.

Ouch. :)

Will fix that. Thanks,

Quoted 7 lines

Otherwise the addition is good.

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Signature · 1 line

Peter Xu

Eric Blake

2018-03-21 12:45 UTC #9 lore raw

In reply to Peter Xu

Quoted 14 lines · Peter Xu

First two patches add OOB detection for current qapi-schema tests (which I missed in the OOB series but pointed out by Eric Blake). The 3rd patch addressed one suggestion from Eric too here:
http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03177.html
I tried to batch the commands in a single string buffer but it's not that easy - because currently qtest_async_qmp() (and finally, qmp_fd_sendv()) does not really support multiple qobjects in a single command buffer. Let's put that aside. After all even calling qtest_async_qmp() many times would be really fast, since we are basically filling things to the write buffer very quickly (I believe that's much faster than the IO really flushed to the receiver side).

Quoted 4 lines

But, adding the "id" field and check that would be far easier, that's what I did in that last patch.
It's fine even for 2.12, but I'll let people decide.

I consider added testsuite coverage of a new feature to be a bug fix (the feature was incomplete if the testsuite doesn't prevent regressions in the feature) and safe for freeze (the testsuite changes don't impact the main binary, so they can't break anything), so I'm happy to queue this through my qapi tree for 2.12 once it is reviewed.

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org