[PATCH 00/14] iotests: ensure all qemu-img calls are either checked or logged

[PATCH 00/14] iotests: ensure all qemu-img calls are either checked or logged

[John Snow]

Based-On: 20220308015728.1269649-1-jsnow@redhat.com

Hi, this series ensures all calls to qemu-img ultimately go through
qemu_img(). After the previous series, qemu_img() is a function that
defaults to raising a VerboseProcessError exception when qemu-img
returns a non-zero exit code.

After this series, all qemu-img invocations from the iotests suite are
guaranteed to go through either qemu_img_log(), which allows non-zero
exit codes (but logs its output), or qemu_img().

Some callsites manually disable the error checking with check=False,
which is a good visual reminder directly at the callsite that additional
verification must be performed. The callsites utilizing this are very
few and far between.

The VerboseProcessError exception will print the command line, return
code, and all console output to the terminal if it goes unhandled. In
effect, all non-logging calls to qemu-img are now guaranteed to print
detailed failure information to the terminal on any unanticipated
behavior.

(Even logging calls will still raise this exception if the exit code was
negative, so you get all of the same failure output whenever qemu-img
crashes *anywhere* in iotests now.)

As a result of this change, some of the helpers change. Here's a summary
of the changes between the last series and this one:

qemu_img()                   => qemu_img().returncode
qemu_img_pipe()              => qemu_img().stdout
qemu_img_pipe_and_status()   => qemu_img()
json.loads(qemu_img_pipe())) => qemu_img_json()
qemu_img_log()               => qemu_img_log()

John Snow (14):
  iotests: add qemu_img_json()
  iotests: use qemu_img_json() when applicable
  iotests: add qemu_img_info()
  iotests/remove-bitmap-from-backing: use qemu_img_info()
  iotests: add qemu_img_map() function
  iotests: change supports_quorum to use qemu_img
  iotests: replace unchecked calls to qemu_img_pipe()
  iotests/149: Remove qemu_img_pipe() call
  iotests: remove remaining calls to qemu_img_pipe()
  iotests: use qemu_img() in has_working_luks()
  iotests: replace qemu_img_log('create', ...) calls
  iotests: remove qemu_img_pipe_and_status()
  iotests: make qemu_img_log() check log level
  iotests: make img_info_log() call qemu_img_log()

 tests/qemu-iotests/041                        |   5 +-
 tests/qemu-iotests/065                        |   7 +-
 tests/qemu-iotests/149                        |   7 +-
 tests/qemu-iotests/149.out                    |  21 ---
 tests/qemu-iotests/194                        |   4 +-
 tests/qemu-iotests/202                        |   4 +-
 tests/qemu-iotests/203                        |   4 +-
 tests/qemu-iotests/211                        |   6 +-
 tests/qemu-iotests/234                        |   4 +-
 tests/qemu-iotests/237                        |   3 +-
 tests/qemu-iotests/237.out                    |   3 -
 tests/qemu-iotests/242                        |   5 +-
 tests/qemu-iotests/255                        |   8 +-
 tests/qemu-iotests/255.out                    |   4 -
 tests/qemu-iotests/262                        |   2 +-
 tests/qemu-iotests/274                        |  17 ++-
 tests/qemu-iotests/274.out                    |  29 ----
 tests/qemu-iotests/280                        |   2 +-
 tests/qemu-iotests/280.out                    |   1 -
 tests/qemu-iotests/296                        |  12 +-
 tests/qemu-iotests/303                        |   2 +-
 tests/qemu-iotests/iotests.py                 | 134 +++++++++++++-----
 tests/qemu-iotests/tests/block-status-cache   |  11 +-
 .../qemu-iotests/tests/parallels-read-bitmap  |   6 +-
 .../tests/remove-bitmap-from-backing          |   6 +-
 25 files changed, 150 insertions(+), 157 deletions(-)

-- 
2.34.1

[PATCH 01/14] iotests: add qemu_img_json()

[John Snow]

qemu_img_json() is a new helper built on top of qemu_img() that tries to
pull a valid JSON document out of the stdout stream.

In the event that the return code is negative (the program crashed), or
the code is greater than zero and did not produce valid JSON output, the
VerboseProcessError raised by qemu_img() is re-raised.

In the event that the return code is zero but we can't parse valid JSON,
allow the JSON deserialization error to be raised.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7057db0686..546b142a6c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -276,6 +276,44 @@ def ordered_qmp(qmsg, conv_keys=True):
 def qemu_img_create(*args: str) -> subprocess.CompletedProcess[str]:
     return qemu_img('create', *args)
 
+def qemu_img_json(*args: str) -> Any:
+    """
+    Run qemu-img and return its output as deserialized JSON.
+
+    :raise CalledProcessError:
+        When qemu-img crashes, or returns a non-zero exit code without
+        producing a valid JSON document to stdout.
+    :raise JSONDecoderError:
+        When qemu-img returns 0, but failed to produce a valid JSON document.
+
+    :return: A deserialized JSON object; probably a dict[str, Any].
+    """
+    json_data = ...  # json.loads can legitimately return 'None'.
+
+    try:
+        res = qemu_img(*args, combine_stdio=False)
+    except subprocess.CalledProcessError as exc:
+        # Terminated due to signal. Don't bother.
+        if exc.returncode < 0:
+            raise
+
+        # Commands like 'check' can return failure (exit codes 2 and 3)
+        # to indicate command completion, but with errors found. For
+        # multi-command flexibility, ignore the exact error codes and
+        # *try* to load JSON.
+        try:
+            json_data = json.loads(exc.stdout)
+        except json.JSONDecodeError:
+            # Nope. This thing is toast. Raise the process error.
+            pass
+
+        if json_data is ...:
+            raise
+
+    if json_data is ...:
+        json_data = json.loads(res.stdout)
+    return json_data
+
 def qemu_img_measure(*args):
     return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
 
-- 
2.34.1

Re: [PATCH 01/14] iotests: add qemu_img_json()

[Hanna Reitz]

On 09.03.22 04:53, John Snow wrote:
> qemu_img_json() is a new helper built on top of qemu_img() that tries to
> pull a valid JSON document out of the stdout stream.
>
> In the event that the return code is negative (the program crashed), or
> the code is greater than zero and did not produce valid JSON output, the
> VerboseProcessError raised by qemu_img() is re-raised.
>
> In the event that the return code is zero but we can't parse valid JSON,
> allow the JSON deserialization error to be raised.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 38 +++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7057db0686..546b142a6c 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -276,6 +276,44 @@ def ordered_qmp(qmsg, conv_keys=True):
>   def qemu_img_create(*args: str) -> subprocess.CompletedProcess[str]:
>       return qemu_img('create', *args)
>   
> +def qemu_img_json(*args: str) -> Any:
> +    """
> +    Run qemu-img and return its output as deserialized JSON.
> +
> +    :raise CalledProcessError:
> +        When qemu-img crashes, or returns a non-zero exit code without
> +        producing a valid JSON document to stdout.
> +    :raise JSONDecoderError:
> +        When qemu-img returns 0, but failed to produce a valid JSON document.
> +
> +    :return: A deserialized JSON object; probably a dict[str, Any].
> +    """
> +    json_data = ...  # json.loads can legitimately return 'None'.

What kind of arcane sigil is this?

I’m trying to read into it, but it seems like...  Well, I won’t swear on 
a public list.  As far as I understand, it’s just a special singleton 
object that you can use for whatever you think is an appropriate use for 
an ellipsis?  And so in this case you use it as a special singleton 
object that would never legitimately appear, to be separate from None?

You’re really, really good at making me hate Python a bit more with 
every series.

It also just doesn’t seem very useful to me in this case.  I’m not sure 
whether this notation is widely known in the Python world, but I have 
only myself to go off of, and I was just very confused, so I would 
prefer not to have this in the code.

> +
> +    try:
> +        res = qemu_img(*args, combine_stdio=False)
> +    except subprocess.CalledProcessError as exc:
> +        # Terminated due to signal. Don't bother.
> +        if exc.returncode < 0:
> +            raise
> +
> +        # Commands like 'check' can return failure (exit codes 2 and 3)
> +        # to indicate command completion, but with errors found. For
> +        # multi-command flexibility, ignore the exact error codes and
> +        # *try* to load JSON.
> +        try:
> +            json_data = json.loads(exc.stdout)

Why not `return json.loads(exc.stdout)`?

> +        except json.JSONDecodeError:
> +            # Nope. This thing is toast. Raise the process error.
> +            pass
> +
> +        if json_data is ...:
> +            raise

And just unconditionally `raise` here.

> +
> +    if json_data is ...:
> +        json_data = json.loads(res.stdout)
> +    return json_data

And just `return json.loads(res.stdout)` here, without any condition.

Hanna

> +
>   def qemu_img_measure(*args):
>       return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
>   

Re: [PATCH 01/14] iotests: add qemu_img_json()

[John Snow]

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

On Thu, Mar 17, 2022, 6:53 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 09.03.22 04:53, John Snow wrote:
> > qemu_img_json() is a new helper built on top of qemu_img() that tries to
> > pull a valid JSON document out of the stdout stream.
> >
> > In the event that the return code is negative (the program crashed), or
> > the code is greater than zero and did not produce valid JSON output, the
> > VerboseProcessError raised by qemu_img() is re-raised.
> >
> > In the event that the return code is zero but we can't parse valid JSON,
> > allow the JSON deserialization error to be raised.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py | 38 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 38 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index 7057db0686..546b142a6c 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -276,6 +276,44 @@ def ordered_qmp(qmsg, conv_keys=True):
> >   def qemu_img_create(*args: str) -> subprocess.CompletedProcess[str]:
> >       return qemu_img('create', *args)
> >
> > +def qemu_img_json(*args: str) -> Any:
> > +    """
> > +    Run qemu-img and return its output as deserialized JSON.
> > +
> > +    :raise CalledProcessError:
> > +        When qemu-img crashes, or returns a non-zero exit code without
> > +        producing a valid JSON document to stdout.
> > +    :raise JSONDecoderError:
> > +        When qemu-img returns 0, but failed to produce a valid JSON
> document.
> > +
> > +    :return: A deserialized JSON object; probably a dict[str, Any].
> > +    """
> > +    json_data = ...  # json.loads can legitimately return 'None'.
>
> What kind of arcane sigil is this?
>

I may genuinely start referring to it as the Arcane Sigil.


> I’m trying to read into it, but it seems like...  Well, I won’t swear on
> a public list.  As far as I understand, it’s just a special singleton
> object that you can use for whatever you think is an appropriate use for
> an ellipsis?  And so in this case you use it as a special singleton
> object that would never legitimately appear, to be separate from None?
>
> You’re really, really good at making me hate Python a bit more with
> every series.
>

I aim to please.

Yes, it's just Another Singleton That Isn't None, because technically a
JSON document can be just "null". Will qemu_img() ever, ever print that
single string all by itself?

