Re: [PATCH v3 2/5] buildtools: script to generate cmdline boilerplate

["Robin Jarry" <rjarry@redhat.com>]

Bruce Richardson, Oct 11, 2023 at 15:33:
> Provide a "dpdk-cmdline-gen.py" script for application developers to
> quickly generate the boilerplate code necessary for using the cmdline
> library.
>
> Example of use:
> The script takes an input file with a list of commands the user wants in
> the app, where the parameter variables are tagged with the type.
> For example:
>
> 	$ cat commands.list
> 	list
> 	add <UINT16>x <UINT16>y
> 	echo <STRING>message
> 	add socket <STRING>path
> 	quit
>
> When run through the script as "./dpdk-cmdline-gen.py commands.list",
> the output will be the contents of a header file with all the
> boilerplate necessary for a commandline instance with those commands.
>
> If the flag --stubs is passed, an output header filename must also be
> passed, in which case both a header file with the definitions and a C
> file with function stubs in it is written to disk. The separation is so
> that the header file can be rewritten at any future point to add more
> commands, while the C file can be kept as-is and extended by the user
> with any additional functions needed.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Hi Bruce,

this is a nice addition, I have a few python style remarks below.

In general, I would advise formatting your code with black[1] to avoid 
debates over coding style. It makes all code homogeneous and lets you 
focus on more important things :)

>  buildtools/dpdk-cmdline-gen.py    | 167 ++++++++++++++++++++++++++++++
>  buildtools/meson.build            |   7 ++
>  doc/guides/prog_guide/cmdline.rst | 131 ++++++++++++++++++++++-
>  3 files changed, 304 insertions(+), 1 deletion(-)
>  create mode 100755 buildtools/dpdk-cmdline-gen.py
>
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
> new file mode 100755
> index 0000000000..3b41fb0493
> --- /dev/null
> +++ b/buildtools/dpdk-cmdline-gen.py
> @@ -0,0 +1,167 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2023 Intel Corporation
> +#
> +"""Script to automatically generate boilerplate for using DPDK cmdline library."""

Multi line (or single line) doc strings are usually formatted as 
follows:

"""
Script to automatically generate boilerplate for using DPDK cmdline library.
"""

It makes adding new lines more readable and saves a bit of characters 
per line.

> +
> +import argparse
> +import sys
> +
> +PARSE_FN_PARAMS = 'void *parsed_result, struct cmdline *cl, void *data'
> +PARSE_FN_BODY = """
> +    /* TODO: command action */
> +    RTE_SET_USED(parsed_result);
> +    RTE_SET_USED(cl);
> +    RTE_SET_USED(data);
> +"""
> +
> +
> +def process_command(tokens, cfile, comment):
> +    """Generate the structures and definitions for a single command."""
> +    name = []
> +
> +    if tokens[0].startswith('<'):
> +        print('Error: each command must start with at least one literal string', file=sys.stderr)
> +        sys.exit(1)

It would be better to raise an exception here and handle it in main() 
for error reporting.

> +    for t in tokens:
> +        if t.startswith('<'):
> +            break
> +        name.append(t)
> +    name = '_'.join(name)
> +
> +    result_struct = []
> +    initializers = []
> +    token_list = []
> +    for t in tokens:
> +        if t.startswith('<'):
> +            t_type, t_name = t[1:].split('>')
> +            t_val = 'NULL'
> +        else:
> +            t_type = 'STRING'
> +            t_name = t
> +            t_val = f'"{t}"'
> +
> +        if t_type == 'STRING':
> +            result_struct.append(f'\tcmdline_fixed_string_t {t_name};')
> +            initializers.append(
> +                    f'static cmdline_parse_token_string_t cmd_{name}_{t_name}_tok =\n' +
> +                    f'\tTOKEN_STRING_INITIALIZER(struct cmd_{name}_result, {t_name}, {t_val});')
> +        elif t_type in ['UINT8', 'UINT16', 'UINT32', 'UINT64', 'INT8', 'INT16', 'INT32', 'INT64']:
> +            result_struct.append(f'\t{t_type.lower()}_t {t_name};')
> +            initializers.append(
> +                    f'static cmdline_parse_token_num_t cmd_{name}_{t_name}_tok =\n' +
> +                    f'\tTOKEN_NUM_INITIALIZER(struct cmd_{name}_result, {t_name}, RTE_{t_type});')
> +        elif t_type in ['IP', 'IP_ADDR', 'IPADDR']:
> +            result_struct.append(f'\tcmdline_ipaddr_t {t_name};')
> +            initializers.append(
> +                    f'cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n' +
> +                    f'\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result, {t_name});')
> +        else:
> +            print(f'Error: unknown token-type {t}', file=sys.stderr)
> +            sys.exit(1)
> +        token_list.append(f'cmd_{name}_{t_name}_tok')
> +
> +    print(f'/* Auto-generated handling for command "{" ".join(tokens)}" */')
> +    # output function prototype
> +    func_sig = f'void\ncmd_{name}_parsed({PARSE_FN_PARAMS})'
> +    print(f'extern {func_sig};\n')
> +    # output function template if C file being written
> +    if (cfile):
> +        print(f'{func_sig}\n{{{PARSE_FN_BODY}}}\n', file=cfile)
> +    # output result data structure
> +    print(
> +            f'struct cmd_{name}_result {{\n' +
> +            '\n'.join(result_struct) +
> +            '\n};\n')
> +    # output the initializer tokens
> +    print('\n'.join(initializers) + '\n')
> +    # output the instance structure
> +    print(
> +            f'static cmdline_parse_inst_t cmd_{name} = {{\n' +
> +            f'\t.f = cmd_{name}_parsed,\n' +
> +            '\t.data = NULL,\n' +
> +            f'\t.help_str = "{comment}",\n' +
> +            '\t.tokens = {')
> +    for t in token_list:
> +        print(f'\t\t(void *)&{t},')
> +    print('\t\tNULL\n' + '\t}\n' + '};\n')
> +
> +    # return the instance structure name
> +    return f'cmd_{name}'
> +
> +
> +def process_commands(infile, hfile, cfile, ctxname):
> +    """Generate boilerplate output for a list of commands from infile."""
> +    instances = []
> +
> +    # redirect stdout to output the header, to save passing file= each print
> +    old_sys_stdout = sys.stdout
> +    sys.stdout = hfile

