[RFC PATCH v10 00/24] Modernize U-Boot shell
Francis Laniel
francis.laniel at amarulasolutions.com
Tue Nov 7 22:52:00 CET 2023
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.laniel at 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 at lepotato:~#
> > root at lepotato:~# reboot
> > ...
> > => cli set 2021
> > => cli get
> > 2021
> > => printenv fdtfile
> > fdtfile=amlogic/meson-gxl-s905x-libretech-cc.dtb
> > => boot
> > ...
> > root at 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.
More information about the U-Boot
mailing list