On Thu, Jan 25, 2024 at 09:06:54 -0800, Andrea Bolognani wrote:
> On Tue, Jan 16, 2024 at 05:12:37PM +0100, Peter Krempa wrote:
> > scripts: Add 'qemu-qmp-replies-tool' script for testing and modifying data for qemucapabilitiestest
>
> s/qmp-//
>
> > + try:
> > + for line in fh:
> > + jsonstr += line
> > +
> > + if line == '\n':
> > + if command is None:
> > + command = json.loads(jsonstr)
> > + else:
> > + conv.append((command, json.loads(jsonstr)))
> > + command = None
> > +
> > + jsonstr = ''
> > +
> > + if command is not None and jsonstr != '':
> > + conv.append((command, json.loads(jsonstr)))
> > + command = None
> > + jsonstr = ''
> > +
> > + except json.decoder.JSONDecodeError as je:
> > + raise qrtException("JSON error:\n'%s'\nwhile processing snippet:\n'%s'" % (je, jsonstr)) from None
>
> I'm not sure this is the most "pythonic" way to use exceptions, but
> I'm also not a purist so I don't mind :)
Mostly this is not a library, just a
>
> 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.
> > +# Process the replies file programmatically here.
> > +# The 'conv' argument contains the whole conversation as a list of
> > +# (command, reply) tuples, where both command and reply are already parsed JSON
> > +# and thus represented by native python types (dict, list, etc ...)
> > +#
> > +# The code below contains a few examples and hints how to use the programatic
> > +# processing. Do not forget to use '--regenerate' flag to uptate the output files.
> > +#
> > +# Beware that this updates the output file which is used as input for any
> > +# subsequent re-run of the tool which can re-apply the modification.
> > +def modify_replies(conv):
> > + return # remove this to enable modifications
> > + version = None # filled with a dictionary with 'major', 'minor', 'micro' keys
> > +
> > + for (cmd, rep) in conv:
> > + if cmd['execute'] == 'query-version':
> > + version = rep['return']['qemu']
> > + break
> > +
> > + if version is None:
> > + raise Exception("'query-version' not found in the .replies file")
> > +
> > + idx = -1
> > + # Find an index of an entry
> > + for i in range(len(conv)):
> > + (cmd, rep) = conv[i]
> > +
> > + if cmd['execute'] == 'device-list-properties':
> > + idx = i
> > +
> > + if idx == -1:
> > + raise Exception("entry not found")
> > +
> > + cmd = {'execute': 'device-list-properties',
> > + 'arguments': {'typename': 'example-device'}}
> > +
> > + reply_unsupp = {'error': {'class': 'DeviceNotFound',
> > + 'desc': "Device 'example-device' not found"}}
> > +
> > + reply = json.loads('''
> > + {
> > + "return": [
> > + {
> > + "name": "dummy_prop",
> > + "type": "str"
> > + },
> > + {
> > + "name": "test",
> > + "type": "str"
> > + }
> > + ]
> > + }
> > + ''')
> > +
> > + if version['major'] >= 8 and version['minor'] > 0:
> > + conv.insert(idx, (cmd, reply))
> > + else:
> > + conv.insert(idx, (cmd, reply_unsupp))
>
> 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.
>
> > +The default mode is validation which checks the following:
> > + - each command has a reply and both are valid JSON
> > + - numbering of the 'id' field is as expected
> > + - the input file has the expected JSON formatting
> > +
> > +The tool can be also used to programmaticaly modify the '.replies' file by
> > +editting the 'modify_replies' method directly in the source, or for
>
> *editing
>
> > +args = parser.parse_args()
> > +
> > +if not args.replyfile and not args.repliesdir:
> > + parser.print_help()
> > + exit(1)
>
> This fails if neither repliesfile or repliesdir have been specified,
> but will happily proceed if both are present, in which case only the
> former will be considered. Please make sure that scenario is also
> handled correctly.
>
> > +if args.replyfile:
> > + if not process_one(args.replyfile, args):
> > + fail = True
> > +else:
> > + files = Path(args.repliesdir).glob('*.replies')
> > +
> > + for file in files:
> > + if not process_one(str(file), args):
> > + fail = True
>
> This can be simplified as
>
> if args.replyfile:
> files = [args.replyfile]
> else:
> files = Path(args.repliesdir).glob('*.replies')
>
> for file in files:
> if not process_one(str(file), args):
> fail = True
>
> 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
> understand that you need --repliesdir because meson doesn't have a
> native way to perform globbing.
but yeah this is needed.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