Why not use hfile.write()?

I think the main issue here is to use print() in process_commands(). It 
would probably be cleaner to have process_command() return a list of 
lines and print them in this function.

> +
> +    print(f'/* File autogenerated by {sys.argv[0]} */')
> +    print('#ifndef GENERATED_COMMANDS_H')
> +    print('#define GENERATED_COMMANDS_H')
> +    print('#include <rte_common.h>')
> +    print('#include <cmdline.h>')
> +    print('#include <cmdline_parse_string.h>')
> +    print('#include <cmdline_parse_num.h>')
> +    print('#include <cmdline_parse_ipaddr.h>')
> +    print('')

You can use a multi-line f-string here with a single print/write.

    hfile.write(f"""/* File autogenerated by {sys.argv[0]} */
#ifndef GENERATED_COMMANDS_H
#define GENERATED_COMMANDS_H

#include <rte_common.h>
#include <cmdline.h>
#include <cmdline_parse_string.h>
#include <cmdline_parse_num.h>
#include <cmdline_parse_ipaddr.h>

""")

> +
> +    for line in infile.readlines():
> +        if line.lstrip().startswith('#'):
> +            continue
> +        if '#' not in line:
> +            line = line + '#'  # ensure split always works, even if no help text
> +        tokens, comment = line.split('#', 1)
> +        instances.append(process_command(tokens.strip().split(), cfile, comment.strip()))

If process_command returns a name and a list of lines, that could be 
transformed as:

           name, lines = process_command(tokens.strip().split(), cfile, comment.strip())
           instances.append(name)
           hfile.write("\n".join(lines) + "\n")

> +
> +    print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
> +    for inst in instances:
> +        print(f'\t&{inst},')
> +    print('\tNULL')
> +    print('};\n')
> +    print('#endif /* GENERATED_COMMANDS_H */')

also multi line print here:

hfile.write("""\tNULL
};
#endif /* GENERATED_COMMANDS_H */
""")

> +
> +    sys.stdout = old_sys_stdout
> +
> +
> +def main():
> +    """Application main entry point."""
> +    ap = argparse.ArgumentParser()

Nit to have a nice description of the command with --help:

       ap = argparse.ArgumentParser(description=__doc__)

> +    ap.add_argument(
> +            '--stubs', action='store_true',
> +            help='Produce C file with empty function stubs for each command')
> +    ap.add_argument(
> +            '--output-file', '-o', default='-',
> +            help='Output header filename [default to stdout]')
> +    ap.add_argument(
> +            '--context-name', default='ctx',
> +            help='Name given to the cmdline context variable in the output header [default=ctx]')
> +    ap.add_argument(
> +            'infile', type=argparse.FileType('r'),
> +            help='File with list of commands')
> +    args = ap.parse_args()
> +
> +    if not args.stubs:
> +        if args.output_file == '-':
> +            process_commands(args.infile, sys.stdout, None, args.context_name)
> +        else:
> +            with open(args.output_file, 'w') as hfile:
> +                process_commands(args.infile, hfile, None, args.context_name)
> +    else:
> +        if not args.output_file.endswith('.h'):
> +            print(
> +                    'Error: output filename must end with ".h" extension when creating stubs',
> +                    file=sys.stderr)
> +            sys.exit(1)

You can replace print to stderr + exit with:

ap.error("-o/--output-file: must end with .h extension when creating stubs")

> +
> +        cfilename = args.output_file[:-2] + '.c'
> +        with open(args.output_file, 'w') as hfile:
> +            with open(cfilename, 'w') as cfile:
> +                print(f'#include "{args.output_file}"\n', file=cfile)
> +                process_commands(args.infile, hfile, cfile, args.context_name)
> +
> +
> +if __name__ == '__main__':
> +    main()

I'll stop here ;) Thanks!