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

[Heinrich Schuchardt]

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 would prefer the solution to stay simple and small meaning just one
parser per U-Boot binary.

>
> Signed-off-by: Francis Laniel <francis.laniel at 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