From: Bruce Richardson <bruce.richardson@intel.com>
To: Robin Jarry <rjarry@redhat.com>
Cc: <dev@dpdk.org>
Subject: Re: [PATCH v3 2/5] buildtools: script to generate cmdline boilerplate
Date: Fri, 13 Oct 2023 13:43:34 +0100 [thread overview]
Message-ID: <ZSk7dpKJwA0UQpRn@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <CW7B45EOBGX2.299H0IN3THDGI@redhat.com>
On Fri, Oct 13, 2023 at 02:23:13PM +0200, Robin Jarry wrote:
> 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 :)
>
Thanks for the feedback. I'll look into black.
> > 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.
>
Sure.
> > +
> > +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.
>
I'll look into that. I probably should include the offending line/token in
the error report too.
> > + 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.
>
Good idea, let me look into that. Just using print was shorter than
hfile.write, and I felt it would save me some line-breaking.
> > +
> > + 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>
>
> """)
>
Yes, I was aware of that. However, I deliberately didn't use it, because
the indentation looks completely wrong with it (at least to my eyes). I
really don't like having each line start at column zero in the middle of a
block of code.
> > +
> > + 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")
>
Will investigate
> > +
> > + 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 */
> """)
>
As above, I didn't like the non-indented nature of it.
> > +
> > + 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")
>
Ack.
> > +
> > + 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!
>
next prev parent reply other threads:[~2023-10-13 12:43 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-02 17:00 [RFC PATCH 0/1] make cmdline library easier to use Bruce Richardson
2023-08-02 17:00 ` [RFC PATCH 1/1] cmdline/dpdk-cmdline-gen: generate boilerplate for simple cmds Bruce Richardson
2023-08-02 18:05 ` [RFC PATCH 0/1] make cmdline library easier to use Stephen Hemminger
2023-08-03 8:11 ` Bruce Richardson
2023-09-18 13:03 ` [RFC PATCH v2 0/5] use script to simplify use of cmdline lib Bruce Richardson
2023-09-18 13:03 ` [RFC PATCH v2 1/5] buildtools/dpdk-cmdline-gen: generate boilerplate for simple cmds Bruce Richardson
2023-09-18 13:03 ` [RFC PATCH v2 2/5] examples/simple_mp: auto-generate cmdline boilerplate Bruce Richardson
2023-09-18 13:03 ` [RFC PATCH v2 3/5] examples/hotplug_mp: " Bruce Richardson
2023-09-18 13:03 ` [RFC PATCH v2 4/5] buildtools/dpdk-cmdline-gen: add IP address support Bruce Richardson
2023-09-18 13:03 ` [RFC PATCH v2 5/5] examples/bond: auto-generate cmdline boilerplate Bruce Richardson
2023-10-11 13:33 ` [PATCH v3 0/5] document and simplify use of cmdline Bruce Richardson
2023-10-11 13:33 ` [PATCH v3 1/5] doc/prog_guide: new chapter on cmdline library Bruce Richardson
2023-10-11 13:33 ` [PATCH v3 2/5] buildtools: script to generate cmdline boilerplate Bruce Richardson
2023-10-13 12:23 ` Robin Jarry
2023-10-13 12:43 ` Bruce Richardson [this message]
2023-10-11 13:33 ` [PATCH v3 3/5] examples/simple_mp: auto-generate " Bruce Richardson
2023-10-11 13:33 ` [PATCH v3 4/5] examples/hotplug_mp: " Bruce Richardson
2023-10-11 13:33 ` [PATCH v3 5/5] examples/bond: " Bruce Richardson
2023-10-12 13:21 ` [PATCH v3 0/5] document and simplify use of cmdline David Marchand
2023-10-12 13:47 ` Bruce Richardson
2023-10-12 13:51 ` Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 0/7] " Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 1/7] doc/prog_guide: new chapter on cmdline library Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 2/7] buildtools: script to generate cmdline boilerplate Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 3/7] ci: allow use of DPDK tools when building examples Bruce Richardson
2023-10-17 12:24 ` Aaron Conole
2023-10-17 12:28 ` Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 4/7] examples/simple_mp: auto-generate cmdline boilerplate Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 5/7] examples/hotplug_mp: " Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 6/7] examples/bond: " Bruce Richardson
2023-10-16 14:06 ` [PATCH v4 7/7] examples/vdpa: " Bruce Richardson
2023-10-17 7:10 ` [PATCH v4 0/7] document and simplify use of cmdline David Marchand
2023-10-17 8:29 ` Bruce Richardson
2023-10-17 12:16 ` Bruce Richardson
2023-10-17 16:23 ` David Marchand
2023-10-17 17:02 ` Bruce Richardson
2023-10-17 17:08 ` Bruce Richardson
2023-10-18 11:21 ` David Marchand
2023-10-18 11:37 ` Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 0/9] " Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 1/9] doc/prog_guide: new chapter on cmdline library Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 2/9] buildtools: script to generate cmdline boilerplate Bruce Richardson
2023-10-25 13:04 ` Robin Jarry
2023-10-25 13:33 ` Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 3/9] ci: allow use of DPDK tools when building examples Bruce Richardson
2023-10-17 14:08 ` Aaron Conole
2023-10-17 12:13 ` [PATCH v5 4/9] examples/simple_mp: auto-generate cmdline boilerplate Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 5/9] examples/hotplug_mp: " Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 6/9] examples/bond: " Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 7/9] examples/vdpa: " Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 8/9] buildtools/dpdk-cmdline-gen: support option strings Bruce Richardson
2023-10-17 12:13 ` [PATCH v5 9/9] examples/ntb: auto-generate cmdline boilerplate Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 0/9] document and simplify use of cmdline Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 1/9] doc/prog_guide: new chapter on cmdline library Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 2/9] buildtools: script to generate cmdline boilerplate Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 3/9] ci: allow use of DPDK tools when building examples Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 4/9] examples/simple_mp: auto-generate cmdline boilerplate Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 5/9] examples/hotplug_mp: " Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 6/9] examples/bond: " Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 7/9] examples/vdpa: " Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 8/9] buildtools/dpdk-cmdline-gen: support option strings Bruce Richardson
2023-10-23 13:15 ` [PATCH v6 9/9] examples/ntb: auto-generate cmdline boilerplate Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 0/9] document and simplify use of cmdline Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 1/9] doc/prog_guide: new chapter on cmdline library Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 2/9] buildtools: script to generate cmdline boilerplate Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 3/9] ci: allow use of DPDK tools when building examples Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 4/9] examples/simple_mp: auto-generate cmdline boilerplate Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 5/9] examples/hotplug_mp: " Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 6/9] examples/bond: " Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 7/9] examples/vdpa: " Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 8/9] buildtools/dpdk-cmdline-gen: support option strings Bruce Richardson
2023-10-27 11:01 ` [PATCH v7 9/9] examples/ntb: auto-generate cmdline boilerplate Bruce Richardson
2023-11-10 14:16 ` [PATCH v7 0/9] document and simplify use of cmdline David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZSk7dpKJwA0UQpRn@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=rjarry@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).