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 <s...@chromium.org> > Signed-off-by: Francis Laniel <francis.lan...@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!