Re: Re: [PATCH 03/10] scripts: Add 'qemu-qmp-replies-tool' script for testing and modifying data for qemucapabilitiestest
On Fri, Jan 26, 2024 at 09:30:29AM +0100, Peter Krempa wrote:
On Thu, Jan 25, 2024 at 09:06:54 -0800, Andrea Bolognani wrote:
> I've never seen the 'raise Foo() from None' pattern though. Can you
> explain why it's needed here?
Python would re-raise the original exception along with the new one
without that.
I guess it doesn't hurt to have it there, but it doesn't seem very
useful either:
$ git diff
diff --git a/scripts/qemu-replies-tool.py b/scripts/qemu-replies-tool.py
index 9ab1c30ee2..bde6b2dad5 100755
--- a/scripts/qemu-replies-tool.py
+++ b/scripts/qemu-replies-tool.py
@@ -46,7 +46,7 @@ def qemu_replies_load(filename):
jsonstr = ''
except json.decoder.JSONDecodeError as je:
- raise qrtException("JSON error:\n'%s'\nwhile processing
snippet:\n'%s'" % (je, jsonstr)) from None
+ raise qrtException("JSON error:\n'%s'\nwhile processing
snippet:\n'%s'" % (je, jsonstr))
if command is not None or jsonstr != '':
if command is not None:
diff --git a/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
b/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
index cc2190dfd3..cf1425e0bb 100644
--- a/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
@@ -1,5 +1,5 @@
{
- "execute": "qmp_capabilities",
+ "execute": } "qmp_capabilities",
"id": "libvirt-1"
}
$ ./scripts/qemu-replies-tool.py
tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
'tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies' ... FAIL
JSON error:
'Expecting value: line 2 column 14 (char 15)'
while processing snippet:
'{
"execute": } "qmp_capabilities",
"id": "libvirt-1"
}
'
The behavior without it is just the same AFAICT.
> The way these examples are all jumbled together makes it IMO a
lot
> harder to understand what each is supposed to do, or even where one
> ends and the next one starts. I suggest you do something like this
> instead:
>
> def modify_replies(conv):
> return # no changes
>
> def _modify_replies_example1(conv):
> # ...
>
> def _modify_replies_example2(conv):
> # ...
The above is really just one example though:
- it shows version lookup
- lookup of a command in the replies list
- insertion of a new command into the list:
- if the version doesn't match
- add an error
- if the version does match
- add the real data
The above is a common pattern if you want to add probe for a new device.
It just shows two distinct (but with a reason) versions how to get the
JSON data. The first one is constructing it manually in python, which is
reasonable for the command or error. The second one is loading it from a
JSON string you'd copy&paste from qemu.
Okay, I clearly completely misunderstood what the code was trying to
do, arguably proving that it's not as obvious to someone else as it
certainly is to you :)
It's fine to keep the function as is, but please add some comments
explaining what it does and why. Some variation of the above will do
nicely.
> Honestly I would prefer it if the way to make the script
process
> multiple files was just to list them all on the command line, but I
I think this is simple thing to do with the argument parser in python
Oh, absolutely! It's just a shame that we can't do things that way
because of meson.
--
Andrea Bolognani / Red Hat / Virtualization