[PATCH 00/15] iotests: add enhanced debugging info to qemu-io failures

[PATCH 00/15] iotests: add enhanced debugging info to qemu-io failures

[John Snow]

Howdy,

This series does for qemu_io() what we've done for qemu_img() and makes
this a function that checks the return code by default and raises an
Exception when things do not go according to plan.

This series removes qemu_io_pipe_and_status(), qemu_io_silent(), and
qemu_io_silent_check() in favor of just qemu_io().

RFC:

- There are a few remaining uses of qemu-io that don't go through qemu_io;
QemuIoInteractive is a user that is used in 205, 298, 299, and 307. It
... did not appear worth it to morph qemu_tool_popen into something that
could be used by both QemuIoInteractive *and* qemu_io(), so I left it
alone. It's probably fine for now. (But it does bother me, a little.)

- qemu_io_popen itself is used by the nbd-reconnect-on-open test, and it
seems like a legitimate use -- it wants concurrency. Like the above
problem, I couldn't find a way to bring it into the fold, so I
didn't. (Meh.) I eventually plan to add asyncio subprocess management to
machine.py, and I could tackle stuff like this then. It's not worth it
now.

- Several patches in this patchset ("fixup" in the title) are designed to
be merged-on-commit. I know that's not usually how we do things, but I
thought it was actually nicer than pre-squashing it because it gives me
more flexibility on re-spin.

- Uh, actually, test 040 fails with this patchset and I don't understand
  if it's intentional, harmless, a test design problem, or worse:

======================================================================
ERROR: test_filterless_commit (__main__.TestCommitWithFilters)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jsnow/src/qemu/tests/qemu-iotests/040", line 822, in tearDown
    self.do_test_io('read')
  File "/home/jsnow/src/qemu/tests/qemu-iotests/040", line 751, in do_test_io
    qemu_io('-f', iotests.imgfmt,
  File "/home/jsnow/src/qemu/tests/qemu-iotests/iotests.py", line 365, in qemu_io
    return qemu_tool(*qemu_io_wrap_args(args),
  File "/home/jsnow/src/qemu/tests/qemu-iotests/iotests.py", line 242, in qemu_tool
    raise VerboseProcessError(

qemu.utils.VerboseProcessError: Command
  '('/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/../../qemu-io',
  '--cache', 'writeback', '--aio', 'threads', '-f', 'qcow2', '-c',
  'read -P 4 3M 1M',
  '/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img')'
  returned non-zero exit status 1.
  ┏━ output ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
  ┃ qemu-io: can't open device
  ┃ /home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img:
  ┃ Could not open backing file: Could not open backing file: Throttle
  ┃ group 'tg' does not exist
  ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

It looks like we start with the img chain 3->2->1->0, then we commit 2
down into 1, but checking '3' fails somewhere in the backing
chain. Maybe a real bug?

John Snow (15):
  iotests: replace calls to log(qemu_io(...)) with qemu_io_log()
  iotests/163: Fix broken qemu-io invocation
  iotests: Don't check qemu_io() output for specific error strings
  iotests/040: Don't check image pattern on zero-length image
  iotests: create generic qemu_tool() function
  iotests: rebase qemu_io() on top of qemu_tool()
  iotests/030: fixup
  iotests/149: fixup
  iotests/205: fixup
  iotests/245: fixup
  iotests/migration-permissions: fixup
  iotests/migration-permissions: use assertRaises() for qemu_io()
    negative test
  iotests: remove qemu_io_pipe_and_status()
  iotests: remove qemu_io_silent() and qemu_io_silent_check().
  iotests: make qemu_io_log() check return codes by default

 tests/qemu-iotests/030                        | 85 +++++++++++--------
 tests/qemu-iotests/040                        | 47 +++++-----
 tests/qemu-iotests/056                        |  2 +-
 tests/qemu-iotests/149                        |  6 +-
 tests/qemu-iotests/163                        |  5 +-
 tests/qemu-iotests/205                        |  4 +-
 tests/qemu-iotests/216                        | 12 +--
 tests/qemu-iotests/218                        |  5 +-
 tests/qemu-iotests/224                        |  4 +-
 tests/qemu-iotests/242                        |  4 +-
 tests/qemu-iotests/245                        |  2 +-
 tests/qemu-iotests/255                        |  4 +-
 tests/qemu-iotests/258                        | 12 +--
 tests/qemu-iotests/298                        | 16 ++--
 tests/qemu-iotests/303                        |  4 +-
 tests/qemu-iotests/310                        | 22 ++---
 tests/qemu-iotests/iotests.py                 | 74 ++++++++--------
 tests/qemu-iotests/tests/image-fleecing       | 14 +--
 .../qemu-iotests/tests/migration-permissions  | 28 +++---
 .../tests/mirror-ready-cancel-error           |  2 +-
 .../qemu-iotests/tests/nbd-reconnect-on-open  |  2 +-
 .../qemu-iotests/tests/stream-error-on-reset  |  4 +-
 22 files changed, 184 insertions(+), 174 deletions(-)

-- 
2.34.1

[PATCH 01/15] iotests: replace calls to log(qemu_io(...)) with qemu_io_log()

[John Snow]

This makes these callsites a little simpler, but the real motivation is
a forthcoming commit will change the return type of qemu_io(), so removing
users of the return value now is helpful.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/242 | 2 +-
 tests/qemu-iotests/255 | 4 +---
 tests/qemu-iotests/303 | 4 ++--
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index b3afd36d72..4b7ec16af6 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -61,7 +61,7 @@ def add_bitmap(bitmap_number, persistent, disabled):
 
 def write_to_disk(offset, size):
     write = 'write {} {}'.format(offset, size)
-    log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+    qemu_io_log('-c', write, disk)
 
 
 def toggle_flag(offset):
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index f86fa851b6..88b29d64b4 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -95,9 +95,7 @@ with iotests.FilePath('src.qcow2') as src_path, \
     iotests.qemu_img_create('-f', iotests.imgfmt, src_path, size_str)
     iotests.qemu_img_create('-f', iotests.imgfmt, dst_path, size_str)
 
-    iotests.log(iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 1M',
-                                src_path),
-                filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
+    iotests.qemu_io_log('-f', iotests.imgfmt, '-c', 'write 0 1M', src_path),
 
     vm.add_object('throttle-group,x-bps-read=4096,id=throttle0')
 
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 93aa5ce9b7..32128b1d32 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -21,7 +21,7 @@
 
 import iotests
 import subprocess
-from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+from iotests import qemu_img_create, qemu_io_log, file_path, log
 
 iotests.script_initialize(supported_fmts=['qcow2'],
                           unsupported_imgopts=['refcount_bits', 'compat'])
@@ -43,7 +43,7 @@ def create_bitmap(bitmap_number, disabled):
 
 def write_to_disk(offset, size):
     write = f'write {offset} {size}'
-    log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+    qemu_io_log('-c', write, disk)
 
 
 def add_bitmap(num, begin, end, disabled):
-- 
2.34.1

Re: [PATCH 01/15] iotests: replace calls to log(qemu_io(...)) with qemu_io_log()

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:41PM -0400, John Snow wrote:
> This makes these callsites a little simpler, but the real motivation is
> a forthcoming commit will change the return type of qemu_io(), so removing
> users of the return value now is helpful.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 01/15] iotests: replace calls to log(qemu_io(...)) with qemu_io_log()

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> This makes these callsites a little simpler, but the real motivation is
> a forthcoming commit will change the return type of qemu_io(), so removing
> users of the return value now is helpful.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/242 | 2 +-
>   tests/qemu-iotests/255 | 4 +---
>   tests/qemu-iotests/303 | 4 ++--
>   3 files changed, 4 insertions(+), 6 deletions(-)

Perfectly reasonable, but...

> diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
> index b3afd36d72..4b7ec16af6 100755
> --- a/tests/qemu-iotests/242
> +++ b/tests/qemu-iotests/242
> @@ -61,7 +61,7 @@ def add_bitmap(bitmap_number, persistent, disabled):
>   
>   def write_to_disk(offset, size):
>       write = 'write {} {}'.format(offset, size)
> -    log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
> +    qemu_io_log('-c', write, disk)

This breaks 242 because qemu_io_log is not imported.

Hanna

[PATCH 02/15] iotests/163: Fix broken qemu-io invocation

[John Snow]

The 'read' commands to qemu-io were malformed, and this invocation only
worked by coincidence because the error messages were identical. Oops.

There's no point in checking the patterning of the reference image, so
just check the empty image by itself instead.

(Note: as of this commit, nothing actually enforces that this command
completes successfully, but a forthcoming commit in this series will
enforce that qemu_io() must have a zero status code.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/163 | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
index e4cd4b230f..c94ad16f4a 100755
--- a/tests/qemu-iotests/163
+++ b/tests/qemu-iotests/163
@@ -113,10 +113,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
         qemu_img('resize',  '-f', iotests.imgfmt, '--shrink', test_img,
                  self.shrink_size)
 
-        self.assertEqual(
-            qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, test_img),
-            qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, check_img),
-            "Verifying image content")
+        qemu_io('-c', f"read -P 0x00 0 {self.shrink_size}", test_img)
 
         self.image_verify()
 
-- 
2.34.1

Re: [PATCH 02/15] iotests/163: Fix broken qemu-io invocation

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:42PM -0400, John Snow wrote:
> The 'read' commands to qemu-io were malformed, and this invocation only
> worked by coincidence because the error messages were identical. Oops.
> 
> There's no point in checking the patterning of the reference image, so
> just check the empty image by itself instead.
> 
> (Note: as of this commit, nothing actually enforces that this command
> completes successfully, but a forthcoming commit in this series will
> enforce that qemu_io() must have a zero status code.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/163 | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
> index e4cd4b230f..c94ad16f4a 100755
> --- a/tests/qemu-iotests/163
> +++ b/tests/qemu-iotests/163
> @@ -113,10 +113,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
>          qemu_img('resize',  '-f', iotests.imgfmt, '--shrink', test_img,
>                   self.shrink_size)
>  
> -        self.assertEqual(
> -            qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, test_img),
> -            qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, check_img),
> -            "Verifying image content")
> +        qemu_io('-c', f"read -P 0x00 0 {self.shrink_size}", test_img)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 02/15] iotests/163: Fix broken qemu-io invocation

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> The 'read' commands to qemu-io were malformed, and this invocation only
> worked by coincidence because the error messages were identical. Oops.
>
> There's no point in checking the patterning of the reference image, so
> just check the empty image by itself instead.
>
> (Note: as of this commit, nothing actually enforces that this command
> completes successfully, but a forthcoming commit in this series will
> enforce that qemu_io() must have a zero status code.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/163 | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/tests/qemu-iotests/163 b/tests/qemu-iotests/163
> index e4cd4b230f..c94ad16f4a 100755
> --- a/tests/qemu-iotests/163
> +++ b/tests/qemu-iotests/163
> @@ -113,10 +113,7 @@ class ShrinkBaseClass(iotests.QMPTestCase):
>           qemu_img('resize',  '-f', iotests.imgfmt, '--shrink', test_img,
>                    self.shrink_size)
>   
> -        self.assertEqual(
> -            qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, test_img),
> -            qemu_io('-c', 'read -P 0x00 %s'%self.shrink_size, check_img),
> -            "Verifying image content")
> +        qemu_io('-c', f"read -P 0x00 0 {self.shrink_size}", test_img)

I’m actually puzzled by the original intent here.  check_img doesn’t 
contain 0x00 in that area...

Well.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

[PATCH 03/15] iotests: Don't check qemu_io() output for specific error strings

[John Snow]

A forthcoming commit updates qemu_io() to raise an exception on non-zero
return by default, and changes its return type.

In preparation, simplify some calls to qemu_io() that assert that
specific error message strings do not appear in qemu-io's
output. Asserting that all of these calls return a status code of zero
will be a more robust way to guard against failure.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/040 | 33 ++++++++++++++++-----------------
 tests/qemu-iotests/056 |  2 +-
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 0e1cfd7e49..adf5815781 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -101,13 +101,13 @@ class TestSingleDrive(ImageCommitTestCase):
 
     def test_commit(self):
         self.run_commit_test(mid_img, backing_img)
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+        qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
     def test_commit_node(self):
         self.run_commit_test("mid", "base", node_names=True)
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+        qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
     @iotests.skip_if_unsupported(['throttle'])
     def test_commit_with_filter_and_quit(self):
