[U-Boot] [PATCH v2 0/4] Fix a few problems with the command parsers

In a few cases the behaviour of both the hush and built-in parsers seems incorrect. One such case was exposed by commit 1992dbf which attempted to execute a simple command using hush and get the correct return value. Further digging exposed the other problems.
Changes in v2: - Rebase to master
Simon Glass (4): Add final result tests for run_command_list() Fix itest mask overflow Fix hush to give the correct return code for a simple command Correct return code from builtin_run_command_list()
common/cli_hush.c | 4 +++- common/cli_simple.c | 2 +- common/cmd_itest.c | 2 +- test/command_ut.c | 5 +++++ 4 files changed, 10 insertions(+), 3 deletions(-)

run_command_list() is supposed to return a return code of 0 for success and 1 for failure. Add a few simple tests that confirm this. These tests work both with the built-in parser and hush.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
test/command_ut.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/test/command_ut.c b/test/command_ut.c index aaa1ee2..b2666bf 100644 --- a/test/command_ut.c +++ b/test/command_ut.c @@ -61,6 +61,11 @@ static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) "setenv list ${list}3", strlen("setenv list 1"), 0); assert(!strcmp("1", getenv("list")));
+ assert(run_command("false", 0) == 1); + assert(run_command("echo", 0) == 0); + assert(run_command_list("false", -1, 0) == 1); + assert(run_command_list("echo", -1, 0) == 0); + #ifdef CONFIG_SYS_HUSH_PARSER /* Test the 'test' command */

On Fri, May 30, 2014 at 02:41:48PM -0600, Simon Glass wrote:
run_command_list() is supposed to return a return code of 0 for success and 1 for failure. Add a few simple tests that confirm this. These tests work both with the built-in parser and hush.
Signed-off-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!

The mask value used in itest overflows and therefore it can return an incorrect result for something like 'itest 0 == 1'. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/cmd_itest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cmd_itest.c b/common/cmd_itest.c index ae2527b..76af62b 100644 --- a/common/cmd_itest.c +++ b/common/cmd_itest.c @@ -63,7 +63,7 @@ static long evalexp(char *s, int w) l = simple_strtoul(s, NULL, 16); }
- return (l & ((1 << (w * 8)) - 1)); + return l & ((1UL << (w * 8)) - 1); }
static char * evalstr(char *s)

On Fri, May 30, 2014 at 02:41:49PM -0600, Simon Glass wrote:
The mask value used in itest overflows and therefore it can return an incorrect result for something like 'itest 0 == 1'. Fix it.
Signed-off-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!

When a simple command like 'false' is provided, hush should return the result of that command. However, hush only does this if the FLAG_EXIT_FROM_LOOP flag is provided. Without this flag, hush will happily execute the empty string command immediate after 'false' and then return a success code.
This behaviour does not seem very useful, and requiring the flag also seems wrong, since it means that hush will execute only the first command in a sequence.
Add a check for empty string and fall out of the loop in that case. That at least fixes the simple command case. This is a change in behaviour but it is unlikely that the old behaviour would be considered correct in any case.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/cli_hush.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/cli_hush.c b/common/cli_hush.c index 0f069b0..e0c436f 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -3215,7 +3215,9 @@ static int parse_stream_outer(struct in_str *inp, int flag) free_pipe_list(ctx.list_head,0); } b_free(&temp); - } while (rcode != -1 && !(flag & FLAG_EXIT_FROM_LOOP)); /* loop on syntax errors, return on EOF */ + /* loop on syntax errors, return on EOF */ + } while (rcode != -1 && !(flag & FLAG_EXIT_FROM_LOOP) && + (inp->peek != static_peek || b_peek(inp))); #ifndef __U_BOOT__ return 0; #else

On Fri, May 30, 2014 at 02:41:50PM -0600, Simon Glass wrote:
When a simple command like 'false' is provided, hush should return the result of that command. However, hush only does this if the FLAG_EXIT_FROM_LOOP flag is provided. Without this flag, hush will happily execute the empty string command immediate after 'false' and then return a success code.
This behaviour does not seem very useful, and requiring the flag also seems wrong, since it means that hush will execute only the first command in a sequence.
Add a check for empty string and fall out of the loop in that case. That at least fixes the simple command case. This is a change in behaviour but it is unlikely that the old behaviour would be considered correct in any case.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!

The return code is not consistent with cli_simple_run_command_list(). For the last command in a sequence, the return code is actually inverted.
Fix it. Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Rebase to master
common/cli_simple.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cli_simple.c b/common/cli_simple.c index 413c2eb..49d5833 100644 --- a/common/cli_simple.c +++ b/common/cli_simple.c @@ -331,7 +331,7 @@ int cli_simple_run_command_list(char *cmd, int flag) ++next; } if (rcode == 0 && *line) - rcode = (cli_simple_run_command(line, 0) >= 0); + rcode = (cli_simple_run_command(line, 0) < 0);
return rcode; }

On Fri, May 30, 2014 at 02:41:51PM -0600, Simon Glass wrote:
The return code is not consistent with cli_simple_run_command_list(). For the last command in a sequence, the return code is actually inverted.
Fix it. Signed-off-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!

Hi Tom,
On 30 May 2014 14:41, Simon Glass sjg@chromium.org wrote:
In a few cases the behaviour of both the hush and built-in parsers seems incorrect. One such case was exposed by commit 1992dbf which attempted to execute a simple command using hush and get the correct return value. Further digging exposed the other problems.
Changes in v2:
- Rebase to master
Here is the rebase, but note I have at least one more thing to look at for this release.
Simon Glass (4): Add final result tests for run_command_list() Fix itest mask overflow Fix hush to give the correct return code for a simple command Correct return code from builtin_run_command_list()
common/cli_hush.c | 4 +++- common/cli_simple.c | 2 +- common/cmd_itest.c | 2 +- test/command_ut.c | 5 +++++ 4 files changed, 10 insertions(+), 3 deletions(-)
-- 1.9.1.423.g4596e3a
Regards, Simon
participants (2)
-
Simon Glass
-
Tom Rini