[RFC PATCH 00/28] cli: Add a new shell
Sean Anderson
seanga2 at gmail.com
Fri Jul 2 16:07:54 CEST 2021
On 7/1/21 4:21 PM, Tom Rini wrote:
> On Thu, Jul 01, 2021 at 02:15:43AM -0400, Sean Anderson wrote:
>
>> Well, this has been sitting on my hard drive for too long without feedback
>> ("Release early, release often"), so here's the first RFC. This is not ready to
>> merge (see the "Future work" section below), but the shell is functional and at
>> least partially tested.
>>
>> The goal is to have 0 bytes gained over Hush. Currently we are around 800 bytes
>> over on sandbox.
>
> A good goal, but perhaps slightly too strict?
Perhaps. But I think getting in the ballpark will significantly help
drive adoption. I want to make it as easy as possible for maintainers to
enable LIL and start using it.
>
>>
>> add/remove: 90/54 grow/shrink: 3/7 up/down: 12834/-12042 (792)
>>
>> = Getting started
>>
>> Enable CONFIG_LIL. If you would like to run tests, enable CONFIG_LIL_FULL. Note
>> that dm_test_acpi_cmd_dump and setexpr_test_str_oper will fail. CONFIG_LIL_POOLS
>> is currently broken (with what appears to be a double free).
>>
>> For an overview of the language as a whole, refer to the original readme [1].
>>
>> [1] http://runtimeterror.com/tech/lil/readme.txt
>>
>> == Key patches
>>
>> The following patches are particularly significant for reviewing and
>> understanding this series:
>>
>> cli: Add LIL shell
>> This contains the LIL shell as originally written by Kostas with some
>> major deletions and some minor additions.
>> cli: lil: Wire up LIL to the rest of U-Boot
>> This allows you to use LIL as a shell just like Hush.
>> cli: lil: Document structures
>> This adds documentation for the major structures of LIL. It is a good
>> place to start looking at the internals.
>> test: Add tests for LIL
>> This adds some basic integration tests and provides some examples of
>> LIL code.
>> cli: lil: Add a distinct parsing step
>> This adds a parser separate from the interpreter. This patch is the
>> largest original work in this series.
>> cli: lil: Load procs from the environment
>> This allows procedures to be saved and loaded like variables.
>>
>> = A new shell
>>
>> This series adds a new shell for U-Boot. The aim is to eventually replace Hush
>> as the primary shell for all boards which currently use it. Hush should be
>> replaced because it has several major problems:
>>
>> - It has not had a major update in two decades, resulting in duplication of
>> effort in finding bugs. Regarding a bug in variable setting, Wolfgang remarks
>>
>> So the specific problem has (long) been fixed in upstream, and
>> instead of adding a patch to our old version, thus cementing the
>> broken behaviour, we should upgrade hush to recent upstream code.
>>
>> -- Wolfgang Denk [2]
>>
>> These lack of updates are further compounded by a significant amount of
>> ifdef-ing in the Hush code. This makes the shell hard to read and debug.
>> Further, the original purpose of such ifdef-ing (upgrading to a newer Hush)
>> has never happened.
>>
>> - It was designed for a preempting OS which supports pipes and processes. This
>> fundamentally does not match the computing model of U-Boot where there is
>> exactly one thread (and every other CPU is spinning or sleeping). Working
>> around these design differences is a significant cause of the aformentioned
>> ifdef-ing.
>>
>> - It lacks many major features expected of even the most basic shells, such
>> as functions and command substitution ($() syntax). This makes it difficult
>> to script with Hush. While it is desirable to write some code in C, much code
>> *must* be written in C because there is no way to express the logic in Hush.
>>
>> I believe that U-Boot should have a shell which is more featureful, has cleaner
>> code, and which is the same size as Hush (or less). The ergonomic advantages
>> afforded by a new shell will make U-Boot easier to use and customize.
>>
>> [2] https://lore.kernel.org/u-boot/872080.1614764732@gemini.denx.de/
>
> First, great! Thanks for doing this. A new shell really is the only
> viable path forward here, and I appreciate you taking the time to
> evaluate several and implement one.
>
>> = Open questions
>>
>> While the primary purpose of this series is of course to get feedback on the
>> code I have already written, there are several decisions where I am not sure
>> what the best course of action is.
>>
>> - What should be done about 'expr'? The 'expr' command is a significant portion
>> of the final code size. It cannot be removed outright, because it is used by
>> several builtin functions like 'if', 'while', 'for', etc. The way I see it,
>> there are two general approaches to take
>>
>> - Rewrite expr to parse expressions and then evaluate them. The parsing could
>> re-use several of the existing parse functions like how parse_list does.
>> This could reduce code, as instead of many functions each with their own
>> while/switch statements, we could have two while/switch statements (one to
>> parse, and one to evaluate). However, this may end up increasing code size
>> (such as when the main language had evaluation split from parsing).
>>
>> - Don't parse infix expressions, and just make arithmetic operators normal
>> functions. This would affect ergonomics a bit. For example, instead of
>>
>> if {$i < 10} { ... }
>>
>> one would need to write
>>
>> if {< $i 10} { ... }
>>
>> and instead of
>>
>> if {$some_bool} { ... }
>>
>> one would need to write
>>
>> if {quote $some_bool} { ... }
>>
>> Though, given how much setexpr is used (not much), this may not be such a
>> big price to pay. This route is almost certain to reduce code size.
>
> So, this is a question because we have cmd/setexpr.c that provides
> "expr" today? Or because this is a likely place to reclaim some of that
> 800 byte growth?
The latter. setexpr cannot be used because it does not return a result,
and instead sets a (global) variable. The expression parsing
functionality is core to LIL and used in many builtin commands (such as
`if` above), and really needs to return a lil_value.
>
>> - How should LIL functions integrate with the rest of U-Boot? At the moment, lil
>> functions and procedures exist in a completely separate world from normal
>> commands. I would like to integrate them more closely, but I am not sure the
>> best way to go about this. At the very minimum, each LIL builtin function
>> needs to get its hands on the LIL interpreter somehow. I'd rather this didn't
>> happen through gd_t or similar so that it is easier to unit test.
>> Additionally, LIL functions expect an array of lil_values instead of strings.
>> We could strip them out, but I worry that might start to impact performance
>> (from all the copying).
>
> I might be missing something here. But, given that whenever we have C
> code run-around and generate a string to then pass to the interpreter to
> run, someone asks why we don't just make API calls directly, perhaps the
> answer is that we don't need to?
err, the issue here is that the signature for regular commands is rougly
int cmd(..., int argc, char **argv, ...)
and the signature for LIL commands is
struct lil_value *cmd(struct lil *lil, size_t argc, struct lil_value **argv)
where lil_value is
struct lil_value {
size_t l;
char *d;
};
so while regular commands can be reimplemented as LIL commands (just
create a new argv containing the strings directly), it is more difficult
to go the other way. I bring this up because I think having two separate
ways to write a command is not the best way to do things going forward.
>>
>> The other half of this is adding LIL features into regular commands. The most
>> important feature here is being able to return a string result. I took an
>> initial crack at it [3], but I think with this series there is a stronger
>> motivating factor (along with things like [4]).
>>
>> [3] https://patchwork.ozlabs.org/project/uboot/list/?series=231377
>> [4] https://patchwork.ozlabs.org/project/uboot/list/?series=251013
>>
>> = Future work
>>
>> The series as presented today is incomplete. The following are the major issues
>> I see with it at the moment. I would like to address all of these issues, but
>> some of them might be postponed until after first merging this series.
>>
>> - There is a serious error handling problem. Most original LIL code never
>> checked errors. In almost every case, errors were silently ignored, even
>> malloc failures! While I have designed new code to handle errors properly,
>> there still remains a significant amount of original code which just ignores
>> errors. In particular, I would like to ensure that the following categories of
>> error conditions are handled:
>>
>> - Running out of memory.
>> - Access to a nonexistant variable.
>> - Passing the wrong number of arguments to a function.
>> - Interpreting a value as the wrong type (e.g. "foo" should not have a numeric
>> representation, instead of just being treated as 1).
>>
>> - There are many deviations from TCL with no purpose. For example, the list
>> indexing function is named "index" and not "lindex". It is perfectly fine to
>> drop features or change semantics to reduce code size, make parsing easier,
>> or make execution easier. But changing things for the sake of it should be
>> avoided.
>>
>> - The test suite is rather anemic compared with the amount of code this
>> series introduces. I would like to expand it significantly. In particular,
>> error conditions are not well tested (only the "happy path" is tested).
>>
>> - While I have documented all new functions I have written, there are many
>> existing functions which remain to be documented. In addition, there is no
>> user documentation, which is critical in driving adoption of any new
>> programming language. Some of this cover letter might be integrated with any
>> documentation written.
>>
>> - Some shell features such as command repetition and secondary shell prompts
>> have not been implemented.
>>
>> - Arguments to native lil functions are incompatible with U-Boot functions. For
>> example, the command
>>
>> foo bar baz
>>
>> would be passed to a U-Boot command as
>>
>> { "foo", "bar", "baz", NULL }
>>
>> but would be passed to a LIL function as
>>
>> { "bar", "baz" }
>>
>> This makes it more difficult to use the same function to parse several
>> different commands. At the moment this is solved by passing the command name
>> in lil->env->proc, but I would like to switch to the U-Boot argument list
>> style.
>>
>> - Several existing tests break when using LIL because they expect no output on
>> failure, but LIL produces some output notifying the user of the failure.
>>
>> - Implement DISTRO_BOOT in LIL. I think this is an important proof-of-concept to
>> show what can be done with LIL, and to determine which features should be
>> moved to LIL_FULL.
>>
>> = Why Lil?
>>
>> When looking for a suitable replacement shell, I evaluated implementations using
>> the following criteria:
>>
>> - It must have a GPLv2-compatible license.
>> - It must be written in C, and have no major external dependencies.
>> - It must support bare function calls. That is, a script such as 'foo bar'
>> should invoke the function 'foo' with the argument 'bar'. This preserves the
>> shell-like syntax we expect.
>> - It must be small. The eventual target is that it compiles to around 10KiB with
>> -Os and -ffunction-sections.
>> - There should be good tests. Any tests at all are good, but a functioning suite
>> is better.
>> - There should be good documentation
>> - There should be comments in the source.
>> - It should be "finished" or have only slow development. This will hopefully
>> make it easier to port changes.
>
> On this last point, I believe this is based on lil20190821 and current
> is now lil20210502. With a quick diff between them, I can see that the
> changes there are small enough that while you've introduced a number of
> changes here, it would be a very easy update.
From what I understand, the only changes are updated copyrights and the
addition of a license file to cover the tests.
--Sean
More information about the U-Boot
mailing list