[U-Boot] Command repeat

[Simon Glass]

Hi Tom,

On 30 May 2014 11:51, Tom Rini <trini at ti.com> wrote:
> On Wed, May 28, 2014 at 09:34:25PM +0200, Thomas.Betker at rohde-schwarz.com wrote:
>
>> Hello all,
>>
>> I have noticed that command repeat doesn't work as it should.
>> CONFIG_SYS_HUSH_PARSER is disabled in my config.
>>
>> Here is what happens: main_loop() expects run_command() to return -1 for
>> failure, 0 or 1 for success (not repeatable or repeatable). However,
>> run_command() actually returns 1 for failure, 0 for success. So if a
>> command is successful, main_loop() always thinks that it is not repeatable
>> ...
>>
>> I guess that instead of run_command(), main_loop() should call
>> builtin_run_command(). Note that main_loop() calls run_command() only when
>> CONFIG_SYS_HUSH_PARSER is not enabled, and in this case, run_command()
>> just calls builtin_run_command() anyway -- except that it remaps the
>> return code.
>>
>> This issue was introduced by the patches in 2012-03-06, and in particular,
>> commit 5307153236caaf2304e578c148e00a4b65cb8604, "Stop using
>> builtin_run_command()".
>>
>> There is one other location which relies on the -1/0/1 logic, viz.,
>> bedbug_main_loop(). Perhaps we need some general function which is called
>> by main_loop() and  bedbug_main_loop():
>>
>>         int run_command_repeatable(const char *cmd, int flag)
>>         {
>>         #ifndef CONFIG_SYS_HUSH_PARSER
>>                 return builtin_run_command(cmd, flag);
>>         #else
>>                 if (parse_string_outer(cmd,
>>                                 FLAG_PARSE_SEMICOLON |
>> FLAG_EXIT_FROM_LOOP))
>>                         return -1;
>>                 return 0;
>>         #endif
>>         }
>>
>> [Note: parse_string_outer() always returns 0 for success, 1 for failure.]
>>
>> Any thoughts on this? I am willing to prepare a patch, but I certainly
>> don't mind if this is handled by somebody else who is deeper into u-boot
>> than me.
>
> Thoughts on this Simon?  Thanks!

Yes I saw it but haven't got to it yet.

I noticed this oddness (command return value changing) when I did the
'empty' hush command series recently, but I didn't realise it was a
bug at the time.

Thomas, a patch is welcome, but we really should add a test for this,
to the command processing unit tests. I've been thinking about being
able to 'inject' commands through a fake/test readline - that might be
one approach. We should continue to expand the test coverage to avoid
such regressions.

Regards,
Simon