Well, I hope not, but. I felt guilty not addressing that technicality
somehow.


> It also just doesn’t seem very useful to me in this case.  I’m not sure
> whether this notation is widely known in the Python world, but I have
> only myself to go off of, and I was just very confused, so I would
> prefer not to have this in the code.
>

You're right, it's a bit too clever. I judged the cleverness:usefulness
ratio poorly.

(I think it's a trick that a lot of long time python people know, because
sooner or later one wants to distinguish between an explicitly provided
"None" and a default value that signifies "No argument provided". It's
definitely a hack. It's not something most users would know.)


> > +
> > +    try:
> > +        res = qemu_img(*args, combine_stdio=False)
> > +    except subprocess.CalledProcessError as exc:
> > +        # Terminated due to signal. Don't bother.
> > +        if exc.returncode < 0:
> > +            raise
> > +
> > +        # Commands like 'check' can return failure (exit codes 2 and 3)
> > +        # to indicate command completion, but with errors found. For
> > +        # multi-command flexibility, ignore the exact error codes and
> > +        # *try* to load JSON.
> > +        try:
> > +            json_data = json.loads(exc.stdout)
>
> Why not `return json.loads(exc.stdout)`?
>

Uh.


> > +        except json.JSONDecodeError:
> > +            # Nope. This thing is toast. Raise the process error.
> > +            pass
> > +
> > +        if json_data is ...:
> > +            raise
>
> And just unconditionally `raise` here.


Uhhh.


> > +
> > +    if json_data is ...:
> > +        json_data = json.loads(res.stdout)
> > +    return json_data
>
> And just `return json.loads(res.stdout)` here, without any condition.
>

Uhhhhhhhh...!

Yeah, that's obviously way better. I'm sorry to have subjected you to an
arcane workaround for no reason :/


>
> Hanna
>
> > +
> >   def qemu_img_measure(*args):
> >       return json.loads(qemu_img_pipe("measure", "--output", "json",
> *args))
> >
>
>

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

Re: [PATCH 01/14] iotests: add qemu_img_json()

[Hanna Reitz]

On 17.03.22 15:42, John Snow wrote:
>
>
> On Thu, Mar 17, 2022, 6:53 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 09.03.22 04:53, John Snow wrote:
>     > qemu_img_json() is a new helper built on top of qemu_img() that
>     tries to
>     > pull a valid JSON document out of the stdout stream.
>     >
>     > In the event that the return code is negative (the program
>     crashed), or
>     > the code is greater than zero and did not produce valid JSON
>     output, the
>     > VerboseProcessError raised by qemu_img() is re-raised.
>     >
>     > In the event that the return code is zero but we can't parse
>     valid JSON,
>     > allow the JSON deserialization error to be raised.
>     >
>     > Signed-off-by: John Snow <jsnow@redhat.com>
>     > ---
>     >   tests/qemu-iotests/iotests.py | 38
>     +++++++++++++++++++++++++++++++++++
>     >   1 file changed, 38 insertions(+)
>     >
>     > diff --git a/tests/qemu-iotests/iotests.py
>     b/tests/qemu-iotests/iotests.py
>     > index 7057db0686..546b142a6c 100644
>     > --- a/tests/qemu-iotests/iotests.py
>     > +++ b/tests/qemu-iotests/iotests.py
>     > @@ -276,6 +276,44 @@ def ordered_qmp(qmsg, conv_keys=True):
>     >   def qemu_img_create(*args: str) ->
>     subprocess.CompletedProcess[str]:
>     >       return qemu_img('create', *args)
>     >
>     > +def qemu_img_json(*args: str) -> Any:
>     > +    """
>     > +    Run qemu-img and return its output as deserialized JSON.
>     > +
>     > +    :raise CalledProcessError:
>     > +        When qemu-img crashes, or returns a non-zero exit code
>     without
>     > +        producing a valid JSON document to stdout.
>     > +    :raise JSONDecoderError:
>     > +        When qemu-img returns 0, but failed to produce a valid
>     JSON document.
>     > +
>     > +    :return: A deserialized JSON object; probably a dict[str, Any].
>     > +    """
>     > +    json_data = ...  # json.loads can legitimately return 'None'.
>
>     What kind of arcane sigil is this?
>
>
> I may genuinely start referring to it as the Arcane Sigil.

Be my guest.  Perhaps one should identify the most arcane sigil for 
every language.  (My C vote goes to the ternary operator.)

>
>     I’m trying to read into it, but it seems like...  Well, I won’t
>     swear on
>     a public list.  As far as I understand, it’s just a special singleton
>     object that you can use for whatever you think is an appropriate
>     use for
>     an ellipsis?  And so in this case you use it as a special singleton
>     object that would never legitimately appear, to be separate from None?
>
>     You’re really, really good at making me hate Python a bit more with
>     every series.
>
>
> I aim to please.
>
> Yes, it's just Another Singleton That Isn't None, because technically 
> a JSON document can be just "null". Will qemu_img() ever, ever print 
> that single string all by itself?

Is there even any case where qemu returns anything but a JSON object?

> Well, I hope not, but. I felt guilty not addressing that technicality 
> somehow.
>
>
>     It also just doesn’t seem very useful to me in this case. I’m not
>     sure
>     whether this notation is widely known in the Python world, but I have
>     only myself to go off of, and I was just very confused, so I would
>     prefer not to have this in the code.
>
>
> You're right, it's a bit too clever. I judged the 
> cleverness:usefulness ratio poorly.
>
> (I think it's a trick that a lot of long time python people know, 
> because sooner or later one wants to distinguish between an explicitly 
> provided "None" and a default value that signifies "No argument 
> provided". It's definitely a hack. It's not something most users would 
> know.)

I hope similarly to how א‎₀ and its companions exist[1], there are also 
multiple instances of `...`, so one can succeed at handling cases where 
a `...` is a valid return type.  I suggest just more dots.

[1] hope I got that \x0e“HEBREW LETTER ALEF” “LEFT-TO-RIGHT MARK” sequence 
right...

Re: [PATCH 01/14] iotests: add qemu_img_json()

[John Snow]

On Thu, Mar 17, 2022 at 10:51 AM Hanna Reitz <hreitz@redhat.com> wrote:

> I hope similarly to how א‎₀ and its companions exist[1], there are also
> multiple instances of `...`, so one can succeed at handling cases where
> a `...` is a valid return type.  I suggest just more dots.

lol.

I'm invested in higher-kinded ellipse theory.

--js

[PATCH 02/14] iotests: use qemu_img_json() when applicable

[John Snow]

qemu_img_json() gives better diagnostic information on failure.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 546b142a6c..7b37938d45 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -314,11 +314,11 @@ def qemu_img_json(*args: str) -> Any:
         json_data = json.loads(res.stdout)
     return json_data
 
-def qemu_img_measure(*args):
-    return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
+def qemu_img_measure(*args: str) -> Any:
+    return qemu_img_json("measure", "--output", "json", *args)
 
-def qemu_img_check(*args):
-    return json.loads(qemu_img_pipe("check", "--output", "json", *args))
+def qemu_img_check(*args: str) -> Any:
+    return qemu_img_json("check", "--output", "json", *args)
 
 def qemu_img_pipe(*args: str) -> str:
     '''Run qemu-img and return its output'''
-- 
2.34.1

Re: [PATCH 02/14] iotests: use qemu_img_json() when applicable

[Hanna Reitz]

On 09.03.22 04:53, John Snow wrote:
> qemu_img_json() gives better diagnostic information on failure.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

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

[PATCH 03/14] iotests: add qemu_img_info()

[John Snow]

Add qemu_img_info() by analogy with qemu_img_measure() and
qemu_img_check(). Modify image_size() to use this function instead to
take advantage of the better diagnostic information on failure provided
(ultimately) by qemu_img().

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/065        |  5 ++---
 tests/qemu-iotests/242        |  5 ++---
 tests/qemu-iotests/iotests.py | 15 +++++++++++----
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index f7c1b68dad..9466ce7df4 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_pipe
+from iotests import qemu_img, qemu_img_info, qemu_img_pipe
 import unittest
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -49,8 +49,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
     human_compare = None
 
     def test_json(self):
-        data = json.loads(qemu_img_pipe('info', '--output=json', test_img))
-        data = data['format-specific']
+        data = qemu_img_info(test_img)['format-specific']
         self.assertEqual(data['type'], iotests.imgfmt)
         self.assertEqual(data['data'], self.json_compare)
 
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 96a30152b0..547bf382e3 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_pipe, \
+from iotests import qemu_img_create, qemu_io, qemu_img_info, \
     file_path, img_info_log, log, filter_qemu_io
 
 iotests.script_initialize(supported_fmts=['qcow2'],
@@ -39,8 +39,7 @@ flag_offset = 0x5000f
 def print_bitmap(extra_args):
     log('qemu-img info dump:\n')
     img_info_log(disk, extra_args=extra_args)
-    result = json.loads(qemu_img_pipe('info', '--force-share',
-                                      '--output=json', disk))
+    result = qemu_img_info('--force-share', disk)
     if 'bitmaps' in result['format-specific']['data']:
         bitmaps = result['format-specific']['data']['bitmaps']
         log('The same bitmaps in JSON format:')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7b37938d45..62f82281a9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -320,6 +320,9 @@ def qemu_img_measure(*args: str) -> Any:
 def qemu_img_check(*args: str) -> Any:
     return qemu_img_json("check", "--output", "json", *args)
 
+def qemu_img_info(*args: str) -> Any:
+    return qemu_img_json('info', "--output", "json", *args)
+
 def qemu_img_pipe(*args: str) -> str:
     '''Run qemu-img and return its output'''
     return qemu_img_pipe_and_status(*args)[0]
@@ -570,10 +573,14 @@ def create_image(name, size):
             file.write(sector)
             i = i + 512
 
-def image_size(img):
-    '''Return image's virtual size'''
-    r = qemu_img_pipe('info', '--output=json', '-f', imgfmt, img)
-    return json.loads(r)['virtual-size']
+def image_size(img: str) -> int:
+    """Return image's virtual size"""
+    value = qemu_img_info('-f', imgfmt, img)['virtual-size']
+    if not isinstance(value, int):
+        type_name = type(value).__name__
+        raise TypeError("Expected 'int' for 'virtual-size', "
+                        f"got '{value}' of type '{type_name}'")
+    return value
 
 def is_str(val):
     return isinstance(val, str)
-- 
2.34.1

Re: [PATCH 03/14] iotests: add qemu_img_info()

[Hanna Reitz]

On 09.03.22 04:53, John Snow wrote:
> Add qemu_img_info() by analogy with qemu_img_measure() and
> qemu_img_check(). Modify image_size() to use this function instead to
> take advantage of the better diagnostic information on failure provided
> (ultimately) by qemu_img().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/065        |  5 ++---
>   tests/qemu-iotests/242        |  5 ++---
>   tests/qemu-iotests/iotests.py | 15 +++++++++++----
>   3 files changed, 15 insertions(+), 10 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7b37938d45..62f82281a9 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -570,10 +573,14 @@ def create_image(name, size):
>               file.write(sector)
>               i = i + 512
>   
> -def image_size(img):
> -    '''Return image's virtual size'''
> -    r = qemu_img_pipe('info', '--output=json', '-f', imgfmt, img)
> -    return json.loads(r)['virtual-size']
> +def image_size(img: str) -> int:
> +    """Return image's virtual size"""
> +    value = qemu_img_info('-f', imgfmt, img)['virtual-size']
> +    if not isinstance(value, int):
> +        type_name = type(value).__name__
> +        raise TypeError("Expected 'int' for 'virtual-size', "
> +                        f"got '{value}' of type '{type_name}'")
> +    return value

Bit overkill for my taste for the scope of iotests.py, but if you want 
to go the extra mile, I’m not stopping you.

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

Re: [PATCH 03/14] iotests: add qemu_img_info()

[John Snow]

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

On Thu, Mar 17, 2022, 7:09 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 09.03.22 04:53, John Snow wrote:
> > Add qemu_img_info() by analogy with qemu_img_measure() and
> > qemu_img_check(). Modify image_size() to use this function instead to
> > take advantage of the better diagnostic information on failure provided
> > (ultimately) by qemu_img().
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/065        |  5 ++---
> >   tests/qemu-iotests/242        |  5 ++---
> >   tests/qemu-iotests/iotests.py | 15 +++++++++++----
> >   3 files changed, 15 insertions(+), 10 deletions(-)
>
> [...]
>
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index 7b37938d45..62f82281a9 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
>
> [...]
>
> > @@ -570,10 +573,14 @@ def create_image(name, size):
> >               file.write(sector)
> >               i = i + 512
> >
> > -def image_size(img):
> > -    '''Return image's virtual size'''
> > -    r = qemu_img_pipe('info', '--output=json', '-f', imgfmt, img)
> > -    return json.loads(r)['virtual-size']
> > +def image_size(img: str) -> int:
> > +    """Return image's virtual size"""
> > +    value = qemu_img_info('-f', imgfmt, img)['virtual-size']
> > +    if not isinstance(value, int):
> > +        type_name = type(value).__name__
> > +        raise TypeError("Expected 'int' for 'virtual-size', "
> > +                        f"got '{value}' of type '{type_name}'")
> > +    return value
>
> Bit overkill for my taste for the scope of iotests.py, but if you want
> to go the extra mile, I’m not stopping you.
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>

As long as it isn't *too* overkill. Tests should be simple and stupid, but
the helper utilities should be *solid*.

I added a type hint but needed to make it true. I could have used an
assert, but asserts should not be used in library code for sanitizing input
data.

(I've got "Trying to make a public PyPI package" brain and it's hard to
switch off. Maybe an assert would've been fine, ...)

--js

>

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

[PATCH 04/14] iotests/remove-bitmap-from-backing: use qemu_img_info()

[John Snow]

This removes two more usages of qemu_img_pipe() and replaces them with
calls to qemu_img(), which provides better diagnostic information on
failure.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/remove-bitmap-from-backing | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing b/tests/qemu-iotests/tests/remove-bitmap-from-backing
index fee3141340..15be32dcb9 100755
--- a/tests/qemu-iotests/tests/remove-bitmap-from-backing
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -19,7 +19,7 @@
 #
 
 import iotests
-from iotests import log, qemu_img_create, qemu_img, qemu_img_pipe
+from iotests import log, qemu_img_create, qemu_img, qemu_img_info
 
 iotests.script_initialize(supported_fmts=['qcow2'],
                           unsupported_imgopts=['compat'])
@@ -33,7 +33,7 @@ qemu_img_create('-f', iotests.imgfmt, '-b', base,
 
 qemu_img('bitmap', '--add', base, 'bitmap0')
 # Just assert that our method of checking bitmaps in the image works.
-assert 'bitmaps' in qemu_img_pipe('info', base)
+assert 'bitmaps' in qemu_img_info(base)['format-specific']['data']
 
 vm = iotests.VM().add_drive(top, 'backing.node-name=base')
 vm.launch()
@@ -68,5 +68,5 @@ if result != {'return': {}}:
 
 vm.shutdown()
 
-if 'bitmaps' in qemu_img_pipe('info', base):
+if 'bitmaps' in qemu_img_info(base)['format-specific']['data']:
     log('ERROR: Bitmap is still in the base image')
-- 
2.34.1

Re: [PATCH 04/14] iotests/remove-bitmap-from-backing: use qemu_img_info()

[Hanna Reitz]

On 09.03.22 04:53, John Snow wrote:
> This removes two more usages of qemu_img_pipe() and replaces them with
> calls to qemu_img(), which provides better diagnostic information on
> failure.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/tests/remove-bitmap-from-backing | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

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

[PATCH 05/14] iotests: add qemu_img_map() function

[John Snow]

Add a qemu_img_map() function by analogy with qemu_img_measure(),
qemu_img_check(), and qemu_img_info() that all return JSON information.

Replace calls to qemu_img_pipe('map', '--output=json', ...) with this
new function, which provides better diagnostic information on failure.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/041                         |  5 ++---
 tests/qemu-iotests/211                         |  6 +++---
 tests/qemu-iotests/iotests.py                  |  3 +++
 tests/qemu-iotests/tests/block-status-cache    | 11 ++++-------
 tests/qemu-iotests/tests/parallels-read-bitmap |  6 ++----
 5 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index db9f5dc540..3e16acee56 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_pipe, qemu_io
+from iotests import qemu_img, qemu_img_map, qemu_io
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
@@ -1360,8 +1360,7 @@ class TestFilters(iotests.QMPTestCase):
 
         self.vm.qmp('blockdev-del', node_name='target')
 
-        target_map = qemu_img_pipe('map', '--output=json', target_img)
-        target_map = json.loads(target_map)
+        target_map = qemu_img_map(target_img)
 
         assert target_map[0]['start'] == 0
         assert target_map[0]['length'] == 512 * 1024
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index f52cadade1..1a3b4596c8 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -59,7 +59,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
     vm.shutdown()
 
     iotests.img_info_log(disk_path)
-    iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+    iotests.log(iotests.qemu_img_map(disk_path))
 
     #
     # Successful image creation (explicit defaults)
@@ -83,7 +83,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
     vm.shutdown()
 
     iotests.img_info_log(disk_path)
-    iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+    iotests.log(iotests.qemu_img_map(disk_path))
 
     #
     # Successful image creation (with non-default options)
@@ -107,7 +107,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
     vm.shutdown()
 
     iotests.img_info_log(disk_path)
-    iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
+    iotests.log(iotests.qemu_img_map(disk_path))
 
     #
     # Invalid BlockdevRef
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 62f82281a9..3896440238 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -323,6 +323,9 @@ def qemu_img_check(*args: str) -> Any:
 def qemu_img_info(*args: str) -> Any:
     return qemu_img_json('info', "--output", "json", *args)
 
+def qemu_img_map(*args: str) -> Any:
+    return qemu_img_json('map', "--output", "json", *args)
+
 def qemu_img_pipe(*args: str) -> str:
     '''Run qemu-img and return its output'''
     return qemu_img_pipe_and_status(*args)[0]
diff --git a/tests/qemu-iotests/tests/block-status-cache b/tests/qemu-iotests/tests/block-status-cache
index 40e648e251..5a7bc2c149 100755
--- a/tests/qemu-iotests/tests/block-status-cache
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -22,7 +22,7 @@
 import os
 import signal
 import iotests
-from iotests import qemu_img_create, qemu_img_pipe, qemu_nbd
+from iotests import qemu_img_create, qemu_img_map, qemu_nbd
 
 
 image_size = 1 * 1024 * 1024
@@ -76,8 +76,7 @@ class TestBscWithNbd(iotests.QMPTestCase):
         # to allocate the first sector to facilitate alignment probing), and
         # then the rest to be zero.  The BSC will thus contain (if anything)
         # one range covering the first sector.
-        map_pre = qemu_img_pipe('map', '--output=json', '--image-opts',
-                                nbd_img_opts)
+        map_pre = qemu_img_map('--image-opts', nbd_img_opts)
 
         # qemu:allocation-depth maps for want_zero=false.
         # want_zero=false should (with the file driver, which the server is
@@ -111,14 +110,12 @@ class TestBscWithNbd(iotests.QMPTestCase):
         # never loop too many times here.
         for _ in range(2):
             # (Ignore the result, this is just to contaminate the cache)
-            qemu_img_pipe('map', '--output=json', '--image-opts',
-                          nbd_img_opts_alloc_depth)
+            qemu_img_map('--image-opts', nbd_img_opts_alloc_depth)
 
         # Now let's see whether the cache reports everything as data, or
         # whether we get correct information (i.e. the same as we got on our
         # first attempt).
-        map_post = qemu_img_pipe('map', '--output=json', '--image-opts',
-                                 nbd_img_opts)
+        map_post = qemu_img_map('--image-opts', nbd_img_opts)
 
         if map_pre != map_post:
             print('ERROR: Map information differs before and after querying ' +
diff --git a/tests/qemu-iotests/tests/parallels-read-bitmap b/tests/qemu-iotests/tests/parallels-read-bitmap
index af6b9c5db3..38ab5fa5b2 100755
--- a/tests/qemu-iotests/tests/parallels-read-bitmap
+++ b/tests/qemu-iotests/tests/parallels-read-bitmap
@@ -18,9 +18,8 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-import json
 import iotests
-from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path
+from iotests import qemu_nbd_popen, qemu_img_map, log, file_path
 
 iotests.script_initialize(supported_fmts=['parallels'])
 
@@ -36,8 +35,7 @@ iotests.unarchive_sample_image('parallels-with-bitmap', disk)
 
 with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
                     f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
-    out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
-    chunks = json.loads(out)
+    chunks = qemu_img_map('--image-opts', nbd_opts)
     cluster = 64 * 1024
 
     log('dirty clusters (cluster size is 64K):')
-- 
2.34.1

Re: [PATCH 05/14] iotests: add qemu_img_map() function

[Hanna Reitz]

On 09.03.22 04:53, John Snow wrote:
> Add a qemu_img_map() function by analogy with qemu_img_measure(),
> qemu_img_check(), and qemu_img_info() that all return JSON information.
>
> Replace calls to qemu_img_pipe('map', '--output=json', ...) with this
> new function, which provides better diagnostic information on failure.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/041                         |  5 ++---
>   tests/qemu-iotests/211                         |  6 +++---
>   tests/qemu-iotests/iotests.py                  |  3 +++
>   tests/qemu-iotests/tests/block-status-cache    | 11 ++++-------
>   tests/qemu-iotests/tests/parallels-read-bitmap |  6 ++----
>   5 files changed, 14 insertions(+), 17 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
> index f52cadade1..1a3b4596c8 100755
> --- a/tests/qemu-iotests/211
> +++ b/tests/qemu-iotests/211
> @@ -59,7 +59,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
>       vm.shutdown()
>   
>       iotests.img_info_log(disk_path)
> -    iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
> +    iotests.log(iotests.qemu_img_map(disk_path))
>   
>       #
>       # Successful image creation (explicit defaults)
> @@ -83,7 +83,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
>       vm.shutdown()
>   
>       iotests.img_info_log(disk_path)
> -    iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
> +    iotests.log(iotests.qemu_img_map(disk_path))
>   
>       #
>       # Successful image creation (with non-default options)
> @@ -107,7 +107,7 @@ with iotests.FilePath('t.vdi') as disk_path, \
>       vm.shutdown()
>   
>       iotests.img_info_log(disk_path)
> -    iotests.log(iotests.qemu_img_pipe('map', '--output=json', disk_path))
> +    iotests.log(iotests.qemu_img_map(disk_path))
>   
>       #
>       # Invalid BlockdevRef

This breaks 211, because originally, the output wasn’t piped through 
Python’s json module, i.e. the original json data was printed as it’s 
generated by qemu.  Now it’s parsed by the json module, and the Python 
object is logged, so the output changes.

Still sounds good to me, but this part will require fixing up the 
reference output.

Hanna

[PATCH 06/14] iotests: change supports_quorum to use qemu_img

[John Snow]

Similar to other recent changes: use the qemu_img() invocation that
supports throwing loud, nasty exceptions when it fails for surprising
reasons.

(Why would "--help" ever fail? I don't know, but eliminating *all* calls
to qemu-img that do not go through qemu_img() is my goal, so
qemu_img_pipe() has to be removed.)

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3896440238..698d5a7fdc 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1433,8 +1433,8 @@ def _verify_imgopts(unsupported: Sequence[str] = ()) -> None:
         notrun(f'not suitable for this imgopts: {imgopts}')
 
 
-def supports_quorum():
-    return 'quorum' in qemu_img_pipe('--help')
+def supports_quorum() -> bool:
+    return 'quorum' in qemu_img('--help').stdout
 
 def verify_quorum():
     '''Skip test suite if quorum support is not available'''
-- 
2.34.1

Re: [PATCH 06/14] iotests: change supports_quorum to use qemu_img

[Hanna Reitz]

On 09.03.22 04:53, John Snow wrote:
> Similar to other recent changes: use the qemu_img() invocation that
> supports throwing loud, nasty exceptions when it fails for surprising
> reasons.
>
> (Why would "--help" ever fail? I don't know, but eliminating *all* calls
> to qemu-img that do not go through qemu_img() is my goal, so
> qemu_img_pipe() has to be removed.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH 07/14] iotests: replace unchecked calls to qemu_img_pipe()

[John Snow]

qemu_img_pipe() discards the return code from qemu-img in favor of
returning just its output. Some tests using this function don't save,
log, or check the output either, though, which is unsafe.

Replace all of these calls with a checked version.

Tests affected are 194, 202, 203, 234, 262, and 303.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/194 | 4 ++--
 tests/qemu-iotests/202 | 4 ++--
 tests/qemu-iotests/203 | 4 ++--
 tests/qemu-iotests/234 | 4 ++--
 tests/qemu-iotests/262 | 2 +-
 tests/qemu-iotests/303 | 2 +-
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index e44b8df728..68894371f5 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -33,8 +33,8 @@ with iotests.FilePath('source.img') as source_img_path, \
      iotests.VM('dest') as dest_vm:
 
     img_size = '1G'
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, source_img_path, img_size)
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, dest_img_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, source_img_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, dest_img_path, img_size)
 
     iotests.log('Launching VMs...')
     (source_vm.add_drive(source_img_path)
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 8eb5f32d15..b784dcd791 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -35,8 +35,8 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \
      iotests.VM() as vm:
 
     img_size = '10M'
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size)
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, disk0_img_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, disk1_img_path, img_size)
 
     iotests.log('Launching VM...')
     vm.launch()
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index ea30e50497..ab80fd0e44 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -33,8 +33,8 @@ with iotests.FilePath('disk0.img') as disk0_img_path, \
      iotests.VM() as vm:
 
     img_size = '10M'
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size)
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, disk0_img_path, img_size)
+    iotests.qemu_img_create('-f', iotests.imgfmt, disk1_img_path, img_size)
 
     iotests.log('Launching VM...')
     (vm.add_object('iothread,id=iothread0')
diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index cb5f1753e0..a9f764bb2c 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -34,8 +34,8 @@ with iotests.FilePath('img') as img_path, \
      iotests.VM(path_suffix='a') as vm_a, \
      iotests.VM(path_suffix='b') as vm_b:
 
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, '64M')
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+    iotests.qemu_img_create('-f', iotests.imgfmt, backing_path, '64M')
+    iotests.qemu_img_create('-f', iotests.imgfmt, img_path, '64M')
 
     os.mkfifo(fifo_a)
     os.mkfifo(fifo_b)
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 32d69988ef..2294fd5ecb 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -51,7 +51,7 @@ with iotests.FilePath('img') as img_path, \
 
         vm.add_device('virtio-blk,drive=%s,iothread=iothread0' % root)
 
