Re: [PATCH v1 9/9] scripts: printing machine type compat properties

[John Snow]

On Wed, Mar 30, 2022 at 11:55 AM Vladimir Sementsov-Ogievskiy
<v.sementsov-og@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.davydov@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.davydov@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