| Message ID | 1414621302-32062-6-git-send-email-rabin@rab.in |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Rabin, On 29 October 2014 16:21, Rabin Vincent <rabin@rab.in> wrote: > Add a couple of tests for quoting. The indirect variable read test is > a fixed version of the following expression from Fedora ARM's boot.scr. > This unfixed expression stopped working after fe9ca3d ("hush: fix some > quoted variable expansion issues"): > > setenv catcat setenv catout\;'setenv catX "setenv catout '\\\\\\\''\$\$catin'\\\\\\\''"' \; run catX > > Signed-off-by: Rabin Vincent <rabin@rab.in> > --- > test/command_ut.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/test/command_ut.c b/test/command_ut.c > index 926573a..21804a4 100644 > --- a/test/command_ut.c > +++ b/test/command_ut.c > @@ -193,6 +193,16 @@ static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > assert(run_command("'", 0) == 1); > > + assert(run_command("setenv ut_var '\"'; setenv ut_var2 \"${ut_var}\"", 0) == 0); > + assert(!strcmp(getenv("ut_var2"), "\"")); > + > + assert(run_command("setenv ut_catcat setenv ut_catout\\;setenv ut_catX setenv ut_catout \\\\\\\\\\\\\\\"\\$\\$ut_catin\\\\\\\\\\\\\\\" \\; run ut_catX", 0) == 0); > + assert(run_command("setenv ut_pointer '1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20'", 0) == 0); Can we reduce this down to 3-4 numbers for easier maintenance? Or do the 20 numbers buy us something? Also did you test this with the simple cli parser too? > + assert(run_command("setenv ut_catin ut_pointer", 0) == 0); > + assert(run_command("run ut_catcat", 0) == 0); > + assert(getenv("ut_catout")); > + assert(!strcmp(getenv("ut_catout"), "1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20")); > + > printf("%s: Everything went swimmingly\n", __func__); > return 0; > } > -- > 2.1.1 > Regards, Simon
On Sat, Nov 01, 2014 at 09:12:37AM -0600, Simon Glass wrote: > On 29 October 2014 16:21, Rabin Vincent <rabin@rab.in> wrote: > > + assert(run_command("setenv ut_var '\"'; setenv ut_var2 \"${ut_var}\"", 0) == 0); > > + assert(!strcmp(getenv("ut_var2"), "\"")); > > + > > + assert(run_command("setenv ut_catcat setenv ut_catout\\;setenv ut_catX setenv ut_catout \\\\\\\\\\\\\\\"\\$\\$ut_catin\\\\\\\\\\\\\\\" \\; run ut_catX", 0) == 0); > > + assert(run_command("setenv ut_pointer '1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20'", 0) == 0); > > Can we reduce this down to 3-4 numbers for easier maintenance? Or do > the 20 numbers buy us something? After 14 arguments, the quotes around them become necessary, so having more than 14 ensures we test that the quotes are still there: => setenv x 1 2 3 4 5 6 7 8 9 10 11 12 13 => setenv x 1 2 3 4 5 6 7 8 9 10 11 12 13 14 => setenv x 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 setenv - set environment variables Usage: setenv [-f] name value ... - [forcibly] set environment variable 'name' to 'value ...' setenv [-f] name - [forcibly] delete environment variable 'name' => setenv x '1 2 3 4 5 6 7 8 9 10 11 12 13 14 15' => > Also did you test this with the simple cli parser too? No, I didn't realize that these tests they would get run without hush. I tried them out and they fail with the simple parser, as do the tests with the empty strings in the third patch. What do you suggest? Drop these new tests, or move them inside the ifdef HUSH?
Hi, On 5 November 2014 13:11, Rabin Vincent <rabin@rab.in> wrote: > On Sat, Nov 01, 2014 at 09:12:37AM -0600, Simon Glass wrote: >> On 29 October 2014 16:21, Rabin Vincent <rabin@rab.in> wrote: >> > + assert(run_command("setenv ut_var '\"'; setenv ut_var2 \"${ut_var}\"", 0) == 0); >> > + assert(!strcmp(getenv("ut_var2"), "\"")); >> > + >> > + assert(run_command("setenv ut_catcat setenv ut_catout\\;setenv ut_catX setenv ut_catout \\\\\\\\\\\\\\\"\\$\\$ut_catin\\\\\\\\\\\\\\\" \\; run ut_catX", 0) == 0); >> > + assert(run_command("setenv ut_pointer '1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20'", 0) == 0); >> >> Can we reduce this down to 3-4 numbers for easier maintenance? Or do >> the 20 numbers buy us something? > > After 14 arguments, the quotes around them become necessary, so having > more than 14 ensures we test that the quotes are still there: > > => setenv x 1 2 3 4 5 6 7 8 9 10 11 12 13 > => setenv x 1 2 3 4 5 6 7 8 9 10 11 12 13 14 > => setenv x 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 > setenv - set environment variables OK sounds good. How about adding a test that CONFIG_SYS_MAXARGS is < you number. Like #if CONFIG_SYS_MAXARGS > 15 #error "..." #endif Otherwise if someone changes it in sandbox the test will change. > > Usage: > setenv [-f] name value ... > - [forcibly] set environment variable 'name' to 'value ...' > setenv [-f] name > - [forcibly] delete environment variable 'name' > => setenv x '1 2 3 4 5 6 7 8 9 10 11 12 13 14 15' > => > >> Also did you test this with the simple cli parser too? > > No, I didn't realize that these tests they would get run without hush. > I tried them out and they fail with the simple parser, as do the tests > with the empty strings in the third patch. What do you suggest? Drop > these new tests, or move them inside the ifdef HUSH? #ifdef HUSH if the tests can't work without it. Regards, Simon
diff --git a/test/command_ut.c b/test/command_ut.c index 926573a..21804a4 100644 --- a/test/command_ut.c +++ b/test/command_ut.c @@ -193,6 +193,16 @@ static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) assert(run_command("'", 0) == 1); + assert(run_command("setenv ut_var '\"'; setenv ut_var2 \"${ut_var}\"", 0) == 0); + assert(!strcmp(getenv("ut_var2"), "\"")); + + assert(run_command("setenv ut_catcat setenv ut_catout\\;setenv ut_catX setenv ut_catout \\\\\\\\\\\\\\\"\\$\\$ut_catin\\\\\\\\\\\\\\\" \\; run ut_catX", 0) == 0); + assert(run_command("setenv ut_pointer '1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20'", 0) == 0); + assert(run_command("setenv ut_catin ut_pointer", 0) == 0); + assert(run_command("run ut_catcat", 0) == 0); + assert(getenv("ut_catout")); + assert(!strcmp(getenv("ut_catout"), "1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20")); + printf("%s: Everything went swimmingly\n", __func__); return 0; }
Add a couple of tests for quoting. The indirect variable read test is a fixed version of the following expression from Fedora ARM's boot.scr. This unfixed expression stopped working after fe9ca3d ("hush: fix some quoted variable expansion issues"): setenv catcat setenv catout\;'setenv catX "setenv catout '\\\\\\\''\$\$catin'\\\\\\\''"' \; run catX Signed-off-by: Rabin Vincent <rabin@rab.in> --- test/command_ut.c | 10 ++++++++++ 1 file changed, 10 insertions(+)