Re: [PATCH v13 15/24] cli: add modern hush as parser for run_command*()

Hi!

Le vendredi 22 décembre 2023, 22:02:35 CET Francis Laniel a écrit :

Enables using, in code, modern hush as parser for run_command function family. It also enables the command run to be used by CLI user of modern hush.

Reviewed-by: Simon Glass sjg@chromium.org Signed-off-by: Francis Laniel francis.laniel@amarulasolutions.com

---

common/cli.c | 67 ++++++++++++++++++++++++++++---------- common/cli_hush_upstream.c | 2 +- test/boot/bootflow.c | 16 ++++++++- 3 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/common/cli.c b/common/cli.c index b3eb512b62..a34938294e 100644 --- a/common/cli.c +++ b/common/cli.c @@ -25,6 +25,14 @@ #include <linux/errno.h>

#ifdef CONFIG_CMDLINE

+static inline bool use_hush_old(void) +{

• return IS_ENABLED(CONFIG_HUSH_SELECTABLE) ?
• gd->flags & GD_FLG_HUSH_OLD_PARSER :
• IS_ENABLED(CONFIG_HUSH_OLD_PARSER);

+}

/*

• Run a command using the selected parser.

@@ -43,15 +51,30 @@ int run_command(const char *cmd, int flag) return 1;

return 0; -#elif CONFIG_IS_ENABLED(HUSH_OLD_PARSER)

• int hush_flags = FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP;
• if (flag & CMD_FLAG_ENV)

• hush_flags |= FLAG_CONT_ON_NEWLINE;
  

• return parse_string_outer(cmd, hush_flags);

-#else /* HUSH_MODERN_PARSER */

• /* Not yet implemented. */
• return 1;

+#else

• if (use_hush_old()) {

• int hush_flags = FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP;
  

• if (flag & CMD_FLAG_ENV)
  

• 	hush_flags |= FLAG_CONT_ON_NEWLINE;
  

• return parse_string_outer(cmd, hush_flags);
  

• }
• /*

• * Possible values for flags are the following:
  

• * FLAG_EXIT_FROM_LOOP: This flags ensures we exit from loop in
  

• * parse_and_run_stream() after first iteration while normal
  

• * behavior, * i.e. when called from cli_loop(), is to loop
  

• * infinitely.
  

• * FLAG_PARSE_SEMICOLON: modern Hush parses ';' and does not stop
  

• * first time it sees one. So, I think we do not need this flag.
  

• * FLAG_REPARSING: For the moment, I do not understand the goal
  

• * of this flag.
  

• * FLAG_CONT_ON_NEWLINE: This flag seems to be used to continue
  

• * parsing even when reading '\n' when coming from
  

• * run_command(). In this case, modern Hush reads until it finds
  

• * '\0'. So, I think we do not need this flag.
  

• */
  

• return parse_string_outer_modern(cmd, FLAG_EXIT_FROM_LOOP);

#endif }

@@ -67,12 +90,23 @@ int run_command_repeatable(const char *cmd, int flag) #ifndef CONFIG_HUSH_PARSER return cli_simple_run_command(cmd, flag); #else

• int ret;
• if (use_hush_old()) {

• ret = parse_string_outer(cmd,
  

• 			 FLAG_PARSE_SEMICOLON
  

• 			 | FLAG_EXIT_FROM_LOOP);
  

• } else {

• ret = parse_string_outer_modern(cmd,
  

• 			      FLAG_PARSE_SEMICOLON
  

• 			      | FLAG_EXIT_FROM_LOOP);
  

• }
• /*
  • parse_string_outer() returns 1 for failure, so clean up
  • its result.
  */

• if (parse_string_outer(cmd,

• 	       FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP))
  

• if (ret) return -1;

  return 0;

@@ -111,12 +145,11 @@ int run_command_list(const char *cmd, int len, int flag) buff[len] = '\0'; } #ifdef CONFIG_HUSH_PARSER -#if CONFIG_IS_ENABLED(HUSH_OLD_PARSER)

• rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);

-#else /* HUSH_MODERN_PARSER */

• /* Not yet implemented. */
• rcode = 1;

-#endif

• if (use_hush_old()) {

• rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
  

• } else {

• rcode = parse_string_outer_modern(buff, FLAG_PARSE_SEMICOLON);
  

• }

#else /* * This function will overwrite any \n it sees with a \0, which diff --git a/common/cli_hush_upstream.c b/common/cli_hush_upstream.c index 7f6b058e6c..11870c51e8 100644 --- a/common/cli_hush_upstream.c +++ b/common/cli_hush_upstream.c @@ -8022,7 +8022,7 @@ static int parse_and_run_string(const char *s) }

#ifdef __U_BOOT__ -int parse_string_outer(const char *cmd, int flags) +int parse_string_outer_modern(const char *cmd, int flags) { int ret; int old_flags; diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c index a9b555c779..104f49deef 100644 --- a/test/boot/bootflow.c +++ b/test/boot/bootflow.c @@ -710,7 +710,21 @@ static int bootflow_scan_menu_boot(struct unit_test_state *uts) ut_assert_skip_to_line("(2 bootflows, 2 valid)");

ut_assert_nextline("Selected: Armbian");

• ut_assert_skip_to_line("Boot failed (err=-14)");

• if (gd->flags & GD_FLG_HUSH_OLD_PARSER) {

• /*
  

•  * With old hush, despite booti failing to boot, i.e. returning
  

•  * CMD_RET_FAILURE, run_command() returns 0 which leads 
  

bootflow_boot(),

as + * we are using bootmeth_script here, to return -EFAULT.

•  */
  

• ut_assert_skip_to_line("Boot failed (err=-14)");
  

• } else if (gd->flags & GD_FLG_HUSH_MODERN_PARSER) {

• /*
  

•  * While with modern one, run_command() propagates 
  

CMD_RET_FAILURE

returned + * by booti, so we get 1 here.

•  */
  

• ut_assert_skip_to_line("Boot failed (err=1)");
  

• }

I would like to give a bit of context here. With the following patch: diff --git a/test/py/tests/test_ut.py b/test/py/tests/test_ut.py index c169c835e3..cc5adda0a3 100644 --- a/test/py/tests/test_ut.py +++ b/test/py/tests/test_ut.py @@ -173,7 +173,7 @@ else fi fi booti ${kernel_addr_r} ${ramdisk_addr_r} ${fdt_addr_r} - +echo $? # Recompile with: # mkimage -C none -A arm -T script -d /boot/boot.cmd /boot/boot.scr ''' % (mmc_dev) We can easily see that booti is failing while running the test: $ ./test/py/test.py -o log_cli=true -s --build -v -k 'test_ut[ut_bootstd_bootflow_scan_menu_boot' ... Aborting! Failed to load '/boot/dtb/rockchip/overlay/-fixup.scr' 1

The problem with old hush, is that the 1 returned here, which corresponds to CMD_RET_FAILURE, is not propagated as the return value of run_command(). So, this lead the -EFAULT branch here to be taken: int bootflow_boot(struct bootflow *bflow) { /* ... */

ret = bootmeth_boot(bflow->method, bflow); if (ret) return log_msg_ret("boot", ret);

/* * internal error, should not get here since we should have booted * something or returned an error */

return log_msg_ret("end", -EFAULT); }

With modern hush, CMD_RET_FAILURE is propagated as the return value of run_command(). As a consequence, we return with log_mst_ret("boot", 1), which leaded to this test to fail. The above modification consists in adapting the expected output to the current shell flavor. I think this is the good thing to do, as I find modern hush behavior better than the old one, i.e. it propagates CMD_RET_FAILURE as return of run_command().

ut_assertnonnull(std->cur_bootflow); ut_assert_console_end();

Happy end of the year!