[PATCH 0/2] enhance iotest 223 coverage

[PATCH 0/2] enhance iotest 223 coverage

[Eric Blake]

Commit 506902c6 dropped non-iothread coverage in order to test iothread,
better is to run things twice.  In doing this, I found it easier to
edit the test when the log shows what commands were triggering various
responses.

Eric Blake (2):
  tests: Make iotest 223 easier to edit
  tests: More iotest 223 improvements

 tests/qemu-iotests/223     | 114 ++++++++++++++++++++++++------
 tests/qemu-iotests/223.out | 138 +++++++++++++++++++++++++++++++++++++
 2 files changed, 231 insertions(+), 21 deletions(-)

-- 
2.21.0

[PATCH 1/2] tests: Make iotest 223 easier to edit

[Eric Blake]

Log the QMP input to qemu, not just the QMP output.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/223     | 48 +++++++++++++++++++++-----------------
 tests/qemu-iotests/223.out | 40 +++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 2ba3d8124b4f..2bcf922df8c4 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -58,6 +58,12 @@ run_qemu()
                           | _filter_actual_image_size
 }

+qemu_cmd()
+{
+    echo " $1" | _filter_testdir
+    _send_qemu_cmd $QEMU_HANDLE "$@"
+}
+
 echo
 echo "=== Create partially sparse image, then add dirty bitmaps ==="
 echo
@@ -110,39 +116,39 @@ echo "=== End dirty bitmaps, and start serving image over NBD ==="
 echo

 _launch_qemu -object iothread,id=io0 2> >(_filter_nbd)
-
-# Intentionally provoke some errors as well, to check error handling
 silent=
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
+
+# Intentionally provoke some errors as well, to check error handling
+qemu_cmd '{"execute":"qmp_capabilities"}' "return"
+qemu_cmd '{"execute":"blockdev-add",
   "arguments":{"driver":"qcow2", "node-name":"n",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-blockdev-set-iothread",
+qemu_cmd '{"execute":"x-blockdev-set-iothread",
   "arguments":{"node-name":"n", "iothread":"io0"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
+qemu_cmd '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"n"}}' "error" # Attempt add without server
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
+qemu_cmd '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
     "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
+qemu_cmd '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
     "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server
 $QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "bitmap":"b"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"n"}}' "error" # Attempt to export same name twice
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2",
   "bitmap":"b2"}}' "error" # enabled vs. read-only
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2",
   "bitmap":"b3"}}' "error" # Missing bitmap
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add",
+qemu_cmd '{"execute":"nbd-server-add",
   "arguments":{"device":"n", "name":"n2", "writable":true,
   "bitmap":"b2"}}' "return"
 $QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
@@ -172,15 +178,15 @@ echo
 echo "=== End qemu NBD server ==="
 echo

