[PATCH v2 1/2] cmd: exit: Fix return value propagation out of environment scripts

[PATCH v2 1/2] cmd: exit: Fix return value propagation out of environment scripts

[Marek Vasut]

Make sure the 'exit' command as well as 'exit $val' command exits
from environment scripts immediately and propagates return value
out of those scripts fully. That means the following behavior is
expected:

"
=> setenv foo 'echo bar ; exit 1' ; run foo ; echo $?
bar
1
=> setenv foo 'echo bar ; exit 0' ; run foo ; echo $?
bar
0
=> setenv foo 'echo bar ; exit -2' ; run foo ; echo $?
bar
0
"

As well as the followin behavior:

"
=> setenv foo 'echo bar ; exit 3 ; echo fail'; run foo; echo $?
bar
3
=> setenv foo 'echo bar ; exit 1 ; echo fail'; run foo; echo $?
bar
1
=> setenv foo 'echo bar ; exit 0 ; echo fail'; run foo; echo $?
bar
0
=> setenv foo 'echo bar ; exit -1 ; echo fail'; run foo; echo $?
bar
0
=> setenv foo 'echo bar ; exit -2 ; echo fail'; run foo; echo $?
bar
0
=> setenv foo 'echo bar ; exit ; echo fail'; run foo; echo $?
bar
0
"

Fixes: 8c4e3b79bd0 ("cmd: exit: Fix return value")
Reviewed-by: Hector Palacios <hector.palacios@digi.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Adrian Vovk <avovk@cc-sw.com>
Cc: Hector Palacios <hector.palacios@digi.com>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
V2: - Add RB from Hector
    - Update exit.rst
---
 cmd/exit.c             |  7 +++++--
 common/cli.c           |  7 ++++---
 common/cli_hush.c      | 21 +++++++++++++++------
 doc/usage/cmd/exit.rst |  4 +++-
 4 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/cmd/exit.c b/cmd/exit.c
index 2c7132693ad..7bf241ec732 100644
--- a/cmd/exit.c
+++ b/cmd/exit.c
@@ -10,10 +10,13 @@
 static int do_exit(struct cmd_tbl *cmdtp, int flag, int argc,
 		   char *const argv[])
 {
+	int r;
+
+	r = 0;
 	if (argc > 1)
-		return dectoul(argv[1], NULL);
+		r = simple_strtoul(argv[1], NULL, 10);
 
-	return 0;
+	return -r - 2;
 }
 
 U_BOOT_CMD(
diff --git a/common/cli.c b/common/cli.c
index a47d6a3f2b4..ba45dad2db5 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -146,7 +146,7 @@ int run_commandf(const char *fmt, ...)
 #if defined(CONFIG_CMD_RUN)
 int do_run(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	int i;
+	int i, ret;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -160,8 +160,9 @@ int do_run(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			return 1;
 		}
 
-		if (run_command(arg, flag | CMD_FLAG_ENV) != 0)
-			return 1;
+		ret = run_command(arg, flag | CMD_FLAG_ENV);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
diff --git a/common/cli_hush.c b/common/cli_hush.c
index 1467ff81b35..b8940b19735 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -1902,7 +1902,7 @@ static int run_list_real(struct pipe *pi)
 			last_return_code = -rcode - 2;
 			return -2;	/* exit */
 		}
-		last_return_code=(rcode == 0) ? 0 : 1;
+		last_return_code = rcode;
 #endif
 #ifndef __U_BOOT__
 		pi->num_progs = save_num_progs; /* restore number of programs */
@@ -3212,7 +3212,15 @@ static int parse_stream_outer(struct in_str *inp, int flag)
 					printf("exit not allowed from main input shell.\n");
 					continue;
 				}
-				break;
+				/*
+				 * DANGER
+				 * Return code -2 is special in this context,
+				 * it indicates exit from inner pipe instead
+				 * of return code itself, the return code is
+				 * stored in 'last_return_code' variable!
+				 * DANGER
+				 */
+				return -2;
 			}
 			if (code == -1)
 			    flag_repeat = 0;
@@ -3249,9 +3257,9 @@ int parse_string_outer(const char *s, int flag)
 #endif	/* __U_BOOT__ */
 {
 	struct in_str input;
+	int rcode;
 #ifdef __U_BOOT__
 	char *p = NULL;
-	int rcode;
 	if (!s)
 		return 1;
 	if (!*s)
@@ -3263,11 +3271,12 @@ int parse_string_outer(const char *s, int flag)
 		setup_string_in_str(&input, p);
 		rcode = parse_stream_outer(&input, flag);
 		free(p);
-		return rcode;
+		return rcode == -2 ? last_return_code : rcode;
 	} else {
 #endif
 	setup_string_in_str(&input, s);
-	return parse_stream_outer(&input, flag);
+	rcode = parse_stream_outer(&input, flag);
+	return rcode == -2 ? last_return_code : rcode;
 #ifdef __U_BOOT__
 	}
 #endif