-    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+    iotests.qemu_img_create('-f', iotests.imgfmt, img_path, '64M')
 
     os.mkfifo(fifo)
 
diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 16c2e10827..93aa5ce9b7 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -38,7 +38,7 @@ def create_bitmap(bitmap_number, disabled):
     if disabled:
         args.append('--disable')
 
-    iotests.qemu_img_pipe(*args)
+    iotests.qemu_img(*args)
 
 
 def write_to_disk(offset, size):
-- 
2.34.1

Re: [PATCH 07/14] iotests: replace unchecked calls to qemu_img_pipe()

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> qemu_img_pipe() discards the return code from qemu-img in favor of
> returning just its output. Some tests using this function don't save,
> log, or check the output either, though, which is unsafe.
>
> Replace all of these calls with a checked version.
>
> Tests affected are 194, 202, 203, 234, 262, and 303.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/194 | 4 ++--
>   tests/qemu-iotests/202 | 4 ++--
>   tests/qemu-iotests/203 | 4 ++--
>   tests/qemu-iotests/234 | 4 ++--
>   tests/qemu-iotests/262 | 2 +-
>   tests/qemu-iotests/303 | 2 +-
>   6 files changed, 10 insertions(+), 10 deletions(-)

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

[PATCH 08/14] iotests/149: Remove qemu_img_pipe() call

