From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 3262B43155;
Fri, 13 Oct 2023 14:23:19 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id D0A81402D3;
Fri, 13 Oct 2023 14:23:18 +0200 (CEST)
Received: from us-smtp-delivery-124.mimecast.com
(us-smtp-delivery-124.mimecast.com [170.10.133.124])
by mails.dpdk.org (Postfix) with ESMTP id 82516402CF
for <dev@dpdk.org>; Fri, 13 Oct 2023 14:23:17 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
s=mimecast20190719; t=1697199797;
h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
to:to:cc:mime-version:mime-version:content-type:content-type:
content-transfer-encoding:content-transfer-encoding:
in-reply-to:in-reply-to:references:references;
bh=VOcvy5uJTM5soUOBVuNsQAvngrSuPIXxYweVI0fJBbw=;
b=JOVnVQN2XeFh4+Eeq52kL5uRzioSQdsU4fcwdXAlj6UGjoGIKu2fLM6pnJ7BKmbYEo44cS
Ow4MscwnPnlAywxB9CnvkRxiipJCVpJX2uQwWt6uy37oeTjNct2Q9zWuPLBRWbfwFiSUl8
RxtdT8stpyt2mRVKoIu/O11MtOrrGJA=
Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com
[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS
(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
us-mta-376-59Q8jaLHPQ2S0HkKoBggwQ-1; Fri, 13 Oct 2023 08:23:15 -0400
X-MC-Unique: 59Q8jaLHPQ2S0HkKoBggwQ-1
Received: by mail-wr1-f70.google.com with SMTP id
ffacd0b85a97d-32d931872cbso909358f8f.0
for <dev@dpdk.org>; Fri, 13 Oct 2023 05:23:15 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1697199794; x=1697804594;
h=in-reply-to:references:to:from:subject:message-id:date
:content-transfer-encoding:mime-version:x-gm-message-state:from:to
:cc:subject:date:message-id:reply-to;
bh=qZ1FFYxWs4fRD3lExJ+UE/sMxQqPIvJ8RjejGxac7Ao=;
b=dFtapEJ6yoe3wxCFB6AvilADy88U3vIhvLj2Q9fZr1big1r+Xl1m0Uz0s9HZfHNL4p
iaaStw8ymsCNvUZ2WFLRJniIwkdIWTsZ+Hhv2QPwTpuz1Rud36nHFpUDcgCzQCnCgC4+
gi8CyJt8m5q/+pTFIEUmRc2SHNDGsVVHk8cWrjFzgu7J90ocStfB1WQ4OMq1pPHTuD4Q
S4H3Ng/rPq70J6G+wxANudfVuxtyip0n6JeSehpUlvFBtN9UyV6lFZCM+MyiTzPcOXw/
IB/IlrMUy5DQF6/TOlAfu0jgJ8PrMkAWrfMEDrIZm2zqdWwwnGqIAKIU8wVKWK2ozoZ3
qKEw==
X-Gm-Message-State: AOJu0YwldGgOGsksWnmozr7h8R9ehpVAtVz9YEJRTysXYygDhMHSUCNL
meXs2kpiB9eqp0XCD8pkmTXVgh19m/5O8gj3keU8CTi2XxNv+HZ5k8nT2QDH0xuj5lWbIipmaUT
j/NM=
X-Received: by 2002:adf:a387:0:b0:32d:8894:6aa2 with SMTP id
l7-20020adfa387000000b0032d88946aa2mr6220762wrb.2.1697199794497;
Fri, 13 Oct 2023 05:23:14 -0700 (PDT)
X-Google-Smtp-Source: AGHT+IFO+DFB7P//LpRhZdERWeEcoQdWGZFlWIZwchpkMubl5mKqqvFFqzXHKMsGBZVp9jVpBGpVwg==
X-Received: by 2002:adf:a387:0:b0:32d:8894:6aa2 with SMTP id
l7-20020adfa387000000b0032d88946aa2mr6220743wrb.2.1697199794049;
Fri, 13 Oct 2023 05:23:14 -0700 (PDT)
Received: from localhost (2a01cb000f8b9700598da2e1679e8383.ipv6.abo.wanadoo.fr.
[2a01:cb00:f8b:9700:598d:a2e1:679e:8383])
by smtp.gmail.com with ESMTPSA id
n6-20020adffe06000000b003140f47224csm20639014wrr.15.2023.10.13.05.23.13
(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);
Fri, 13 Oct 2023 05:23:13 -0700 (PDT)
Mime-Version: 1.0
Date: Fri, 13 Oct 2023 14:23:13 +0200
Message-Id: <CW7B45EOBGX2.299H0IN3THDGI@redhat.com>
Subject: Re: [PATCH v3 2/5] buildtools: script to generate cmdline boilerplate
From: "Robin Jarry" <rjarry@redhat.com>
To: "Bruce Richardson" <bruce.richardson@intel.com>, <dev@dpdk.org>
X-Mailer: aerc/0.16.0-22-gb445f7bb08fb-dirty
References: <20230802170052.955323-1-bruce.richardson@intel.com>
<20231011133357.111058-1-bruce.richardson@intel.com>
<20231011133357.111058-3-bruce.richardson@intel.com>
In-Reply-To: <20231011133357.111058-3-bruce.richardson@intel.com>
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8; format=Flowed
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
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:
>
> =09$ cat commands.list
> =09list
> =09add <UINT16>x <UINT16>y
> =09echo <STRING>message
> =09add socket <STRING>path
> =09quit
>
> 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=20
debates over coding style. It makes all code homogeneous and lets you=20
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 l=
ibrary."""
Multi line (or single line) doc strings are usually formatted as=20
follows:
"""
Script to automatically generate boilerplate for using DPDK cmdline library=
.
"""
It makes adding new lines more readable and saves a bit of characters=20
per line.
> +
> +import argparse
> +import sys
> +
> +PARSE_FN_PARAMS =3D 'void *parsed_result, struct cmdline *cl, void *data=
'
> +PARSE_FN_BODY =3D """
> + /* 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 =3D []
> +
> + if tokens[0].startswith('<'):
> + print('Error: each command must start with at least one literal =
string', file=3Dsys.stderr)
> + sys.exit(1)
It would be better to raise an exception here and handle it in main()=20
for error reporting.
> + for t in tokens:
> + if t.startswith('<'):
> + break
> + name.append(t)
> + name =3D '_'.join(name)
> +
> + result_struct =3D []
> + initializers =3D []
> + token_list =3D []
> + for t in tokens:
> + if t.startswith('<'):
> + t_type, t_name =3D t[1:].split('>')
> + t_val =3D 'NULL'
> + else:
> + t_type =3D 'STRING'
> + t_name =3D t
> + t_val =3D f'"{t}"'
> +
> + if t_type =3D=3D '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 =3D\n' +
> + f'\tTOKEN_STRING_INITIALIZER(struct cmd_{name}_resul=
t, {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_nam=
e}_tok =3D\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}_t=
ok =3D\n' +
> + f'\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result,=
{t_name});')
> + else:
> + print(f'Error: unknown token-type {t}', file=3Dsys.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 =3D 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=3Dcfile)
> + # 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} =3D {{\n' +
> + f'\t.f =3D cmd_{name}_parsed,\n' +
> + '\t.data =3D NULL,\n' +
> + f'\t.help_str =3D "{comment}",\n' +
> + '\t.tokens =3D {')
> + 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 =3D []
> +
> + # redirect stdout to output the header, to save passing file=3D each=
print
> + old_sys_stdout =3D sys.stdout
> + sys.stdout =3D hfile
Why not use hfile.write()?
I think the main issue here is to use print() in process_commands(). It=20
would probably be cleaner to have process_command() return a list of=20
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 =3D line + '#' # ensure split always works, even if no=
help text
> + tokens, comment =3D 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=20
transformed as:
name, lines =3D process_command(tokens.strip().split(), cfile, c=
omment.strip())
instances.append(name)
hfile.write("\n".join(lines) + "\n")
> +
> + print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] =3D {{')
> + 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 =3D old_sys_stdout
> +
> +
> +def main():
> + """Application main entry point."""
> + ap =3D argparse.ArgumentParser()
Nit to have a nice description of the command with --help:
ap =3D argparse.ArgumentParser(description=3D__doc__)
> + ap.add_argument(
> + '--stubs', action=3D'store_true',
> + help=3D'Produce C file with empty function stubs for each co=
mmand')
> + ap.add_argument(
> + '--output-file', '-o', default=3D'-',
> + help=3D'Output header filename [default to stdout]')
> + ap.add_argument(
> + '--context-name', default=3D'ctx',
> + help=3D'Name given to the cmdline context variable in the ou=
tput header [default=3Dctx]')
> + ap.add_argument(
> + 'infile', type=3Dargparse.FileType('r'),
> + help=3D'File with list of commands')
> + args =3D ap.parse_args()
> +
> + if not args.stubs:
> + if args.output_file =3D=3D '-':
> + 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=3Dsys.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 =3D 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=3Dcfile)
> + process_commands(args.infile, hfile, cfile, args.context=
_name)
> +
> +
> +if __name__ =3D=3D '__main__':
> + main()
I'll stop here ;) Thanks!