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. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot