[RFC PATCH 03/28] cli: lil: Replace strclone with strdup

[Simon Glass]

Hi,

On Mon, 5 Jul 2021 at 08:42, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 7/5/21 1:07 AM, Steve Bennett wrote:
> > On 4 Jul 2021, at 5:26 am, Wolfgang Denk <wd at denx.de> wrote:
> >>
> >> Dear Sean,
> >>
> >> In message <d3a91238-db2f-edbe-ecec-ddb5dc848ed7 at gmail.com> you wrote:
> >>>
> >>> Well, since Hush was never updated, I don't believe LIL will be either.
> >>
> >> Let's please be exact here: Hus has never been updated _in_U-Boot_,
> >> but it has seen a lot of changes upstream, which apparently fix all
> >> the issues that motivated you to look for a replacement.
> >>
> >>> I think reducing the amount of ifdefs makes the code substantially
> >>> easier to maintain. My intention is to just use LIL as a starting point
> >>> which can be modified as needed to better suit U-Boot.
> >>>
> >>> The other half of this is that LIL is not particularly actively
> >>> developed. I believe the author sees his work as essentially
> >>> feature-complete, so I expect no major features which we might like to
> >>> backport.
> >>
> >> This sounds like an advantage, indeed, but then you can also
> >> interpret this as betting on a dead horse...
> >
> > My 2c on this.
> >
> > I am the maintainer of JimTcl (and I agree it is too big to be considered a candidate).
> > LIL source code has almost zero comments, poor error checking and no test suite.
>
> FWIW I added a (small) test suite in "[RFC PATCH 17/28] test: Add tests
> for LIL" based on the tests included in the LIL distibution. However, I
> really would like to expand upon it.
>
> > I would be very hesitant to adopt it in u-boot without serious work.
>
> I think around half of the "serious work" has already been done. I have
> worked on most of the core of LIL, and added error handling and
> comments. I believe that most of the remaining instances of dropping
> errors lie in the built-in commands.
>
> > I would much rather see effort put into updating hush to upstream.
>
> AIUI hush has diverged significantly from what U-Boot has. This would
> not be an "update" moreso than a complete port in the style of the
> current series.
>
> > My guess is that Denys would be amenable to small changes to make it easier to synchronise
> > with busybox in the future.
>
> I don't think sh-style shells are a good match for U-Boot's execution
> environment in the first place. The fundamental idea of an sh-style
> shell is that the output of one command can be redirected to the input
> (or arguments) of another command. This cannot be done (or rather would
> be difficult to do) in U-Boot for a few reasons
>
> * U-Boot does not support multithreading. Existing shells tend to depend
>    strongly on this feature of the enviromnent. Many of the changes to
>    U-Boot's hush are solely to deal with the lack of this feature.
>
> * Existing commands do not read from stdin, nor do they print useful
>    information to stdout. Command output is designed for human
>    consumption and is substantially more verbose than typical unix
>    commands.
>
> * Tools such as grep, cut, tr, sed, sort, uniq, etc. which are extremely
>    useful when working with streams are not present in U-Boot.
>
> And of course, this feature is currently not present in U-Boot. To get
> around this, commands resort to two of my least-favorite hacks: passing
> in the name of a environmental variable and overloading the return
> value. For an example of the first, consider
>
>         => part uuid mmc 0:1 my_uuid
>
> which will set my_uuid to the uuid of the selected partition. My issue
> with this is threefold: every command must add new syntax to do this,
> that syntax is inconsistent, and it prevents easy composition. Consider
> a script which wants to iterate over partitions. Instead of doing
>
>         for p in $(part list mmc 0); do
>                 # ...
>         done
>
> it must instead do
>
>         part list mmc 0 partitions
>         for p in $partitions; do
>                 # ...
>         done
>
> which unnecessarily adds an extra step. This overhead accumulates with
> each command which adds something like this.
>
> The other way to return more information is to use the return value.
> Consider the button command; it currently returns
>
>         0       ON, the button is pressed
>         1       OFF, the button is released
>         0       button list was shown
>         1       button not found
>         1       invalid arguments
>
> and so there is no way to distinguish between whether the button is off,
> whether the button does not exist, or whether there was a problem with
> the button driver.
>
> Both of these workarounds are natural consequences of using a sh-tyle
> shell in an environment it is not suited for. If we are going to go to
> the effort of porting a new language (which must be done no matter if
> we use Hush or some other language), we should pick one which has better
> support for single-threaded programming.

I think these are good points, particularly the thing about
mutiltasking. In fact I think it would be better if hush could
implement things like '| grep xxx' since it would be useful in U-Boot.

But in any case, adding lil does not preclude someone coming along and
adaptive hush for U-Boot (and this time upstreaming the changes!). I
have seen discussions about updating hush for several years and no one
has done it. I took a quick look and found it was about 3x the size it
used to be and was not even sure where to start in terms of adapting
it for single-threaded use.

Re this patch, I think it should be sent upstream.

Regards,
Simon