[John Snow]

qemu_img_pipe calls blank their output when the command being run is a
'create' call and the command succeeds. Thus, the normative output for
this command in iotest 149 is to print a blank line. We can remove the
logging from this invocation and use a checked invocation, but we still
need to inspect the actual output to see if we want to retroactively
skip the test due to missing cipher support.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/149     |  7 +++++--
 tests/qemu-iotests/149.out | 21 ---------------------
 2 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index d49646ca60..9bb96d6a1d 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -265,8 +265,11 @@ def qemu_img_create(config, size_mb):
             "%dM" % size_mb]
 
     iotests.log("qemu-img " + " ".join(args), filters=[iotests.filter_test_dir])
-    iotests.log(check_cipher_support(config, iotests.qemu_img_pipe(*args)),
-                filters=[iotests.filter_test_dir])
+    try:
+        iotests.qemu_img(*args)
+    except subprocess.CalledProcessError as exc:
+        check_cipher_support(config, exc.output)
+        raise
 
 def qemu_io_image_args(config, dev=False):
     """Get the args for access an image or device with qemu-io"""
diff --git a/tests/qemu-iotests/149.out b/tests/qemu-iotests/149.out
index ab879596ce..2cc5b82f7c 100644
--- a/tests/qemu-iotests/149.out
+++ b/tests/qemu-iotests/149.out
@@ -61,7 +61,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha1.img
 # ================= qemu-img aes-256-xts-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-aes-256-xts-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha1.img qiotest-145-aes-256-xts-plain64-sha1
 # Write test pattern 0xa7
@@ -180,7 +179,6 @@ unlink TEST_DIR/luks-twofish-256-xts-plain64-sha1.img
 # ================= qemu-img twofish-256-xts-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=twofish-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-twofish-256-xts-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-twofish-256-xts-plain64-sha1.img qiotest-145-twofish-256-xts-plain64-sha1
 # Write test pattern 0xa7
@@ -299,7 +297,6 @@ unlink TEST_DIR/luks-serpent-256-xts-plain64-sha1.img
 # ================= qemu-img serpent-256-xts-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=serpent-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-serpent-256-xts-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-serpent-256-xts-plain64-sha1.img qiotest-145-serpent-256-xts-plain64-sha1
 # Write test pattern 0xa7
@@ -418,7 +415,6 @@ unlink TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img
 # ================= qemu-img cast5-128-cbc-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=cast5-128,cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-cast5-128-cbc-plain64-sha1.img qiotest-145-cast5-128-cbc-plain64-sha1
 # Write test pattern 0xa7
@@ -538,7 +534,6 @@ unlink TEST_DIR/luks-aes-256-cbc-plain-sha1.img
 # ================= qemu-img aes-256-cbc-plain-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=plain,hash-alg=sha1 TEST_DIR/luks-aes-256-cbc-plain-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-plain-sha1.img qiotest-145-aes-256-cbc-plain-sha1
 # Write test pattern 0xa7
@@ -657,7 +652,6 @@ unlink TEST_DIR/luks-aes-256-cbc-plain64-sha1.img
 # ================= qemu-img aes-256-cbc-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-aes-256-cbc-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-plain64-sha1.img qiotest-145-aes-256-cbc-plain64-sha1
 # Write test pattern 0xa7
@@ -776,7 +770,6 @@ unlink TEST_DIR/luks-aes-256-cbc-essiv-sha256-sha1.img
 # ================= qemu-img aes-256-cbc-essiv-sha256-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=essiv,hash-alg=sha1,ivgen-hash-alg=sha256 TEST_DIR/luks-aes-256-cbc-essiv-sha256-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-essiv-sha256-sha1.img qiotest-145-aes-256-cbc-essiv-sha256-sha1
 # Write test pattern 0xa7
@@ -895,7 +888,6 @@ unlink TEST_DIR/luks-aes-256-xts-essiv-sha256-sha1.img
 # ================= qemu-img aes-256-xts-essiv-sha256-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=essiv,hash-alg=sha1,ivgen-hash-alg=sha256 TEST_DIR/luks-aes-256-xts-essiv-sha256-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-essiv-sha256-sha1.img qiotest-145-aes-256-xts-essiv-sha256-sha1
 # Write test pattern 0xa7
@@ -1014,7 +1006,6 @@ unlink TEST_DIR/luks-aes-128-xts-plain64-sha256-sha1.img
 # ================= qemu-img aes-128-xts-plain64-sha256-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-128,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-aes-128-xts-plain64-sha256-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-128-xts-plain64-sha256-sha1.img qiotest-145-aes-128-xts-plain64-sha256-sha1
 # Write test pattern 0xa7