-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+qemu_cmd '{"execute":"nbd-server-remove",
   "arguments":{"name":"n"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+qemu_cmd '{"execute":"nbd-server-remove",
   "arguments":{"name":"n2"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove",
+qemu_cmd '{"execute":"nbd-server-remove",
   "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "error" # Again
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return"
+qemu_cmd '{"execute":"nbd-server-stop"}' "return"
+qemu_cmd '{"execute":"nbd-server-stop"}' "error" # Again
+qemu_cmd '{"execute":"quit"}' "return"
 wait=yes _cleanup_qemu

 echo
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 23b34fcd202e..8895ea838cbb 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -24,19 +24,50 @@ wrote 2097152/2097152 bytes at offset 2097152

 === End dirty bitmaps, and start serving image over NBD ===

+ {"execute":"qmp_capabilities"}
 {"return": {}}
+ {"execute":"blockdev-add",
+  "arguments":{"driver":"qcow2", "node-name":"n",
+    "file":{"driver":"file", "filename":"TEST_DIR/t.qcow2"}}}
 {"return": {}}
+ {"execute":"x-blockdev-set-iothread",
+  "arguments":{"node-name":"n", "iothread":"io0"}}
 {"return": {}}
+ {"execute":"block-dirty-bitmap-disable",
+  "arguments":{"node":"n", "name":"b"}}
 {"return": {}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}
 {"error": {"class": "GenericError", "desc": "NBD server not running"}}
+ {"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"TEST_DIR/nbd"}}}}
 {"return": {}}
+ {"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"TEST_DIR/nbd1"}}}}
 {"error": {"class": "GenericError", "desc": "NBD server already running"}}
 exports available: 0
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "bitmap":"b"}}
 {"return": {}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"nosuch"}}
 {"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}
 {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b2"}}
 {"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b3"}}
 {"error": {"class": "GenericError", "desc": "Bitmap 'b3' is not found"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2", "writable":true,
+  "bitmap":"b2"}}
 {"return": {}}
 exports available: 2
  export: 'n'
@@ -84,11 +115,20 @@ read 2097152/2097152 bytes at offset 2097152

 === End qemu NBD server ===

+ {"execute":"nbd-server-remove",
+  "arguments":{"name":"n"}}
 {"return": {}}
+ {"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}
 {"return": {}}
+ {"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}
 {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
+ {"execute":"nbd-server-stop"}
 {"return": {}}
+ {"execute":"nbd-server-stop"}
 {"error": {"class": "GenericError", "desc": "NBD server not running"}}
+ {"execute":"quit"}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}

-- 
2.21.0

[PATCH 2/2] tests: More iotest 223 improvements

[Eric Blake]

Run the test twice, once without iothreads, and again with, for more
coverage of both setups.

Suggested-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/223     | 66 +++++++++++++++++++++++++
 tests/qemu-iotests/223.out | 98 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)

diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 2bcf922df8c4..2aa428d406f5 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -123,6 +123,72 @@ qemu_cmd '{"execute":"qmp_capabilities"}' "return"
 qemu_cmd '{"execute":"blockdev-add",
   "arguments":{"driver":"qcow2", "node-name":"n",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
+qemu_cmd '{"execute":"block-dirty-bitmap-disable",
+  "arguments":{"node":"n", "name":"b"}}' "return"
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "error" # Attempt add without server
+qemu_cmd '{"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return"
+qemu_cmd '{"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server
+$QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"n", "bitmap":"b"}}' "return"
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}' "error" # Attempt to export same name twice
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b2"}}' "error" # enabled vs. read-only
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b3"}}' "error" # Missing bitmap
+qemu_cmd '{"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2", "writable":true,
+  "bitmap":"b2"}}' "return"
+$QEMU_NBD_PROG -L -k "$TEST_DIR/nbd"
+
+echo
+echo "=== Contrast normal status to large granularity dirty-bitmap ==="
+echo
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+IMG="driver=nbd,export=n,server.type=unix,server.path=$TEST_DIR/nbd"
+$QEMU_IO -r -c 'r -P 0x22 512 512' -c 'r -P 0 512k 512k' -c 'r -P 0x11 1m 1m' \
+  -c 'r -P 0x33 2m 2m' --image-opts "$IMG" | _filter_qemu_io
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG" | _filter_qemu_img_map
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map
+
+echo
+echo "=== Contrast to small granularity dirty-bitmap ==="
+echo
+
+IMG="driver=nbd,export=n2,server.type=unix,server.path=$TEST_DIR/nbd"
+$QEMU_IMG map --output=json --image-opts \
+  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+
+echo
+echo "=== End qemu NBD server ==="
+echo
+
+qemu_cmd '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n"}}' "return"
+qemu_cmd '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}' "return"
+qemu_cmd '{"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean
+qemu_cmd '{"execute":"nbd-server-stop"}' "return"
+qemu_cmd '{"execute":"nbd-server-stop"}' "error" # Again
+
+echo
+echo "=== Repeat, but using iothreads ==="
+echo
+
 qemu_cmd '{"execute":"x-blockdev-set-iothread",
   "arguments":{"node-name":"n", "iothread":"io0"}}' "return"
 qemu_cmd '{"execute":"block-dirty-bitmap-disable",
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 8895ea838cbb..9dcab03ef9c8 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -30,6 +30,104 @@ wrote 2097152/2097152 bytes at offset 2097152
   "arguments":{"driver":"qcow2", "node-name":"n",
     "file":{"driver":"file", "filename":"TEST_DIR/t.qcow2"}}}
 {"return": {}}
