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

[Francis Laniel <francis.laniel@amarulasolutions.com>]

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!