| Message ID | 20220309035407.1848654-2-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: ensure all qemu-img calls are either checked or logged | expand |
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)) >
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)) > > > >
On 17.03.22 15:42, John Snow wrote: > > > 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. Be my guest. Perhaps one should identify the most arcane sigil for every language. (My C vote goes to the ternary operator.) > > 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? Is there even any case where qemu returns anything but a JSON object? > 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.) I hope similarly to how א₀ and its companions exist[1], there are also multiple instances of `...`, so one can succeed at handling cases where a `...` is a valid return type. I suggest just more dots. [1] hope I got that “HEBREW LETTER ALEF” “LEFT-TO-RIGHT MARK” sequence right...
On Thu, Mar 17, 2022 at 10:51 AM Hanna Reitz <hreitz@redhat.com> wrote: > I hope similarly to how א₀ and its companions exist[1], there are also > multiple instances of `...`, so one can succeed at handling cases where > a `...` is a valid return type. I suggest just more dots. lol. I'm invested in higher-kinded ellipse theory. --js
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'. + + 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) + except json.JSONDecodeError: + # Nope. This thing is toast. Raise the process error. + pass + + if json_data is ...: + raise + + if json_data is ...: + json_data = json.loads(res.stdout) + return json_data + def qemu_img_measure(*args): return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
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(+)