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))
>