[Qemu-devel] [PATCH 0/3] tests: trivial enhancements for OOB

[Qemu-devel] [PATCH 0/3] tests: trivial enhancements for OOB

[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).

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

-- 
2.14.3

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

[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 <peterx@redhat.com>
---
 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(-)

diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 430b5a87db..63058b1590 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -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
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index ee3b34e623..82213aa51d 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -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
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index a79935e8c3..862678f8f4 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -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
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 012e7fc06a..4f43370017 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -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
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 10e68b01d9..c1a144ba29 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -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))
-- 
2.14.3

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

[Eric Blake]

On 03/21/2018 01:55 AM, Peter Xu wrote:
> 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 <peterx@redhat.com>
> ---
>   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
> @@ -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 <eblake@redhat.com>

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

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

[Peter Xu]

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 0b277036df..059523e2d1 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -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
diff --git a/tests/qapi-schema/oob-test.err b/tests/qapi-schema/oob-test.err
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/oob-test.exit b/tests/qapi-schema/oob-test.exit
new file mode 100644
index 0000000000..573541ac97
--- /dev/null
+++ b/tests/qapi-schema/oob-test.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/oob-test.json b/tests/qapi-schema/oob-test.json
new file mode 100644
index 0000000000..096c34b2fb
--- /dev/null
+++ b/tests/qapi-schema/oob-test.json
@@ -0,0 +1,2 @@
+# Some Out-Of-Band specific tests
+{ 'command': 'an-oob-command', 'allow-oob': true }
diff --git a/tests/qapi-schema/oob-test.out b/tests/qapi-schema/oob-test.out
new file mode 100644
index 0000000000..ce5a130dcf
--- /dev/null
+++ b/tests/qapi-schema/oob-test.out
@@ -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
-- 
2.14.3

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

[Eric Blake]

On 03/21/2018 01:55 AM, Peter Xu wrote:
> 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.

> +++ b/tests/qapi-schema/oob-test.out
> @@ -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.

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

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

[Peter Xu]

On Wed, Mar 21, 2018 at 07:52:06AM -0500, Eric Blake wrote:
> On 03/21/2018 01:55 AM, Peter Xu wrote:
> > 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...)

> 
> > +++ b/tests/qapi-schema/oob-test.out
> > @@ -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,

-- 
Peter Xu

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

[Peter Xu]

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

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 07c0b87e27..c861c3b550 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -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);
     }
 
-- 
2.14.3

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

[Eric Blake]

On 03/21/2018 01:55 AM, Peter Xu wrote:
> Add "id" fields to the commands, and check that the command returns are
> in order.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   tests/qmp-test.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 

> +++ b/tests/qmp-test.c
> @@ -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.

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

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

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

[Peter Xu]

On Wed, Mar 21, 2018 at 07:55:19AM -0500, Eric Blake wrote:
> On 03/21/2018 01:55 AM, Peter Xu wrote:
> > Add "id" fields to the commands, and check that the command returns are
> > in order.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   tests/qmp-test.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> 
> > +++ b/tests/qmp-test.c
> > @@ -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.
> 
> >       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.

Ouch. :)

Will fix that. Thanks,

> 
> Otherwise the addition is good.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/3] tests: trivial enhancements for OOB

[Eric Blake]

On 03/21/2018 01:55 AM, Peter Xu wrote:
> 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.

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.

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