From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2EB41C11F69 for ; Fri, 2 Jul 2021 14:08:07 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3BE6E6142B for ; Fri, 2 Jul 2021 14:08:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3BE6E6142B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id F04FB81ED0; Fri, 2 Jul 2021 16:08:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="BwwvNTTA"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6ED7681668; Fri, 2 Jul 2021 16:08:02 +0200 (CEST) Received: from mail-qt1-x82a.google.com (mail-qt1-x82a.google.com [IPv6:2607:f8b0:4864:20::82a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 157A281668 for ; Fri, 2 Jul 2021 16:07:58 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=seanga2@gmail.com Received: by mail-qt1-x82a.google.com with SMTP id v10so6659258qto.1 for ; Fri, 02 Jul 2021 07:07:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:subject:to:cc:references:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=WpgdwcDn2w4smGRKxccg/vluZqCba19EV0ktbDhnai0=; b=BwwvNTTA6WvAv7GO8gboZdATqnR98HXIviHcBkL414z9CuFgL7lXGTIPl5o1xa+jSG E9nzpxb70uWUkzVoE0A0ffUZNsykLXGXIvWl4uuMVhE8YVfnNCT0wCuoeGcXq5gbKAfB wJTDtm94aZMWSwFnIsLMd1955ojt1SxxqQBiAmw6eg8BlEjIR7oknzJXmlVFjyeNWUNS tz77n3vtQXHrYKJ/1mWN1xDVggOq4+p/sTFyB1nyzm8g1/ojD/95MIZhzvxsluPjem2H AjFZJkfsPo2paZHBLwj5DeK2XFiAK44uajkVXT6F52G8og+jmvEgEo/lp/X8qW9sf0qS ST3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=WpgdwcDn2w4smGRKxccg/vluZqCba19EV0ktbDhnai0=; b=VoecmEan2QghpPUL9jr8+InwNAZlVmueFSi0dNT79VI16IvRUVM9HAzmmwKMQJ7C3/ mkKk5fbzaUA9jirVU0FBZDXYGzdYvzeqEmqVV0US/5YPiIpD4dPm4uSpnSu9GVqgYwGn zE+fCvXIRGCouES6CYXdCBoeON80iKrN3Mndvd6lu3oOxP4WrwMsK3TWU5550yRtRrk/ 7zvlKVHtSa3vkopRF+r0+57brNMlqeM/5ERvcsCr0HZXEZ+qmI+1tTP7ORx0bUjpINGb GJlOahVFkk0zdj+Y+hFyoB8NSwGM7HPiSCRMzZZIP71Hdoexmn2174DCyDcgZhqTI0E4 bWcg== X-Gm-Message-State: AOAM531fh1gICTsW9lDUNzsyfTWa3Q16qOLtuLH5uD8/a88oRrudUa5J O8qaP95OqOKjkg2j7K6Syrc= X-Google-Smtp-Source: ABdhPJwA7m+k8WGmaF+896L7YU1QvbjWPENbltbegXg5kTkXMZTDvyIzIRt8qn9MNflp5KM7Rvp6wA== X-Received: by 2002:ac8:1008:: with SMTP id z8mr5153224qti.232.1625234876683; Fri, 02 Jul 2021 07:07:56 -0700 (PDT) Received: from [192.168.1.201] (pool-74-96-87-9.washdc.fios.verizon.net. [74.96.87.9]) by smtp.googlemail.com with ESMTPSA id u4sm1479398qkb.99.2021.07.02.07.07.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 02 Jul 2021 07:07:56 -0700 (PDT) From: Sean Anderson Subject: Re: [RFC PATCH 00/28] cli: Add a new shell To: Tom Rini Cc: u-boot@lists.denx.de, =?UTF-8?Q?Marek_Beh=c3=ban?= , Wolfgang Denk , Simon Glass , Roland Gaudig , Heinrich Schuchardt , Kostas Michalopoulos References: <20210701061611.957918-1-seanga2@gmail.com> <20210701202155.GQ9516@bill-the-cat> Message-ID: Date: Fri, 2 Jul 2021 10:07:54 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20210701202155.GQ9516@bill-the-cat> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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