+ {"execute":"block-dirty-bitmap-disable",
+  "arguments":{"node":"n", "name":"b"}}
+{"return": {}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}
+{"error": {"class": "GenericError", "desc": "NBD server not running"}}
+ {"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"TEST_DIR/nbd"}}}}
+{"return": {}}
+ {"execute":"nbd-server-start",
+  "arguments":{"addr":{"type":"unix",
+    "data":{"path":"TEST_DIR/nbd1"}}}}
+{"error": {"class": "GenericError", "desc": "NBD server already running"}}
+exports available: 0
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "bitmap":"b"}}
+{"return": {}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"nosuch"}}
+{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n"}}
+{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b2"}}
+{"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2",
+  "bitmap":"b3"}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'b3' is not found"}}
+ {"execute":"nbd-server-add",
+  "arguments":{"device":"n", "name":"n2", "writable":true,
+  "bitmap":"b2"}}
+{"return": {}}
+exports available: 2
+ export: 'n'
+  size:  4194304
+  flags: 0x58f ( readonly flush fua df multi cache )
+  min block: 1
+  opt block: 4096
+  max block: 33554432
+  available meta contexts: 2
+   base:allocation
+   qemu:dirty-bitmap:b
+ export: 'n2'
+  size:  4194304
+  flags: 0xced ( flush fua trim zeroes df cache fast-zero )
+  min block: 1
+  opt block: 4096
+  max block: 33554432
+  available meta contexts: 2
+   base:allocation
+   qemu:dirty-bitmap:b2
+
+=== Contrast normal status to large granularity dirty-bitmap ===
+
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 2097152/2097152 bytes at offset 2097152
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 4096, "length": 1044480, "depth": 0, "zero": true, "data": false, "offset": OFFSET},
+{ "start": 1048576, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": false},
+{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
+
+=== Contrast to small granularity dirty-bitmap ===
+
+[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
+{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
+
+=== End qemu NBD server ===
+
+ {"execute":"nbd-server-remove",
+  "arguments":{"name":"n"}}
+{"return": {}}
+ {"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}
+{"return": {}}
+ {"execute":"nbd-server-remove",
+  "arguments":{"name":"n2"}}
+{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}}
+ {"execute":"nbd-server-stop"}
+{"return": {}}
+ {"execute":"nbd-server-stop"}
+{"error": {"class": "GenericError", "desc": "NBD server not running"}}
+
+=== Repeat, but using iothreads ===
+
  {"execute":"x-blockdev-set-iothread",
   "arguments":{"node-name":"n", "iothread":"io0"}}
 {"return": {}}
-- 
2.21.0

Re: [PATCH 0/2] enhance iotest 223 coverage

[Vladimir Sementsov-Ogievskiy]

24.09.2019 17:35, Eric Blake wrote:
> Commit 506902c6 dropped non-iothread coverage in order to test iothread,
> better is to run things twice.  In doing this, I found it easier to
> edit the test when the log shows what commands were triggering various
> responses.

Did you consider adding -iothread to tests/qemu-iotests/check instead, to be
able to run any (or some) tests with or without iothread?

> 
> Eric Blake (2):
>    tests: Make iotest 223 easier to edit
>    tests: More iotest 223 improvements
> 
>   tests/qemu-iotests/223     | 114 ++++++++++++++++++++++++------
>   tests/qemu-iotests/223.out | 138 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 231 insertions(+), 21 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/2] enhance iotest 223 coverage

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1222 bytes --]

On 9/24/19 2:26 PM, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2019 17:35, Eric Blake wrote:
>> Commit 506902c6 dropped non-iothread coverage in order to test iothread,
>> better is to run things twice.  In doing this, I found it easier to
>> edit the test when the log shows what commands were triggering various
>> responses.
> 
> Did you consider adding -iothread to tests/qemu-iotests/check instead, to be
> able to run any (or some) tests with or without iothread?

I don't know how many of the existing iotests would benefit from the
ability supply iothread as an option, nor how likely it is that someone
would actually remember to run the testsuite twice to cover the use of
that option.  I also don't know how hard it would be to retrofit the
addition of optional iothread support into all the tests.  Rather, I
addressed the more immediate concern of the fact that my recent addition
to using iothread in 223 lost the previous ability of that test to cover
non-iothread, and where this patch series now makes it cover both
scenarios with a single iotests run.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] enhance iotest 223 coverage

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1817 bytes --]

On 24.09.19 21:51, Eric Blake wrote:
> On 9/24/19 2:26 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 24.09.2019 17:35, Eric Blake wrote:
>>> Commit 506902c6 dropped non-iothread coverage in order to test iothread,
>>> better is to run things twice.  In doing this, I found it easier to
>>> edit the test when the log shows what commands were triggering various
>>> responses.
>>
>> Did you consider adding -iothread to tests/qemu-iotests/check instead, to be
>> able to run any (or some) tests with or without iothread?
> 
> I don't know how many of the existing iotests would benefit from the
> ability supply iothread as an option, nor how likely it is that someone
> would actually remember to run the testsuite twice to cover the use of
> that option.

I would, because I already run all qcow2 tests without any creation
options, with compat=0.10, and with refcount_bits=1.  (And plan to add
data_file=$TEST_IMG.data_file in the future.)

(And I let the iotests run through a script from some json description
files, so I won’t forget.)

> I also don't know how hard it would be to retrofit the
> addition of optional iothread support into all the tests.

That I don’t know either.  Though I think the point wouldn’t be to
retrofit iothread support into all tests, but just start with some.
(For example, 262 uses “an iothread just for fun”.  So it could clearly
run with both configurations.)

I do think it’s an interesting idea, but I don’t think it’s important
right now.

Max

> Rather, I
> addressed the more immediate concern of the fact that my recent addition
> to using iothread in 223 lost the previous ability of that test to cover
> non-iothread, and where this patch series now makes it cover both
> scenarios with a single iotests run.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] tests: Make iotest 223 easier to edit

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 864 bytes --]