@@ -192,13 +192,13 @@ class TestSingleDrive(ImageCommitTestCase):
 
     def test_top_is_active(self):
         self.run_commit_test(test_img, backing_img, need_ready=True)
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+        qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
     def test_top_is_default_active(self):
         self.run_default_commit_test()
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+        qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
+        qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()
@@ -334,8 +334,8 @@ class TestRelativePaths(ImageCommitTestCase):
 
     def test_commit(self):
         self.run_commit_test(self.mid_img, self.backing_img)
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
+        qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', self.backing_img_abs)
+        qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', self.backing_img_abs)
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % self.mid_img)
@@ -361,8 +361,8 @@ class TestRelativePaths(ImageCommitTestCase):
 
     def test_top_is_active(self):
         self.run_commit_test(self.test_img, self.backing_img)
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', self.backing_img_abs).find("verification failed"))
-        self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', self.backing_img_abs).find("verification failed"))
+        qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', self.backing_img_abs)
+        qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', self.backing_img_abs)
 
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()
@@ -738,11 +738,10 @@ class TestCommitWithFilters(iotests.QMPTestCase):
 
     def do_test_io(self, read_or_write):
         for index, pattern_file in enumerate(self.pattern_files):
-            result = qemu_io('-f', iotests.imgfmt,
-                             '-c',
-                             f'{read_or_write} -P {index + 1} {index}M 1M',
-                             pattern_file)
-            self.assertFalse('Pattern verification failed' in result)
+            qemu_io('-f', iotests.imgfmt,
+                    '-c',
+                    f'{read_or_write} -P {index + 1} {index}M 1M',
+                    pattern_file)
 
     @iotests.skip_if_unsupported(['throttle'])
     def setUp(self):
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index b459a3f1e8..bef865eec4 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -102,7 +102,7 @@ class TestSyncModesNoneAndTop(iotests.QMPTestCase):
 
         self.vm.shutdown()
         time.sleep(1)
-        self.assertEqual(-1, qemu_io('-c', 'read -P0x41 0 512', target_img).find("verification failed"))
+        qemu_io('-c', 'read -P0x41 0 512', target_img)
 
 class TestBeforeWriteNotifier(iotests.QMPTestCase):
     def setUp(self):
-- 
2.34.1

Re: [PATCH 03/15] iotests: Don't check qemu_io() output for specific error strings

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:43PM -0400, John Snow wrote:
> A forthcoming commit updates qemu_io() to raise an exception on non-zero
> return by default, and changes its return type.
> 
> In preparation, simplify some calls to qemu_io() that assert that
> specific error message strings do not appear in qemu-io's
> output. Asserting that all of these calls return a status code of zero
> will be a more robust way to guard against failure.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/040 | 33 ++++++++++++++++-----------------
>  tests/qemu-iotests/056 |  2 +-
>  2 files changed, 17 insertions(+), 18 deletions(-)
>

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 03/15] iotests: Don't check qemu_io() output for specific error strings

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> A forthcoming commit updates qemu_io() to raise an exception on non-zero
> return by default, and changes its return type.
>
> In preparation, simplify some calls to qemu_io() that assert that
> specific error message strings do not appear in qemu-io's
> output. Asserting that all of these calls return a status code of zero
> will be a more robust way to guard against failure.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/040 | 33 ++++++++++++++++-----------------
>   tests/qemu-iotests/056 |  2 +-
>   2 files changed, 17 insertions(+), 18 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

[PATCH 04/15] iotests/040: Don't check image pattern on zero-length image

[John Snow]

qemu-io fails on read/write with zero-length raw images, so skip these
when running the zero-length image tests.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/040 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index adf5815781..c4a90937dc 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -86,8 +86,10 @@ class TestSingleDrive(ImageCommitTestCase):
         qemu_img('create', '-f', iotests.imgfmt,
                  '-o', 'backing_file=%s' % mid_img,
                  '-F', iotests.imgfmt, test_img)
-        qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
-        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
+        if self.image_len:
+            qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
+            qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288',
+                    mid_img)
         self.vm = iotests.VM().add_drive(test_img, "node-name=top,backing.node-name=mid,backing.backing.node-name=base", interface="none")
         self.vm.add_device('virtio-scsi')
         self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
@@ -101,11 +103,15 @@ class TestSingleDrive(ImageCommitTestCase):
 
     def test_commit(self):
         self.run_commit_test(mid_img, backing_img)
+        if not self.image_len:
+            return
         qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
         qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
     def test_commit_node(self):
         self.run_commit_test("mid", "base", node_names=True)
+        if not self.image_len:
+            return
         qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
         qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
@@ -192,11 +198,15 @@ class TestSingleDrive(ImageCommitTestCase):
 
     def test_top_is_active(self):
         self.run_commit_test(test_img, backing_img, need_ready=True)
+        if not self.image_len:
+            return
         qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
         qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
     def test_top_is_default_active(self):
         self.run_default_commit_test()
+        if not self.image_len:
+            return
         qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 524288', backing_img)
         qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 524288', backing_img)
 
-- 
2.34.1

Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:44PM -0400, John Snow wrote:
> qemu-io fails on read/write with zero-length raw images, so skip these
> when running the zero-length image tests.

On my first read, I wondered what we accomplish by rejecting
zero-length reads on a zero-length image, and whether entering the
rabbit hole of trying to make that corner case "work" differently
makes more sense...

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/040 | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index adf5815781..c4a90937dc 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -86,8 +86,10 @@ class TestSingleDrive(ImageCommitTestCase):
>          qemu_img('create', '-f', iotests.imgfmt,
>                   '-o', 'backing_file=%s' % mid_img,
>                   '-F', iotests.imgfmt, test_img)
> -        qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
> -        qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
> +        if self.image_len:
> +            qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
> +            qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288',
> +                    mid_img)

...but now it is obvious - one of our test cases is attempting a
non-zero-length modification to a zero-length file, and it does make
sense for that modification attempt to fail, in which case, making the
test special case the zero-length file is the right thing to do.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> qemu-io fails on read/write with zero-length raw images, so skip these
> when running the zero-length image tests.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/040 | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)

Doesn’t look specific to zero-length images, but the fact that we do I/O 
beyond the image size, i.e. any image below 1 MB would be affected.

Anyway, the zero-length image is the only one tested with a size of less 
than 1 MB, so this works.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image

[John Snow]

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

On Tue, Mar 22, 2022, 10:22 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 18.03.22 21:36, John Snow wrote:
> > qemu-io fails on read/write with zero-length raw images, so skip these
> > when running the zero-length image tests.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/040 | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
>
> Doesn’t look specific to zero-length images, but the fact that we do I/O
> beyond the image size, i.e. any image below 1 MB would be affected.
>
> Anyway, the zero-length image is the only one tested with a size of less
> than 1 MB, so this works.
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

