Re: [PATCH 28/29] iotests: Factor out qemu_tool_pipe_and_status()

[Max Reitz]

On 07.09.20 20:20, Kevin Wolf wrote:
> We have three almost identical functions that call an external process
> and return its output and return code. Refactor them into small wrappers
> around a common function.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 50 +++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 64ccaf9061..6fed77487e 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -90,21 +90,30 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
>  luks_default_key_secret_opt = 'key-secret=keysec0'
>  
>  
> -def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
> +def qemu_tool_pipe_and_status(tool: str, *args: str,

If this is just an internal function not to be used outside of
iotests.py, I wonder if “args: Sequence[str]” wouldn’t be cleaner.

(Just because I feel like the *args notation is useful to provide a
nicer-looking external interface, but is somehow unclean, so I
personally avoid it unless it actually does make things look nicer.
Just a very personal feeling, though, of course.)

> +                              connect_stderr: bool = True) -> Tuple[str, 
> int]:
>      """
> -    Run qemu-img and return both its output and its exit code
> +    Run a tool and return both its output and its exit code
>      """
> -    subp = subprocess.Popen(qemu_img_args + list(args),
> +    stderr = subprocess.STDOUT if connect_stderr else None

I really despise this notation.  It’s like ternary operators in C
weren’t bad enough.

(Again with the personal feelings.  Sorry.)

Reviewed-by: Max Reitz <mreitz@redhat.com>

signature.asc
Description: OpenPGP digital signature