Hi.
First I hope you are fine and the same for your relatives.
During 2021 summer, Sean Anderson wrote a contribution to add a new shell, based on LIL, to U-Boot [1][2]. While one of the goals of this contribution was to address the fact actual U-Boot shell, which is based on Busybox hush, is old there was a discussion about adding a new shell versus updating the actual one [3][4].
So, in this series, with Harald Seiler, we updated the actual U-Boot shell to reflect what is currently in Busybox source code. Basically, this contribution is about taking a snapshot of Busybox shell/hush.c file (as it exists in commit 37460f5da) and adapt it to suit U-Boot needs.
This contribution was written to be as backward-compatible as possible to avoid breaking the existing. So, the 2021 hush flavor offers the same as the actual, that is to say: 1. Variable expansion. 2. Instruction lists (;, && and ||). 3. If, then and else. 4. Loops (for, while and until). No new features offered by Busybox hush were implemented (e.g. functions).
It is possible to change the parser at runtime using the "parser" command: => parser print old => parser set 2021 2021> parser print 2021 2021> parser set old => The default parser is the old one.
In terms of testing, new unit tests were added to ut to ensure the new behavior is the same as the old one and it does not add regression. Nonetheless, if old behavior was buggy and fixed upstream, the fix is then added to U-Boot [5]. In sandbox, all of these tests pass smoothly: => printenv board board=sandbox => ut hush Running 20 hush tests ... Failures: 0 => parser set 2021 2021> ut hush Running 20 hush tests ... Failures: 0
Thanks to the effort of Harald Seiler, I was successful booting a board: 2021> printenv board_rev board_rev=iMX8MP 2021> boot ... root@iMX8MPboard:~# fw_printenv board_rev board_rev=iMX8MP
I also tested another board where both parser were successful to boot it: => printenv fdtfile fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb => boot ... root@lepotato:~# root@lepotato:~# reboot ... => parser set 2021 2021> printenv fdtfile fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb 2021> boot ... root@lepotato:~#
Sadly, I was not able to have the CI passes smoothly... To pass it and be aware of all the troubles, I added the 3 last commits as "trick" commits. So, I would like to have your opinion on these commits and particularly on the underlying problems: 1. When I added a test to ensure variable expansion behavior is correct, I added some tests which should print "unknown command" (to test the shell behavior when a bad output is given). Sadly, having "unknown command" printed will make the CI fails. I think we can simply remove this "negative" test but what do you think? 2. When running ut setexpr setexpr_test_str from test.py the test fails while running the following assert: ut_assertok(ut_check_delta(start_mem)); I am able to reproduce it while running within test.py but not from sandbox. So, I would like to have your reviews or to get a bit of help to know how I can better debug this specific case? 3. The keymile board is the only board to call get_local_var(), set_local_var() and unset_local_var(). Sadly, these functions are static in this contribution. I could have change all of them to introduce code like this: *_local_var(/*...*/) { if (gd->flags & GD_FLG_HUSH_OLD_PARSER) return *_local_var_old(/*...*/); if (gd->flags & GD_FLG_HUSH_2021_PARSER) return *_local_var_2021(/*...*/); } But this would have mean renaming all old hush functions calls and I did not want to change the old hush particularly to avoid breaking things. Instead, I change the keymile board to use environment variable instead of local ones. Do you think this a good change or I should change the old hush instead?
For all these reasons, I marked this contribution as RFC to indeed collect your opinions. My goal is not to change suddenly actual shell to this one, we clearly need a transition period to think about it. I think it is better to see this contribution as a proof of concept which shows it is possible to update the actual shell.
If you want to review it - your review will really be appreciated - here are some information regarding the commits: * commits marked as "test:" deal with unit tests. * commit "cli: Add Busybox upstream hush.c file." copies Busybox shell/hush.c into U-Boot tree, this explain why this commit contains around 12000 additions. * commit "cli: Port Busybox 2021 hush to U-Boot." modifies previously added file to permit us to use this as new shell. The really good idea of #include'ing Busybox code into a wrapper file to define some particular functions while minimizing modifications to upstream code comes from Harald Seiler. * commit "cmd: Add new parser command" adds a new command which permits selecting parser at runtime. I am not really satisfied with the fact it calls cli_init() and cli_loop() each time the parser is set, so your reviews would be welcomed. * Other commits focus on enabling features we need (e.g. if).
Changes since v2: * Added a small fix to compile sandbox with NO_SDL=1. * Added a command to change parser at runtime. * Added 2021 parser function to all run_command*().
Changes since v3: * Various bug fixes pointed by the CI. * Added upstream busybox hush commits until 6th February 2022.
Francis Laniel (27): video: sandbox: Add dummy function for sandbox_sdl_remove_display(). test: Add framework to test hush behavior test: hush: Test hush if/else test/py: hush_if_test: Remove the test file test: hush: Test hush variable expansion test: hush: Test hush commands list test: hush: Test hush loops cli: Add Busybox upstream hush.c file cli: Port Busybox 2021 hush to U-Boot cli: Add menu for hush parser global_data.h: add GD_FLG_HUSH_OLD_PARSER flag. cmd: Add new parser command. cli: Enables using hush 2021 parser as command line parser cli: hush_2021: Enable variables expansion for hush 2021 cli: hush_2021: Add functions to be called from run_command() cli: add hush 2021 as parser for run_command*(). test: hush: Fix instructions list tests for hush 2021 test: hush: Fix variable expansion tests for hush 2021 cli: hush_2021: Enable using < and > as string compare operators cli: hush_2021: Enable if keyword test: hush: Fix if tests for hush 2021 cli: hush_2021: Enable loops test: hush: Fix loop tests for hush 2021 cli: hush_2021: Add upstream commits up to 6th February 2022. for test purpose only: Comment out dollar tests which prints error messages. for test purpose only: Comment out failed function which fails only in CI. board: keymile: common: Use environment to store IVM_* variables.
Tom Rini (1): Modernize U-Boot shell
arch/sandbox/include/asm/sdl.h | 5 + board/keymile/common/common.c | 8 +- board/keymile/common/ivm.c | 9 +- cmd/Kconfig | 21 + cmd/Makefile | 2 + cmd/parser.c | 125 + common/Makefile | 3 +- common/cli.c | 81 +- common/cli_hush_2021.c | 303 + common/cli_hush_upstream.c | 12910 +++++++++++++++++++++++++++ include/asm-generic/global_data.h | 8 + include/cli_hush.h | 49 +- include/test/hush.h | 15 + include/test/suites.h | 1 + test/Makefile | 3 + test/cmd/setexpr.c | 2 +- test/cmd_ut.c | 6 + test/hush/Makefile | 10 + test/hush/cmd_ut_hush.c | 20 + test/hush/dollar.c | 226 + test/hush/if.c | 353 + test/hush/list.c | 140 + test/hush/loop.c | 90 + test/py/tests/test_hush_if_test.py | 184 - 24 files changed, 14364 insertions(+), 210 deletions(-) create mode 100644 cmd/parser.c create mode 100644 common/cli_hush_2021.c create mode 100644 common/cli_hush_upstream.c create mode 100644 include/test/hush.h create mode 100644 test/hush/Makefile create mode 100644 test/hush/cmd_ut_hush.c create mode 100644 test/hush/dollar.c create mode 100644 test/hush/if.c create mode 100644 test/hush/list.c create mode 100644 test/hush/loop.c delete mode 100644 test/py/tests/test_hush_if_test.py
Best regards and thank you in advance for your reviews. --- [1] https://lists.denx.de/pipermail/u-boot/2021-July/453347.html [2] https://runtimeterror.com/tech/lil/ [3] https://lists.denx.de/pipermail/u-boot/2021-July/453790.html [4] https://lists.denx.de/pipermail/u-boot/2021-July/453848.html [5] https://lists.denx.de/pipermail/u-boot/2021-August/458569.html -- 2.25.1