Re: [PATCH v12 00/24] Modernize U-Boot shell

[Francis Laniel <francis.laniel@amarulasolutions.com>]

Hi!


Le dimanche 10 décembre 2023, 00:16:31 CET Tom Rini a écrit :
> On Fri, Dec 08, 2023 at 11:30:01PM +0100, Francis Laniel 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 modern 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 modern
> > => cli print
> > modern
> > => cli set old
> > The default parser is the old one.
> > Note that to use both parser, you would need to set both
> > CONFIG_HUSH_MODERN_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
> > => cli set modern
> > => 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 modern
> > => cli get
> > modern
> > => printenv fdtfile
> > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb
> > => boot
> > ...
> > root@lepotato:~#
> > 
> > This contribution indeed adds a lot of code and there were concern about
> > its size [6, 7].
> > With regard to the amount of code added, the cli_hush_upstream.c is 13030
> > lines long but it seems a smaller subset is really used:
> > gcc -D__U_BOOT__ -E common/cli_hush_upstream.c | wc -l
> > 2870
> > Despite this, it is better to still have the whole upstream code for the
> > sake of easing maintenance.
> > With regard to memory size, I conducted some experiments for version 8 of
> > this series and for a subset of arm64 boards and found the worst case to
> > be 4K [8]. Tom Rini conducted more research on this and also found the
> > increase to be acceptable [9].
> > 
> > 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.
> >  
> >  v11:
> >   * Renamed everything containing 2021 to modern.
> >   * Fixed cmd Kconfig indentation.
> >   * Set modern parser as default for majority of boards except some.
> 
> Thanks, this looks good overall and I'll put things in CI soon and aim
> to get this in to -next and v2024.04.

Thank you!
In the meanwhile, I will take a look time to time to upstream busybox to keep 
our version of hush up to date!

I will also take a look to enabling function, but I guarantee anything on this 
side!


Best regards.