@@ -3287,7 +3296,7 @@ int parse_file_outer(void)
 	setup_file_in_str(&input);
 #endif
 	rcode = parse_stream_outer(&input, FLAG_PARSE_SEMICOLON);
-	return rcode;
+	return rcode == -2 ? last_return_code : rcode;
 }
 
 #ifdef __U_BOOT__
diff --git a/doc/usage/cmd/exit.rst b/doc/usage/cmd/exit.rst
index 769223c4775..3edb12809ce 100644
--- a/doc/usage/cmd/exit.rst
+++ b/doc/usage/cmd/exit.rst
@@ -37,4 +37,6 @@ executed.
 Return value
 ------------
 
-$? is always set to 0 (true).
+$? is default set to 0 (true). In case zero or positive integer parameter
+is passed to the command, the return value is the parameter value. In case
+negative integer parameter is passed to the command, the return value is 0.
-- 
2.35.1

[PATCH v2 2/2] test: cmd: exit: Add unit test for exit and partly run commands

[Marek Vasut]

Add a test which validates that exit from environment script works as
expected, including return value propagation and clipping to positive
integers.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Adrian Vovk <avovk@cc-sw.com>
Cc: Hector Palacios <hector.palacios@digi.com>
Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
V2: Add RB from Simon
---
 include/test/suites.h |   1 +
 test/cmd/Makefile     |   2 +-
 test/cmd/exit.c       | 135 ++++++++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c         |   1 +
 4 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 test/cmd/exit.c

diff --git a/include/test/suites.h b/include/test/suites.h
index a01000e127b..9ce49cbb031 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -38,6 +38,7 @@ int do_ut_compression(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[]);
 int do_ut_dm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
