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

[Sean Anderson]

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.

--Sean