(Check the cover letter, too - this patch is still good, but iotest 040
still fails and I'm not 100% clear as to why.)

>

[-- Attachment #2: Type: text/html, Size: 1542 bytes --]

Re: [PATCH 04/15] iotests/040: Don't check image pattern on zero-length image

[Hanna Reitz]

On 22.03.22 17:19, John Snow wrote:
>
>
> On Tue, Mar 22, 2022, 10:22 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 18.03.22 21:36, John Snow wrote:
>     > qemu-io fails on read/write with zero-length raw images, so skip
>     these
>     > when running the zero-length image tests.
>     >
>     > Signed-off-by: John Snow <jsnow@redhat.com>
>     > ---
>     >   tests/qemu-iotests/040 | 14 ++++++++++++--
>     >   1 file changed, 12 insertions(+), 2 deletions(-)
>
>     Doesn’t look specific to zero-length images, but the fact that we
>     do I/O
>     beyond the image size, i.e. any image below 1 MB would be affected.
>
>     Anyway, the zero-length image is the only one tested with a size
>     of less
>     than 1 MB, so this works.
>
>     Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
> (Check the cover letter, too - this patch is still good, but iotest 
> 040 still fails and I'm not 100% clear as to why.)

Sure, but I didn’t see anything wrong with the patch, so...

 From a glance, I believe the problem to be that the commit job changed 
one image’s backing file string to contain a JSON description of a block 
graph including a throttle node.  The accompanying throttle group of 
course doesn’t exist outside of qemu, so qemu-io complains.

We never noticed because we just checked for “pattern verification 
failed”, which isn’t in the error message, so this was a pass.  I’ll 
take a closer look.

Hanna

[PATCH 05/15] iotests: create generic qemu_tool() function

[John Snow]

reimplement qemu_img() in terms of qemu_tool() in preparation for doing
the same with qemu_io().

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 37 +++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6cd8374c81..974a2b0c8d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -207,15 +207,13 @@ def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
 
     return result
 
-def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
+
+def qemu_tool(*args: str, check: bool = True, combine_stdio: bool = True
              ) -> subprocess.CompletedProcess[str]:
     """
-    Run qemu_img and return the status code and console output.
+    Run a qemu tool and return its status code and console output.
 
-    This function always prepends QEMU_IMG_OPTIONS and may further alter
-    the args for 'create' commands.
-
-    :param args: command-line arguments to qemu-img.
+    :param args: command-line arguments to a QEMU cli tool.
     :param check: Enforce a return code of zero.
     :param combine_stdio: set to False to keep stdout/stderr separated.
 
@@ -227,14 +225,13 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
         handled, the command-line, return code, and all console output
         will be included at the bottom of the stack trace.
 
-    :return: a CompletedProcess. This object has args, returncode, and
-        stdout properties. If streams are not combined, it will also
-        have a stderr property.
+    :return:
+        A CompletedProcess. This object has args, returncode, and stdout
+        properties. If streams are not combined, it will also have a
+        stderr property.
     """
-    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
-
     subp = subprocess.run(
-        full_args,
+        args,
         stdout=subprocess.PIPE,
         stderr=subprocess.STDOUT if combine_stdio else subprocess.PIPE,
         universal_newlines=True,
@@ -243,7 +240,7 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
 
     if check and subp.returncode or (subp.returncode < 0):
         raise VerboseProcessError(
-            subp.returncode, full_args,
+            subp.returncode, args,
             output=subp.stdout,
             stderr=subp.stderr,
         )
@@ -251,6 +248,20 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
     return subp
 
 
+def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
+             ) -> subprocess.CompletedProcess[str]:
+    """
+    Run QEMU_IMG_PROG and return its status code and console output.
+
+    This function always prepends QEMU_IMG_OPTIONS and may further alter
+    the args for 'create' commands.
+
+    See `qemu_tool()` for greater detail.
+    """
+    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
+    return qemu_tool(*full_args, check=check, combine_stdio=combine_stdio)
+
+
 def ordered_qmp(qmsg, conv_keys=True):
     # Dictionaries are not ordered prior to 3.6, therefore:
     if isinstance(qmsg, list):
-- 
2.34.1

Re: [PATCH 05/15] iotests: create generic qemu_tool() function

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:45PM -0400, John Snow wrote:
> reimplement qemu_img() in terms of qemu_tool() in preparation for doing
> the same with qemu_io().
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 37 +++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 6cd8374c81..974a2b0c8d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -207,15 +207,13 @@ def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
>  
>      return result
>  
> -def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
> +
> +def qemu_tool(*args: str, check: bool = True, combine_stdio: bool = True
>               ) -> subprocess.CompletedProcess[str]:

Does this line need reindentation?

> @@ -227,14 +225,13 @@ def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
>          handled, the command-line, return code, and all console output
>          will be included at the bottom of the stack trace.
>  
> -    :return: a CompletedProcess. This object has args, returncode, and
> -        stdout properties. If streams are not combined, it will also
> -        have a stderr property.
> +    :return:
> +        A CompletedProcess. This object has args, returncode, and stdout
> +        properties. If streams are not combined, it will also have a
> +        stderr property.

Should this reflow be squashed in some earlier patch?

As those are both cosemetic only,

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 05/15] iotests: create generic qemu_tool() function

[John Snow]

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

On Mon, Mar 21, 2022, 11:13 AM Eric Blake <eblake@redhat.com> wrote:

> On Fri, Mar 18, 2022 at 04:36:45PM -0400, John Snow wrote:
> > reimplement qemu_img() in terms of qemu_tool() in preparation for doing
> > the same with qemu_io().
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  tests/qemu-iotests/iotests.py | 37 +++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index 6cd8374c81..974a2b0c8d 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -207,15 +207,13 @@ def qemu_img_create_prepare_args(args: List[str])
> -> List[str]:
> >
> >      return result
> >
> > -def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
> > +
> > +def qemu_tool(*args: str, check: bool = True, combine_stdio: bool = True
> >               ) -> subprocess.CompletedProcess[str]:
>
> Does this line need reindentation?
>

Huh, I'll check. Maybe I fixed this by accident in a later patch and didn't
notice. Or maybe git diff is playing tricks on me.


> > @@ -227,14 +225,13 @@ def qemu_img(*args: str, check: bool = True,
> combine_stdio: bool = True
> >          handled, the command-line, return code, and all console output
> >          will be included at the bottom of the stack trace.
> >
> > -    :return: a CompletedProcess. This object has args, returncode, and
> > -        stdout properties. If streams are not combined, it will also
> > -        have a stderr property.
> > +    :return:
> > +        A CompletedProcess. This object has args, returncode, and stdout
> > +        properties. If streams are not combined, it will also have a
> > +        stderr property.
>
> Should this reflow be squashed in some earlier patch?
>

Aw, you caught me. 😅

I need to respin the qemu-img stuff anyway due to CI failures, so I can fix
it where it appears first.

(When I wrote this, I didn't realize that the qemu-img series was failing
CI yet.)


> As those are both cosemetic only,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

[-- Attachment #2: Type: text/html, Size: 3738 bytes --]

Re: [PATCH 05/15] iotests: create generic qemu_tool() function

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> reimplement qemu_img() in terms of qemu_tool() in preparation for doing
> the same with qemu_io().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 37 +++++++++++++++++++++++------------
>   1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 6cd8374c81..974a2b0c8d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -207,15 +207,13 @@ def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
>   
>       return result
>   
> -def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
> +
> +def qemu_tool(*args: str, check: bool = True, combine_stdio: bool = True
>                ) -> subprocess.CompletedProcess[str]:
>       """
> -    Run qemu_img and return the status code and console output.
> +    Run a qemu tool and return its status code and console output.
>   
> -    This function always prepends QEMU_IMG_OPTIONS and may further alter
> -    the args for 'create' commands.
> -
> -    :param args: command-line arguments to qemu-img.
> +    :param args: command-line arguments to a QEMU cli tool.

This makes me ask how I am to specify which tool to use.  Perhaps it 
should just be “full command line to run” or something.

Might be nice™, but:

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 05/15] iotests: create generic qemu_tool() function

[John Snow]

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

On Tue, Mar 22, 2022, 10:49 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 18.03.22 21:36, John Snow wrote:
> > reimplement qemu_img() in terms of qemu_tool() in preparation for doing
> > the same with qemu_io().
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py | 37 +++++++++++++++++++++++------------
> >   1 file changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index 6cd8374c81..974a2b0c8d 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -207,15 +207,13 @@ def qemu_img_create_prepare_args(args: List[str])
> -> List[str]:
> >
> >       return result
> >
> > -def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
> > +
> > +def qemu_tool(*args: str, check: bool = True, combine_stdio: bool = True
> >                ) -> subprocess.CompletedProcess[str]:
> >       """
> > -    Run qemu_img and return the status code and console output.
> > +    Run a qemu tool and return its status code and console output.
> >
> > -    This function always prepends QEMU_IMG_OPTIONS and may further alter
> > -    the args for 'create' commands.
> > -
> > -    :param args: command-line arguments to qemu-img.
> > +    :param args: command-line arguments to a QEMU cli tool.
>
> This makes me ask how I am to specify which tool to use.  Perhaps it
> should just be “full command line to run” or something.
>
> Might be nice™, but:
>

I see what you mean. I did away with the "tool name" parameter because we
were only using it for an error message print I also removed.

I'll update the docstring a little to make what's going on more clear.
Maybe someday (tm) I could split args into (tool, args) parameters to make
it more explicit, and change the way the environment variables are parsed
to keep the tool/args separate.

Pretty minor kind of thing, though, so I'm not in a hurry.


> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>

[-- Attachment #2: Type: text/html, Size: 3089 bytes --]

[PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()

[John Snow]

Rework qemu_io() to be analogous to qemu_img(); a function that requires
a return code of zero by default unless disabled explicitly.

Tests that use qemu_io():
030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
migrate-during-backup migration-permissions

Test that use qemu_io_log():
242 245 255 274 303 307 nbd-reconnect-on-open

Signed-off-by: John Snow <jsnow@redhat.com>

---

Note: This breaks several tests at this point. I'll be fixing each
broken test one by one in the subsequent commits. We can squash them all
on merge to avoid test regressions.

(Seems like a way to have your cake and eat it too with regards to
maintaining bisectability while also having nice mailing list patches.)

Copy-pastables:

./check -qcow2 030 040 041 044 055 056 124 129 132 151 152 163 165 209 \
               219 236 242 245 248 254 255 257 260 264 274 \
               280 298 300 302 303 304 307 image-fleecing \
               migrate-bitmaps-postcopy-test migrate-bitmaps-test \
               migrate-during-backup nbd-reconnect-on-open

./check -raw 093 136 148 migration-permissions

./check -nbd 205

# ./configure configure --disable-gnutls --enable-gcrypt
# this ALSO requires passwordless sudo.
./check -luks 149


# Just the ones that fail:
./check -qcow2 030 040 242 245
./check -raw migration-permissions
./check -nbd 205
./check -luks 149

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 974a2b0c8d..58ea766568 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -354,16 +354,23 @@ def qemu_io_wrap_args(args: Sequence[str]) -> List[str]:
 def qemu_io_popen(*args):
     return qemu_tool_popen(qemu_io_wrap_args(args))
 
-def qemu_io(*args):
-    '''Run qemu-io and return the stdout data'''
-    return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))[0]
+def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
+            ) -> subprocess.CompletedProcess[str]:
+    """
+    Run QEMU_IO_PROG and return the status code and console output.
+
+    This function always prepends either QEMU_IO_OPTIONS or
+    QEMU_IO_OPTIONS_NO_FMT.
+    """
+    return qemu_tool(*qemu_io_wrap_args(args),
+                     check=check, combine_stdio=combine_stdio)
 
 def qemu_io_pipe_and_status(*args):
     return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))
 
-def qemu_io_log(*args):
-    result = qemu_io(*args)
-    log(result, filters=[filter_testfiles, filter_qemu_io])
+def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
+    result = qemu_io(*args, check=False)
+    log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
     return result
 
 def qemu_io_silent(*args):
-- 
2.34.1

Re: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:46PM -0400, John Snow wrote:
> Rework qemu_io() to be analogous to qemu_img(); a function that requires
> a return code of zero by default unless disabled explicitly.
> 
> Tests that use qemu_io():
> 030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
> 209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
> image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
> migrate-during-backup migration-permissions
> 
> Test that use qemu_io_log():
> 242 245 255 274 303 307 nbd-reconnect-on-open
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> ---
> 
> Note: This breaks several tests at this point. I'll be fixing each
> broken test one by one in the subsequent commits. We can squash them all
> on merge to avoid test regressions.
> 
> (Seems like a way to have your cake and eat it too with regards to
> maintaining bisectability while also having nice mailing list patches.)

Interesting approach; it does appear to have made reviewing a bit
easier, so thanks for trying it.

I'll withhold actual R-b until the last squashed patch, but so far, I
haven't seen anything that causes me grief other than the lack of
bisectability that you already have documented how it will be
addressed.  [less wordy - this patch is incomplete, as advertised, but
looks good]

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

Re: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()

[John Snow]

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

On Mon, Mar 21, 2022, 11:29 AM Eric Blake <eblake@redhat.com> wrote:

> On Fri, Mar 18, 2022 at 04:36:46PM -0400, John Snow wrote:
> > Rework qemu_io() to be analogous to qemu_img(); a function that requires
> > a return code of zero by default unless disabled explicitly.
> >
> > Tests that use qemu_io():
> > 030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
> > 209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
> > image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
> > migrate-during-backup migration-permissions
> >
> > Test that use qemu_io_log():
> > 242 245 255 274 303 307 nbd-reconnect-on-open
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > Note: This breaks several tests at this point. I'll be fixing each
> > broken test one by one in the subsequent commits. We can squash them all
> > on merge to avoid test regressions.
> >
> > (Seems like a way to have your cake and eat it too with regards to
> > maintaining bisectability while also having nice mailing list patches.)
>
> Interesting approach; it does appear to have made reviewing a bit
> easier, so thanks for trying it.
>
> I'll withhold actual R-b until the last squashed patch, but so far, I
> haven't seen anything that causes me grief other than the lack of
> bisectability that you already have documented how it will be
> addressed.  [less wordy - this patch is incomplete, as advertised, but
> looks good]
>

Meta chat about QEMU patch process:

I have to admit that I often "work backwards" and I prototype things by
just making a function behave like how I want it to, and then I try and
measure how many things broke post-hoc and use that to decide if the
refactoring is even tractable.

Often the slowest part of writing a series for me is breaking apart the
"WIP" commit into a series of smaller steps that don't break the bisect.

Sometimes this even involves a complete rewrite of an intermediate data
structure to handle the in-between step.

It feels like a lot of work just to delete it several commits later,
sometimes. I realize giant merge commits are tough to backport, but
sometimes I really just get stumped on how to not create twice as much work
for myself just to arrive at an end point I've already arrived at.

Of course, making things like this reviewable is a primary concern too.

I'm not sure I'm an advocate of the squash-on-merge school of thought
entirely, but maybe it's not so bad to use it sparingly, sometimes. I find
keeping mini-commits separate can sometimes help me iterate on the design
of a series quicker, too.

In this case, I did try to position fixes that would work independently of
the switch ahead of the pivot, but I couldn't quite get everything. Most of
what's left is really just cases where the return type matters.

Eh.

This is definitely "Software Engineering" and not "Computer Science".

Thanks for taking a look.


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

[-- Attachment #2: Type: text/html, Size: 4536 bytes --]

Re: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> Rework qemu_io() to be analogous to qemu_img(); a function that requires
> a return code of zero by default unless disabled explicitly.
>
> Tests that use qemu_io():
> 030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
> 209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
> image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
> migrate-during-backup migration-permissions
>
> Test that use qemu_io_log():
> 242 245 255 274 303 307 nbd-reconnect-on-open
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Note: This breaks several tests at this point. I'll be fixing each
> broken test one by one in the subsequent commits. We can squash them all
> on merge to avoid test regressions.

Well, absolutely.

> (Seems like a way to have your cake and eat it too with regards to
> maintaining bisectability while also having nice mailing list patches.)

I personally find reviewability to not be affected whether this is one 
patch or multiple, given that the changes are in different files anyway.

I am afraid someone might forgot to squash when merging this series, 
though...

Also, I don’t know how to squash R-b tags, and I don’t feel like I can 
give an R-b for a patch that decidedly breaks tests.

>
> Copy-pastables:
>
> ./check -qcow2 030 040 041 044 055 056 124 129 132 151 152 163 165 209 \
>                 219 236 242 245 248 254 255 257 260 264 274 \
>                 280 298 300 302 303 304 307 image-fleecing \
>                 migrate-bitmaps-postcopy-test migrate-bitmaps-test \
>                 migrate-during-backup nbd-reconnect-on-open
>
> ./check -raw 093 136 148 migration-permissions
>
> ./check -nbd 205
>
> # ./configure configure --disable-gnutls --enable-gcrypt
> # this ALSO requires passwordless sudo.
> ./check -luks 149
>
>
> # Just the ones that fail:
> ./check -qcow2 030 040 242 245
> ./check -raw migration-permissions
> ./check -nbd 205
> ./check -luks 149
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 974a2b0c8d..58ea766568 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -354,16 +354,23 @@ def qemu_io_wrap_args(args: Sequence[str]) -> List[str]:
>   def qemu_io_popen(*args):
>       return qemu_tool_popen(qemu_io_wrap_args(args))
>   
> -def qemu_io(*args):
> -    '''Run qemu-io and return the stdout data'''
> -    return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))[0]
> +def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
> +            ) -> subprocess.CompletedProcess[str]:

I guess this return type probably has to be quoted.

> +    """
> +    Run QEMU_IO_PROG and return the status code and console output.
> +
> +    This function always prepends either QEMU_IO_OPTIONS or
> +    QEMU_IO_OPTIONS_NO_FMT.
> +    """
> +    return qemu_tool(*qemu_io_wrap_args(args),
> +                     check=check, combine_stdio=combine_stdio)
>   
>   def qemu_io_pipe_and_status(*args):
>       return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))
>   
> -def qemu_io_log(*args):
> -    result = qemu_io(*args)
> -    log(result, filters=[filter_testfiles, filter_qemu_io])
> +def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:

