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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4366 bytes --]

On Wed, Nov 08, 2023 at 12:14:28PM +0000, Peter Robinson wrote:
> On Wed, Nov 8, 2023 at 12:49 AM Francis Laniel
> <francis.laniel@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.

I've put the world build on current next, before/after this series over
at: https://gist.github.com/trini/53d9a3d59c797ecdbb3aec8edbbb9a12 and
I'll have more commentary (aside from dropping common.h) tomorrow.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]