Fix a few problems with the command parsers

PATCH New Simon Glass 3 participants 8 messages May 24, 2014 – May 30, 2014

Share & tools

Simon Glass

author

2014-05-24 03:11 UTC #0 lore raw

cover 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.

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/cmd_itest.c | 2 +-
 common/hush.c      | 4 +++-
 common/main.c      | 2 +-
 test/command_ut.c  | 5 +++++
 4 files changed, 10 insertions(+), 3 deletions(-)

Signature · 1 line

1.9.1.423.g4596e3a

Simon Glass

author

2014-05-24 03:11 UTC #1 lore raw

PATCH 1/4 Add final result tests for run_command_list()

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 <[email protected]>

---

 test/command_ut.c | 5 +++++
 1 file changed, 5 insertions(+)

test/command_ut.c +5 -0

index aaa1ee2..b2666bf 100644
@@ -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 */

Signature · 1 line

1.9.1.423.g4596e3a

Simon Glass

author

2014-05-24 03:11 UTC #2 lore raw

PATCH 2/4 Fix itest mask overflow

In reply to Simon Glass

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 <[email protected]>

---

 common/cmd_itest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/cmd_itest.c +1 -1

index ae2527b..76af62b 100644
@@ -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)

Signature · 1 line

1.9.1.423.g4596e3a

Simon Glass

author

2014-05-24 03:11 UTC #3 lore raw

PATCH 3/4 Fix hush to give the correct return code for a simple command

In reply to Simon Glass

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 <[email protected]>

Signed-off-by Simon Glass <[email protected]>

---

 common/hush.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

common/hush.c +3 -1

index 5b43224..7a16795 100644
@@ -3218,7 +3218,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

Signature · 1 line

1.9.1.423.g4596e3a

Stefan Herbrechtsmeier

2014-05-25 08:08 UTC #4 lore raw

PATCH 3/4 Fix hush to give the correct return code for a simple command

Am 24.05.2014 05:11, schrieb Simon Glass:

Quoted 28 lines

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 <[email protected]>
Signed-off-by Simon Glass <[email protected]>
---
  common/hush.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
common/hush.c +0 -0
index 5b43224..7a16795 100644

common/hush.c static int parse_stream_outer(struct in_str *inp, int flag) · line 3226

Quoted 10 lines

common/hush.c +3 -1

@@ -3218,7 +3218,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

Tested-by Stefan Herbrechtsmeier<[email protected]>

Simon Glass

author

2014-05-24 03:11 UTC #5 lore raw

PATCH 4/4 Correct return code from builtin_run_command_list()

In reply to Simon Glass

The return code is not consistent with builtin_run_command_list(). For the last command in a sequence, the return code is actually inverted.

Fix it.

Signed-off-by Simon Glass <[email protected]>

---

 common/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/main.c +1 -1

index 9bee7bd..e2350bd 100644
@@ -1485,7 +1485,7 @@ static int builtin_run_command_list(char *cmd, int flag)
 		++next;
 	}
 	if (rcode == 0 && *line)
-		rcode = (builtin_run_command(line, 0) >= 0);
+		rcode = (builtin_run_command(line, 0) < 0);
 
 	return rcode;
 }

Signature · 1 line

1.9.1.423.g4596e3a

Tom Rini

2014-05-30 18:59 UTC #6 lore raw

cover Fix a few problems with the command parsers

In reply to Simon Glass

Quoted 4 lines · Simon Glass

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.

Can you please rebase this on master again? I imagine it doesn't quite apply again with the splitting out in main.c series that's applied now, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140530/dc8e6dc7/attachment.pgp>

Simon Glass

author

2014-05-30 19:16 UTC #7 lore raw

cover Fix a few problems with the command parsers

Hi Tom,

Quoted 10 lines · Tom Rini

Quoted 4 lines · Simon Glass
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.
Can you please rebase this on master again? I imagine it doesn't quite apply again with the splitting out in main.c series that's applied now, thanks!

Yes, I'll take a look now.

Regards,
Simon