...and this one.

Hanna

> +    result = qemu_io(*args, check=False)
> +    log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
>       return result
>   
>   def qemu_io_silent(*args):

Re: [PATCH 06/15] iotests: rebase qemu_io() on top of qemu_tool()

[John Snow]

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

On Tue, Mar 22, 2022, 11:04 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 18.03.22 21:36, John Snow wrote:
> > Rework qemu_io() to be analogous to qemu_img(); a function that requires
> > a return code of zero by default unless disabled explicitly.
> >
> > Tests that use qemu_io():
> > 030 040 041 044 055 056 093 124 129 132 136 148 149 151 152 163 165 205
> > 209 219 236 245 248 254 255 257 260 264 280 298 300 302 304
> > image-fleecing migrate-bitmaps-postcopy-test migrate-bitmaps-test
> > migrate-during-backup migration-permissions
> >
> > Test that use qemu_io_log():
> > 242 245 255 274 303 307 nbd-reconnect-on-open
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > Note: This breaks several tests at this point. I'll be fixing each
> > broken test one by one in the subsequent commits. We can squash them all
> > on merge to avoid test regressions.
>
> Well, absolutely.
>
> > (Seems like a way to have your cake and eat it too with regards to
> > maintaining bisectability while also having nice mailing list patches.)
>
> I personally find reviewability to not be affected whether this is one
> patch or multiple, given that the changes are in different files anyway.
>
> I am afraid someone might forgot to squash when merging this series,
> though...
>
> Also, I don’t know how to squash R-b tags, and I don’t feel like I can
> give an R-b for a patch that decidedly breaks tests.
>
> >
> > Copy-pastables:
> >
> > ./check -qcow2 030 040 041 044 055 056 124 129 132 151 152 163 165 209 \
> >                 219 236 242 245 248 254 255 257 260 264 274 \
> >                 280 298 300 302 303 304 307 image-fleecing \
> >                 migrate-bitmaps-postcopy-test migrate-bitmaps-test \
> >                 migrate-during-backup nbd-reconnect-on-open
> >
> > ./check -raw 093 136 148 migration-permissions
> >
> > ./check -nbd 205
> >
> > # ./configure configure --disable-gnutls --enable-gcrypt
> > # this ALSO requires passwordless sudo.
> > ./check -luks 149
> >
> >
> > # Just the ones that fail:
> > ./check -qcow2 030 040 242 245
> > ./check -raw migration-permissions
> > ./check -nbd 205
> > ./check -luks 149
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py | 19 +++++++++++++------
> >   1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index 974a2b0c8d..58ea766568 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -354,16 +354,23 @@ def qemu_io_wrap_args(args: Sequence[str]) ->
> List[str]:
> >   def qemu_io_popen(*args):
> >       return qemu_tool_popen(qemu_io_wrap_args(args))
> >
> > -def qemu_io(*args):
> > -    '''Run qemu-io and return the stdout data'''
> > -    return qemu_tool_pipe_and_status('qemu-io',
> qemu_io_wrap_args(args))[0]
> > +def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
> > +            ) -> subprocess.CompletedProcess[str]:
>
> I guess this return type probably has to be quoted.
>

Yep. Sent this just before I figured out the problem from the prior series.
I'll make sure this whole series passes CI before I send it out a second
time.

I'll rebase on your staging branch and take my time with v2.


> > +    """
> > +    Run QEMU_IO_PROG and return the status code and console output.
> > +
> > +    This function always prepends either QEMU_IO_OPTIONS or
> > +    QEMU_IO_OPTIONS_NO_FMT.
> > +    """
> > +    return qemu_tool(*qemu_io_wrap_args(args),
> > +                     check=check, combine_stdio=combine_stdio)
> >
> >   def qemu_io_pipe_and_status(*args):
> >       return qemu_tool_pipe_and_status('qemu-io',
> qemu_io_wrap_args(args))
> >
> > -def qemu_io_log(*args):
> > -    result = qemu_io(*args)
> > -    log(result, filters=[filter_testfiles, filter_qemu_io])
> > +def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
>
> ...and this one.
>
> Hanna
>
> > +    result = qemu_io(*args, check=False)
> > +    log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
> >       return result
> >
> >   def qemu_io_silent(*args):
>
>

[-- Attachment #2: Type: text/html, Size: 5669 bytes --]

[PATCH 07/15] iotests/030: fixup

[John Snow]

(Merge into prior patch.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/030 | 85 ++++++++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 567bf1da67..3a2de920a3 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -64,16 +64,18 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
-                         qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
-                         'image file map does not match backing file after streaming')
+        self.assertEqual(
+            qemu_io('-f', 'raw', '-c', 'map', backing_img).stdout,
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img).stdout,
+            'image file map does not match backing file after streaming')
 
     def test_stream_intermediate(self):
         self.assert_no_active_block_jobs()
 
-        self.assertNotEqual(qemu_io('-f', 'raw', '-rU', '-c', 'map', backing_img),
-                            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', mid_img),
-                            'image file map matches backing file before streaming')
+        self.assertNotEqual(
+            qemu_io('-f', 'raw', '-rU', '-c', 'map', backing_img).stdout,
+            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', mid_img).stdout,
+            'image file map matches backing file before streaming')
 
         result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
         self.assert_qmp(result, 'return', {})
@@ -83,9 +85,10 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
-                         qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
-                         'image file map does not match backing file after streaming')
+        self.assertEqual(
+            qemu_io('-f', 'raw', '-c', 'map', backing_img).stdout,
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img).stdout,
+            'image file map does not match backing file after streaming')
 
     def test_stream_pause(self):
         self.assert_no_active_block_jobs()
@@ -113,15 +116,17 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', 'raw', '-c', 'map', backing_img),
-                         qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
-                         'image file map does not match backing file after streaming')
+        self.assertEqual(
+            qemu_io('-f', 'raw', '-c', 'map', backing_img).stdout,
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img).stdout,
+            'image file map does not match backing file after streaming')
 
     def test_stream_no_op(self):
         self.assert_no_active_block_jobs()
 
         # The image map is empty before the operation
-        empty_map = qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', test_img)
+        empty_map = qemu_io(
+            '-f', iotests.imgfmt, '-rU', '-c', 'map', test_img).stdout
 
         # This is a no-op: no data should ever be copied from the base image
         result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
@@ -132,8 +137,9 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
-                         empty_map, 'image file map changed after a no-op')
+        self.assertEqual(
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img).stdout,
+            empty_map, 'image file map changed after a no-op')
 
     def test_stream_partial(self):
         self.assert_no_active_block_jobs()
@@ -146,9 +152,10 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img),
-                         qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img),
-                         'image file map does not match backing file after streaming')
+        self.assertEqual(
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img).stdout,
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img).stdout,
+            'image file map does not match backing file after streaming')
 
     def test_device_not_found(self):
         result = self.vm.qmp('block-stream', device='nonexistent')
@@ -236,9 +243,10 @@ class TestParallelOps(iotests.QMPTestCase):
 
         # Check that the maps don't match before the streaming operations
         for i in range(2, self.num_imgs, 2):
-            self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]),
-                                qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]),
-                                'image file map matches backing file before streaming')
+            self.assertNotEqual(
+                qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]).stdout,
+                qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]).stdout,
+                'image file map matches backing file before streaming')
 
         # Create all streaming jobs
         pending_jobs = []
@@ -278,9 +286,10 @@ class TestParallelOps(iotests.QMPTestCase):
 
         # Check that all maps match now
         for i in range(2, self.num_imgs, 2):
-            self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]),
-                             qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]),
-                             'image file map does not match backing file after streaming')
+            self.assertEqual(
+                qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]).stdout,
+                qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]).stdout,
+                'image file map does not match backing file after streaming')
 
     # Test that it's not possible to perform two block-stream
     # operations if there are nodes involved in both.
@@ -509,9 +518,10 @@ class TestParallelOps(iotests.QMPTestCase):
     def test_stream_base_node_name(self):
         self.assert_no_active_block_jobs()
 
-        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[4]),
-                            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[3]),
-                            'image file map matches backing file before streaming')
+        self.assertNotEqual(
+            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[4]).stdout,
+            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[3]).stdout,
+            'image file map matches backing file before streaming')
 
         # Error: the base node does not exist
         result = self.vm.qmp('block-stream', device='node4', base_node='none', job_id='stream')
@@ -542,9 +552,10 @@ class TestParallelOps(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]),
-                         qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[3]),
-                         'image file map matches backing file after streaming')
+        self.assertEqual(
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]).stdout,
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[3]).stdout,
+            'image file map matches backing file after streaming')
 
 class TestQuorum(iotests.QMPTestCase):
     num_children = 3
@@ -583,9 +594,10 @@ class TestQuorum(iotests.QMPTestCase):
             os.remove(img)
 
     def test_stream_quorum(self):
-        self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.children[0]),
-                            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.backing[0]),
-                            'image file map matches backing file before streaming')
+        self.assertNotEqual(
+            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.children[0]).stdout,
+            qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.backing[0]).stdout,
+            'image file map matches backing file before streaming')
 
         self.assert_no_active_block_jobs()
 
@@ -597,9 +609,10 @@ class TestQuorum(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
-        self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.children[0]),
-                         qemu_io('-f', iotests.imgfmt, '-c', 'map', self.backing[0]),
-                         'image file map does not match backing file after streaming')
+        self.assertEqual(
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.children[0]).stdout,
+            qemu_io('-f', iotests.imgfmt, '-c', 'map', self.backing[0]).stdout,
+            'image file map does not match backing file after streaming')
 
 class TestSmallerBackingFile(iotests.QMPTestCase):
     backing_len = 1 * 1024 * 1024 # MB
-- 
2.34.1

[PATCH 08/15] iotests/149: fixup

[John Snow]

(Merge into prior patch.)

Notes: I don't quite like this change, but I'm at a loss for what would
be cleaner. This is a funky test.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/149 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 9bb96d6a1d..2ae318f16f 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -295,7 +295,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb, size_mb, dev=False):
     args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
     args.extend(qemu_io_image_args(config, dev))
     iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-    iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+    output = iotests.qemu_io(*args, check=False).stdout
