Re: [RFC PATCH v5 09/24] cli: Add menu for hush parser

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

Hi.


Le mardi 8 novembre 2022, 21:15:12 CET Simon Glass a écrit :
> Hi,
> 
> On Tue, 8 Nov 2022 at 08:21, Tom Rini <trini@konsulko.com> wrote:
> > On Mon, Nov 07, 2022 at 08:28:42AM -0700, Simon Glass wrote:
> > > Hi Patrick,
> > > 
> > > On Mon, 7 Nov 2022 at 05:32, Patrick DELAUNAY
> > > 
> > > <patrick.delaunay@foss.st.com> wrote:
> > > > Hi,
> > > > 
> > > > On 11/1/22 20:20, Francis Laniel wrote:
> > > > > For the moment, the menu contains only entry: HUSH_OLD_PARSER which
> > > > > is the
> > > > > default.
> > > > > The goal is to prepare the field to add a new hush parser which
> > > > > guarantees
> > > > > actual behavior is still correct.
> > > > > 
> > > > > Signed-off-by: Francis Laniel <francis.laniel@amarulasolutions.com>
> > > > > ---
> > > > > 
> > > > >   cmd/Kconfig     | 21 +++++++++++++++++++++
> > > > >   common/Makefile |  3 ++-
> > > > >   2 files changed, 23 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > index 3f6bc70d43..c15d7c51f7 100644
> > > > > --- a/cmd/Kconfig
> > > > > +++ b/cmd/Kconfig
> > > > > @@ -23,6 +23,27 @@ config HUSH_PARSER
> > > > > 
> > > > >         If disabled, you get the old, much simpler behaviour with a
> > > > >         somewhat
> > > > >         smaller memory footprint.
> > > > > 
> > > > > +menu "Hush flavor to use"
> > > > > +     depends on HUSH_PARSER
> > > > > +
> > > > > +     config HUSH_OLD_PARSER
> > > > > +             bool "Use hush old parser"
> > > > > +             default y
> > > > > +             help
> > > > > +               This option enables the old flavor of hush based on
> > > > > hush Busybox from +               2005.
> > > > > +
> > > > > +               It is actually the default U-Boot shell when decided
> > > > > to use hush as shell. +
> > > > > +     config HUSH_2021_PARSER
> > > > > +             bool "Use hush 2021 parser"
> > > > > +             help
> > > > > +               This option enables the new flavor of hush based on
> > > > > hush Busybox from +               2021.
> > > > > +
> > > > > +               For the moment, it is highly experimental and should
> > > > > be used at own risks. +endmenu
> > > > > +
> > > > 
> > > > I think "choice" can be made sense here
> > > > 
> > > > => only one version is used
> > > > 
> > > > 
> > > > choice
> > > > 
> > > >      prompt "Hush flavor to use"
> > > >      default HUSH_OLD_PARSER
> > > >      
> > > >      depends on HUSH_PARSER
> > > > 
> > > > config HUSH_OLD_PARSER
> > > > 
> > > >      bool  "Use hush old parser"
> > > > 
> > > > config HUSH_2021_PARSER
> > > > 
> > > >    bool "Use hush 2021 parser"
> > > > 
> > > > endchoice
> > > 
> > > We need to be able to build both and then select the correct one at
> > > runtime, at least for sandbox. Otherwise we would need yet another
> > > sandbox build. So I think what we have here makes sense.
> > 
> > I think choice is fine, as that's for testing. Once we're ready to merge
> > this we'll not keep both around for long.
> 
> Oh that's good. I heard people worrying about compatibility and size
> of the new shell, so thought we might need both.

Thank all of you for your feedback!

I think being able to change the shell at run time with "parser set" could 
also be useful for people to test if they are worried their board will not 
work with the new one.
So, they can test a feature with the old shell, change at runtime to the new 
one and see if everything is correct.
 
> Regards,
> Simon


Best regards.