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

[Sean Anderson <seanga2@gmail.com>]

On 7/5/21 11:29 AM, Simon Glass wrote:
> Hi,
> 
> On Mon, 5 Jul 2021 at 08:42, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 7/5/21 1:07 AM, Steve Bennett wrote:
>>> On 4 Jul 2021, at 5:26 am, Wolfgang Denk <wd@denx.de> wrote:
>>>>
>>>> Dear Sean,
>>>>
>>>> In message <d3a91238-db2f-edbe-ecec-ddb5dc848ed7@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.

I believe it will not be accepted, since strdup is POSIX and not C99.

--Sean