@@ -1133,7 +1124,6 @@ unlink TEST_DIR/luks-aes-192-xts-plain64-sha256-sha1.img
 # ================= qemu-img aes-192-xts-plain64-sha256-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-192,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-aes-192-xts-plain64-sha256-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-192-xts-plain64-sha256-sha1.img qiotest-145-aes-192-xts-plain64-sha256-sha1
 # Write test pattern 0xa7
@@ -1252,7 +1242,6 @@ unlink TEST_DIR/luks-twofish-128-xts-plain64-sha1.img
 # ================= qemu-img twofish-128-xts-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=twofish-128,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-twofish-128-xts-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-twofish-128-xts-plain64-sha1.img qiotest-145-twofish-128-xts-plain64-sha1
 # Write test pattern 0xa7
@@ -1372,7 +1361,6 @@ unlink TEST_DIR/luks-serpent-128-xts-plain64-sha1.img
 # ================= qemu-img serpent-128-xts-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=serpent-128,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-serpent-128-xts-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-serpent-128-xts-plain64-sha1.img qiotest-145-serpent-128-xts-plain64-sha1
 # Write test pattern 0xa7
@@ -1491,7 +1479,6 @@ unlink TEST_DIR/luks-serpent-192-xts-plain64-sha1.img
 # ================= qemu-img serpent-192-xts-plain64-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=serpent-192,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1 TEST_DIR/luks-serpent-192-xts-plain64-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-serpent-192-xts-plain64-sha1.img qiotest-145-serpent-192-xts-plain64-sha1
 # Write test pattern 0xa7
@@ -1612,7 +1599,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha224.img
 # ================= qemu-img aes-256-xts-plain64-sha224 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha224 TEST_DIR/luks-aes-256-xts-plain64-sha224.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha224.img qiotest-145-aes-256-xts-plain64-sha224
 # Write test pattern 0xa7
@@ -1731,7 +1717,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha256.img
 # ================= qemu-img aes-256-xts-plain64-sha256 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha256 TEST_DIR/luks-aes-256-xts-plain64-sha256.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha256.img qiotest-145-aes-256-xts-plain64-sha256
 # Write test pattern 0xa7
@@ -1850,7 +1835,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha384.img
 # ================= qemu-img aes-256-xts-plain64-sha384 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha384 TEST_DIR/luks-aes-256-xts-plain64-sha384.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha384.img qiotest-145-aes-256-xts-plain64-sha384
 # Write test pattern 0xa7
@@ -1969,7 +1953,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha512.img
 # ================= qemu-img aes-256-xts-plain64-sha512 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha512 TEST_DIR/luks-aes-256-xts-plain64-sha512.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha512.img qiotest-145-aes-256-xts-plain64-sha512
 # Write test pattern 0xa7
@@ -2088,7 +2071,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-ripemd160.img
 # ================= qemu-img aes-256-xts-plain64-ripemd160 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=ripemd160 TEST_DIR/luks-aes-256-xts-plain64-ripemd160.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-ripemd160.img qiotest-145-aes-256-xts-plain64-ripemd160
 # Write test pattern 0xa7
@@ -2281,7 +2263,6 @@ unlink TEST_DIR/luks-aes-256-xts-plain-sha1-pwallslots.img
 # ================= qemu-img aes-256-xts-plain-sha1-pwallslots =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=c2xvdDE=,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain,hash-alg=sha1 TEST_DIR/luks-aes-256-xts-plain-sha1-pwallslots.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain-sha1-pwallslots.img qiotest-145-aes-256-xts-plain-sha1-pwallslots
 # Write test pattern 0xa7
@@ -2400,7 +2381,6 @@ unlink TEST_DIR/luks-aes-256-cbc-essiv-auto-sha1.img
 # ================= qemu-img aes-256-cbc-essiv-auto-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=essiv,hash-alg=sha1 TEST_DIR/luks-aes-256-cbc-essiv-auto-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-essiv-auto-sha1.img qiotest-145-aes-256-cbc-essiv-auto-sha1
 # Write test pattern 0xa7
@@ -2519,7 +2499,6 @@ unlink TEST_DIR/luks-aes-256-cbc-plain64-sha256-sha1.img
 # ================= qemu-img aes-256-cbc-plain64-sha256-sha1 =================
 # Create image
 qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha1,ivgen-hash-alg=sha256 TEST_DIR/luks-aes-256-cbc-plain64-sha256-sha1.img 4194304M
-
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-cbc-plain64-sha256-sha1.img qiotest-145-aes-256-cbc-plain64-sha256-sha1
 # Write test pattern 0xa7
-- 
2.34.1

Re: [PATCH 08/14] iotests/149: Remove qemu_img_pipe() call

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> qemu_img_pipe calls blank their output when the command being run is a
> 'create' call and the command succeeds. Thus, the normative output for
> this command in iotest 149 is to print a blank line. We can remove the
> logging from this invocation and use a checked invocation, but we still
> need to inspect the actual output to see if we want to retroactively
> skip the test due to missing cipher support.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/149     |  7 +++++--
>   tests/qemu-iotests/149.out | 21 ---------------------
>   2 files changed, 5 insertions(+), 23 deletions(-)

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

[PATCH 09/14] iotests: remove remaining calls to qemu_img_pipe()

[John Snow]

As part of moving all python iotest invocations of qemu-img onto a
single qemu_img() implementation, remove a few lingering uses of
qemu_img_pipe() from outside of iotests.py itself.

Several cases here rely on the knowledge that qemu_img_pipe() suppresses
*all* output on a successful case when the command being issued is
'create'.

065: This call's output is inspected, but it appears as if it's expected
     to succeed. Replace this call with the checked qemu_img() variant
     instead to get better diagnostics if/when qemu-img itself fails.

237: "create" call output isn't actually logged. Use qemu_img_create()
     instead, which checks the return code. Remove the empty lines from
     the test output.

296: Two calls;
     -create: Expected to succeed. Like other create calls, the output
              isn't actually logged.  Switch to a checked variant
              (qemu_img_create) instead. The output for this test is
              a mixture of both test styles, so actually replace the
              blank line for readability.
     -amend:  This is expected to fail. Log the output.

After this patch, the only uses of qemu_img_pipe are internal to
iotests.py and will be removed in subsequent patches.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/065     |  4 ++--
 tests/qemu-iotests/237     |  3 +--
 tests/qemu-iotests/237.out |  3 ---
 tests/qemu-iotests/296     | 12 ++++++------
 4 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 9466ce7df4..ba94e19349 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_info, qemu_img_pipe
+from iotests import qemu_img, qemu_img_info
 import unittest
 
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -54,7 +54,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
         self.assertEqual(data['data'], self.json_compare)
 
     def test_human(self):
-        data = qemu_img_pipe('info', '--output=human', test_img).split('\n')
+        data = qemu_img('info', '--output=human', test_img).stdout.split('\n')
         data = data[(data.index('Format specific information:') + 1)
                     :data.index('')]
         for field in data:
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
index 43dfd3bd40..5ea13eb01f 100755
--- a/tests/qemu-iotests/237
+++ b/tests/qemu-iotests/237
@@ -165,8 +165,7 @@ with iotests.FilePath('t.vmdk') as disk_path, \
     iotests.log("")
 
     for path in [ extent1_path, extent2_path, extent3_path ]:
-        msg = iotests.qemu_img_pipe('create', '-f', imgfmt, path, '0')
-        iotests.log(msg, [iotests.filter_testfiles])
+        iotests.qemu_img_create('-f', imgfmt, path, '0')
 
     vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path))
     vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path))
diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
index aeb9724492..62b8865677 100644
--- a/tests/qemu-iotests/237.out
+++ b/tests/qemu-iotests/237.out
@@ -129,9 +129,6 @@ Job failed: Cannot find device='this doesn't exist' nor node-name='this doesn't
 
 === Other subformats ===
 
-
-
-
 == Missing extent ==
 
 {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "vmdk", "file": "node0", "size": 33554432, "subformat": "monolithicFlat"}}}
diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index f80ef3434a..0d21b740a7 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -76,7 +76,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
     # create the encrypted block device using qemu-img
     def createImg(self, file, secret):
 
-        output = iotests.qemu_img_pipe(
+        iotests.qemu_img(
             'create',
             '--object', *secret.to_cmdline_object(),
             '-f', iotests.imgfmt,
@@ -84,8 +84,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
             '-o', 'iter-time=10',
             file,
             '1M')
-
-        iotests.log(output, filters=[iotests.filter_test_dir])
+        iotests.log('')
 
     # attempts to add a key using qemu-img
     def addKey(self, file, secret, new_secret):
@@ -99,7 +98,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
                 }
             }
 
-        output = iotests.qemu_img_pipe(
+        output = iotests.qemu_img(
             'amend',
             '--object', *secret.to_cmdline_object(),
             '--object', *new_secret.to_cmdline_object(),
@@ -108,8 +107,9 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
             '-o', 'new-secret=' + new_secret.id(),
             '-o', 'iter-time=10',
 
-            "json:" + json.dumps(image_options)
-            )
+            "json:" + json.dumps(image_options),
+            check=False  # Expected to fail. Log output.
+        ).stdout
 
         iotests.log(output, filters=[iotests.filter_test_dir])
 
-- 
2.34.1

Re: [PATCH 09/14] iotests: remove remaining calls to qemu_img_pipe()

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> As part of moving all python iotest invocations of qemu-img onto a
> single qemu_img() implementation, remove a few lingering uses of
> qemu_img_pipe() from outside of iotests.py itself.
>
> Several cases here rely on the knowledge that qemu_img_pipe() suppresses
> *all* output on a successful case when the command being issued is
> 'create'.
>
> 065: This call's output is inspected, but it appears as if it's expected
>       to succeed. Replace this call with the checked qemu_img() variant
>       instead to get better diagnostics if/when qemu-img itself fails.
>
> 237: "create" call output isn't actually logged. Use qemu_img_create()
>       instead, which checks the return code. Remove the empty lines from
>       the test output.
>
> 296: Two calls;
>       -create: Expected to succeed. Like other create calls, the output
>                isn't actually logged.  Switch to a checked variant
>                (qemu_img_create) instead. The output for this test is
>                a mixture of both test styles, so actually replace the
>                blank line for readability.
>       -amend:  This is expected to fail. Log the output.
>
> After this patch, the only uses of qemu_img_pipe are internal to
> iotests.py and will be removed in subsequent patches.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/065     |  4 ++--
>   tests/qemu-iotests/237     |  3 +--
>   tests/qemu-iotests/237.out |  3 ---
>   tests/qemu-iotests/296     | 12 ++++++------
>   4 files changed, 9 insertions(+), 13 deletions(-)

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

[PATCH 10/14] iotests: use qemu_img() in has_working_luks()

[John Snow]

