Hi Thomas,
On 30 May 2014 11:55, Simon Glass <s...@chromium.org> wrote:
> Hi Tom,
>
> On 30 May 2014 11:51, Tom Rini <tr...@ti.com> wrote:
>> On Wed, May 28, 2014 at 09:34:25PM +0200, thomas.bet...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
