Re: [RFC PATCH v8 11/23] cmd: Add new cli command

[Francis Laniel <francis.laniel@amarulasolutions.com>]

Hi.


Le samedi 13 mai 2023, 02:18:27 WEST Heinrich Schuchardt a écrit :
> On 5/12/23 22:03, Francis Laniel wrote:
> > This command can be used to print the current parser with 'cli print'.
> > It can also be used to set the current parser with 'cli set'.
> > For the moment, only one value is valid for set: old.
> 
> What would be the benefit having multiple alternative parsers in one
> U-Boot instance?

I do not think we should push this contribution as the default parser for the 
moment.
I mean, once this contribution would be merged, we should stick with the two 
parsers (the new and old) for a bit of time before deprecating the old one in 
favor of the new one.

So, permit switching of parser at runtime if the perfect help for developers 
to check their boards behave correctly with the new parser.

> I would prefer the solution to stay simple and small meaning just one
> parser per U-Boot binary.

You can perfectly have only one parser per U-Boot binary.
To do so, you just need to check only one parser, you will then not be able to 
change the parser at runtime:
=> cli get
2021
=> cli set old
Want to set current parser to old, but its code was not compiled!

So, when developing for your board, just enable the two parser to check which 
one suits your needs.
Then, when you release, just release only with the CONFIG_HUSH_*_PARSER which 
suits your needs.

> > Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
> > ---
> > 
> >   cmd/Makefile          |   2 +
> >   cmd/cli.c             | 120 ++++++++++++++++++++++++++++++++++++++++++
> >   common/cli.c          |   3 +-
> >   doc/usage/cmd/cli.rst |  59 +++++++++++++++++++++
> >   doc/usage/index.rst   |   1 +
> >   5 files changed, 184 insertions(+), 1 deletion(-)
> >   create mode 100644 cmd/cli.c
> >   create mode 100644 doc/usage/cmd/cli.rst
> > 
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 6c37521b4e..706e934836 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -224,6 +224,8 @@ obj-$(CONFIG_CMD_AVB) += avb.o
> > 
> >   # Foundries.IO SCP03
> >   obj-$(CONFIG_CMD_SCP03) += scp03.o
> > 
> > +obj-$(CONFIG_HUSH_PARSER) += cli.o
> > +
> > 
> >   obj-$(CONFIG_ARM) += arm/
> >   obj-$(CONFIG_RISCV) += riscv/
> >   obj-$(CONFIG_SANDBOX) += sandbox/
> > 
> > diff --git a/cmd/cli.c b/cmd/cli.c
> > new file mode 100644
> > index 0000000000..7671785b83
> > --- /dev/null
> > +++ b/cmd/cli.c
> > @@ -0,0 +1,120 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +#include <common.h>
> > +#include <cli.h>
> > +#include <command.h>
> > +#include <string.h>
> > +#include <asm/global_data.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +static const char *gd_flags_to_parser(void)
> > +{
> > +	if (gd->flags & GD_FLG_HUSH_OLD_PARSER)
> > +		return "old";
> > +	return NULL;
> > +}
> > +
> > +static int do_cli_get(struct cmd_tbl *cmdtp, int flag, int argc,
> > +			 char *const argv[])
> > +{
> > +	const char *current = gd_flags_to_parser();
> > +
> > +	if (!current) {
> > +		printf("current cli value is not valid, this should not happen!
\n");
> 
> Please, use log_err() for errors. We want to keep our code small. "this
> should not happen!" does not convey any additional information.
> 
> > +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	printf("%s\n", current);
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int parser_string_to_gd_flags(const char *parser)
> > +{
> > +	if (!strcmp(parser, "old"))
> > +		return GD_FLG_HUSH_OLD_PARSER;
> > +	return -1;
> > +}
> > +
> > +static void reset_parser_gd_flags(void)
> > +{
> > +	gd->flags &= ~GD_FLG_HUSH_OLD_PARSER;
> > +}
> > +
> > +static int do_cli_set(struct cmd_tbl *cmdtp, int flag, int argc,
> > +		      char *const argv[])
> > +{
> > +	char *parser_name;
> > +	int parser_flag;
> > +
> > +	if (argc < 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	parser_name = argv[1];
> > +
> > +	parser_flag = parser_string_to_gd_flags(parser_name);
> > +	if (parser_flag == -1) {
> > +		printf("Bad value for parser name: %s\n", parser_name);
> > +		return CMD_RET_USAGE;
> > +	}
> > +
> > +	if (parser_flag == GD_FLG_HUSH_OLD_PARSER &&
> > +		!CONFIG_IS_ENABLED(HUSH_OLD_PARSER)) {
> > +		printf("Want to set current parser to old, but its code was not
> > compiled!\n"); +		return CMD_RET_FAILURE;
> > +	}
> 
> We want to keep the code small. We don't need different strings "Parser
> %d is not available" would be enough information.
> 
> > +.
> > +	if (parser_flag == GD_FLG_HUSH_2021_PARSER &&
> > +		!CONFIG_IS_ENABLED(HUSH_2021_PARSER)) {
> > +		printf("Want to set current parser to 2021, but its code was 
not
> > compiled!\n"); +		return CMD_RET_FAILURE;
> > +	}
> > +
> > +	reset_parser_gd_flags();
> > +	gd->flags |= parser_flag;
> > +
> > +	cli_init();
> > +	cli_loop();
> > +
> > +	/* cli_loop() should never return. */
> > +	return CMD_RET_FAILURE;
> > +}
> > +
> > +static struct cmd_tbl parser_sub[] = {
> > +	U_BOOT_CMD_MKENT(get, 1, 1, do_cli_get, "", ""),
> > +	U_BOOT_CMD_MKENT(set, 2, 1, do_cli_set, "", ""),
> > +};
> > +
> > +static int do_cli(struct cmd_tbl *cmdtp, int flag, int argc,
> > +		  char *const argv[])
> > +{
> > +	struct cmd_tbl *cp;
> > +
> > +	if (argc < 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	/* drop initial "parser" arg */
> > +	argc--;
> > +	argv++;
> > +
> > +	cp = find_cmd_tbl(argv[0], parser_sub, ARRAY_SIZE(parser_sub));
> > +	if (cp)
> > +		return cp->cmd(cmdtp, flag, argc, argv);
> > +
> > +	return CMD_RET_USAGE;
> > +}
> > +
> > +#if CONFIG_IS_ENABLED(SYS_LONGHELP)
> > +static char cli_help_text[] =
> > +	"get - print current cli\n"
> > +	"set - set the current cli, possible value is: old"
> > +	;
> > +#endif
> > +
> > +U_BOOT_CMD(cli, 3, 1, do_cli,
> > +	   "cli",
> > +#if CONFIG_IS_ENABLED(SYS_LONGHELP)
> > +	   cli_help_text
> > +#endif
> > +);
> > diff --git a/common/cli.c b/common/cli.c
> > index e5fe1060d0..d419671e8c 100644
> > --- a/common/cli.c
> > +++ b/common/cli.c
> > @@ -268,7 +268,8 @@ void cli_loop(void)
> > 
> >   void cli_init(void)
> >   {
> >   #ifdef CONFIG_HUSH_PARSER
> > 
> > -	if (!(gd->flags & GD_FLG_HUSH_OLD_PARSER))
> > +	if (!(gd->flags & GD_FLG_HUSH_OLD_PARSER)
> 
> This constraint is superfluous.
> 
> Best regards
> 
> Heinrich
> 
> > +		&& CONFIG_IS_ENABLED(HUSH_OLD_PARSER))
> > 
> >   		gd->flags |= GD_FLG_HUSH_OLD_PARSER;
> >   	
> >   	u_boot_hush_start();
> >   
> >   #endif
> > 
> > diff --git a/doc/usage/cmd/cli.rst b/doc/usage/cmd/cli.rst
> > new file mode 100644
> > index 0000000000..89ece3203d
> > --- /dev/null
> > +++ b/doc/usage/cmd/cli.rst
> > @@ -0,0 +1,59 @@
> > +.. SPDX-License-Identifier: GPL-2.0+
> > +
> > +cli command
> > +===========
> > +
> > +Synopis
> > +-------
> > +
> > +::
> > +
> > +    cli get
> > +    cli set cli_flavor
> > +
> > +Description
> > +-----------
> > +
> > +The cli command permits getting and changing the current parser at
> > runtime. +
> > +cli get
> > +~~~~~~~
> > +
> > +It shows the current value of the parser used by the CLI.
> > +
> > +cli set
> > +~~~~~~~
> > +
> > +It permits setting the value of the parser used by the CLI.
> > +
> > +Possible values are old and 2021.
> > +Note that, to use a specific parser its code should have been compiled,
> > that +is to say you need to enable the corresponding CONFIG_HUSH*.
> > +Otherwise, an error message is printed.
> > +
> > +Examples
> > +--------
> > +
> > +Get the current parser::
> > +
> > +    => cli get
> > +    old
> > +
> > +Change the current parser::
> > +
> > +    => cli set old
> > +
> > +Trying to set the current parser to an unknown value::
> > +
> > +    => cli set foo
> > +    Bad value for parser name: foo
> > +    cli - cli
> > +
> > +    Usage:
> > +    cli get - print current cli
> > +    set - set the current cli, possible value is: old
> > +
> > +Return value
> > +------------
> > +
> > +The return value $? indicates whether the command succeeded.
> > diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> > index 0fde130a54..d0e26d1aee 100644
> > --- a/doc/usage/index.rst
> > +++ b/doc/usage/index.rst
> > @@ -37,6 +37,7 @@ Shell commands
> > 
> >      cmd/bootz
> >      cmd/cat
> >      cmd/cbsysinfo
> > 
> > +   cmd/cli
> > 
> >      cmd/cls
> >      cmd/cmp
> >      cmd/coninfo