Admittedly a mostly lateral move, but qemu_img() is essentially the
replacement for qemu_img_pipe_and_status(). It will give slightly better
diagnostics on crash.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 698d5a7fdc..1b6e28ba64 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1450,20 +1450,20 @@ def has_working_luks() -> Tuple[bool, str]:
     """
 
     img_file = f'{test_dir}/luks-test.luks'
-    (output, status) = \
-        qemu_img_pipe_and_status('create', '-f', 'luks',
-                                 '--object', luks_default_secret_object,
-                                 '-o', luks_default_key_secret_opt,
-                                 '-o', 'iter-time=10',
-                                 img_file, '1G')
+    res = qemu_img('create', '-f', 'luks',
+                   '--object', luks_default_secret_object,
+                   '-o', luks_default_key_secret_opt,
+                   '-o', 'iter-time=10',
+                   img_file, '1G',
+                   check=False)
     try:
         os.remove(img_file)
     except OSError:
         pass
 
-    if status != 0:
-        reason = output
-        for line in output.splitlines():
+    if res.returncode:
+        reason = res.stdout
+        for line in res.stdout.splitlines():
             if img_file + ':' in line:
                 reason = line.split(img_file + ':', 1)[1].strip()
                 break
-- 
2.34.1

Re: [PATCH 10/14] iotests: use qemu_img() in has_working_luks()

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> Admittedly a mostly lateral move, but qemu_img() is essentially the
> replacement for qemu_img_pipe_and_status(). It will give slightly better
> diagnostics on crash.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)

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

[PATCH 11/14] iotests: replace qemu_img_log('create', ...) calls

[John Snow]

qemu_img_log() calls into qemu_img_pipe(), which always removes output
for 'create' commands on success anyway. Replace all of these calls to
the simpler qemu_img_create(...) which doesn't log, but raises a
detailed exception object on failure instead.

Blank lines are removed from output files where appropriate.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/255     |  8 ++++----
 tests/qemu-iotests/255.out |  4 ----
 tests/qemu-iotests/274     | 17 ++++++++---------
 tests/qemu-iotests/274.out | 29 -----------------------------
 tests/qemu-iotests/280     |  2 +-
 tests/qemu-iotests/280.out |  1 -
 6 files changed, 13 insertions(+), 48 deletions(-)

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 3d6d0e80cb..f86fa851b6 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -42,8 +42,8 @@ with iotests.FilePath('t.qcow2') as disk_path, \
     size_str = str(size)
 
     iotests.create_image(base_path, size)
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, mid_path, size_str)
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, disk_path, size_str)
+    iotests.qemu_img_create('-f', iotests.imgfmt, mid_path, size_str)
+    iotests.qemu_img_create('-f', iotests.imgfmt, disk_path, size_str)
 
     # Create a backing chain like this:
     # base <- [throttled: bps-read=4096] <- mid <- overlay
@@ -92,8 +92,8 @@ with iotests.FilePath('src.qcow2') as src_path, \
     size = 128 * 1024 * 1024
     size_str = str(size)
 
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, src_path, size_str)
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, dst_path, size_str)
+    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),
diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
index 11a05a5213..2e837cbb5f 100644
--- a/tests/qemu-iotests/255.out
+++ b/tests/qemu-iotests/255.out
@@ -3,8 +3,6 @@ Finishing a commit job with background reads
 
 === Create backing chain and start VM ===
 
-
-
 === Start background read requests ===
 
 === Run a commit job ===
@@ -21,8 +19,6 @@ Closing the VM while a job is being cancelled
 
 === Create images and start VM ===
 
-
-
 wrote 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
index 080a90f10f..2495e051a2 100755
--- a/tests/qemu-iotests/274
+++ b/tests/qemu-iotests/274
@@ -31,12 +31,11 @@ size_long = 2 * 1024 * 1024
 size_diff = size_long - size_short
 
 def create_chain() -> None:
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
-                         str(size_long))
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base,
-                         '-F', iotests.imgfmt, mid, str(size_short))
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid,
-                         '-F', iotests.imgfmt, top, str(size_long))
+    iotests.qemu_img_create('-f', iotests.imgfmt, base, str(size_long))
+    iotests.qemu_img_create('-f', iotests.imgfmt, '-b', base,
+                            '-F', iotests.imgfmt, mid, str(size_short))
+    iotests.qemu_img_create('-f', iotests.imgfmt, '-b', mid,
+                            '-F', iotests.imgfmt, top, str(size_long))
 
     iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
 
@@ -160,9 +159,9 @@ with iotests.FilePath('base') as base, \
             ('off',      '512k', '256k', '500k', '436k')]:
 
         iotests.log('=== preallocation=%s ===' % prealloc)
-        iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, base_size)
-        iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base,
-                             '-F', iotests.imgfmt, top, top_size_old)
+        iotests.qemu_img_create('-f', iotests.imgfmt, base, base_size)
+        iotests.qemu_img_create('-f', iotests.imgfmt, '-b', base,
+                                '-F', iotests.imgfmt, top, top_size_old)
         iotests.qemu_io_log('-c', 'write -P 1 %s 64k' % off, base)
 
         # After this, top_size_old to base_size should be allocated/zeroed.
diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
index 1ce40d839a..acd8b166a6 100644
--- a/tests/qemu-iotests/274.out
+++ b/tests/qemu-iotests/274.out
@@ -1,7 +1,4 @@
 == Commit tests ==
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -63,9 +60,6 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing HMP commit (top -> mid) ===
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -92,9 +86,6 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing QMP active commit (top -> mid) ===
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -127,9 +118,6 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing qemu-img commit (top -> base) ===
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -154,9 +142,6 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 === Testing QMP active commit (top -> base) ===
-
-
-
 wrote 2097152/2097152 bytes at offset 0
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -190,8 +175,6 @@ read 1048576/1048576 bytes at offset 1048576
 
 == Resize tests ==
 === preallocation=off ===
-
-
 wrote 65536/65536 bytes at offset 5368709120
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -207,8 +190,6 @@ read 65536/65536 bytes at offset 5368709120
 { "start": 1073741824, "length": 7516192768, "depth": 0, "present": true, "zero": true, "data": false}]
 
 === preallocation=metadata ===
-
-
 wrote 65536/65536 bytes at offset 33285996544
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -229,8 +210,6 @@ read 65536/65536 bytes at offset 33285996544
 { "start": 34896609280, "length": 536870912, "depth": 0, "present": true, "zero": true, "data": false, "offset": 2685075456}]
 
 === preallocation=falloc ===
-
-
 wrote 65536/65536 bytes at offset 9437184
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -246,8 +225,6 @@ read 65536/65536 bytes at offset 9437184
 { "start": 5242880, "length": 10485760, "depth": 0, "present": true, "zero": false, "data": true, "offset": 327680}]
 
 === preallocation=full ===
-
-
 wrote 65536/65536 bytes at offset 11534336
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -263,8 +240,6 @@ read 65536/65536 bytes at offset 11534336
 { "start": 8388608, "length": 4194304, "depth": 0, "present": true, "zero": false, "data": true, "offset": 327680}]
 
 === preallocation=off ===
-
-
 wrote 65536/65536 bytes at offset 259072
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -281,8 +256,6 @@ read 65536/65536 bytes at offset 259072
 { "start": 262144, "length": 262144, "depth": 0, "present": true, "zero": true, "data": false}]
 
 === preallocation=off ===
-
-
 wrote 65536/65536 bytes at offset 344064
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -298,8 +271,6 @@ read 65536/65536 bytes at offset 344064
 { "start": 262144, "length": 262144, "depth": 0, "present": true, "zero": true, "data": false}]
 
 === preallocation=off ===
-
-
 wrote 65536/65536 bytes at offset 446464
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
diff --git a/tests/qemu-iotests/280 b/tests/qemu-iotests/280
index 628f3c33ca..5f50500fdb 100755
--- a/tests/qemu-iotests/280
+++ b/tests/qemu-iotests/280
@@ -33,7 +33,7 @@ with iotests.FilePath('base') as base_path , \
      iotests.FilePath('top') as top_path, \
      iotests.VM() as vm:
 
-    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base_path, '64M')
+    iotests.qemu_img_create('-f', iotests.imgfmt, base_path, '64M')
 
     iotests.log('=== Launch VM ===')
     vm.add_object('iothread,id=iothread0')
diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out
index e39164c579..c75f437c00 100644
--- a/tests/qemu-iotests/280.out
+++ b/tests/qemu-iotests/280.out
@@ -1,4 +1,3 @@
-
 === Launch VM ===
 Enabling migration QMP events on VM...
 {"return": {}}
-- 
2.34.1

Re: [PATCH 11/14] iotests: replace qemu_img_log('create', ...) calls

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> qemu_img_log() calls into qemu_img_pipe(), which always removes output
> for 'create' commands on success anyway. Replace all of these calls to
> the simpler qemu_img_create(...) which doesn't log, but raises a
> detailed exception object on failure instead.
>
> Blank lines are removed from output files where appropriate.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/255     |  8 ++++----
>   tests/qemu-iotests/255.out |  4 ----
>   tests/qemu-iotests/274     | 17 ++++++++---------
>   tests/qemu-iotests/274.out | 29 -----------------------------
>   tests/qemu-iotests/280     |  2 +-
>   tests/qemu-iotests/280.out |  1 -
>   6 files changed, 13 insertions(+), 48 deletions(-)

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

[PATCH 12/14] iotests: remove qemu_img_pipe_and_status()

[John Snow]

With the exceptional 'create' calls removed in the prior commit, change
qemu_img_log() and img_info_log() to call qemu_img() directly
instead.

In keeping with the spirit of diff-based tests, allow these calls to
qemu_img() to return an unchecked non-zero status code -- because any
error we'd see from the output is going into the log anyway.

Every last call to qemu-img is now either checked for a return code of
zero or has its output logged. It should be very hard to accidentally
ignore the return code *or* output from qemu-img now; intentional malice
remains unhandled.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1b6e28ba64..f8d966cd73 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -207,15 +207,6 @@ def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
 
     return result
 
-def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
-    """
-    Run qemu-img and return both its output and its exit code
-    """
-    is_create = bool(args and args[0] == 'create')
-    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
-    return qemu_tool_pipe_and_status('qemu-img', full_args,
-                                     drop_successful_output=is_create)
-
 def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
              ) -> subprocess.CompletedProcess[str]:
     """
@@ -326,17 +317,14 @@ def qemu_img_info(*args: str) -> Any:
 def qemu_img_map(*args: str) -> Any:
     return qemu_img_json('map', "--output", "json", *args)
 
-def qemu_img_pipe(*args: str) -> str:
-    '''Run qemu-img and return its output'''
-    return qemu_img_pipe_and_status(*args)[0]
-
-def qemu_img_log(*args):
-    result = qemu_img_pipe(*args)
-    log(result, filters=[filter_testfiles])
+def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
+    result = qemu_img(*args, check=False)
+    log(result.stdout, filters=[filter_testfiles])
     return result
 
