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

[Hanna Reitz <hreitz@redhat.com>]

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

Well, absolutely.

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

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

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

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

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

I guess this return type probably has to be quoted.

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

...and this one.

Hanna

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