+    iotests.log(check_cipher_support(config, output),
                 filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
@@ -307,7 +308,8 @@ def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
     args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
     args.extend(qemu_io_image_args(config, dev))
     iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
-    iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
+    output = iotests.qemu_io(*args, check=False).stdout
+    iotests.log(check_cipher_support(config, output),
                 filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
 
 
-- 
2.34.1

Re: [PATCH 08/15] iotests/149: fixup

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> (Merge into prior patch.)
>
> Notes: I don't quite like this change, but I'm at a loss for what would
> be cleaner. This is a funky test.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/149 | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

I mean, looks fine to me, fwiw.

Hanna

> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> index 9bb96d6a1d..2ae318f16f 100755
> --- a/tests/qemu-iotests/149
> +++ b/tests/qemu-iotests/149
> @@ -295,7 +295,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb, size_mb, dev=False):
>       args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
>       args.extend(qemu_io_image_args(config, dev))
>       iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
> -    iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
> +    output = iotests.qemu_io(*args, check=False).stdout
> +    iotests.log(check_cipher_support(config, output),
>                   filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
>   
>   
> @@ -307,7 +308,8 @@ def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
>       args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
>       args.extend(qemu_io_image_args(config, dev))
>       iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
> -    iotests.log(check_cipher_support(config, iotests.qemu_io(*args)),
> +    output = iotests.qemu_io(*args, check=False).stdout
> +    iotests.log(check_cipher_support(config, output),
>                   filters=[iotests.filter_test_dir, iotests.filter_qemu_io])
>   
>   

[PATCH 09/15] iotests/205: fixup

[John Snow]

(Merge into prior patch.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/205 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/205 b/tests/qemu-iotests/205
index c0e107328f..15f798288a 100755
--- a/tests/qemu-iotests/205
+++ b/tests/qemu-iotests/205
@@ -85,13 +85,13 @@ class TestNbdServerRemove(iotests.QMPTestCase):
 
     def do_test_connect_after_remove(self, mode=None):
         args = ('-r', '-f', 'raw', '-c', 'read 0 512', nbd_uri)
-        self.assertReadOk(qemu_io(*args))
+        self.assertReadOk(qemu_io(*args).stdout)
 
         result = self.remove_export('exp', mode)
         self.assert_qmp(result, 'return', {})
 
         self.assertExportNotFound('exp')
-        self.assertConnectFailed(qemu_io(*args))
+        self.assertConnectFailed(qemu_io(*args, check=False).stdout)
 
     def test_connect_after_remove_default(self):
         self.do_test_connect_after_remove()
-- 
2.34.1

[PATCH 10/15] iotests/245: fixup

[John Snow]

(Merge with prior patch.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/242 | 2 +-
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 4b7ec16af6..ecc851582a 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -22,7 +22,7 @@
 import iotests
 import json
 import struct
-from iotests import qemu_img_create, qemu_io, qemu_img_info, \
+from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
     file_path, img_info_log, log, filter_qemu_io
 
 iotests.script_initialize(supported_fmts=['qcow2'],
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 8cbed7821b..efdad1a0c4 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
     # Reopen an image several times changing some of its options
     def test_reopen(self):
         # Check whether the filesystem supports O_DIRECT
-        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c', 'quit', hd_path[0]):
+        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c', 'quit', hd_path[0]).stdout:
             supports_direct = False
         else:
             supports_direct = True
-- 
2.34.1

Re: [PATCH 10/15] iotests/245: fixup

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:50PM -0400, John Snow wrote:
> (Merge with prior patch.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/242 | 2 +-
>  tests/qemu-iotests/245 | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>

Okay, now that I've looked at the fixups, and played a bit more with
applying the patches locally to this point, you can give the squashed
patch:

Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 10/15] iotests/245: fixup

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> (Merge with prior patch.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/242 | 2 +-
>   tests/qemu-iotests/245 | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
> index 4b7ec16af6..ecc851582a 100755
> --- a/tests/qemu-iotests/242
> +++ b/tests/qemu-iotests/242
> @@ -22,7 +22,7 @@
>   import iotests
>   import json
>   import struct
> -from iotests import qemu_img_create, qemu_io, qemu_img_info, \
> +from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
>       file_path, img_info_log, log, filter_qemu_io
>   
>   iotests.script_initialize(supported_fmts=['qcow2'],
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index 8cbed7821b..efdad1a0c4 100755
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>       # Reopen an image several times changing some of its options
>       def test_reopen(self):
>           # Check whether the filesystem supports O_DIRECT
> -        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c', 'quit', hd_path[0]):
> +        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c', 'quit', hd_path[0]).stdout:

This is to verify that O_DIRECT works or not.  If it doesn’t work, this 
will fail, so we need to pass check=False here.

(Or this test fails on tmpfs.)

Hanna

>               supports_direct = False
>           else:
>               supports_direct = True

Re: [PATCH 10/15] iotests/245: fixup

[John Snow]

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

On Tue, Mar 22, 2022, 12:30 PM Hanna Reitz <hreitz@redhat.com> wrote:

> On 18.03.22 21:36, John Snow wrote:
> > (Merge with prior patch.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/242 | 2 +-
> >   tests/qemu-iotests/245 | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
> > index 4b7ec16af6..ecc851582a 100755
> > --- a/tests/qemu-iotests/242
> > +++ b/tests/qemu-iotests/242
> > @@ -22,7 +22,7 @@
> >   import iotests
> >   import json
> >   import struct
> > -from iotests import qemu_img_create, qemu_io, qemu_img_info, \
> > +from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
> >       file_path, img_info_log, log, filter_qemu_io
> >
> >   iotests.script_initialize(supported_fmts=['qcow2'],
> > diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> > index 8cbed7821b..efdad1a0c4 100755
> > --- a/tests/qemu-iotests/245
> > +++ b/tests/qemu-iotests/245
> > @@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
> >       # Reopen an image several times changing some of its options
> >       def test_reopen(self):
> >           # Check whether the filesystem supports O_DIRECT
> > -        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c',
> 'quit', hd_path[0]):
> > +        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none', '-c',
> 'quit', hd_path[0]).stdout:
>
> This is to verify that O_DIRECT works or not.  If it doesn’t work, this
> will fail, so we need to pass check=False here.
>
> (Or this test fails on tmpfs.)
>
> Hanna
>

Oh, I didn't realize a solitary "quit" command could still fail. Thanks for
the tip.

[-- Attachment #2: Type: text/html, Size: 2541 bytes --]

Re: [PATCH 10/15] iotests/245: fixup

[Hanna Reitz]

On 22.03.22 17:36, John Snow wrote:
>
>
> On Tue, Mar 22, 2022, 12:30 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 18.03.22 21:36, John Snow wrote:
>     > (Merge with prior patch.)
>     >
>     > Signed-off-by: John Snow <jsnow@redhat.com>
>     > ---
>     >   tests/qemu-iotests/242 | 2 +-
>     >   tests/qemu-iotests/245 | 2 +-
>     >   2 files changed, 2 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
>     > index 4b7ec16af6..ecc851582a 100755
>     > --- a/tests/qemu-iotests/242
>     > +++ b/tests/qemu-iotests/242
>     > @@ -22,7 +22,7 @@
>     >   import iotests
>     >   import json
>     >   import struct
>     > -from iotests import qemu_img_create, qemu_io, qemu_img_info, \
>     > +from iotests import qemu_img_create, qemu_io_log, qemu_img_info, \
>     >       file_path, img_info_log, log, filter_qemu_io
>     >
>     >   iotests.script_initialize(supported_fmts=['qcow2'],
>     > diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
>     > index 8cbed7821b..efdad1a0c4 100755
>     > --- a/tests/qemu-iotests/245
>     > +++ b/tests/qemu-iotests/245
>     > @@ -217,7 +217,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>     >       # Reopen an image several times changing some of its options
>     >       def test_reopen(self):
>     >           # Check whether the filesystem supports O_DIRECT
>     > -        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none',
>     '-c', 'quit', hd_path[0]):
>     > +        if 'O_DIRECT' in qemu_io('-f', 'raw', '-t', 'none',
>     '-c', 'quit', hd_path[0]).stdout:
>
>     This is to verify that O_DIRECT works or not.  If it doesn’t work,
>     this
>     will fail, so we need to pass check=False here.
>
>     (Or this test fails on tmpfs.)
>
>     Hanna
>
>
> Oh, I didn't realize a solitary "quit" command could still fail. 
> Thanks for the tip.
>

What’s being tested is the implicit `open`. :)

Hanna

Re: [PATCH 10/15] iotests/245: fixup

[John Snow]

On Fri, Mar 18, 2022 at 4:37 PM John Snow <jsnow@redhat.com> wrote:
>
> (Merge with prior patch.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/242 | 2 +-

^ Oh, there's the stray import changes that needed to be folded into
patch #1. Fixed for v2.

[PATCH 11/15] iotests/migration-permissions: fixup

[John Snow]

(Merge into prior commit)

Note, this is a quick hack band-aid, but a follow-up patch spends the
time to refactor it a bit. This is just the quick stop-gap to prevent
bisection failures.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/migration-permissions | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/tests/migration-permissions b/tests/qemu-iotests/tests/migration-permissions
index 6be02581c7..c7afb1bd2c 100755
--- a/tests/qemu-iotests/tests/migration-permissions
+++ b/tests/qemu-iotests/tests/migration-permissions
@@ -69,7 +69,7 @@ class TestMigrationPermissions(iotests.QMPTestCase):
     def test_post_migration_permissions(self):
         # Try to access the image R/W, which should fail because virtio-blk
         # has not been configured with share-rw=on
-        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
         if not log.strip():
             print('ERROR (pre-migration): qemu-io should not be able to '
                   'access this image, but it reported no error')
@@ -84,7 +84,7 @@ class TestMigrationPermissions(iotests.QMPTestCase):
 
         # Try the same qemu-io access again, verifying that the WRITE
         # permission remains unshared
-        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
         if not log.strip():
             print('ERROR (post-migration): qemu-io should not be able to '
                   'access this image, but it reported no error')
-- 
2.34.1

[PATCH 12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test

[John Snow]

Modify this test to use assertRaises for its negative testing of
qemu_io. If the exception raised does not match the one we tell it to
expect, we get *that* exception unhandled. If we get no exception, we
get a unittest assertion failure and the provided emsg printed to
screen.

If we get the CalledProcessError exception but the output is not what we
expect, we re-raise the original CalledProcessError.

Tidy.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/tests/migration-permissions b/tests/qemu-iotests/tests/migration-permissions
index c7afb1bd2c..4e1da369c9 100755
--- a/tests/qemu-iotests/tests/migration-permissions
+++ b/tests/qemu-iotests/tests/migration-permissions
@@ -18,6 +18,8 @@
 #
 
 import os
+from subprocess import CalledProcessError
+
 import iotests
 from iotests import imgfmt, qemu_img_create, qemu_io
 
@@ -69,13 +71,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
     def test_post_migration_permissions(self):
         # Try to access the image R/W, which should fail because virtio-blk
         # has not been configured with share-rw=on
-        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
-        if not log.strip():
-            print('ERROR (pre-migration): qemu-io should not be able to '
-                  'access this image, but it reported no error')
-        else:
-            # This is the expected output
-            assert 'Is another process using the image' in log
+        emsg = ('ERROR (pre-migration): qemu-io should not be able to '
+                'access this image, but it reported no error')
+        with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
+            qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+        if 'Is another process using the image' not in ctx.exception.stdout:
+            raise ctx.exception
 
         # Now migrate the VM
         self.vm_s.qmp('migrate', uri=f'unix:{mig_sock}')
@@ -84,13 +85,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
 
         # Try the same qemu-io access again, verifying that the WRITE
         # permission remains unshared
-        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
-        if not log.strip():
-            print('ERROR (post-migration): qemu-io should not be able to '
-                  'access this image, but it reported no error')
-        else:
-            # This is the expected output
-            assert 'Is another process using the image' in log
+        emsg = ('ERROR (post-migration): qemu-io should not be able to '
+                'access this image, but it reported no error')
+        with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
+            qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+        if 'Is another process using the image' not in ctx.exception.stdout:
+            raise ctx.exception
 
 
 if __name__ == '__main__':
-- 
2.34.1

Re: [PATCH 12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:52PM -0400, John Snow wrote:
> Modify this test to use assertRaises for its negative testing of
> qemu_io. If the exception raised does not match the one we tell it to
> expect, we get *that* exception unhandled. If we get no exception, we
> get a unittest assertion failure and the provided emsg printed to
> screen.
> 
> If we get the CalledProcessError exception but the output is not what we
> expect, we re-raise the original CalledProcessError.
> 
> Tidy.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
>  1 file changed, 14 insertions(+), 14 deletions(-)

The 11/15 patch is also included in my R-b of the squashed patch.

For this patch, it's a wash on lines of code, but certainly a more
robust test now than with the hack of the fixup patch 11/15.

> 
> diff --git a/tests/qemu-iotests/tests/migration-permissions b/tests/qemu-iotests/tests/migration-permissions
> index c7afb1bd2c..4e1da369c9 100755
> --- a/tests/qemu-iotests/tests/migration-permissions
> +++ b/tests/qemu-iotests/tests/migration-permissions
> @@ -18,6 +18,8 @@
>  #
>  
>  import os
> +from subprocess import CalledProcessError
> +
>  import iotests
>  from iotests import imgfmt, qemu_img_create, qemu_io
>  
> @@ -69,13 +71,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
>      def test_post_migration_permissions(self):
>          # Try to access the image R/W, which should fail because virtio-blk
>          # has not been configured with share-rw=on
> -        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
> -        if not log.strip():
> -            print('ERROR (pre-migration): qemu-io should not be able to '
> -                  'access this image, but it reported no error')
> -        else:
> -            # This is the expected output
> -            assert 'Is another process using the image' in log
> +        emsg = ('ERROR (pre-migration): qemu-io should not be able to '
> +                'access this image, but it reported no error')
> +        with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
> +            qemu_io('-f', imgfmt, '-c', 'quit', test_img)
> +        if 'Is another process using the image' not in ctx.exception.stdout:

Wait a minute - is 'ctx' still in scope after the 'with ... as ctx:'
with scope ends?  Python continues to surprise me.

> +            raise ctx.exception
>  
>          # Now migrate the VM
>          self.vm_s.qmp('migrate', uri=f'unix:{mig_sock}')
> @@ -84,13 +85,12 @@ class TestMigrationPermissions(iotests.QMPTestCase):
>  
>          # Try the same qemu-io access again, verifying that the WRITE
>          # permission remains unshared
> -        log = qemu_io('-f', imgfmt, '-c', 'quit', test_img, check=False).stdout
> -        if not log.strip():
> -            print('ERROR (post-migration): qemu-io should not be able to '
> -                  'access this image, but it reported no error')
> -        else:
> -            # This is the expected output
> -            assert 'Is another process using the image' in log
> +        emsg = ('ERROR (post-migration): qemu-io should not be able to '
> +                'access this image, but it reported no error')
> +        with self.assertRaises(CalledProcessError, msg=emsg) as ctx:
> +            qemu_io('-f', imgfmt, '-c', 'quit', test_img)
> +        if 'Is another process using the image' not in ctx.exception.stdout:
> +            raise ctx.exception

The cover letter didn't mention a Based-on: tag, and so my first
attempt to reproduce this hit a rebase error on 5/15 followed by
'./check -raw migration-permissions' on this patch failing with:

+Traceback (most recent call last):
+  File "/home/eblake/qemu/tests/qemu-iotests/tests/migration-permissions", line 77, in test_post_migration_permissions
+    qemu_io('-f', imgfmt, '-c', 'quit', test_img)
+  File "/home/eblake/qemu/tests/qemu-iotests/iotests.py", line 334, in qemu_io
+    return qemu_tool(*qemu_io_wrap_args(args),
+  File "/home/eblake/qemu/tests/qemu-iotests/iotests.py", line 249, in qemu_tool
+    raise VerboseProcessError(
+NameError: name 'VerboseProcessError' is not defined

so it's pretty obvious I didn't apply the pre-requisite patch that
introduced VerboseProcessError in order to fix qemu_img() first.  But
with more churn on my end (applying your v4 qemu_img series first), it
looks like accessing ctx.exception after the with clause ends (even
though the with clause is what brought ctx into existance) works, so:

Reviewed-by: Eric Blake <eblake@redhat.com>
Tested-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> Modify this test to use assertRaises for its negative testing of
> qemu_io. If the exception raised does not match the one we tell it to
> expect, we get *that* exception unhandled. If we get no exception, we
> get a unittest assertion failure and the provided emsg printed to
> screen.
>
> If we get the CalledProcessError exception but the output is not what we
> expect, we re-raise the original CalledProcessError.
>
> Tidy.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
>   1 file changed, 14 insertions(+), 14 deletions(-)

Just like Eric I don’t find it so tidy that `ctx` exists outside of the 
`with` block, but re-raising the exception is indeed better, so:

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 12/15] iotests/migration-permissions: use assertRaises() for qemu_io() negative test

[John Snow]

On Tue, Mar 22, 2022 at 12:37 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 18.03.22 21:36, John Snow wrote:
> > Modify this test to use assertRaises for its negative testing of
> > qemu_io. If the exception raised does not match the one we tell it to
> > expect, we get *that* exception unhandled. If we get no exception, we
> > get a unittest assertion failure and the provided emsg printed to
> > screen.
> >
> > If we get the CalledProcessError exception but the output is not what we
> > expect, we re-raise the original CalledProcessError.
> >
> > Tidy.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   .../qemu-iotests/tests/migration-permissions  | 28 +++++++++----------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
>
> Just like Eric I don’t find it so tidy that `ctx` exists outside of the
> `with` block, but re-raising the exception is indeed better, so:
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

"Sorry about Python."

You still have the object, but __exit__() will have been called, so
resources will have been freed, references to live objects severed,
etc. unittests is designed around this for Exception testing, etc.
It's not a *real* scope. It just looks like one.

I'll add a little note in the commit message for the next person who wonders.

--js

[PATCH 13/15] iotests: remove qemu_io_pipe_and_status()

[John Snow]

I know we just added it, sorry. This is done in favor of qemu_io() which
*also* returns the console output and status, but with more robust error
handling on failure.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py           |  3 ---
 tests/qemu-iotests/tests/image-fleecing | 12 +++---------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 58ea766568..e8f38e7ad3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -365,9 +365,6 @@ def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
     return qemu_tool(*qemu_io_wrap_args(args),
                      check=check, combine_stdio=combine_stdio)
 
-def qemu_io_pipe_and_status(*args):
-    return qemu_tool_pipe_and_status('qemu-io', qemu_io_wrap_args(args))
-
 def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
     result = qemu_io(*args, check=False)
     log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index b7e5076104..07a4ea7bc4 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -23,8 +23,7 @@
 # Creator/Owner: John Snow <jsnow@redhat.com>
 
 import iotests
-from iotests import log, qemu_img, qemu_io, qemu_io_silent, \
-    qemu_io_pipe_and_status
+from iotests import log, qemu_img, qemu_io, qemu_io_silent
 
 iotests.script_initialize(
     supported_fmts=['qcow2'],
@@ -185,10 +184,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
         for p in patterns + zeroes:
             cmd = 'read -P%s %s %s' % p
             log(cmd)
-            out, ret = qemu_io_pipe_and_status('-r', '-f', 'raw', '-c', cmd,
-                                               nbd_uri)
-            if ret != 0:
-                print(out)
+            qemu_io('-r', '-f', 'raw', '-c', cmd, nbd_uri)
 
     log('')
     log('--- Testing COW ---')
@@ -228,9 +224,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
             args += [target_img_path]
         else:
             args += ['-f', 'raw', nbd_uri]
-        out, ret = qemu_io_pipe_and_status(*args)
-        if ret != 0:
-            print(out)
+        qemu_io(*args)
 
     log('')
     log('--- Cleanup ---')
-- 
2.34.1

Re: [PATCH 13/15] iotests: remove qemu_io_pipe_and_status()

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:53PM -0400, John Snow wrote:
> I know we just added it, sorry. This is done in favor of qemu_io() which
> *also* returns the console output and status, but with more robust error
> handling on failure.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py           |  3 ---
>  tests/qemu-iotests/tests/image-fleecing | 12 +++---------
>  2 files changed, 3 insertions(+), 12 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 13/15] iotests: remove qemu_io_pipe_and_status()

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> I know we just added it, sorry. This is done in favor of qemu_io() which
> *also* returns the console output and status, but with more robust error
> handling on failure.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py           |  3 ---
>   tests/qemu-iotests/tests/image-fleecing | 12 +++---------
>   2 files changed, 3 insertions(+), 12 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 13/15] iotests: remove qemu_io_pipe_and_status()

[John Snow]

On Tue, Mar 22, 2022 at 12:39 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 18.03.22 21:36, John Snow wrote:
> > I know we just added it, sorry. This is done in favor of qemu_io() which
> > *also* returns the console output and status, but with more robust error
> > handling on failure.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py           |  3 ---
> >   tests/qemu-iotests/tests/image-fleecing | 12 +++---------
> >   2 files changed, 3 insertions(+), 12 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>

I goofed this patch -- some of the failures in this test are expected
and this patch breaks the test. Dropping the R-Bs. v2 will explain
what's up in the commit message.

--js

[PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().

[John Snow]

Like qemu-img, qemu-io returning 0 should be the norm and not the
exception. Remove all calls to qemu_io_silent that just assert the
return code is zero (That's every last call, as it turns out), and
replace them with a normal qemu_io() call.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/216                        | 12 +++++-----
 tests/qemu-iotests/218                        |  5 ++---
 tests/qemu-iotests/224                        |  4 ++--
 tests/qemu-iotests/258                        | 12 +++++-----
 tests/qemu-iotests/298                        | 16 ++++++--------
 tests/qemu-iotests/310                        | 22 +++++++++----------
 tests/qemu-iotests/iotests.py                 | 16 --------------
 tests/qemu-iotests/tests/image-fleecing       |  4 ++--
 .../tests/mirror-ready-cancel-error           |  2 +-
 .../qemu-iotests/tests/stream-error-on-reset  |  4 ++--
 10 files changed, 39 insertions(+), 58 deletions(-)

diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216
index 88b385afa3..97de1cda61 100755
--- a/tests/qemu-iotests/216
+++ b/tests/qemu-iotests/216
@@ -21,7 +21,7 @@
 # Creator/Owner: Max Reitz <mreitz@redhat.com>
 
 import iotests
-from iotests import log, qemu_img, qemu_io_silent
+from iotests import log, qemu_img, qemu_io
 
 # Need backing file support
 iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'],
@@ -52,10 +52,10 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('')
 
     qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
-    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
+    qemu_io(base_img_path, '-c', 'write -P 1 0M 1M')
     qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
              '-F', iotests.imgfmt, top_img_path)
-    assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0
+    qemu_io(top_img_path,  '-c', 'write -P 2 1M 1M')
 
     log('Done')
 
@@ -110,8 +110,8 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('--- Checking COR result ---')
     log('')
 
-    assert qemu_io_silent(base_img_path, '-c', 'discard 0 64M') == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 1 0M 1M') == 0
-    assert qemu_io_silent(top_img_path,  '-c', 'read -P 2 1M 1M') == 0
+    qemu_io(base_img_path, '-c', 'discard 0 64M')
+    qemu_io(top_img_path,  '-c', 'read -P 1 0M 1M')
+    qemu_io(top_img_path,  '-c', 'read -P 2 1M 1M')
 
     log('Done')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 853ed52b34..0c717c9b7f 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -28,7 +28,7 @@
 # Creator/Owner: Max Reitz <mreitz@redhat.com>
 
 import iotests
-from iotests import log, qemu_img, qemu_io_silent
+from iotests import log, qemu_img, qemu_io
 
 iotests.script_initialize(supported_fmts=['qcow2', 'raw'])
 
@@ -146,8 +146,7 @@ with iotests.VM() as vm, \
      iotests.FilePath('src.img') as src_img_path:
 
     qemu_img('create', '-f', iotests.imgfmt, src_img_path, '64M')
-    assert qemu_io_silent('-f', iotests.imgfmt, src_img_path,
-                          '-c', 'write -P 42 0M 64M') == 0
+    qemu_io('-f', iotests.imgfmt, src_img_path, '-c', 'write -P 42 0M 64M')
 
     vm.launch()
 
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index c31c55b49d..8eb3ceb8d1 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -22,7 +22,7 @@
 # Creator/Owner: Max Reitz <mreitz@redhat.com>
 
 import iotests
-from iotests import log, qemu_img, qemu_io_silent, filter_qmp_testfiles, \
+from iotests import log, qemu_img, qemu_io, filter_qmp_testfiles, \
                     filter_qmp_imgfmt
 import json
 
@@ -54,7 +54,7 @@ for filter_node_name in False, True:
                  '-F', iotests.imgfmt, top_img_path)
 
         # Something to commit
-        assert qemu_io_silent(mid_img_path, '-c', 'write -P 1 0 1M') == 0
+        qemu_io(mid_img_path, '-c', 'write -P 1 0 1M')
 
         vm.launch()
 
diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index 7798a04d7d..e11a0e09b3 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -21,7 +21,7 @@
 # Creator/Owner: Max Reitz <mreitz@redhat.com>
 
 import iotests
-from iotests import log, qemu_img, qemu_io_silent, \
+from iotests import log, qemu_img, qemu_io, \
         filter_qmp_testfiles, filter_qmp_imgfmt
 
 # Returns a node for blockdev-add
@@ -86,15 +86,15 @@ def test_concurrent_finish(write_to_stream_node):
         if write_to_stream_node:
             # This is what (most of the time) makes commit finish
             # earlier and then pull in stream
-            assert qemu_io_silent(node2_path,
-                                  '-c', 'write %iK 64K' % (65536 - 192),
-                                  '-c', 'write %iK 64K' % (65536 -  64)) == 0
+            qemu_io(node2_path,
+                    '-c', 'write %iK 64K' % (65536 - 192),
+                    '-c', 'write %iK 64K' % (65536 -  64))
 
             stream_throttle='tg'
         else:
             # And this makes stream finish earlier
-            assert qemu_io_silent(node1_path,
-                                  '-c', 'write %iK 64K' % (65536 - 64)) == 0
+            qemu_io(node1_path,
+                    '-c', 'write %iK 64K' % (65536 - 64))
 
             commit_throttle='tg'
 
diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
index fae72211b1..9d6d95d953 100755
--- a/tests/qemu-iotests/298
+++ b/tests/qemu-iotests/298
@@ -129,16 +129,14 @@ class TestTruncate(iotests.QMPTestCase):
         os.remove(refdisk)
 
     def do_test(self, prealloc_mode, new_size):
-        ret = iotests.qemu_io_silent('--image-opts', '-c', 'write 0 10M', '-c',
-                                     f'truncate -m {prealloc_mode} {new_size}',
-                                     drive_opts)
-        self.assertEqual(ret, 0)
+        iotests.qemu_io('--image-opts', '-c', 'write 0 10M', '-c',
+                        f'truncate -m {prealloc_mode} {new_size}',
+                        drive_opts)
 
-        ret = iotests.qemu_io_silent('-f', iotests.imgfmt, '-c', 'write 0 10M',
-                                     '-c',
-                                     f'truncate -m {prealloc_mode} {new_size}',
-                                     refdisk)
-        self.assertEqual(ret, 0)
+        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 10M',
+                        '-c',
+                        f'truncate -m {prealloc_mode} {new_size}',
+                        refdisk)
 
         stat = os.stat(disk)
         refstat = os.stat(refdisk)
diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
index e3bfedc7fd..2496495f50 100755
--- a/tests/qemu-iotests/310
+++ b/tests/qemu-iotests/310
@@ -21,7 +21,7 @@
 #
 
 import iotests
-from iotests import log, qemu_img, qemu_io_silent
+from iotests import log, qemu_img, qemu_io
 
 # Need backing file support
 iotests.script_initialize(supported_fmts=['qcow2'],
@@ -44,15 +44,15 @@ with iotests.FilePath('base.img') as base_img_path, \
     log('')
 
     qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
-    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
-    assert qemu_io_silent(base_img_path, '-c', 'write -P 1 3M 1M') == 0
+    qemu_io(base_img_path, '-c', 'write -P 1 0M 1M')
+    qemu_io(base_img_path, '-c', 'write -P 1 3M 1M')
     qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
              '-F', iotests.imgfmt, mid_img_path)
-    assert qemu_io_silent(mid_img_path, '-c', 'write -P 3 2M 1M') == 0
-    assert qemu_io_silent(mid_img_path, '-c', 'write -P 3 4M 1M') == 0
+    qemu_io(mid_img_path, '-c', 'write -P 3 2M 1M')
+    qemu_io(mid_img_path, '-c', 'write -P 3 4M 1M')
     qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
              '-F', iotests.imgfmt, top_img_path)
-    assert qemu_io_silent(top_img_path, '-c', 'write -P 2 1M 1M') == 0
+    qemu_io(top_img_path, '-c', 'write -P 2 1M 1M')
 
 #      0 1 2 3 4
 # top    2
@@ -107,10 +107,10 @@ with iotests.FilePath('base.img') as base_img_path, \
     # Detach backing to check that we can read the data from the top level now
     qemu_img('rebase', '-u', '-b', '', '-f', iotests.imgfmt, top_img_path)
 
-    assert qemu_io_silent(top_img_path, '-c', 'read -P 0 0 1M') == 0
-    assert qemu_io_silent(top_img_path, '-c', 'read -P 2 1M 1M') == 0
-    assert qemu_io_silent(top_img_path, '-c', 'read -P 3 2M 1M') == 0
-    assert qemu_io_silent(top_img_path, '-c', 'read -P 0 3M 1M') == 0
-    assert qemu_io_silent(top_img_path, '-c', 'read -P 3 4M 1M') == 0
+    qemu_io(top_img_path, '-c', 'read -P 0 0 1M')
+    qemu_io(top_img_path, '-c', 'read -P 2 1M 1M')
+    qemu_io(top_img_path, '-c', 'read -P 3 2M 1M')
+    qemu_io(top_img_path, '-c', 'read -P 0 3M 1M')
+    qemu_io(top_img_path, '-c', 'read -P 3 4M 1M')
 
     log('Done')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e8f38e7ad3..0b631e1f8c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -370,22 +370,6 @@ def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
     log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
     return result
 
-def qemu_io_silent(*args):
-    '''Run qemu-io and return the exit code, suppressing stdout'''
-    args = qemu_io_wrap_args(args)
-    result = subprocess.run(args, stdout=subprocess.DEVNULL, check=False)
-    if result.returncode < 0:
-        sys.stderr.write('qemu-io received signal %i: %s\n' %
-                         (-result.returncode, ' '.join(args)))
-    return result.returncode
-
-def qemu_io_silent_check(*args):
-    '''Run qemu-io and return the true if subprocess returned 0'''
-    args = qemu_io_wrap_args(args)
-    result = subprocess.run(args, stdout=subprocess.DEVNULL,
-                            stderr=subprocess.STDOUT, check=False)
-    return result.returncode == 0
-
 class QemuIoInteractive:
     def __init__(self, *args):
         self.args = qemu_io_wrap_args(args)
diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index 07a4ea7bc4..8d5b0192c6 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -23,7 +23,7 @@
 # Creator/Owner: John Snow <jsnow@redhat.com>
 
 import iotests
-from iotests import log, qemu_img, qemu_io, qemu_io_silent
+from iotests import log, qemu_img, qemu_io
 
 iotests.script_initialize(
     supported_fmts=['qcow2'],
@@ -254,7 +254,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
     for p in overwrite + remainder:
         cmd = 'read -P%s %s %s' % p
         log(cmd)
-        assert qemu_io_silent(base_img_path, '-c', cmd) == 0
+        qemu_io(base_img_path, '-c', cmd)
 
     log('')
     log('Done')
diff --git a/tests/qemu-iotests/tests/mirror-ready-cancel-error b/tests/qemu-iotests/tests/mirror-ready-cancel-error
index 1d0e333b5e..01217459b9 100755
--- a/tests/qemu-iotests/tests/mirror-ready-cancel-error
+++ b/tests/qemu-iotests/tests/mirror-ready-cancel-error
@@ -37,7 +37,7 @@ class TestMirrorReadyCancelError(iotests.QMPTestCase):
         # Ensure that mirror will copy something before READY so the
         # target format layer will forward the pre-READY flush to its
         # file child
-        assert iotests.qemu_io_silent('-c', 'write -P 1 0 64k', source) == 0
+        iotests.qemu_io('-c', 'write -P 1 0 64k', source)
 
         self.vm = iotests.VM()
         self.vm.launch()
diff --git a/tests/qemu-iotests/tests/stream-error-on-reset b/tests/qemu-iotests/tests/stream-error-on-reset
index 389ae822b8..5a8c3a9e8d 100755
--- a/tests/qemu-iotests/tests/stream-error-on-reset
+++ b/tests/qemu-iotests/tests/stream-error-on-reset
@@ -21,7 +21,7 @@
 
 import os
 import iotests
-from iotests import imgfmt, qemu_img_create, qemu_io_silent, QMPTestCase
+from iotests import imgfmt, qemu_img_create, qemu_io, QMPTestCase
 
 
 image_size = 1 * 1024 * 1024
@@ -55,7 +55,7 @@ class TestStreamErrorOnReset(QMPTestCase):
         - top image is attached to a virtio-scsi device
         """
         qemu_img_create('-f', imgfmt, base, str(image_size))
-        assert qemu_io_silent('-c', f'write 0 {data_size}', base) == 0
+        qemu_io('-c', f'write 0 {data_size}', base)
         qemu_img_create('-f', imgfmt, top, str(image_size))
 
         self.vm = iotests.VM()
-- 
2.34.1

Re: [PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:54PM -0400, John Snow wrote:
> Like qemu-img, qemu-io returning 0 should be the norm and not the
> exception. Remove all calls to qemu_io_silent that just assert the
> return code is zero (That's every last call, as it turns out), and
> replace them with a normal qemu_io() call.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/216                        | 12 +++++-----
>  tests/qemu-iotests/218                        |  5 ++---
>  tests/qemu-iotests/224                        |  4 ++--
>  tests/qemu-iotests/258                        | 12 +++++-----
>  tests/qemu-iotests/298                        | 16 ++++++--------
>  tests/qemu-iotests/310                        | 22 +++++++++----------
>  tests/qemu-iotests/iotests.py                 | 16 --------------
>  tests/qemu-iotests/tests/image-fleecing       |  4 ++--
>  .../tests/mirror-ready-cancel-error           |  2 +-
>  .../qemu-iotests/tests/stream-error-on-reset  |  4 ++--
>  10 files changed, 39 insertions(+), 58 deletions(-)

> +++ b/tests/qemu-iotests/258
> @@ -21,7 +21,7 @@
>  # Creator/Owner: Max Reitz <mreitz@redhat.com>
>  
>  import iotests
> -from iotests import log, qemu_img, qemu_io_silent, \
> +from iotests import log, qemu_img, qemu_io, \
>          filter_qmp_testfiles, filter_qmp_imgfmt
>  
>  # Returns a node for blockdev-add
> @@ -86,15 +86,15 @@ def test_concurrent_finish(write_to_stream_node):
>          if write_to_stream_node:
>              # This is what (most of the time) makes commit finish
>              # earlier and then pull in stream
> -            assert qemu_io_silent(node2_path,
> -                                  '-c', 'write %iK 64K' % (65536 - 192),
> -                                  '-c', 'write %iK 64K' % (65536 -  64)) == 0
> +            qemu_io(node2_path,
> +                    '-c', 'write %iK 64K' % (65536 - 192),
> +                    '-c', 'write %iK 64K' % (65536 -  64))
>  
>              stream_throttle='tg'
>          else:
>              # And this makes stream finish earlier
> -            assert qemu_io_silent(node1_path,
> -                                  '-c', 'write %iK 64K' % (65536 - 64)) == 0
> +            qemu_io(node1_path,
> +                    '-c', 'write %iK 64K' % (65536 - 64))

This could fit on one line.  But the split matches the instance
earlier in the hunk that needed two lines.

>  
>              commit_throttle='tg'
>  
> diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
> index fae72211b1..9d6d95d953 100755
> --- a/tests/qemu-iotests/298
> +++ b/tests/qemu-iotests/298
> @@ -129,16 +129,14 @@ class TestTruncate(iotests.QMPTestCase):
>          os.remove(refdisk)
>  
>      def do_test(self, prealloc_mode, new_size):
> -        ret = iotests.qemu_io_silent('--image-opts', '-c', 'write 0 10M', '-c',
> -                                     f'truncate -m {prealloc_mode} {new_size}',
> -                                     drive_opts)
> -        self.assertEqual(ret, 0)
> +        iotests.qemu_io('--image-opts', '-c', 'write 0 10M', '-c',
> +                        f'truncate -m {prealloc_mode} {new_size}',
> +                        drive_opts)
>  
> -        ret = iotests.qemu_io_silent('-f', iotests.imgfmt, '-c', 'write 0 10M',
> -                                     '-c',
> -                                     f'truncate -m {prealloc_mode} {new_size}',
> -                                     refdisk)
> -        self.assertEqual(ret, 0)
> +        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 10M',
> +                        '-c',
> +                        f'truncate -m {prealloc_mode} {new_size}',

And as long as I'm pontificating on line wraps, putting '-c' and
f'truncate...' on the same line might make sense.

At any rate, whether or not you choose to do anything about my
observations on cosmetic line wraps:

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().

[John Snow]

On Mon, Mar 21, 2022 at 2:16 PM Eric Blake <eblake@redhat.com> wrote:
>
> On Fri, Mar 18, 2022 at 04:36:54PM -0400, John Snow wrote:
> > Like qemu-img, qemu-io returning 0 should be the norm and not the
> > exception. Remove all calls to qemu_io_silent that just assert the
> > return code is zero (That's every last call, as it turns out), and
> > replace them with a normal qemu_io() call.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  tests/qemu-iotests/216                        | 12 +++++-----
> >  tests/qemu-iotests/218                        |  5 ++---
> >  tests/qemu-iotests/224                        |  4 ++--
> >  tests/qemu-iotests/258                        | 12 +++++-----
> >  tests/qemu-iotests/298                        | 16 ++++++--------
> >  tests/qemu-iotests/310                        | 22 +++++++++----------
> >  tests/qemu-iotests/iotests.py                 | 16 --------------
> >  tests/qemu-iotests/tests/image-fleecing       |  4 ++--
> >  .../tests/mirror-ready-cancel-error           |  2 +-
> >  .../qemu-iotests/tests/stream-error-on-reset  |  4 ++--
> >  10 files changed, 39 insertions(+), 58 deletions(-)
>
> > +++ b/tests/qemu-iotests/258
> > @@ -21,7 +21,7 @@
> >  # Creator/Owner: Max Reitz <mreitz@redhat.com>
> >
> >  import iotests
> > -from iotests import log, qemu_img, qemu_io_silent, \
> > +from iotests import log, qemu_img, qemu_io, \
> >          filter_qmp_testfiles, filter_qmp_imgfmt
> >
> >  # Returns a node for blockdev-add
> > @@ -86,15 +86,15 @@ def test_concurrent_finish(write_to_stream_node):
> >          if write_to_stream_node:
> >              # This is what (most of the time) makes commit finish
> >              # earlier and then pull in stream
> > -            assert qemu_io_silent(node2_path,
> > -                                  '-c', 'write %iK 64K' % (65536 - 192),
> > -                                  '-c', 'write %iK 64K' % (65536 -  64)) == 0
> > +            qemu_io(node2_path,
> > +                    '-c', 'write %iK 64K' % (65536 - 192),
> > +                    '-c', 'write %iK 64K' % (65536 -  64))
> >
> >              stream_throttle='tg'
> >          else:
> >              # And this makes stream finish earlier
> > -            assert qemu_io_silent(node1_path,
> > -                                  '-c', 'write %iK 64K' % (65536 - 64)) == 0
> > +            qemu_io(node1_path,
> > +                    '-c', 'write %iK 64K' % (65536 - 64))
>
> This could fit on one line.  But the split matches the instance
> earlier in the hunk that needed two lines.
>
> >
> >              commit_throttle='tg'
> >
> > diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298
> > index fae72211b1..9d6d95d953 100755
> > --- a/tests/qemu-iotests/298
> > +++ b/tests/qemu-iotests/298
> > @@ -129,16 +129,14 @@ class TestTruncate(iotests.QMPTestCase):
> >          os.remove(refdisk)
> >
> >      def do_test(self, prealloc_mode, new_size):
> > -        ret = iotests.qemu_io_silent('--image-opts', '-c', 'write 0 10M', '-c',
> > -                                     f'truncate -m {prealloc_mode} {new_size}',
> > -                                     drive_opts)
> > -        self.assertEqual(ret, 0)
> > +        iotests.qemu_io('--image-opts', '-c', 'write 0 10M', '-c',
> > +                        f'truncate -m {prealloc_mode} {new_size}',
> > +                        drive_opts)
> >
> > -        ret = iotests.qemu_io_silent('-f', iotests.imgfmt, '-c', 'write 0 10M',
> > -                                     '-c',
> > -                                     f'truncate -m {prealloc_mode} {new_size}',
> > -                                     refdisk)
> > -        self.assertEqual(ret, 0)
> > +        iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 10M',
> > +                        '-c',
> > +                        f'truncate -m {prealloc_mode} {new_size}',
>
> And as long as I'm pontificating on line wraps, putting '-c' and
> f'truncate...' on the same line might make sense.
>
> At any rate, whether or not you choose to do anything about my
> observations on cosmetic line wraps:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Rolled them in, why not.

--js

Re: [PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> Like qemu-img, qemu-io returning 0 should be the norm and not the
> exception. Remove all calls to qemu_io_silent that just assert the
> return code is zero (That's every last call, as it turns out), and
> replace them with a normal qemu_io() call.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/216                        | 12 +++++-----
>   tests/qemu-iotests/218                        |  5 ++---
>   tests/qemu-iotests/224                        |  4 ++--
>   tests/qemu-iotests/258                        | 12 +++++-----
>   tests/qemu-iotests/298                        | 16 ++++++--------
>   tests/qemu-iotests/310                        | 22 +++++++++----------
>   tests/qemu-iotests/iotests.py                 | 16 --------------
>   tests/qemu-iotests/tests/image-fleecing       |  4 ++--
>   .../tests/mirror-ready-cancel-error           |  2 +-
>   .../qemu-iotests/tests/stream-error-on-reset  |  4 ++--
>   10 files changed, 39 insertions(+), 58 deletions(-)

qemu_io_silent_check() was unused anyway, right...?

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 14/15] iotests: remove qemu_io_silent() and qemu_io_silent_check().

[John Snow]

On Tue, Mar 22, 2022 at 1:00 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 18.03.22 21:36, John Snow wrote:
> > Like qemu-img, qemu-io returning 0 should be the norm and not the
> > exception. Remove all calls to qemu_io_silent that just assert the
> > return code is zero (That's every last call, as it turns out), and
> > replace them with a normal qemu_io() call.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/216                        | 12 +++++-----
> >   tests/qemu-iotests/218                        |  5 ++---
> >   tests/qemu-iotests/224                        |  4 ++--
> >   tests/qemu-iotests/258                        | 12 +++++-----
> >   tests/qemu-iotests/298                        | 16 ++++++--------
> >   tests/qemu-iotests/310                        | 22 +++++++++----------
> >   tests/qemu-iotests/iotests.py                 | 16 --------------
> >   tests/qemu-iotests/tests/image-fleecing       |  4 ++--
> >   .../tests/mirror-ready-cancel-error           |  2 +-
> >   .../qemu-iotests/tests/stream-error-on-reset  |  4 ++--
> >   10 files changed, 39 insertions(+), 58 deletions(-)
>
> qemu_io_silent_check() was unused anyway, right...?
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

As far as I can tell.

[PATCH 15/15] iotests: make qemu_io_log() check return codes by default

[John Snow]

Just like qemu_img_log(), upgrade qemu_io_log() to enforce a return code
of zero by default.

Affected tests: 242 245 255 274 303 307 nbd-reconnect-on-open

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py                  | 5 +++--
 tests/qemu-iotests/tests/nbd-reconnect-on-open | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0b631e1f8c..c05637bd57 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -365,8 +365,9 @@ def qemu_io(*args: str, check: bool = True, combine_stdio: bool = True
     return qemu_tool(*qemu_io_wrap_args(args),
                      check=check, combine_stdio=combine_stdio)
 
-def qemu_io_log(*args: str) -> subprocess.CompletedProcess[str]:
-    result = qemu_io(*args, check=False)
+def qemu_io_log(*args: str, check: bool = True
+                ) -> subprocess.CompletedProcess[str]:
+    result = qemu_io(*args, check=check)
     log(result.stdout, filters=[filter_testfiles, filter_qemu_io])
     return result
 
diff --git a/tests/qemu-iotests/tests/nbd-reconnect-on-open b/tests/qemu-iotests/tests/nbd-reconnect-on-open
index 8be721a24f..d0b401b060 100755
--- a/tests/qemu-iotests/tests/nbd-reconnect-on-open
+++ b/tests/qemu-iotests/tests/nbd-reconnect-on-open
@@ -39,7 +39,7 @@ def check_fail_to_connect(open_timeout):
     log(f'Check fail to connect with {open_timeout} seconds of timeout')
 
     start_t = time.time()
-    qemu_io_log(*create_args(open_timeout))
+    qemu_io_log(*create_args(open_timeout), check=False)
     delta_t = time.time() - start_t
 
     max_delta = open_timeout + 0.2
-- 
2.34.1

Re: [PATCH 15/15] iotests: make qemu_io_log() check return codes by default

[Eric Blake]

On Fri, Mar 18, 2022 at 04:36:55PM -0400, John Snow wrote:
> Just like qemu_img_log(), upgrade qemu_io_log() to enforce a return code
> of zero by default.
> 
> Affected tests: 242 245 255 274 303 307 nbd-reconnect-on-open
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py                  | 5 +++--
>  tests/qemu-iotests/tests/nbd-reconnect-on-open | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>

If I'm reading the commit message correctly, 'Affected tests' are all
tests that used qemu_io_log, but only nbd-reconnect-on-open needed a
change because it was the only one that explicitly tested a scenario
that triggers an expected non-zero status.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 15/15] iotests: make qemu_io_log() check return codes by default

[John Snow]

On Mon, Mar 21, 2022 at 2:22 PM Eric Blake <eblake@redhat.com> wrote:
>
> On Fri, Mar 18, 2022 at 04:36:55PM -0400, John Snow wrote:
> > Just like qemu_img_log(), upgrade qemu_io_log() to enforce a return code
> > of zero by default.
> >
> > Affected tests: 242 245 255 274 303 307 nbd-reconnect-on-open
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  tests/qemu-iotests/iotests.py                  | 5 +++--
> >  tests/qemu-iotests/tests/nbd-reconnect-on-open | 2 +-
> >  2 files changed, 4 insertions(+), 3 deletions(-)
> >
>
> If I'm reading the commit message correctly, 'Affected tests' are all
> tests that used qemu_io_log, but only nbd-reconnect-on-open needed a
> change because it was the only one that explicitly tested a scenario
> that triggers an expected non-zero status.

Yeah, it was meant more as a test aid. I'll change the wording a little bit.

>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks. By the way, did you check out iotest 040 in all of this? There
*was* a failure I wasn't sure how to address, I wrote about it in the
cover letter.

--js

Re: [PATCH 15/15] iotests: make qemu_io_log() check return codes by default

[Hanna Reitz]

On 18.03.22 21:36, John Snow wrote:
> Just like qemu_img_log(), upgrade qemu_io_log() to enforce a return code
> of zero by default.
>
> Affected tests: 242 245 255 274 303 307 nbd-reconnect-on-open
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py                  | 5 +++--
>   tests/qemu-iotests/tests/nbd-reconnect-on-open | 2 +-
>   2 files changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 00/15] iotests: add enhanced debugging info to qemu-io failures

[Hanna Reitz]

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

On 18.03.22 21:36, John Snow wrote:
> Howdy,

Heya,

[...]

> - Uh, actually, test 040 fails with this patchset and I don't understand
>    if it's intentional, harmless, a test design problem, or worse:
>
> ======================================================================
> ERROR: test_filterless_commit (__main__.TestCommitWithFilters)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>    File "/home/jsnow/src/qemu/tests/qemu-iotests/040", line 822, in tearDown
>      self.do_test_io('read')
>    File "/home/jsnow/src/qemu/tests/qemu-iotests/040", line 751, in do_test_io
>      qemu_io('-f', iotests.imgfmt,
>    File "/home/jsnow/src/qemu/tests/qemu-iotests/iotests.py", line 365, in qemu_io
>      return qemu_tool(*qemu_io_wrap_args(args),
>    File "/home/jsnow/src/qemu/tests/qemu-iotests/iotests.py", line 242, in qemu_tool
>      raise VerboseProcessError(
>
> qemu.utils.VerboseProcessError: Command
>    '('/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/../../qemu-io',
>    '--cache', 'writeback', '--aio', 'threads', '-f', 'qcow2', '-c',
>    'read -P 4 3M 1M',
>    '/home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img')'
>    returned non-zero exit status 1.
>    ┏━ output ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
>    ┃ qemu-io: can't open device
>    ┃ /home/jsnow/src/qemu/bin/git/tests/qemu-iotests/scratch/3.img:
>    ┃ Could not open backing file: Could not open backing file: Throttle
>    ┃ group 'tg' does not exist
>    ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
>
> It looks like we start with the img chain 3->2->1->0, then we commit 2
> down into 1, but checking '3' fails somewhere in the backing
> chain. Maybe a real bug?

Looks like my hunch was right: The problem is that it’s hard to figure 
out a good backing file string when there are filters involved, and so 
in one test here we generate one that contains a JSON description of the 
backing subgraph including a throttle node. Outside of qemu, that 
doesn’t make much sense, hence the error.

(And because we checked only for “pattern verification failed” 
specifically, that error here never surfaced.)

I think (hope?) we can expect management tools to manually specify 
backing file strings in such cases, like the attached diff does. That 
seems to fix the problem.

Hanna

[-- Attachment #2: 040.diff --]
[-- Type: text/x-patch, Size: 1091 bytes --]

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index c4a90937dc..30eb97829e 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -836,7 +836,8 @@ class TestCommitWithFilters(iotests.QMPTestCase):
                              job_id='commit',
                              device='top-filter',
                              top_node='cow-2',
-                             base_node='cow-1')
+                             base_node='cow-1',
+                             backing_file=self.img1)
         self.assert_qmp(result, 'return', {})
         self.wait_until_completed(drive='commit')
 
@@ -852,7 +853,8 @@ class TestCommitWithFilters(iotests.QMPTestCase):
                              job_id='commit',
                              device='top-filter',
                              top_node='cow-1',
-                             base_node='cow-0')
+                             base_node='cow-0',
+                             backing_file=self.img0)
         self.assert_qmp(result, 'return', {})
         self.wait_until_completed(drive='commit')