+int do_ut_exit(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_font(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_lib(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index bc961df3dce..09e410ec30e 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -8,7 +8,7 @@ endif
 ifdef CONFIG_CONSOLE_RECORD
 obj-$(CONFIG_CMD_PAUSE) += test_pause.o
 endif
-obj-y += mem.o
+obj-y += exit.o mem.o
 obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
 obj-$(CONFIG_CMD_FDT) += fdt.o
 obj-$(CONFIG_CONSOLE_TRUETYPE) += font.o
diff --git a/test/cmd/exit.c b/test/cmd/exit.c
new file mode 100644
index 00000000000..ca34abef899
--- /dev/null
+++ b/test/cmd/exit.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Tests for exit command
+ *
+ * Copyright 2022 Marek Vasut <marex@denx.de>
+ */
+
+#include <common.h>
+#include <console.h>
+#include <mapmem.h>
+#include <asm/global_data.h>
+#include <test/suites.h>
+#include <test/ut.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/* Declare a new exit test */
+#define EXIT_TEST(_name, _flags)	UNIT_TEST(_name, _flags, exit_test)
+
+/* Test 'exit addr' getting/setting address */
+static int cmd_exit_test(struct unit_test_state *uts)
+{
+	int i;
+
+	/*
+	 * Test 'exit' with parameter -3, -2, -1, 0, 1, 2, 3 . Use all those
+	 * parameters to cover also the special return value -2 that is used
+	 * in HUSH to detect exit command.
+	 *
+	 * Always test whether 'exit' command:
+	 * - exits out of the 'run' command
+	 * - return value is propagated out of the 'run' command
+	 * - return value can be tested on outside of 'run' command
+	 * - return value can be printed outside of 'run' command
+	 */
+	for (i = -3; i <= 3; i++) {
+		ut_assertok(console_record_reset_enable());
+		ut_assertok(run_commandf("setenv foo 'echo bar ; exit %d ; echo baz' ; run foo ; echo $?", i));
+		ut_assert_nextline("bar");
+		ut_assert_nextline("%d", i > 0 ? i : 0);
+		ut_assertok(ut_check_console_end(uts));
+
+		ut_assertok(console_record_reset_enable());
+		ut_assertok(run_commandf("setenv foo 'echo bar ; exit %d ; echo baz' ; run foo && echo quux ; echo $?", i));
+		ut_assert_nextline("bar");
+		if (i <= 0)
+			ut_assert_nextline("quux");
+		ut_assert_nextline("%d", i > 0 ? i : 0);
+		ut_assertok(ut_check_console_end(uts));
+
+		ut_assertok(console_record_reset_enable());
+		ut_assertok(run_commandf("setenv foo 'echo bar ; exit %d ; echo baz' ; run foo || echo quux ; echo $?", i));
+		ut_assert_nextline("bar");
+		if (i > 0)
+			ut_assert_nextline("quux");
+		/* Either 'exit' returns 0, or 'echo quux' returns 0 */
+		ut_assert_nextline("0");
+		ut_assertok(ut_check_console_end(uts));
+	}
+
+	/* Validate that 'exit' behaves the same way as 'exit 0' */
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo && echo quux ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("quux");
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; exit ; echo baz' ; run foo || echo quux ; echo $?", i));
+	ut_assert_nextline("bar");
+	/* Either 'exit' returns 0, or 'echo quux' returns 0 */
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	/* Validate that return value still propagates from 'run' command */
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo && echo quux ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("quux");
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; true' ; run foo || echo quux ; echo $?", i));
+	ut_assert_nextline("bar");
+	/* The 'true' returns 0 */
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("1");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo && echo quux ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("1");
+	ut_assertok(ut_check_console_end(uts));
+
+	ut_assertok(console_record_reset_enable());
+	ut_assertok(run_commandf("setenv foo 'echo bar ; false' ; run foo || echo quux ; echo $?", i));
+	ut_assert_nextline("bar");
+	ut_assert_nextline("quux");
+	/* The 'echo quux' returns 0 */
+	ut_assert_nextline("0");
+	ut_assertok(ut_check_console_end(uts));
+
+	return 0;
+}
+
+EXIT_TEST(cmd_exit_test, UT_TESTF_CONSOLE_REC);
+
+int do_ut_exit(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+{
+	struct unit_test *tests = UNIT_TEST_SUITE_START(exit_test);
+	const int n_ents = UNIT_TEST_SUITE_COUNT(exit_test);
+
+	return cmd_ut_category("cmd_exit", "exit_test_", tests, n_ents,
+			       argc, argv);
+}
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index 2736582f11c..067bd0828a1 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -65,6 +65,7 @@ static struct cmd_tbl cmd_ut_sub[] = {
 #if defined(CONFIG_UT_ENV)
 	U_BOOT_CMD_MKENT(env, CONFIG_SYS_MAXARGS, 1, do_ut_env, "", ""),
 #endif
+	U_BOOT_CMD_MKENT(exit, CONFIG_SYS_MAXARGS, 1, do_ut_exit, "", ""),
 #ifdef CONFIG_CMD_FDT
 	U_BOOT_CMD_MKENT(fdt, CONFIG_SYS_MAXARGS, 1, do_ut_fdt, "", ""),
 #endif
-- 
2.35.1

Re: [PATCH v2 1/2] cmd: exit: Fix return value propagation out of environment scripts

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On Tue, Dec 20, 2022 at 07:25:59AM +0100, Marek Vasut wrote:

> Make sure the 'exit' command as well as 'exit $val' command exits
> from environment scripts immediately and propagates return value
> out of those scripts fully. That means the following behavior is
> expected:
> 
> "
> => setenv foo 'echo bar ; exit 1' ; run foo ; echo $?
> bar
> 1
> => setenv foo 'echo bar ; exit 0' ; run foo ; echo $?
> bar
> 0
> => setenv foo 'echo bar ; exit -2' ; run foo ; echo $?
> bar
> 0
> "
> 
> As well as the followin behavior:
> 
> "
> => setenv foo 'echo bar ; exit 3 ; echo fail'; run foo; echo $?
> bar
> 3
> => setenv foo 'echo bar ; exit 1 ; echo fail'; run foo; echo $?
> bar
> 1
> => setenv foo 'echo bar ; exit 0 ; echo fail'; run foo; echo $?
> bar
> 0
> => setenv foo 'echo bar ; exit -1 ; echo fail'; run foo; echo $?
> bar
> 0
> => setenv foo 'echo bar ; exit -2 ; echo fail'; run foo; echo $?
> bar
> 0
> => setenv foo 'echo bar ; exit ; echo fail'; run foo; echo $?
> bar
> 0
> "
> 
> Fixes: 8c4e3b79bd0 ("cmd: exit: Fix return value")
> Reviewed-by: Hector Palacios <hector.palacios@digi.com>
> Signed-off-by: Marek Vasut <marex@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/2] test: cmd: exit: Add unit test for exit and partly run commands

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

On Tue, Dec 20, 2022 at 07:26:00AM +0100, Marek Vasut wrote:

> Add a test which validates that exit from environment script works as
> expected, including return value propagation and clipping to positive
> integers.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Marek Vasut <marex@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]