[RFC PATCH v10 11/24] cmd: Add new cli command

[Heinrich Schuchardt]

On 10/4/23 18:42, Francis Laniel wrote:
> This command can be used to print the current parser with 'cli print'.

Please, provide a commit message that matches the code.

> It can also be used to set the current parser with 'cli set'.
> For the moment, only one value is valid for set: old.

If there is only one valid value, we should not provide the 'cli'
command to save code size. The patch seems to be in the wrong spot in
the series.

>
> 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 9bebf321c3..d468cc5065 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -226,6 +226,8 @@ obj-$(CONFIG_CMD_AVB) += avb.o
>   # Foundries.IO SCP03
>   obj-$(CONFIG_CMD_SCP03) += scp03.o
>
> +obj-$(CONFIG_HUSH_PARSER) += cli.o

Don't waste binary code size. We only need the command if at least two
parsers are available.

ifeq ($(CONFIG_HUSH_OLD_PARSER)$(HUSH_2021_PARSER),yy)
obj-y += cli.o
endif

The symbol CONFIG_HUSH_PARSER should be eliminated.

> +
>   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");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	printf("%s\n", current);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +

Please, describe this function (and all the others) in Sphinx style.

/**
  * parser_string_to_gd_flags() - converts parser name to bit mask
  *
  * @parser:	parser name
  * Return:	valid bit mask or -1
  */

> +static int parser_string_to_gd_flags(const char *parser)
> +{
> +	if (!strcmp(parser, "old"))
> +		return GD_FLG_HUSH_OLD_PARSER;

Please, do not return an invalid bit mask.

if (CONFIG_IS_ENABLED(HUSH_OLD_PARSER) && !strcmp(parser, "old"))
	return GD_FLG_HUSH_OLD_PARSER;
if (CONFIG_IS_ENABLED(HUSH_2021_PARSER) && !strcmp(parser, "2021"))
	return GD_FLG_HUSH_201_PARSER;

> +	return -1;
> +}
> +
> +static void reset_parser_gd_flags(void)
> +{
> +	gd->flags &= ~GD_FLG_HUSH_OLD_PARSER;

gd->flags &= ~(GD_FLG_HUSH_OLD_PARSER | GD_FLG_HUSH_2021_PARSER);

If there were only one parser, we wouldn't create this command.

> +}
> +
> +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);

This function should return -1 for for any value that is not supported
be it 'foo', 'old', or '2021'. Then you can eliminate a bunch of error
checking code below.

> +	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;
> +	}

Superfluous check, see above.

> +
> +	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;
> +	}

Superfluous check, see above.

> +
> +	reset_parser_gd_flags();
> +	gd->flags |= parser_flag;
> +
> +	cli_init();
> +	cli_loop();
> +
> +	/* cli_loop() should never return. */
> +	return CMD_RET_FAILURE;

Consuming stack space for every invocation of the 'cli set' command
cannot be intended.

Why don't we define cli_loop as __noreturn and ensure that it has no
return statement? Then gcc can generate a simple jump without consuming
stack.

cli_loop should() invoke panic() instead of returning.

> +}
> +
> +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"

"get - display parser\n"

> +	"set - set the current cli, possible value is: old"

'cli ' is only printed automatically in the first line.

"cli set - set parser to one of:\n"
#ifdef CONFIG_HUSH_OLD_PARSER
"\t- old\n"
#endif
#ifdef CONFIG_HUSH_2021_PARSER
"\t- 2021\n"
#endif

Why would we need the 'cli set' command at all if there is only one
parser enabled?
Should we better disable the sub-command in that case?

> +	;
> +#endif
> +
> +U_BOOT_CMD(cli, 3, 1, do_cli,
> +	   "cli",
> +#if CONFIG_IS_ENABLED(SYS_LONGHELP)
> +	   cli_help_text
> +#endif

This is wrong. You must pass an empty string at least. Please, follow
the style of the other commands and place the #if in the long text.

> +);
> 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

This Kconfig symbol is superfluous when you already have
CONFIG_HUSH_OLD_PARSER and CONFIG_HUSH_2021_PARSER.

Please, remove CONFIG_HUSH_PARSER from Kconfig.

> -	if (!(gd->flags & GD_FLG_HUSH_OLD_PARSER))
> +	if (!(gd->flags & GD_FLG_HUSH_OLD_PARSER)

The first part of the if statement is superfluous.
There is no harm in setting the bit again.

> +		&& 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

We should show the available values here:

cli set [old|2021]

> +
> +Description
> +-----------
> +
> +The cli command permits getting and changing the current parser at runtime.

The cli command permits displaying and setting the command line parsers.
Currently two parsers are implemented:

old
    The 'old' parser is what U-Boot provided until v2023.10.

2021
    The '2021' parser provides ....

Please, describe here why the '2021' parser might be preferable. Please,
describe which parser is the default.

> +
> +cli get
> +~~~~~~~
> +
> +It shows the current value of the parser used by the CLI.

The command display the parser used by the command line interface ('old'
or '2021').

> +
> +cli set
> +~~~~~~~
> +
> +It permits setting the value of the parser used by the CLI.

The command sets the parser used by the command line interface.

> +
> +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.

Should we use .. note:: here to highlight the information?

> +
> +Examples
> +--------
> +
> +Get the current parser::
> +
> +    => cli get
> +    old
> +
> +Change the current parser::
> +
> +    => cli set old

=> cli set 2021

Who would want to set the current value?

> +
> +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

This looks like a bug. I would have expected at least two possible
values. Otherwise we would not need a 'cli' command.

Please, add a 'Configuration' section describing the relevant
configuration variables.

Best regards

Heinrich

> +
> +Return value
> +------------
> +
> +The return value $? indicates whether the command succeeded.
> diff --git a/doc/usage/index.rst b/doc/usage/index.rst
> index fa702920fa..90a38c5713 100644
> --- a/doc/usage/index.rst
> +++ b/doc/usage/index.rst
> @@ -42,6 +42,7 @@ Shell commands
>      cmd/cat
>      cmd/cbsysinfo
>      cmd/cedit
> +   cmd/cli
>      cmd/cls
>      cmd/cmp
>      cmd/coninfo