On 24.09.19 16:35, Eric Blake wrote:
> Log the QMP input to qemu, not just the QMP output.

Why not just add this functionality to _send_qemu_cmd directly?  (Like
silent already does for replies, although it’s inverted.)

(Although I’m not quite sold on the indentation for commands, because
(1) we don’t do that in other tests, (2) I’d prefer some prefix like
->/<-, and (3) there is generally no need because commands start with
“execute” and replies start with “return”, “error”, or “event”, so they
already have clear prefixes to distinguish the two classes.)

Max

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qemu-iotests/223     | 48 +++++++++++++++++++++-----------------
>  tests/qemu-iotests/223.out | 40 +++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 21 deletions(-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] tests: More iotest 223 improvements

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 655 bytes --]

On 24.09.19 16:35, Eric Blake wrote:
> Run the test twice, once without iothreads, and again with, for more
> coverage of both setups.
> 
> Suggested-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qemu-iotests/223     | 66 +++++++++++++++++++++++++
>  tests/qemu-iotests/223.out | 98 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 164 insertions(+)

I think this can be done easier by just having a “for i in 0 1” loop
span the range from block-dirty-bitmap-disable to nbd-server-stop (and
then, at the end of the first iteration, do the x-blockdev-set-iothread).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] tests: Make iotest 223 easier to edit

[Eric Blake]

On 10/7/19 7:05 AM, Max Reitz wrote:
> On 24.09.19 16:35, Eric Blake wrote:
>> Log the QMP input to qemu, not just the QMP output.
> 
> Why not just add this functionality to _send_qemu_cmd directly?  (Like
> silent already does for replies, although it’s inverted.)

Interesting idea.  I'll give it a shot (it may have a larger effect on 
more .out files, but that's probably a good thing).

> 
> (Although I’m not quite sold on the indentation for commands, because
> (1) we don’t do that in other tests, (2) I’d prefer some prefix like
> ->/<-, and (3) there is generally no need because commands start with
> “execute” and replies start with “return”, “error”, or “event”, so they
> already have clear prefixes to distinguish the two classes.)

I don't mind avoiding indentation; as you say, direction can be inferred 
by contents.

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

Re: [PATCH 1/2] tests: Make iotest 223 easier to edit

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1223 bytes --]

On 07.10.19 22:06, Eric Blake wrote:
> On 10/7/19 7:05 AM, Max Reitz wrote:
>> On 24.09.19 16:35, Eric Blake wrote:
>>> Log the QMP input to qemu, not just the QMP output.
>>
>> Why not just add this functionality to _send_qemu_cmd directly?  (Like
>> silent already does for replies, although it’s inverted.)
> 
> Interesting idea.  I'll give it a shot (it may have a larger effect on
> more .out files, but that's probably a good thing).

I was thinking about making it conditional, like it’s done with the
$silent parameter.  But I mean, there is actually no good reason for it
to print the output but omit the input, I suppose...  (Other than that
this change will affect many .out files, as you say)

Max

>> (Although I’m not quite sold on the indentation for commands, because
>> (1) we don’t do that in other tests, (2) I’d prefer some prefix like
>> ->/<-, and (3) there is generally no need because commands start with
>> “execute” and replies start with “return”, “error”, or “event”, so they
>> already have clear prefixes to distinguish the two classes.)
> 
> I don't mind avoiding indentation; as you say, direction can be inferred
> by contents.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]