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.lan...@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
