[U-Boot] [PATCH 6/6] hush: add some tests for quoting

[Simon Glass]

Hi,

On 5 November 2014 13:11, Rabin Vincent <rabin at 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 at 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