qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [PATCH 01/14] iotests: add qemu_img_json()
Date: Thu, 17 Mar 2022 10:42:26 -0400



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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]