-def img_info_log(filename, filter_path=None, use_image_opts=False,
-                 extra_args=()):
+def img_info_log(filename: str, filter_path: Optional[str] = None,
+                 use_image_opts: bool = False, extra_args: Sequence[str] = (),
+                 ) -> None:
     args = ['info']
     if use_image_opts:
         args.append('--image-opts')
@@ -345,7 +333,7 @@ def img_info_log(filename, filter_path=None, use_image_opts=False,
     args += extra_args
     args.append(filename)
 
-    output = qemu_img_pipe(*args)
+    output = qemu_img(*args, check=False).stdout
     if not filter_path:
         filter_path = filename
     log(filter_img_info(output, filter_path))
-- 
2.34.1

Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> With the exceptional 'create' calls removed in the prior commit, change
> qemu_img_log() and img_info_log() to call qemu_img() directly
> instead.
>
> In keeping with the spirit of diff-based tests, allow these calls to
> qemu_img() to return an unchecked non-zero status code -- because any
> error we'd see from the output is going into the log anyway.

:(

I’d prefer having an exception that points exactly to where in the test 
the offending qemu-img call was.  But then again, I dislike such 
log-based tests anyway, and this is precisely one reason for it...

I think Kevin disliked my approach of just `assert qemu_img() == 0` 
mainly because you don’t get the stderr output with it.  But you’ve 
solved that problem now, so I don’t think there’s a reason why we 
wouldn’t want a raised exception.

Hanna

> Every last call to qemu-img is now either checked for a return code of
> zero or has its output logged. It should be very hard to accidentally
> ignore the return code *or* output from qemu-img now; intentional malice
> remains unhandled.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 26 +++++++-------------------
>   1 file changed, 7 insertions(+), 19 deletions(-)

Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()

[John Snow]

On Thu, Mar 17, 2022 at 11:28 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 09.03.22 04:54, John Snow wrote:
> > With the exceptional 'create' calls removed in the prior commit, change
> > qemu_img_log() and img_info_log() to call qemu_img() directly
> > instead.
> >
> > In keeping with the spirit of diff-based tests, allow these calls to
> > qemu_img() to return an unchecked non-zero status code -- because any
> > error we'd see from the output is going into the log anyway.
>
> :(
>
> I’d prefer having an exception that points exactly to where in the test
> the offending qemu-img call was.  But then again, I dislike such
> log-based tests anyway, and this is precisely one reason for it...
>
> I think Kevin disliked my approach of just `assert qemu_img() == 0`
> mainly because you don’t get the stderr output with it.  But you’ve
> solved that problem now, so I don’t think there’s a reason why we
> wouldn’t want a raised exception.
>
> Hanna
>

I thought you and Kevin actually preferred diff-based tests, maybe I
misunderstood. I know that there was a strong dislike of the unittest
based tests, and that the new script-style was more preferred. I
thought inherent to that was an actual preference for diff-based
itself, but maybe not?

I'd say negative tests are easier with the diff-based as one benefit.
I'm a little partial to that kind of test. (I noticed that bitmap
tests were the most habitual user of negative tests involving
qemu-img, haha.) Otherwise, I guess I don't really care what the test
mechanism is provided that the error output is informative. Happy to
defer to consensus between you and Kevin.

Anyway, this patch (and the ones that follow it, I haven't read your
feedback on 13-14 yet) doesn't close the door on making everything
Except-by-default, it would just be further work that needs to happen
after the fact. How do you want to move forward?

- Replace calls to qemu_img_log() with qemu_img()
- Make qemu_img_log() raise by default, but log output on success cases
- Something else?

--js

Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()

[Hanna Reitz]

On 17.03.22 16:58, John Snow wrote:
> On Thu, Mar 17, 2022 at 11:28 AM Hanna Reitz <hreitz@redhat.com> wrote:
>> On 09.03.22 04:54, John Snow wrote:
>>> With the exceptional 'create' calls removed in the prior commit, change
>>> qemu_img_log() and img_info_log() to call qemu_img() directly
>>> instead.
>>>
>>> In keeping with the spirit of diff-based tests, allow these calls to
>>> qemu_img() to return an unchecked non-zero status code -- because any
>>> error we'd see from the output is going into the log anyway.
>> :(
>>
>> I’d prefer having an exception that points exactly to where in the test
>> the offending qemu-img call was.  But then again, I dislike such
>> log-based tests anyway, and this is precisely one reason for it...
>>
>> I think Kevin disliked my approach of just `assert qemu_img() == 0`
>> mainly because you don’t get the stderr output with it.  But you’ve
>> solved that problem now, so I don’t think there’s a reason why we
>> wouldn’t want a raised exception.
>>
>> Hanna
>>
> I thought you and Kevin actually preferred diff-based tests, maybe I
> misunderstood. I know that there was a strong dislike of the unittest
> based tests,

Oh gosh not from me.  I really like them, because they allow skipping 
test cases so easily (and because reviewing patches for such tests tend 
to be easier, because the code is explicit about what results it expects).

> and that the new script-style was more preferred. I
> thought inherent to that was an actual preference for diff-based
> itself, but maybe not?
>
> I'd say negative tests are easier with the diff-based as one benefit.
> I'm a little partial to that kind of test. (I noticed that bitmap
> tests were the most habitual user of negative tests involving
> qemu-img, haha.) Otherwise, I guess I don't really care what the test
> mechanism is provided that the error output is informative. Happy to
> defer to consensus between you and Kevin.

I don’t think we have a consensus. :)

But AFAIU the main disagreement was based on what each of us found more 
important when something fails.  Kevin found it more important to see 
what the failing process wrote to stderr, I found it more important to 
see what call failed exactly.  Since your exception model gives us both, 
I believe we can both be happy with it.

> Anyway, this patch (and the ones that follow it, I haven't read your
> feedback on 13-14 yet) doesn't close the door on making everything
> Except-by-default, it would just be further work that needs to happen
> after the fact. How do you want to move forward?
>
> - Replace calls to qemu_img_log() with qemu_img()
> - Make qemu_img_log() raise by default, but log output on success cases
> - Something else?

Second one sounds good to me for this series, because I’d expect it’d 
mean the least amount of changes...?

Hanna

Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()

[John Snow]

On Thu, Mar 17, 2022 at 12:04 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 17.03.22 16:58, John Snow wrote:
> > On Thu, Mar 17, 2022 at 11:28 AM Hanna Reitz <hreitz@redhat.com> wrote:
> >> On 09.03.22 04:54, John Snow wrote:
> >>> With the exceptional 'create' calls removed in the prior commit, change
> >>> qemu_img_log() and img_info_log() to call qemu_img() directly
> >>> instead.
> >>>
> >>> In keeping with the spirit of diff-based tests, allow these calls to
> >>> qemu_img() to return an unchecked non-zero status code -- because any
> >>> error we'd see from the output is going into the log anyway.
> >> :(
> >>
> >> I’d prefer having an exception that points exactly to where in the test
> >> the offending qemu-img call was.  But then again, I dislike such
> >> log-based tests anyway, and this is precisely one reason for it...
> >>
> >> I think Kevin disliked my approach of just `assert qemu_img() == 0`
> >> mainly because you don’t get the stderr output with it.  But you’ve
> >> solved that problem now, so I don’t think there’s a reason why we
> >> wouldn’t want a raised exception.
> >>
> >> Hanna
> >>
> > I thought you and Kevin actually preferred diff-based tests, maybe I
> > misunderstood. I know that there was a strong dislike of the unittest
> > based tests,
>
> Oh gosh not from me.  I really like them, because they allow skipping
> test cases so easily (and because reviewing patches for such tests tend
> to be easier, because the code is explicit about what results it expects).

Oh! Today-I-Learned. Yeah, the ability to skip cases is nice indeed.

>
> > and that the new script-style was more preferred. I
> > thought inherent to that was an actual preference for diff-based
> > itself, but maybe not?
> >
> > I'd say negative tests are easier with the diff-based as one benefit.
> > I'm a little partial to that kind of test. (I noticed that bitmap
> > tests were the most habitual user of negative tests involving
> > qemu-img, haha.) Otherwise, I guess I don't really care what the test
> > mechanism is provided that the error output is informative. Happy to
> > defer to consensus between you and Kevin.
>
> I don’t think we have a consensus. :)
>
> But AFAIU the main disagreement was based on what each of us found more
> important when something fails.  Kevin found it more important to see
> what the failing process wrote to stderr, I found it more important to
> see what call failed exactly.  Since your exception model gives us both,
> I believe we can both be happy with it.
>

Yeah, I like having both. Knowing which call failed exactly is helpful
for fixing the test and/or reproducing it on your own. Seeing the
output can give extremely important clues about the nature of the
failure.

> > Anyway, this patch (and the ones that follow it, I haven't read your
> > feedback on 13-14 yet) doesn't close the door on making everything
> > Except-by-default, it would just be further work that needs to happen
> > after the fact. How do you want to move forward?
> >
> > - Replace calls to qemu_img_log() with qemu_img()
> > - Make qemu_img_log() raise by default, but log output on success cases
> > - Something else?
>
> Second one sounds good to me for this series, because I’d expect it’d
> mean the least amount of changes...?
>

Alright, I'll see what I can do. Thanks for the review, I know churn
like this isn't the most fulfilling thing.

> Hanna
>

[PATCH 13/14] iotests: make qemu_img_log() check log level

[John Snow]

Improve qemu_img_log() to actually check if logging is turned on. If it
isn't, revert to the behavior of qemu_img(). This is done so that there
really is no way to avoid scrutinizing qemu-ing subprocess calls by
accident.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f8d966cd73..6af503058f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -318,7 +318,19 @@ def qemu_img_map(*args: str) -> Any:
     return qemu_img_json('map', "--output", "json", *args)
 
 def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
-    result = qemu_img(*args, check=False)
+    """
+    Logged, unchecked variant of qemu_img() that allows non-zero exit codes.
+
+    If logging is perceived to be disabled, this function will fall back
+    to prohibiting non-zero return codes.
+
+    :raise VerboseProcessError:
+        When the return code is negative, or when logging is disabled
+        on any non-zero return code.
+
+    :return: a CompletedProcess instance with returncode and console output.
+    """
+    result = qemu_img(*args, check=not logging_enabled())
     log(result.stdout, filters=[filter_testfiles])
     return result
 
@@ -1640,6 +1652,11 @@ def activate_logging():
     test_logger.setLevel(logging.INFO)
     test_logger.propagate = False
 
+def logging_enabled() -> bool:
+    """Return True if iotest logging is active."""
+    return (test_logger.hasHandlers()
+            and test_logger.getEffectiveLevel() >= logging.INFO)
+
 # This is called from script-style iotests without a single point of entry
 def script_initialize(*args, **kwargs):
     """Initialize script-style tests without running any tests."""
-- 
2.34.1

