On Wed, Mar 30, 2022 at 11:55 AM Vladimir Sementsov-Ogievskiy
<v.sementsov...@mail.ru> wrote:
>
> 29.03.2022 00:15, Maxim Davydov wrote:
> > This script makes the information that can be obtained from
> > query-init-properties and query-machines easy to read.
> >
> > Note: some init values from the devices can't be available like properties
> > from virtio-9p when configure has --disable-virtfs. Such values are
> > replaced by "DEFAULT". Another exception is properties of abstract
> > classes. The default value of the abstract class property is common
> > value of all child classes. But if the values of the child classes are
> > different the default value will be "BASED_ON_CHILD" (for example, old
> > x86_64-cpu can have unsupported feature).
> >
> > Example:
> >
> > 1) virsh qemu-monitor-command VM --pretty \
> > '{"execute" : "query-init-properties"}' > init_props.json
> >
> > 2) virsh qemu-monitor-command VM --pretty \
> > '{"execute" : "query-machines", "arguments" : {"is-full" : true}}' \
> > > compat_props.json
> >
> > 3) scripts/print_MT.py --MT_compat_props compat_props.json\
> > --init_props init_props.json --mt pc-q35-7.0 pc-q35-6.1
> >
Being able to parse existing JSON files is nice, but have you also
considered baking the QMP querying directly into this script?
> > Output:
> > ╒═══════════════════════════════════╤══════════════╤══════════════╕
> > │ property_name │ pc-q35-7.0 │ pc-q35-6.1 │
> > ╞═══════════════════════════════════╪══════════════╪══════════════╡
> > │ ICH9-LPC-x-keep-pci-slot-hpc │ True │ False │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ nvme-ns-shared │ True │ off │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ vhost-user-vsock-device-seqpacket │ auto │ off │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ virtio-mem-unplugged-inaccessible │ auto │ off │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ x86_64-cpu-hv-version-id-build │ 14393 │ 0x1bbc │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ x86_64-cpu-hv-version-id-major │ 10 │ 0x0006 │
> > ├───────────────────────────────────┼──────────────┼──────────────┤
> > │ x86_64-cpu-hv-version-id-minor │ 0 │ 0x0001 │
> > ╘═══════════════════════════════════╧══════════════╧══════════════╛
> >
> > Signed-off-by: Maxim Davydov <maxim.davy...@openvz.org>
> > ---
> > scripts/print_MT.py | 274 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 274 insertions(+)
> > create mode 100755 scripts/print_MT.py
> >
> > diff --git a/scripts/print_MT.py b/scripts/print_MT.py
> > new file mode 100755
> > index 0000000000..8be13be8d7
> > --- /dev/null
> > +++ b/scripts/print_MT.py
> > @@ -0,0 +1,274 @@
> > +#! /usr/bin/python3
>
> Standard shebang line for python3 is:
>
> #!/usr/bin/env python3
>
>
> > +#
> > +# Script for printing machine type compatible features. It uses two JSON
> > files
> > +# that should be generated by qmp-init-properties and query-machines.
> > +#
> > +# Copyright (c) 2022 Maxim Davydov <maxim.davy...@openvz.org>
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +import pandas as pd
> > +import json
> > +from tabulate import tabulate
> > +from argparse import ArgumentParser
> > +
> > +
> > +# used for aliases and other names that can be changed
> > +aliases = { "e1000-82540em": "e1000" }
> > +
> > +
> > +def get_major(mt):
> > + splited = mt.split('.')
> > + if (len(splited) >= 2):
> > + return int(mt.split('.')[1])
>
> why to call split() again? You may use splited[1]
>
> > + else:
> > + return 0
> > +
> > +
> > +def get_prefix(mt):
> > + splited = mt.split('.')
> > + if (len(splited) >= 1):
>
> In python you don't need braces around if condition:
>
> if len(splited) >= 1:
>
> is the same thing.
>
> > + return mt.split('.')[0]
> > + else:
> > + return mt
>
> seems, split() never return empty list, so len is always >=1.
>
> You can do simply
>
> return mt.split(',', 1)[0]
>
> > +
> > +
> > +def get_mt_sequence(mt_data):
> > + mt_list = [mt['name'] for mt in mt_data['return']]
> > + mt_list.remove('none')
> > +
> > + mt_list.sort(key=get_major, reverse=True)
> > + mt_list.sort(key=get_prefix, reverse=True)
>
> Aha. You may use one sort() call, if use tuple as key. Something like this
> should work:
>
> def parse_mt(mt):
> spl = mt.split('.')
> if len(spl) >= 2:
> return spl[0], spl[1]
> else:
> return mt, 0
>
> ...
>
> mt_list.sort(key=parse_mt, ...)
>
> > +
> > + return mt_list
> > +
> > +
> > +def get_req_props(defval_data, prop_set, abstr_class_to_features):
> > + req_prop_values = dict()
> > + req_abstr_prop_values = dict()
>
> {} construction is native way to create empty dict:
>
> req_prop_values = {}
>
> > +
> > + for device in defval_data['return']:
> > + # Skip cpu devices that will break all default values for cpus
> > + if device['name'] == 'base-x86_64-cpu':
> > + continue
> > + if device['name'] == 'max-x86_64-cpu':
> > + continue
> > +
> > + # some features in mt set as one absract class
> > + # but this features are owned by another class
>
> Hmm. That all hard for me to review, because I don't know the whole model of
> machine types.. Don't we have a documentation somewhere? Which objects,
> classes, abstart classes and properties we have and what that all mean.
>
> > + device_props_owners = dict()
> > + for props_class in device['props']:
> > + if not 'classprops' in props_class: # for example, Object class
>
> More pythonic is:
>
> if 'classprops' not in props_class:
>
> > + continue
> > +
> > + for prop in props_class['classprops']:
> > + if not 'value' in prop:
> > + continue
> > +
> > + prop_name = device['name'] + '-' + prop['name']
> > + device_props_owners[prop['name']] = prop['value']
> > + if prop_name in prop_set:
> > + req_prop_values[prop_name] = prop['value']
> > +
> > + for props_class in device['props']:
> > + if not props_class['classname'] in abstr_class_to_features:
> > + continue
> > +
> > + for req_prop in
> > abstr_class_to_features[props_class['classname']]:
> > + if not req_prop in device_props_owners:
> > + continue
> > +
> > + prop_value = device_props_owners[req_prop]
> > + prop_name = props_class['classname'] + '-' + req_prop
> > + if req_abstr_prop_values.setdefault(prop_name, prop_value)
> > \
> > + != prop_value:
> > + req_abstr_prop_values[prop_name] = 'BASED_ON_CHILD'
> > +
> > + return req_prop_values, req_abstr_prop_values
> > +
> > +
> > +def make_definition_table(mt_to_compat_props, prop_set,
> > + req_props, req_abstr_props, is_full):
> > + mt_table = dict()
> > + for prop in sorted(prop_set):
> > + if not is_full:
> > + values = set()
> > + for mt in mt_to_compat_props:
> > + if prop in mt_to_compat_props[mt]:
> > + values.add(mt_to_compat_props[mt][prop])
> > + else:
> > + if prop in req_props:
> > + values.add(req_props[prop])
> > + else:
> > + values.add('DEFAULT')
> > + # Skip the property if its value is the same for
> > + # all required machines
> > + if len(values) == 1:
> > + continue
> > +
> > + mt_table.setdefault('property_name', []).append(prop)
> > + for mt in mt_to_compat_props:
> > + if prop in mt_to_compat_props[mt]:
> > + value = mt_to_compat_props[mt][prop]
> > + mt_table.setdefault(mt, []).append(value)
> > + else:
> > + if prop in req_props:
> > + mt_table.setdefault(mt, []).append(req_props[prop])
> > + else:
> > + value = req_abstr_props.get(prop, 'DEFAULT')
> > + mt_table.setdefault(mt, []).append(value)
> > +
> > + return mt_table
> > +
> > +
> > +def get_standard_form(value):
> > + if type(value) is str:
> > + out = value.upper()
> > + if out.isnumeric():
> > + return int(out)
> > + if out == 'TRUE':
> > + return True
> > + if out == 'FALSE':
> > + return False
> > +
> > + return value
> > +
> > +
> > +def get_features(mt_data, defval_data, mts, is_full):
> > + prop_set = set()
> > + abstr_prop_set = set()
> > + mt_to_compat_props = dict()
> > + # It will be used for searching appropriate feature (sometimes class
> > name
> > + # in machine type definition and real class name are different)
> > + abstr_class_to_features = dict()
> > +
> > + for mt in mt_data['return']:
> > + if mt['name'] == 'none':
> > + continue
> > +
> > + if not mt['name'] in mts:
> > + continue
> > +
> > + mt_to_compat_props[mt['name']] = dict()
> > + for prop in mt['compat-props']:
> > + driver_name = aliases.get(prop['driver'], prop['driver'])
> > + prop_name = prop['driver'] + '-' + prop['property']
> > + real_name = driver_name + '-' + prop['property']
> > + # value is always string
> > + prop_val = get_standard_form(prop['value'])
> > + if prop['abstract']:
> > + mt_to_compat_props[mt['name']][real_name] = prop_val
> > + abstr_prop_set.add(real_name)
> > + abstr_class_to_features.setdefault(driver_name,
> > +
> > set()).add(prop['property'])
> > + else:
> > + mt_to_compat_props[mt['name']][real_name] = prop_val
> > + prop_set.add(real_name)
> > +
> > + req_props, req_abstr_props = get_req_props(defval_data, prop_set,
> > + abstr_class_to_features)
> > +
> > + # join sets for global sorting by name
> > + prop_set.update(abstr_prop_set)
> > + mt_table = make_definition_table(mt_to_compat_props, prop_set,
> > req_props,
> > + req_abstr_props, is_full)
> > + # to save mt sequence
> > + df = pd.DataFrame({'property_name': mt_table['property_name']})
> > + for mt in mts:
> > + if not mt in mt_table:
> > + print('Error: {0} no found'.format(mt))
> > + continue
> > + df[mt] = mt_table[mt]
> > +
> > + return df
> > +
> > +
> > +def main():
> > + parser = ArgumentParser(description='''Print definition of machine
> > + type (compatible features)''')
> > + parser.add_argument('--MT_compat_props', type=str, required=True,
> > + help='''Path to JSON file with current machine type
> > + definition. It must be generated via
> > + query-machines with is-full option.''')
> > + parser.add_argument('--init_props', type=str, required=True,
> > + help='''Path to JSON file with initial features. It
> > + must be generated via
> > + query-init-properties.''')
> > + parser.add_argument('--format', type=str,
> > + choices=['table', 'csv', 'html', 'json'],
> > + default='table', help='Format of the output file')
> > + parser.add_argument('--file', type=str,
> > + help='''Path to output file. It must be set with
> > csv
> > + and html formats.''')
> > + parser.add_argument('--all', action='store_true',
> > + help='''Print all available machine types (list of
> > + machine types will be ignored''')
> > + parser.add_argument('--mt', nargs="*", type=str,
> > + help='List of machine types that will be compared')
> > + parser.add_argument('--full', action='store_true',
> > + help='''Print all defined properties (by default,
> > + only properties with different values are
> > + printed)''')
> > +
> > + args = parser.parse_args()
> > +
> > + if args.all == 0 and args.mt == None:
>
> Don't compare boolean value with zero, use it as is (I'm about args.all, but
> comparing mt with None doesn't make real sense here too):
>
> if not(args.all or args.mt):
>
> > + print('Enter the list of required machine types (list of all '\
> > + 'machine types : qemu-system-x86_64 --machine help)')
> > + return
> > +
> > + with open(args.MT_compat_props) as mt_json_file:
> > + mt_data = json.load(mt_json_file)
> > +
> > + with open(args.init_props) as defval_json_file:
> > + defval_data = json.load(defval_json_file)
> > +
> > + if args.all:
> > + df = get_features(mt_data, defval_data, get_mt_sequence(mt_data),
> > + args.full)
> > + else:
> > + df = get_features(mt_data, defval_data, args.mt, args.full)
>
> I'd write it like this:
>
> mt = args.mt or get_mt_sequence(mt_data)
> df = get_feature(..., mt, args.full)
>
> > +
> > + if args.format == 'csv':
> > + if args.file == None:
> > + print('Error: csv format requires path to output file')
> > + return
> > + df.to_csv(args.file)
> > + elif args.format == 'html':
> > + if args.file == None:
> > + print('Error: html format requires path to output file')
> > + return
> > + with open(args.file, 'w') as output_html:
> > + output_html.write(df.to_html(justify='center',
> > col_space='400px',
> > + index=False))
> > + elif args.format == 'json':
> > + json_table = df.to_json()
> > + if args.file == None:
> > + print(json_table)
> > + else:
> > + with open(args.file, 'w') as output_json:
> > + output_json.write(json_table)
> > + elif args.format == 'table':
> > + table = tabulate(df, showindex=False, stralign='center',
> > + tablefmt='fancy_grid', headers='keys')
> > + if args.file == None:
> > + print(table)
> > + else:
> > + with open(args.file, 'w') as output_table:
> > + output_table.write(table)
>
> For me this looks like extra logic.
>
> Why to restrict printing csv/html directly to stdout? It's native to use
> output redirection to save output to some file. I think you can simply drop
> --file argument always print to stdout.
>
> Or, if you still want this option, it's better to prepare the whole output in
> string variable, and then handle it once:
>
> if ...
> elif ...
> elif ...
>
> ...
>
> if args.file:
> with open(...) as f:
> f.write(output)
> else:
> print(output)
>
> > +
> > +
> > +if __name__ == '__main__':
> > + main()
>
>
> --
> Best regards,
> Vladimir
>
I trust Vladimir's review on python scripting otherwise.
--js
