On Wed, Oct 25, 2023 at 03:04:05PM +0200, Robin Jarry wrote:
> Bruce Richardson, Oct 17, 2023 at 14:13:
> > 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.richard...@intel.com>
> > ---
> > buildtools/dpdk-cmdline-gen.py | 190 ++++++++++++++++++++++++++++++
> > buildtools/meson.build | 7 ++
> > doc/guides/prog_guide/cmdline.rst | 131 +++++++++++++++++++-
> > 3 files changed, 327 insertions(+), 1 deletion(-)
> > create mode 100755 buildtools/dpdk-cmdline-gen.py
>
> Hi Bruce,
>
> thanks for the respin! I have some small remarks inline.
>
> > diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
> > new file mode 100755
> > index 0000000000..6cb7610de4
> > --- /dev/null
> > +++ b/buildtools/dpdk-cmdline-gen.py
> > @@ -0,0 +1,190 @@
> > +#!/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.
> > +"""
> > +
> > +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);
> > +"""
> > +NUMERIC_TYPES = [
> > + "UINT8",
> > + "UINT16",
> > + "UINT32",
> > + "UINT64",
> > + "INT8",
> > + "INT16",
> > + "INT32",
> > + "INT64",
> > +]
> > +
> > +
> > +def process_command(lineno, tokens, comment):
> > + """Generate the structures and definitions for a single command."""
> > + out = []
> > + cfile_out = []
> > +
> > + if tokens[0].startswith("<"):
> > + raise ValueError(f"Error line {lineno + 1}: command must start
> > with a literal string")
> > +
> > + name_tokens = []
> > + for t in tokens:
> > + if t.startswith("<"):
> > + break
> > + name_tokens.append(t)
> > + name = "_".join(name_tokens)
> > +
> > + 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});"
>
> Since you are now using multiline strings in process_commands(), why not use
> them everywhere?
>
> It would make the code more readable in my opinion and would avoid inline
> f-string concatenation.
>
I'm a bit unsure about this case. I notice I can at least remove the "+"
symbol and have implicit string concat, but I really don't like the way the
indentation gets adjusted when we use multi-line strings, since the indent
has to match the C-code indent rather than the python indentation levels.
Therefore, I'm going to leave these pairs of lines as they are.
> > + )
> > + elif t_type in NUMERIC_TYPES:
> > + 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:
> > + raise TypeError(f"Error line {lineno + 1}: unknown token type
> > '{t_type}'")
> > + token_list.append(f"cmd_{name}_{t_name}_tok")
> > +
> > + out.append(f'/* Auto-generated handling for command "{"
> > ".join(tokens)}" */')
> > + # output function prototype
> > + func_sig = f"void\ncmd_{name}_parsed({PARSE_FN_PARAMS})"
> > + out.append(f"extern {func_sig};\n")
> > + # output result data structure
> > + out.append(f"struct cmd_{name}_result {{\n" + "\n".join(result_struct)
> > + "\n};\n")
> > + # output the initializer tokens
> > + out.append("\n".join(initializers) + "\n")
> > + # output the instance structure
> > + out.append(
> > + 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 = {"
>
> Especially here :)
>
I'll see about converting this and what it looks like. Maybe see if
textwrap.dedent will allow sensible indentation with it.
> > + )
> > + for t in token_list:
> > + out.append(f"\t\t(void *)&{t},")
> > + out.append("\t\tNULL\n" + "\t}\n" + "};\n")
> > + # output function template if C file being written
> > + cfile_out.append(f"{func_sig}\n{{{PARSE_FN_BODY}}}\n")
> > +
> > + # return the instance structure name
> > + return (f"cmd_{name}", out, cfile_out)
> > +
> > +
> > +def process_commands(infile, hfile, cfile, ctxname):
> > + """Generate boilerplate output for a list of commands from infile."""
> > + instances = []
> > +
> > + 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 lineno, line in enumerate(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)
> > + cmd_inst, h_out, c_out = process_command(lineno,
> > tokens.strip().split(), comment.strip())
> > + hfile.write("\n".join(h_out))
> > + if cfile:
> > + cfile.write("\n".join(c_out))
> > + instances.append(cmd_inst)
> > +
> > + inst_join_str = ",\n\t&"
> > + hfile.write(
> > + f"""
> > +static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{
> > +\t&{inst_join_str.join(instances)},
> > +\tNULL
> > +}};
> > +
> > +#endif /* GENERATED_COMMANDS_H */
> > +"""
>
> By the way, you can put literal tabs in the multiline strings. That way the
> indentation will look the same than in the generated C code.
>
> f"""
> static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{
> &{inst_join_str.join(instances)},
> NULL
> }};
>
> #endif /* GENERATED_COMMANDS_H */
> """
>
Trouble with that is that for some editors when using python, the tabs are
automatically expanded with spaces. For me using eclipse right now, I
physically can't insert leading tabs into the file! I need to open in vim
or some other editor to do it (or maybe use copy-paste from a C file), and
while that's not a big deal for me, it will be a problem for anyone else
editing this in future who has similar settings.
/Bruce
