On Wed, Nov 8, 2023 at 12:49 AM Francis Laniel <francis.lan...@amarulasolutions.com> wrote: > > Hi. > > > 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 "cli" command: > => cli print > old > => cli set 2021 > => cli print > 2021 > => cli set old > The default parser is the old one. > Note that to use both parser, you would need to set both > CONFIG_HUSH_2021_PARSER > and CONFIG_HUSH_OLD_PARSER. > > 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 > => ut hush > Running 20 hush tests > ... > Failures: 0 > > Thanks to the effort of Harald Seiler, I was successful booting a board: > => printenv fdtfile > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb > => cli get > old > => boot > ... > root@lepotato:~# > root@lepotato:~# reboot > ... > => cli set 2021 > => cli get > 2021 > => printenv fdtfile > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb > => boot > ... > root@lepotato:~# > > I had to not use CONFIG_HUSH_2021_PARSER for the keymile board. > Indeed, the keymile board family is the only set of boards 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. > I think this particularities can be addressed in future works. > > I also had to enable CONFIG_LTO for kirkwoord sheevaplug and phytec bk4r1, so > they do not hit their limits.
With nearly 15K lines of code added and the above limits being reached how much does this increase the size of a binary if it's selected? Those sorts of details are useful in these cover letters. Also of the 13k lines in cli_hush_upstream.c how much of that functionality is actually used in U-Boot? You mention functions are not, while I understand adding it straight from busybox has it's advantages for easier rebasing, if it's also pulling in a lot of code that is never used there's also detractions to adding 13k lines of code. > 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*(). > v3: > * Various bug fixes pointed by the CI. > * Added upstream busybox hush commits until 6th February 2022. > v4: > * Various cleaning. > * Modified python test to accept failure output when the test are designed > to > fail. > * Bumped upstream busybox hush commits until 24h March 2022. > v5: > * Bumped upstream busybox hush commits until 30th January 2023. > * Fix how hush interprets '<' and '>', indeed we needed to escape them but I > removed this behavior as tests are handled by test command and not hush > itself. This permitted to have the ut fdt to pass. > * Fix a problem with how exit was handled. This was reported by the ut exit > test. > v6: > * There was no v6 and I got mixed up with version. > v7: > * Bumped upstream busybox hush commits until 9th May 2023. > * Renamed parser command to change parser at runtime to cli and added > documentation. > * Added better separation of patches. > * Removed code about __gnu_thumb1_case_si as it was merged in another > series. > * Various cleaning. > v8: > * Bumped upstream busybox hush commits until 25th May 2023. > v9: > * Bumped upstream busybox hush commits until 2nd October 2023. > v10: > * Fixed a build error in commit adding cli command. > * Add new CONFIG_HUSH_SELECTABLE to only build cli command if the two shell > flavors are selected. > > Francis Laniel (24): > 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 cli 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 > cli: hush_2021: Enable loops > test: hush: Fix loop tests for hush 2021 > cli: hush_2021: Add upstream commits up to 2nd October 2023. > DO NOT MERGE: only to make CI happy > DO NOT MERGE: ci: Build the world in any case. > > .azure-pipelines.yml | 1 + > cmd/Kconfig | 26 + > cmd/Makefile | 2 + > cmd/cli.c | 134 + > common/Makefile | 3 +- > common/cli.c | 82 +- > common/cli_hush_2021.c | 324 + > common/cli_hush_upstream.c | 13030 +++++++++++++++++++++++++++ > configs/sheevaplug_defconfig | 1 + > doc/usage/cmd/cli.rst | 74 + > doc/usage/index.rst | 1 + > include/asm-generic/global_data.h | 8 + > include/cli_hush.h | 51 +- > include/test/hush.h | 15 + > include/test/suites.h | 1 + > test/Makefile | 3 + > 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 | 316 + > test/hush/list.c | 140 + > test/hush/loop.c | 91 + > test/py/tests/test_hush_if_test.py | 197 - > test/py/tests/test_ut.py | 8 +- > tools/patman/series.py | 4 + > 26 files changed, 14563 insertions(+), 211 deletions(-) > create mode 100644 cmd/cli.c > create mode 100644 common/cli_hush_2021.c > create mode 100644 common/cli_hush_upstream.c > create mode 100644 doc/usage/cmd/cli.rst > 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. > --- > [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.34.1 >