Hi!
Le lundi 9 octobre 2023, 20:56:30 EET Simon Glass a écrit : > Hi Francis, > > On Wed, 4 Oct 2023 at 10:42, 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 > > This is a really nice integration. > > I think it could benefit from a new Kconfig like CONFIG_HUSH_SELECTABLE > which enables the cli command and selection of parser...this could perhaps > be enabled automatically if two parsers are selected. Then the code growth > (about 1KB on Thumb 2) would mostly go away. Excellent idea! I added this in v11 but did not find a way to count how many options were set, so HUSH_SELECTABLE is set when the two hush flavors are selected. > For checking parsers, the code you use: > > if (gd->flags & GD_FLG_HUSH_OLD_PARSER) { > int hush_flags = FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP; > > if (flag & CMD_FLAG_ENV) > hush_flags |= FLAG_CONT_ON_NEWLINE; > return parse_string_outer(cmd, hush_flags); > } else if (gd->flags & GD_FLG_HUSH_2021_PARSER) { > ... > return parse_string_outer_2021(cmd, FLAG_EXIT_FROM_LOOP); > } > > could be written to reduce code size...i.e. there is only a need to check > the gd flag if HUSH_SELECTABLE is enabled, so: > > static inline bool use_hush_old(void) > { > return IS_ENABLED(CONFIG_HUSH_SELECTABLE) ? > gd->flags & GD_FLG_HUSH_OLD_PARSER : > IS_ENABLD(CONFIG_HUSH_OLD_PARSER); > } > > Then use that function in the code able. Indeed! Thank you for the suggestion! > Size growth when swtiching to the the new parser is only 3KB, from my test. > I thought it would be much more? > > > => 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 agree. > > > I also had to enable CONFIG_LTO for kirkwoord sheevaplug and phytec > > bk4r1, so > > > they do not hit their limits. > > > > 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. > > It would be great if this could go in this cycle. > > One minor nit is that the commit "cmd: Add new cli command" introduces a > build error which is fixed in the next commit. It would be better to have > the GD flag added in that first commit. I normally addressed this in the v11, I will think to use buildman in the future to avoid this type of mistake! > It is great to see such a simple diff from 2 years of busybox activity. It > augurs well for future maintenance. Indeed! This should be doable to maintain this code if we take a look at upstream busybox time to time. > I feel that in general, if if() were used instead of #if, some of the > #ifdef stuff might go away. For example, if some #ifdef'd code calls foo(), > then foo() itself needs an #ifdef but if the code which calls foo() is 'if > (!U_BOOT) foo()' then we don't need the #ifdef around foo(). > > Another point is that I wonder if it is worth #ifdefing out struct members. > Perhaps it is safer that way, but it does create more #ifdefs, where if() > could be used. > > In a few cases I saw chunks of code being added - it might be better for > maintenance to create a new function with the U-Boot code and call it from > the original site. To be honest with you, there are plenty of things to improve with the current code. I first went with #ifdef as it was already used with the old hush and I wanted to get something which works. But yes, it would be better to add more functions in cli_hush_2021.c and touch as few as possible cli_hush_upstream.c. > These are all minor things, though. I suggest sending a non-RFC as I > believe it is mostly ready to go in. > > > * 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. > > > > 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 | 22 + > > cmd/Makefile | 2 + > > cmd/cli.c | 125 + > > 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, 14549 insertions(+), 212 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 > > > > -- > > 2.34.1 > > Regards, > Simon Best regards.