Re: [PATCH 13/14] iotests: make qemu_img_log() check log level

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> Improve qemu_img_log() to actually check if logging is turned on. If it
> isn't, revert to the behavior of qemu_img(). This is done so that there
> really is no way to avoid scrutinizing qemu-ing subprocess calls by
> accident.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)

Looks OK to me, so

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

But I’d really just prefer `qemu_img_log()` to check the exit status all 
the time.

[PATCH 14/14] iotests: make img_info_log() call qemu_img_log()

[John Snow]

Add configurable filters to qemu_img_log(), and re-write img_info_log()
to call into qemu_img_log() with a custom filter instead.

After this patch, every last call to qemu_img() is now guaranteed to
either have its return code checked for zero, OR have its output
actually visibly logged somewhere.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6af503058f..0c69327c00 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -317,10 +317,14 @@ def qemu_img_info(*args: str) -> Any:
 def qemu_img_map(*args: str) -> Any:
     return qemu_img_json('map', "--output", "json", *args)
 
-def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
+def qemu_img_log(
+        *args: str,
+        filters: Iterable[Callable[[str], str]] = (),
+) -> subprocess.CompletedProcess[str]:
     """
     Logged, unchecked variant of qemu_img() that allows non-zero exit codes.
 
+    By default, output will be filtered through filter_testfiles().
     If logging is perceived to be disabled, this function will fall back
     to prohibiting non-zero return codes.
 
@@ -331,7 +335,7 @@ def qemu_img_log(*args: str) -> subprocess.CompletedProcess[str]:
     :return: a CompletedProcess instance with returncode and console output.
     """
     result = qemu_img(*args, check=not logging_enabled())
-    log(result.stdout, filters=[filter_testfiles])
+    log(result.stdout, filters=filters or [filter_testfiles])
     return result
 
 def img_info_log(filename: str, filter_path: Optional[str] = None,
@@ -345,10 +349,11 @@ def img_info_log(filename: str, filter_path: Optional[str] = None,
     args += extra_args
     args.append(filename)
 
-    output = qemu_img(*args, check=False).stdout
     if not filter_path:
         filter_path = filename
-    log(filter_img_info(output, filter_path))
+    qemu_img_log(
+        *args,
+        filters=[lambda output: filter_img_info(output, filter_path)])
 
 def qemu_io_wrap_args(args: Sequence[str]) -> List[str]:
     if '-f' in args or '--image-opts' in args:
-- 
2.34.1

Re: [PATCH 14/14] iotests: make img_info_log() call qemu_img_log()

[Hanna Reitz]

On 09.03.22 04:54, John Snow wrote:
> Add configurable filters to qemu_img_log(), and re-write img_info_log()
> to call into qemu_img_log() with a custom filter instead.
>
> After this patch, every last call to qemu_img() is now guaranteed to
> either have its return code checked for zero, OR have its output
> actually visibly logged somewhere.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)

 From my POV, this is a regression because before this patch (not this 
series, though, admittedly), `img_info_log()` would throw an exception 
on error, and with patch 12 being as it is, it will revert to its 
pre-series behavior of not throwing an exception.  I prefer exceptions 
to failed reference output diffs, because an exception tells me which 
call failed.

Hanna

Re: [PATCH 14/14] iotests: make img_info_log() call qemu_img_log()

[John Snow]

On Thu, Mar 17, 2022 at 11:39 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 09.03.22 04:54, John Snow wrote:
> > Add configurable filters to qemu_img_log(), and re-write img_info_log()
> > to call into qemu_img_log() with a custom filter instead.
> >
> > After this patch, every last call to qemu_img() is now guaranteed to
> > either have its return code checked for zero, OR have its output
> > actually visibly logged somewhere.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
>
>  From my POV, this is a regression because before this patch (not this
> series, though, admittedly), `img_info_log()` would throw an exception
> on error, and with patch 12 being as it is, it will revert to its
> pre-series behavior of not throwing an exception.  I prefer exceptions
> to failed reference output diffs, because an exception tells me which
> call failed.

Hm, yeah. I just need to figure out if *all* of the qemu_img_log()
calls are safe to enforce the return code of zero on... or how many
need work if I change the default behavior. Let me see what I can do.

I suppose it's maybe a bit late to try and squeak any of this in for
freeze, so I can roll everything back up into one big series again and
send a new revision.

Re: [PATCH 14/14] iotests: make img_info_log() call qemu_img_log()

[John Snow]

On Thu, Mar 17, 2022 at 1:00 PM John Snow <jsnow@redhat.com> wrote:
>
> On Thu, Mar 17, 2022 at 11:39 AM Hanna Reitz <hreitz@redhat.com> wrote:
> >
> > On 09.03.22 04:54, John Snow wrote:
> > > Add configurable filters to qemu_img_log(), and re-write img_info_log()
> > > to call into qemu_img_log() with a custom filter instead.
> > >
> > > After this patch, every last call to qemu_img() is now guaranteed to
> > > either have its return code checked for zero, OR have its output
> > > actually visibly logged somewhere.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >   tests/qemu-iotests/iotests.py | 13 +++++++++----
> > >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> >  From my POV, this is a regression because before this patch (not this
> > series, though, admittedly), `img_info_log()` would throw an exception
> > on error, and with patch 12 being as it is, it will revert to its
> > pre-series behavior of not throwing an exception.  I prefer exceptions

Oh, actually... patch #12 does this:

-    output = qemu_img_pipe(*args)
+    output = qemu_img(*args, check=False).stdout

so I never actually toggled error checking on for this function at
all. This isn't a regression.

At a glance, img_info_log() calls fail as a matter of course in 242
and 266 and ... hm, I can't quite test 207, it doesn't work for me,
even before this series.

I didn't test *all* qemu_img calls yet either, but ... I'm going to
gently suggest that "converting logged calls to qemu_img() to be
checked calls" is "for another series" material.

--js

> > to failed reference output diffs, because an exception tells me which
> > call failed.
>
> Hm, yeah. I just need to figure out if *all* of the qemu_img_log()
> calls are safe to enforce the return code of zero on... or how many
> need work if I change the default behavior. Let me see what I can do.
>
> I suppose it's maybe a bit late to try and squeak any of this in for
> freeze, so I can roll everything back up into one big series again and
> send a new revision.

Re: [PATCH 14/14] iotests: make img_info_log() call qemu_img_log()

[Hanna Reitz]

On 17.03.22 18:45, John Snow wrote:
> On Thu, Mar 17, 2022 at 1:00 PM John Snow <jsnow@redhat.com> wrote:
>> On Thu, Mar 17, 2022 at 11:39 AM Hanna Reitz <hreitz@redhat.com> wrote:
>>> On 09.03.22 04:54, John Snow wrote:
>>>> Add configurable filters to qemu_img_log(), and re-write img_info_log()
>>>> to call into qemu_img_log() with a custom filter instead.
>>>>
>>>> After this patch, every last call to qemu_img() is now guaranteed to
>>>> either have its return code checked for zero, OR have its output
>>>> actually visibly logged somewhere.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/iotests.py | 13 +++++++++----
>>>>    1 file changed, 9 insertions(+), 4 deletions(-)
>>>   From my POV, this is a regression because before this patch (not this
>>> series, though, admittedly), `img_info_log()` would throw an exception
>>> on error, and with patch 12 being as it is, it will revert to its
>>> pre-series behavior of not throwing an exception.  I prefer exceptions
> Oh, actually... patch #12 does this:
>
> -    output = qemu_img_pipe(*args)
> +    output = qemu_img(*args, check=False).stdout

:(

You’re right, I missed that.

> so I never actually toggled error checking on for this function at
> all. This isn't a regression.
>
> At a glance, img_info_log() calls fail as a matter of course in 242
> and 266 and ... hm, I can't quite test 207, it doesn't work for me,
> even before this series.

Ugh, broken in e3296cc796aeaf319f3ed4e064ec309baf5e4da4.

(putting that on my TOFIX list)

> I didn't test *all* qemu_img calls yet either, but ... I'm going to
> gently suggest that "converting logged calls to qemu_img() to be
> checked calls" is "for another series" material.

:C

I mean, adding a `check` parameter to `img_info_log()` and 
`qemu_img_log()` would be something like a 9+/5- diff (including 242 and 
266 changes, but disregarding the necessary comment changes in 
`qemu_img_log()`).  I think that’d be fine, and a bit thin for its own 
“series”. O:)

Hanna

Re: [PATCH 14/14] iotests: make img_info_log() call qemu_img_log()

[John Snow]

On Thu, Mar 17, 2022 at 2:27 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 17.03.22 18:45, John Snow wrote:
> > On Thu, Mar 17, 2022 at 1:00 PM John Snow <jsnow@redhat.com> wrote:
> >> On Thu, Mar 17, 2022 at 11:39 AM Hanna Reitz <hreitz@redhat.com> wrote:
> >>> On 09.03.22 04:54, John Snow wrote:
> >>>> Add configurable filters to qemu_img_log(), and re-write img_info_log()
> >>>> to call into qemu_img_log() with a custom filter instead.
> >>>>
> >>>> After this patch, every last call to qemu_img() is now guaranteed to
> >>>> either have its return code checked for zero, OR have its output
> >>>> actually visibly logged somewhere.
> >>>>
> >>>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>>> ---
> >>>>    tests/qemu-iotests/iotests.py | 13 +++++++++----
> >>>>    1 file changed, 9 insertions(+), 4 deletions(-)
> >>>   From my POV, this is a regression because before this patch (not this
> >>> series, though, admittedly), `img_info_log()` would throw an exception
> >>> on error, and with patch 12 being as it is, it will revert to its
> >>> pre-series behavior of not throwing an exception.  I prefer exceptions
> > Oh, actually... patch #12 does this:
> >
> > -    output = qemu_img_pipe(*args)
> > +    output = qemu_img(*args, check=False).stdout
>
> :(
>
> You’re right, I missed that.
>
> > so I never actually toggled error checking on for this function at
> > all. This isn't a regression.
> >
> > At a glance, img_info_log() calls fail as a matter of course in 242
> > and 266 and ... hm, I can't quite test 207, it doesn't work for me,
> > even before this series.
>
> Ugh, broken in e3296cc796aeaf319f3ed4e064ec309baf5e4da4.
>
> (putting that on my TOFIX list)
>
> > I didn't test *all* qemu_img calls yet either, but ... I'm going to
> > gently suggest that "converting logged calls to qemu_img() to be
> > checked calls" is "for another series" material.
>
> :C
>
> I mean, adding a `check` parameter to `img_info_log()` and
> `qemu_img_log()` would be something like a 9+/5- diff (including 242 and
> 266 changes, but disregarding the necessary comment changes in
> `qemu_img_log()`).  I think that’d be fine, and a bit thin for its own
> “series”. O:)

You're right. I uh. actually started doing this after I told you I
wasn't going to, because I was surprised by how few calls there were.
so I changed my mind about not wanting to audit them.

Merry Christmas!

--js