[U-Boot] Command repeat

[Simon Glass]

Hi Thomas,

On 30 May 2014 11:55, Simon Glass <sjg at chromium.org> wrote:
> 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.

Are you planning to work on a patch?

Regards,
Simon