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

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

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
> 
> 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