Re: [PATCH v3 4/5] iotests: make qemu_img raise on non-zero rc by default

[Hanna Reitz <hreitz@redhat.com>]

On 08.03.22 02:57, John Snow wrote:
> re-write qemu_img() as a function that will by default raise a
> VerboseProcessException (extended from CalledProcessException) on
> non-zero return codes. This will produce a stack trace that will show
> the command line arguments and return code from the failed process run.

Why not qemu_img_pipe_and_status() as the central function where all 
qemu-img calls go through?

It seems like this makes qemu_img() a second version of 
qemu_img_pipe_and_status(), which is a bit weird.

(Or perhaps it should actually be qemu_tool_pipe_and_status() that 
receives this treatment, with qemu-io functions just passing check=False 
by default.)

> Users that want something more flexible (there appears to be only one)
> can use check=False and manage the return themselves. However, when the
> return code is negative, the Exception will be raised no matter what.
> This is done under the belief that there's no legitimate reason, even in
> negative tests, to see a crash from qemu-img.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/qemu-iotests/257        |  8 +++--
>   tests/qemu-iotests/iotests.py | 56 ++++++++++++++++++++++++++++++-----
>   2 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
> index fb5359c581..e7e7a2317e 100755
> --- a/tests/qemu-iotests/257
> +++ b/tests/qemu-iotests/257
> @@ -241,11 +241,13 @@ def compare_images(image, reference, baseimg=None, expected_match=True):
>       expected_ret = 0 if expected_match else 1
>       if baseimg:
>           qemu_img("rebase", "-u", "-b", baseimg, '-F', iotests.imgfmt, image)
> -    ret = qemu_img("compare", image, reference)
> +
> +    sub = qemu_img("compare", image, reference, check=False)
> +
>       log('qemu_img compare "{:s}" "{:s}" ==> {:s}, {:s}'.format(
>           image, reference,
> -        "Identical" if ret == 0 else "Mismatch",
> -        "OK!" if ret == expected_ret else "ERROR!"),
> +        "Identical" if sub.returncode == 0 else "Mismatch",
> +        "OK!" if sub.returncode == expected_ret else "ERROR!"),
>           filters=[iotests.filter_testfiles])
>   
>   def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 508adade9e..ec4568b24a 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -37,9 +37,10 @@
>   
>   from contextlib import contextmanager
>   
> +from qemu.aqmp.legacy import QEMUMonitorProtocol
>   from qemu.machine import qtest
>   from qemu.qmp import QMPMessage
> -from qemu.aqmp.legacy import QEMUMonitorProtocol
> +from qemu.utils import VerboseProcessError
>   
>   # Use this logger for logging messages directly from the iotests module
>   logger = logging.getLogger('qemu.iotests')
> @@ -215,9 +216,49 @@ def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
>       return qemu_tool_pipe_and_status('qemu-img', full_args,
>                                        drop_successful_output=is_create)
>   
> -def qemu_img(*args: str) -> int:
> -    '''Run qemu-img and return the exit code'''
> -    return qemu_img_pipe_and_status(*args)[1]
> +def qemu_img(*args: str, check: bool = True, combine_stdio: bool = True
> +             ) -> subprocess.CompletedProcess[str]:
> +    """
> +    Run qemu_img and return the status code and console output.
> +
> +    This function always prepends QEMU_IMG_OPTIONS and may further alter
> +    the args for 'create' commands.
> +
> +    :param args: command-line arguments to qemu-img.
> +    :param check: Enforce a return code of zero.
> +    :param combine_stdio: set to False to keep stdout/stderr separated.
> +
> +    :raise VerboseProcessError:
> +        When the return code is negative, or on any non-zero exit code
> +        when 'check=True' was provided (the default). This exception has
> +        'stdout', 'stderr', and 'returncode' properties that may be
> +        inspected to show greater detail. If this exception is not
> +        handled, the command-line, return code, and all console output
> +        will be included at the bottom of the stack trace.
> +
> +    :return: a CompletedProcess. This object has args, returncode, and
> +        stdout properties. If streams are not combined, it will also
> +        have a stderr property.
> +    """
> +    full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
> +
> +    subp = subprocess.run(
> +        full_args,
> +        stdout=subprocess.PIPE,
> +        stderr=subprocess.STDOUT if combine_stdio else subprocess.PIPE,
> +        universal_newlines=True,
> +        check=False
> +    )
> +
> +    if check and subp.returncode or (subp.returncode < 0):

I wouldn’t expect these parentheses here in any other language, are they 
required in Python?

> +        raise VerboseProcessError(
> +            subp.returncode, full_args,
> +            output=subp.stdout,
> +            stderr=subp.stderr,
> +        )

I trust these parameters are correct, because it really sometimes seems 
like Python doc doesn’t want to tell me about the arguments that 
constructors take.  (The only thing I found out is that `stdout` works 
as an alias for `output`, so passing `output` here and reading 
`self.stdout` in `VerboseProcesError` should(tm) be fine.)

Hanna

> +
> +    return subp
> +
>   
>   def ordered_qmp(qmsg, conv_keys=True):
>       # Dictionaries are not ordered prior to 3.6, therefore:
> @@ -232,7 +273,7 @@ def ordered_qmp(qmsg, conv_keys=True):
>           return od
>       return qmsg
>   
> -def qemu_img_create(*args):
> +def qemu_img_create(*args: str) -> subprocess.CompletedProcess[str]:
>       return qemu_img('create', *args)
>   
>   def qemu_img_measure(*args):
> @@ -467,8 +508,9 @@ def qemu_nbd_popen(*args):
>   
>   def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
>       '''Return True if two image files are identical'''
> -    return qemu_img('compare', '-f', fmt1,
> -                    '-F', fmt2, img1, img2) == 0
> +    res = qemu_img('compare', '-f', fmt1,
> +                   '-F', fmt2, img1, img2, check=False)
> +    return res.returncode == 0
>   
>   def create_image(name, size):
>       '''Create a fully-allocated raw image with sector markers'''