[PATCH 0/3] add "call" command

[PATCH 0/3] add "call" command

[Rasmus Villemoes]

This adds a way to call a "function" defined in the environment with
arguments. I.e., whereas

  run foo

requires one to set the (shell or environment) variables referenced
from foo beforehand, with this one can instead do

  call foo arg1 arg2 arg3

and use $1... up to $9 in the definition of foo. $# is set so foo can
make decisions based on that, and ${3:-default} works as expected.

As I write in patch 2, it should be possible to get rid of the "call"
and simply allow

  foo arg1 arg2 arg3

i.e. if the search for a command named foo fails, try an environment
variable by that name and do it as "call". But that change of
behaviour, I think, requires a separate opt-in config knob, and can be
done later if someone actually wants that.

Rasmus Villemoes (3):
  cli_hush.c: refactor handle_dollar() to prepare for cmd_call
  cli_hush.c: add "call" command
  ut: add small hush tests

 cmd/Kconfig                 |  8 ++++
 common/cli_hush.c           | 93 +++++++++++++++++++++++++++++++++----
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 include/test/suites.h       |  1 +
 test/cmd/Makefile           |  1 +
 test/cmd/hush.c             | 90 +++++++++++++++++++++++++++++++++++
 test/cmd_ut.c               |  6 +++
 8 files changed, 191 insertions(+), 10 deletions(-)
 create mode 100644 test/cmd/hush.c

-- 
2.23.0

[PATCH 1/3] cli_hush.c: refactor handle_dollar() to prepare for cmd_call

[Rasmus Villemoes]

A later patch will add handling of $1 through $9 as well as $#, using
the same SPECIAL_VAR_SYMBOL handling as is currently used for $?. So
move that case to an explicit #ifdef __U_BOOT__ branch, and
consolidate a few of the #ifndef __U_BOOT__ cases, making it easier to
see the original hush code.

No functional change.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/cli_hush.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/common/cli_hush.c b/common/cli_hush.c
index 5b1f119074..072b871f1e 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -2863,6 +2863,16 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i
 		advance = 1;
 #endif
 	} else switch (ch) {
+#ifdef __U_BOOT__
+		case '?':
+			ctx->child->sp++;
+			b_addchr(dest, SPECIAL_VAR_SYMBOL);
+			b_addchr(dest, '$');
+			b_addchr(dest, '?');
+			b_addchr(dest, SPECIAL_VAR_SYMBOL);
+			advance = 1;
+			break;
+#endif
 #ifndef __U_BOOT__
 		case '$':
 			b_adduint(dest,getpid());
@@ -2872,20 +2882,10 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i
 			if (last_bg_pid > 0) b_adduint(dest, last_bg_pid);
 			advance = 1;
 			break;
-#endif
 		case '?':
-#ifndef __U_BOOT__
 			b_adduint(dest,last_return_code);
-#else
-			ctx->child->sp++;
-			b_addchr(dest, SPECIAL_VAR_SYMBOL);
-			b_addchr(dest, '$');
-			b_addchr(dest, '?');
-			b_addchr(dest, SPECIAL_VAR_SYMBOL);
-#endif
 			advance = 1;
 			break;
-#ifndef __U_BOOT__
 		case '#':
 			b_adduint(dest,global_argc ? global_argc-1 : 0);
 			advance = 1;
-- 
2.23.0

[PATCH 2/3] cli_hush.c: add "call" command

[Rasmus Villemoes]

Currently, the only way to emulate functions with arguments in the
busybox shell is by doing "foo=arg1; bar=arg2; run func" and having
"func" refer to $foo and $bar. That works, but is a bit clunky, and
also suffers from foo and bar being set globally - if func itself wants
to run other "functions" defined in the environment, those other
functions better not use the same parameter names:

  setenv g 'do_g_stuff $foo'
  setenv f 'do_f_stuff $foo $bar; foo=123; run g; do_more_f_stuff $foo $bar'

Sure, f could do a "saved_foo=$foo; .... foo=$saved_foo" dance, but
that makes everything even more clunky.

In order to increase readability, add a little helper "call" that is
like "run", but which sets local shell variables $1 through
$9 (and $#). As in a "real" shell, they are local to the current
function, so if f is called with two arguments, and f calls g with one
argument, g sees $2 as unset. Then the above can be written

  setenv g 'do_g_stuff $1'
  setenv f 'do_f_stuff $1 $2; call g 123; do_more_f_stuff $1 $2'

Everything except

-                       b_addchr(dest, '?');
+                       b_addchr(dest, ch);

is under CONFIG_CMD_CALL, and when CONFIG_CMD_CALL=n, the ch there can
only be '?'. So no functional change when CONFIG_CMD_CALL is not
selected.

"Real shells" have special syntax for defining a function, but calling
a function is the same as calling builtins or external commands. So
the "call" may admittedly be seen as a bit of a kludge. It
should be rather easy to make custom (i.e., defined in the
environment) functions "transparently callable" on top of this
infrastructure, i.e. so one could just say

  f a b c

instead of

  call f a b c

However, that behaviour should be controlled by a separate config
knob, and can be added later if anyone actually wants it.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig       |  8 +++++
 common/cli_hush.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 0c984d735d..306f115c32 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -443,6 +443,14 @@ config CMD_RUN
 	help
 	  Run the command in the given environment variable.
 
+config CMD_CALL
+	bool "call"
+	depends on HUSH_PARSER
+	depends on CMD_RUN
+	help
+	  Call function defined in environment variable, setting
+	  positional arguments $1..$9.
+
 config CMD_IMI
 	bool "iminfo"
 	default y
diff --git a/common/cli_hush.c b/common/cli_hush.c
index 072b871f1e..e17fba99ee 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -135,6 +135,17 @@ DECLARE_GLOBAL_DATA_PTR;
 #define syntax() syntax_err()
 #define xstrdup strdup
 #define error_msg printf
+
+#ifdef CONFIG_CMD_CALL
+#define MAX_CALL_ARGS 9
+struct call_args {
+	struct call_args *prev;
+	int count;
+	char *args[MAX_CALL_ARGS]; /* [0] holds $1 etc. */
+};
+static struct call_args *current_call_args;
+#endif
+
 #else
 typedef enum {
 	REDIRECT_INPUT     = 1,
@@ -2144,6 +2155,10 @@ char *get_local_var(const char *s)
 #ifdef __U_BOOT__
 	if (*s == '$')
 		return get_dollar_var(s[1]);
+	/* To make ${1:-default} work: */
+	if (IS_ENABLED(CONFIG_CMD_CALL) &&
+	    '1' <= s[0] && s[0] <= '9' && !s[1])
+		return get_dollar_var(s[0]);
 #endif
 
 	for (cur = top_vars; cur; cur=cur->next)
@@ -2826,6 +2841,23 @@ static char *get_dollar_var(char ch)
 		case '?':
 			sprintf(buf, "%u", (unsigned int)last_return_code);
 			break;
+#ifdef CONFIG_CMD_CALL
+		case '#':
+			if (!current_call_args)
+				return NULL;
+			sprintf(buf, "%u", current_call_args->count);
+			break;
+		case '1' ... '9': {
+			const struct call_args *ca = current_call_args;
+			int i = ch - '1';
+
+			if (!ca)
+				return NULL;
+			if (i >= ca->count)
+				return NULL;
+			return ca->args[i];
+		}
+#endif
 		default:
 			return NULL;
 	}
@@ -2865,10 +2897,14 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i
 	} else switch (ch) {
 #ifdef __U_BOOT__
 		case '?':
+#ifdef CONFIG_CMD_CALL
+		case '1' ... '9':
+		case '#':
+#endif
 			ctx->child->sp++;
 			b_addchr(dest, SPECIAL_VAR_SYMBOL);
 			b_addchr(dest, '$');
-			b_addchr(dest, '?');
+			b_addchr(dest, ch);
 			b_addchr(dest, SPECIAL_VAR_SYMBOL);
 			advance = 1;
 			break;
@@ -3711,5 +3747,42 @@ U_BOOT_CMD(
 	"    - print value of hushshell variable 'name'"
 );
 
+#ifdef CONFIG_CMD_CALL
+static int do_cmd_call(struct cmd_tbl *cmdtp, int flag, int argc,
+		      char *const argv[])
+{
+	struct call_args ca;
+	char *run_args[2];
+	int i, ret;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	ca.count = argc - 2;
+	for (i = 2; i < argc; ++i)
+		ca.args[i - 2] = argv[i];
+	ca.prev = current_call_args;
+	current_call_args = &ca;
+
+	run_args[0] = "run";
+	run_args[1] = argv[1];
+	ret = do_run(cmdtp, flag, 2, run_args);
+
+	current_call_args = ca.prev;
+
+	return ret;
+}
+
+U_BOOT_CMD_COMPLETE(
+	call, 1 + 1 + MAX_CALL_ARGS, 0, do_cmd_call,
+	"call command in environment variable, setting positional arguments $1..$9",
+        "var [args...]\n"
+        "    - run the command(s) in the environment variable 'var',\n"
+	"      with $1..$9 set to the positional arguments",
+	var_complete
+);
+
+#endif
+
 #endif
 /****************************************************************************/
-- 
2.23.0

[PATCH 3/3] ut: add small hush tests

[Rasmus Villemoes]

This is primarily to add a test of the new "call" command, but we
might as well add some very basic tests as well.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 include/test/suites.h       |  1 +
 test/cmd/Makefile           |  1 +
 test/cmd/hush.c             | 90 +++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c               |  6 +++
 6 files changed, 100 insertions(+)
 create mode 100644 test/cmd/hush.c

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index c3ca796d51..7e156e9813 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -25,6 +25,7 @@ CONFIG_CMD_LICENSE=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_BOOTEFI_HELLO=y
 # CONFIG_CMD_ELF is not set
+CONFIG_CMD_CALL=y
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
 CONFIG_CMD_ERASEENV=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 6e9f029cc9..ac06a8dc67 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -30,6 +30,7 @@ CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_BOOTEFI_HELLO=y
 CONFIG_CMD_ABOOTIMG=y
 # CONFIG_CMD_ELF is not set
+CONFIG_CMD_CALL=y
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
 CONFIG_CMD_ERASEENV=y
diff --git a/include/test/suites.h b/include/test/suites.h
index ab7b3bd9ca..082b87ce52 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -32,6 +32,7 @@ int do_ut_compression(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[]);
 int do_ut_dm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
+int do_ut_hush(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_lib(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_log(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_mem(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 859dcda239..adc5ba0307 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -4,3 +4,4 @@
 
 obj-y += mem.o
 obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
+obj-$(CONFIG_HUSH_PARSER) += hush.o
diff --git a/test/cmd/hush.c b/test/cmd/hush.c
new file mode 100644
index 0000000000..c4ad7b5e94
--- /dev/null
+++ b/test/cmd/hush.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <common.h>
+#include <console.h>
+#include <test/suites.h>
+#include <test/ut.h>
+
+#define HUSH_TEST(_name, _flags) UNIT_TEST(_name, _flags, hush_test)
+
+int do_ut_hush(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+{
+	struct unit_test *tests = ll_entry_start(struct unit_test, hush_test);
+	const int n_ents = ll_entry_count(struct unit_test, hush_test);
+
+	return cmd_ut_category("cmd_hush", "cmd_hush_", tests, n_ents, argc,
+			       argv);
+}
+
+static int hush_test_basic(struct unit_test_state *uts)
+{
+	run_command("true", 0);
+	ut_assert_console_end();
+
+	run_command("echo $?", 0);
+	ut_assert_nextline("0");
+	ut_assert_console_end();
+
+	run_command("false", 0);
+	ut_assert_console_end();
+
+	run_command("echo $?", 0);
+	ut_assert_nextline("1");
+	ut_assert_console_end();
+
+	return 0;
+}
+HUSH_TEST(hush_test_basic, UT_TESTF_CONSOLE_REC);
+
+static int hush_test_cond(struct unit_test_state *uts)
+{
+	run_command("if true ; then echo yay ; else echo nay ; fi", 0);
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if false ; then echo yay ; else echo nay ; fi", 0);
+	ut_assert_nextline("nay");
+	ut_assert_console_end();
+
+	/* Short-circuiting */
+	run_command("if true || echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if false || echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("x");
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if true && echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("x");
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if false && echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("nay");
+	ut_assert_console_end();
+
+	return 0;
+}
+HUSH_TEST(hush_test_cond, UT_TESTF_CONSOLE_REC);
+
+#ifdef CONFIG_CMD_CALL
+static int hush_test_call(struct unit_test_state *uts)
+{
+	run_command("env set f 'echo f: argc=$#, [$1] [${2}] [${3:-z}]; call g $2; echo f: [$1] [${2}]'", 0);
+	ut_assert_console_end();
+
+	run_command("env set g 'echo g: argc=$#, [$1] [$2] [${2:-y}]'", 0);
+	ut_assert_console_end();
+
+	run_command("call f 11 22", 0);
+	ut_assert_nextline("f: argc=2, [11] [22] [z]");
+	ut_assert_nextline("g: argc=1, [22] [] [y]");
+	ut_assert_nextline("f: [11] [22]");
+	ut_assert_console_end();
+
+	return 0;
+}
+HUSH_TEST(hush_test_call, UT_TESTF_CONSOLE_REC);
+#endif
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index 8f0bc688a2..cd903186b9 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -81,6 +81,9 @@ static struct cmd_tbl cmd_ut_sub[] = {
 #if CONFIG_IS_ENABLED(UT_UNICODE) && !defined(API_BUILD)
 	U_BOOT_CMD_MKENT(unicode, CONFIG_SYS_MAXARGS, 1, do_ut_unicode, "", ""),
 #endif
+#ifdef CONFIG_HUSH_PARSER
+	U_BOOT_CMD_MKENT(hush, CONFIG_SYS_MAXARGS, 1, do_ut_hush, "", ""),
+#endif
 #ifdef CONFIG_SANDBOX
 	U_BOOT_CMD_MKENT(compression, CONFIG_SYS_MAXARGS, 1, do_ut_compression,
 			 "", ""),
@@ -140,6 +143,9 @@ static char ut_help_text[] =
 #ifdef CONFIG_UT_ENV
 	"ut env [test-name]\n"
 #endif
+#ifdef CONFIG_HUSH_PARSER
+	"ut hush - test (hush) shell functionality\n"
+#endif
 #ifdef CONFIG_UT_LIB
 	"ut lib [test-name] - test library functions\n"
 #endif
-- 
2.23.0

[PATCH 0/3] add "call" command

[Heinrich Schuchardt]

On 25.09.20 13:19, Rasmus Villemoes wrote:
> This adds a way to call a "function" defined in the environment with
> arguments. I.e., whereas
>
>   run foo
>
> requires one to set the (shell or environment) variables referenced
> from foo beforehand, with this one can instead do
>
>   call foo arg1 arg2 arg3
>
> and use $1... up to $9 in the definition of foo. $# is set so foo can
> make decisions based on that, and ${3:-default} works as expected.
>
> As I write in patch 2, it should be possible to get rid of the "call"
> and simply allow
>
>   foo arg1 arg2 arg3
>
> i.e. if the search for a command named foo fails, try an environment
> variable by that name and do it as "call". But that change of
> behaviour, I think, requires a separate opt-in config knob, and can be
> done later if someone actually wants that.

If the behavior is configurable, this will result in users complaining
that a script which they copied does not run on another machine. Please,
do not introduce any configurability here.

Further we cannot first introduce a command call and then eliminate it
due to backward compatibility. We should decide on the final version
beforehand.

In the Linux world you can override a command using an alias. So I am
not sure if a built in command should take precedence over a variable of
the same name or the other way round.

Consider that we already have two types of variables in the shell. Those
that are in the environment and those that are not. Here the environment
variables take precedence.

Try

    setenv a;a=4;echo $a;setenv a 5;echo $a;setenv a;echo $a

Best regards

Heinrich

>
> Rasmus Villemoes (3):
>   cli_hush.c: refactor handle_dollar() to prepare for cmd_call
>   cli_hush.c: add "call" command
>   ut: add small hush tests
>
>  cmd/Kconfig                 |  8 ++++
>  common/cli_hush.c           | 93 +++++++++++++++++++++++++++++++++----
>  configs/sandbox64_defconfig |  1 +
>  configs/sandbox_defconfig   |  1 +
>  include/test/suites.h       |  1 +
>  test/cmd/Makefile           |  1 +
>  test/cmd/hush.c             | 90 +++++++++++++++++++++++++++++++++++
>  test/cmd_ut.c               |  6 +++
>  8 files changed, 191 insertions(+), 10 deletions(-)
>  create mode 100644 test/cmd/hush.c
>

[PATCH 0/3] add "call" command

[Rasmus Villemoes]

On 25/09/2020 13.52, Heinrich Schuchardt wrote:
> On 25.09.20 13:19, Rasmus Villemoes wrote:
>> This adds a way to call a "function" defined in the environment with
>> arguments. I.e., whereas
>>
>>   run foo
>>
>> requires one to set the (shell or environment) variables referenced
>> from foo beforehand, with this one can instead do
>>
>>   call foo arg1 arg2 arg3
>>
>> and use $1... up to $9 in the definition of foo. $# is set so foo can
>> make decisions based on that, and ${3:-default} works as expected.
>>
>> As I write in patch 2, it should be possible to get rid of the "call"
>> and simply allow
>>
>>   foo arg1 arg2 arg3
>>
>> i.e. if the search for a command named foo fails, try an environment
>> variable by that name and do it as "call". But that change of
>> behaviour, I think, requires a separate opt-in config knob, and can be
>> done later if someone actually wants that.
> 
> If the behavior is configurable, this will result in users complaining
> that a script which they copied does not run on another machine. Please,
> do not introduce any configurability here.

OK, but I'm actually not intending to add that functionality at all, I
was merely mentioning it if somebody would like it. But note that the
same argument can be used for any script that uses any command (or other
functionality) which is config-dependent - if I copy some snippet that
uses setexpr, that won't work on another machine that doesn't have
CONFIG_CMD_SETEXPR.

> Further we cannot first introduce a command call and then eliminate it
> due to backward compatibility. We should decide on the final version
> beforehand.
> 
> In the Linux world you can override a command using an alias. So I am
> not sure if a built in command should take precedence over a variable of
> the same name or the other way round.

POSIX(-like) shells have "command":

  The command utility shall cause the shell to treat the arguments as a
simple command, suppressing the shell function lookup

So I'd be inclined to make the normal commands have precedence, given
that there's a "call " that can be used to invoke the "function" variant
rather than the "builtin command" variant. But it's all moot if nobody
actually wants to be able to avoid the "call ".

> Consider that we already have two types of variables in the shell. Those
> that are in the environment and those that are not. Here the environment
> variables take precedence.
> 
> Try
> 
>     setenv a;a=4;echo $a;setenv a 5;echo $a;setenv a;echo $a

I know. So if you do 'env set 1 haha"', ${1} will not work (though I
think bare $1 will) in called functions. But nobody sets such
environment variables.

While I was trying to figure out how to implement this, I stumbled on
some code that tries to prevent the above (or rather, the converse:
setting a shell variable when an env variable of that name exists). But
that code is buggy and hence dead because it does the lookup on the
whole "a=4" string, before splitting on =. So currently we get

=> env set foo abc
=> foo=x
=>

moving the check:

--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -2185,14 +2185,6 @@ int set_local_var(const char *s, int flg_export)

        name=strdup(s);

-#ifdef __U_BOOT__
-       if (env_get(name) != NULL) {
-               printf ("ERROR: "
-                               "There is a global environment variable
with the same name.\n");
-               free(name);
-               return -1;
-       }
-#endif
        /* Assume when we enter this function that we are already in
         * NAME=VALUE format.  So the first order of business is to
         * split 's' on the '=' into 'name' and 'value' */
@@ -2203,6 +2195,15 @@ int set_local_var(const char *s, int flg_export)
        }
        *value++ = 0;

+#ifdef __U_BOOT__
+       if (env_get(name) != NULL) {
+               printf ("ERROR: "
+                               "There is a global environment variable
with the same name.\n");
+               free(name);
+               return -1;
+       }
+#endif
+

we get

=> env set foo abc
=> foo=x
ERROR: There is a global environment variable with the same name.
=>

But that code is ancient, so I don't know if it should be fixed to work
as clearly intended, or one should just delete it to remove a few bytes
of dead .text. In any case it does nothing to prevent setting an env
variable with the same name as an existing shell variable.

Rasmus

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Rasmus,

In message <20200925111942.4629-1-rasmus.villemoes@prevas.dk> you wrote:
> This adds a way to call a "function" defined in the environment with
> arguments. I.e., whereas
>
>   run foo
>
> requires one to set the (shell or environment) variables referenced
> from foo beforehand, with this one can instead do
>
>   call foo arg1 arg2 arg3
>
> and use $1... up to $9 in the definition of foo. $# is set so foo can
> make decisions based on that, and ${3:-default} works as expected.

This is definitely a useful idea.

But...

...the current version of hush in U-Boot is old, has a number of
known bugs and shortcomings, and I really recommend not to adding
any new features to it, because that would makie an update to a more
recent version even less likely.

So the first step before such extensions should be to update hush.
In that process (which might be more of a new port) one should
consider the possibility of keeping a little more of the
functionality - memory restrictins today are not so strict any more
as they were when hush was originally added.  One feature that would
definitely be useful is command substitution.

All this needs a bit of a long term maintainable concept.  Quick
hacking of the ancient code is not a good idea.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
God runs electromagnetics by wave theory on  Monday,  Wednesday,  and
Friday,  and the Devil runs them by quantum theory on Tuesday, Thurs-
day, and Saturday.                                   -- William Bragg

[PATCH 1/3] cli_hush.c: refactor handle_dollar() to prepare for cmd_call

[Wolfgang Denk]

Dear Rasmus,

In message <20200925111942.4629-2-rasmus.villemoes@prevas.dk> you wrote:
> A later patch will add handling of $1 through $9 as well as $#, using
> the same SPECIAL_VAR_SYMBOL handling as is currently used for $?. So
> move that case to an explicit #ifdef __U_BOOT__ branch, and
> consolidate a few of the #ifndef __U_BOOT__ cases, making it easier to
> see the original hush code.

I won't comment on these and the other patches - you know my
opinion: instead of hacking the current code, we should 1) come up
with a plan and 2) update.

Please consider this a soft-NAK ;-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The good Christian should beware of mathematicians and all those who
make empty prophecies. The danger already exists that  mathematicians
have  made a covenant with the devil to darken the spirit and confine
man in the bonds of Hell."                          - Saint Augustine

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Heinrich Schuchardt,

In message <4b00225d-d960-4a14-9aec-110ddddf7f30@gmx.de> you wrote:
>
> Further we cannot first introduce a command call and then eliminate it
> due to backward compatibility. We should decide on the final version
> beforehand.

Full agreement.  we need a concept of what is needed / wanted first.
And then we should look how current vrsions of hush fit into this.

> In the Linux world you can override a command using an alias. So I am
> not sure if a built in command should take precedence over a variable of
> the same name or the other way round.

This is simple. The PoLA (Principle of Least Astonishment) applies
here.  Behaviour must be the same as in other (to some extent POSIX
compatible) shells.  A shell should parse it's input, not adhoculate
it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Ada is PL/I trying to be Smalltalk.                 - Codoso diBlini

[PATCH 2/3] cli_hush.c: add "call" command

[Rasmus Villemoes]

On 25/09/2020 13.19, Rasmus Villemoes wrote:
> Currently, the only way to emulate functions with arguments in the
> busybox shell is by doing "foo=arg1; bar=arg2; run func" and having

I have no idea why I always end up writing "busybox shell" when I mean
"U-Boot shell". Sorry. I hope the meaning is clear anyway.

Rasmus

[PATCH 0/3] add "call" command

[Rasmus Villemoes]

On 25/09/2020 15.09, Wolfgang Denk wrote:
> Dear Heinrich Schuchardt,
> 
> In message <4b00225d-d960-4a14-9aec-110ddddf7f30@gmx.de> you wrote:
>>
>> Further we cannot first introduce a command call and then eliminate it
>> due to backward compatibility. We should decide on the final version
>> beforehand.

Please note that I never meant for

  f a b c

to be an eventual replacement for

  call f a b c

the latter syntax would continue to be accepted. And I'm personally
completely fine with that being the _only_ way to call a
function-defined-in-the-environment-with-positional-args, which also makes

>> I am
>> not sure if a built in command should take precedence over a variable of
>> the same name or the other way round.

a moot point.

So can we instead discuss whether the "call" command is worth having at
all. I notice that Wolfgang calls this a nice idea (thanks), but
soft-NAKs it because he'd rather see all of hush updated before adding
extra features. The thing is, I can't take that monumental task on me
(especially with all the backwards-compatibility concerns there'd be,
with various scripts in the wild that may have come to rely on U-Boot's
hush parser's behaviour in corner cases), but doing cmd_call is about
100 lines of mostly stand-alone code.

Rasmus

[PATCH 0/3] add "call" command

[Heinrich Schuchardt]

On 25.09.20 15:09, Wolfgang Denk wrote:
> Dear Heinrich Schuchardt,
>
> In message <4b00225d-d960-4a14-9aec-110ddddf7f30@gmx.de> you wrote:
>>
>> Further we cannot first introduce a command call and then eliminate it
>> due to backward compatibility. We should decide on the final version
>> beforehand.
>
> Full agreement.  we need a concept of what is needed / wanted first.
> And then we should look how current vrsions of hush fit into this.
>
>> In the Linux world you can override a command using an alias. So I am
>> not sure if a built in command should take precedence over a variable of
>> the same name or the other way round.
>
> This is simple. The PoLA (Principle of Least Astonishment) applies
> here.  Behaviour must be the same as in other (to some extent POSIX
> compatible) shells.  A shell should parse it's input, not adhoculate
> it.

For me this could be realized by enhancing the run command to allow:

run varname1 varname2 ... varnameN --args argv1 argv2 argv3

Arguments argv1, argv2, ... are passed to the script identified by the
last variable (varnameN).

No new command to learn. Just a new option.

Best regards

Heinrich

[PATCH 0/3] add "call" command

[Rasmus Villemoes]

On 25/09/2020 15.38, Heinrich Schuchardt wrote:
> On 25.09.20 15:09, Wolfgang Denk wrote:
>> Dear Heinrich Schuchardt,
>>
>> In message <4b00225d-d960-4a14-9aec-110ddddf7f30@gmx.de> you wrote:
>>>
>>> Further we cannot first introduce a command call and then eliminate it
>>> due to backward compatibility. We should decide on the final version
>>> beforehand.
>>
>> Full agreement.  we need a concept of what is needed / wanted first.
>> And then we should look how current vrsions of hush fit into this.
>>
>>> In the Linux world you can override a command using an alias. So I am
>>> not sure if a built in command should take precedence over a variable of
>>> the same name or the other way round.
>>
>> This is simple. The PoLA (Principle of Least Astonishment) applies
>> here.  Behaviour must be the same as in other (to some extent POSIX
>> compatible) shells.  A shell should parse it's input, not adhoculate
>> it.
> 
> For me this could be realized by enhancing the run command to allow:
> 
> run varname1 varname2 ... varnameN --args argv1 argv2 argv3
> 
> Arguments argv1, argv2, ... are passed to the script identified by the
> last variable (varnameN).
> 
> No new command to learn. Just a new option.

Yes, this is really more to be thought of as a "run_with_args" command
than an extension of hush (though the $1 treatment does need to hook
into the hush code, which is both why I made it dependent on HUSH_PARSER
and made it live in cli_hush.c). I'm certainly open to extending the
existing run command instead of creating a new "toplevel" command.
Though I'd probably make it

  run varname -- arg1 arg2 arg3

instead: Just use -- as a separator [that has precedent as "stop doing
X, use the rest as argv", though X is normally "interpret options" and
now it would be "read function names to run"], and only allow a single
"function" to be called. Otherwise, I don't there's any natural answer
to whether all the varnameX or only the last should be called with the
positional arguments. It's pretty simple to do "for x in v1 v2 v3; do
run $x -- arg1 arg2 arg3 ; done if one has a bunch of functions that
should be called in turn, and it's even more simple to do

  run varname1 varname2 varname{N-1}
  run varnameN -- arg1 arg2 arg3

if one has a bunch of parameter-less functions to call before varnameN.

Rasmus

Rasmus

[PATCH 0/3] add "call" command

[Simon Glass]

Hi,

On Fri, 25 Sep 2020 at 06:59, Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Rasmus,
>
> In message <20200925111942.4629-1-rasmus.villemoes@prevas.dk> you wrote:
> > This adds a way to call a "function" defined in the environment with
> > arguments. I.e., whereas
> >
> >   run foo
> >
> > requires one to set the (shell or environment) variables referenced
> > from foo beforehand, with this one can instead do
> >
> >   call foo arg1 arg2 arg3
> >
> > and use $1... up to $9 in the definition of foo. $# is set so foo can
> > make decisions based on that, and ${3:-default} works as expected.
>
> This is definitely a useful idea.
>
> But...
>
> ...the current version of hush in U-Boot is old, has a number of
> known bugs and shortcomings, and I really recommend not to adding
> any new features to it, because that would makie an update to a more
> recent version even less likely.

I would like to see this also. Is the busybox version the latest?
There might even be some tests although I can't see any.

One concern is that it seems to be 3x the line count. Hopefully that
doesn't result in 3x the code size. It also seems to have developed an
even more severe case of #ifdef disease.

>
> So the first step before such extensions should be to update hush.
> In that process (which might be more of a new port) one should
> consider the possibility of keeping a little more of the
> functionality - memory restrictins today are not so strict any more
> as they were when hush was originally added.  One feature that would
> definitely be useful is command substitution.
>
> All this needs a bit of a long term maintainable concept.  Quick
> hacking of the ancient code is not a good idea.
>

Regards,
Simon

[PATCH 2/3] cli_hush.c: add "call" command

[Wolfgang Denk]

Dear Rasmus,

In message <dadb16c9-1943-67d7-c4e4-fefc56ddd7f1@prevas.dk> you wrote:
> On 25/09/2020 13.19, Rasmus Villemoes wrote:
> > Currently, the only way to emulate functions with arguments in the
> > busybox shell is by doing "foo=arg1; bar=arg2; run func" and having
>
> I have no idea why I always end up writing "busybox shell" when I mean
> "U-Boot shell". Sorry. I hope the meaning is clear anyway.

U-Boot copied the code from BusyBox, where the hush shell originated
AFAIK.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What is tolerance? -- it is the consequence of humanity. We  are  all
formed  of frailty and error; let us pardon reciprocally each other's
folly -- that is the first law of nature.                  - Voltaire

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Heinrich,

In message <99821acf-b921-2c06-05b8-dd32058f28f2@gmx.de> you wrote:
>
> For me this could be realized by enhancing the run command to allow:
>
> run varname1 varname2 ... varnameN --args argv1 argv2 argv3
>
> Arguments argv1, argv2, ... are passed to the script identified by the
> last variable (varnameN).

Nice idea!  Only we should do a better syntax (options preceeding
argument), i. e.

	run [ --args argv ] varname1 varname2 ... 

where argv would be the name of a variale to hold the arguments (as
a comma (?) separated list) ?

Do you have an idea how the "script" would pull out the arguments
from that variable?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What was sliced bread the greatest thing since?

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Rasmus,

In message <823e5d31-7022-346e-b3a3-e36a4a295764@prevas.dk> you wrote:
>
> Though I'd probably make it
>
>   run varname -- arg1 arg2 arg3

Or rather

	run arg1 arg2 ... -- varname1 varname2 ...


> instead: Just use -- as a separator [that has precedent as "stop doing
> X, use the rest as argv", though X is normally "interpret options" and
> now it would be "read function names to run"], and only allow a single
> "function" to be called. Otherwise, I don't there's any natural answer
> to whether all the varnameX or only the last should be called with the
> positional arguments.

"run" has always taken any number of vaiable names to run, and this
behaviour should be kept.  If there are arguments, these are the
same for all of these.  If you need indivisual argument settings,
you must break the sommand in parts combined with &&.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
(null cookie; hope that's ok)

[PATCH 0/3] add "call" command

[Heinrich Schuchardt]

On 9/26/20 10:51 AM, Wolfgang Denk wrote:
> Dear Heinrich,
>
> In message <99821acf-b921-2c06-05b8-dd32058f28f2@gmx.de> you wrote:
>>
>> For me this could be realized by enhancing the run command to allow:
>>
>> run varname1 varname2 ... varnameN --args argv1 argv2 argv3
>>
>> Arguments argv1, argv2, ... are passed to the script identified by the
>> last variable (varnameN).
>
> Nice idea!  Only we should do a better syntax (options preceeding
> argument), i. e.
>
> 	run [ --args argv ] varname1 varname2 ...
>
> where argv would be the name of a variale to hold the arguments (as
> a comma (?) separated list) ?
In another mail you suggested "run arg1 arg2 ... -- varname1 varname2".

Whether arguments or script names are first or last does not make much
of a difference for the implementation effort. Any way you have to loop
over all arguments to find the separator "--".

"better syntax" does not apply here as the two alternatives have the
same expressivity, and need the same amount of typing and learning. It
is a matter of taste.

>
> Do you have an idea how the "script" would pull out the arguments
> from that variable?

Rasmus already suggested $1 .. $9 for the positional arguments (where
counting should not stop at 9).

Best regards

Heinrich

>
> Best regards,
>
> Wolfgang Denk
>

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ27owL439SkG3_V38KQnBkUDL5u2Ay7vmPeJ=mCmvHnzA@mail.gmail.com> you wrote:
>
> I would like to see this also. Is the busybox version the latest?
> There might even be some tests although I can't see any.

I have never been able to locate any other origin of the housh shell
source code, so - exept for other ports - this is the only current
souce I know of.

> One concern is that it seems to be 3x the line count. Hopefully that
> doesn't result in 3x the code size. It also seems to have developed an
> even more severe case of #ifdef disease.

ouch...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
As the system comes up, the component builders will from time to time
appear, bearing hot new versions of their pieces -- faster,  smaller,
more complete, or putatively less buggy. The replacement of a working
component  by a new version requires the same systematic testing pro-
cedure that adding a new component does, although it  should  require
less time, for more complete and efficient test cases will usually be
available.           - Frederick Brooks Jr., "The Mythical Man Month"

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Heinrich,

In message <b2395001-191c-31f3-2682-ac3e9fcff236@gmx.de> you wrote:
>
> > Nice idea!  Only we should do a better syntax (options preceeding
> > argument), i. e.
> >
> > 	run [ --args argv ] varname1 varname2 ...
> >
> > where argv would be the name of a variale to hold the arguments (as
> > a comma (?) separated list) ?
> In another mail you suggested "run arg1 arg2 ... -- varname1 varname2".

I was commenting on another suggestion.

> Whether arguments or script names are first or last does not make much
> of a difference for the implementation effort. Any way you have to loop
> over all arguments to find the separator "--".

It does not make a differenc in terms of implementation, but
tradition in UNIX systems is to have

	command_name [ -options ... ] arguments

i. e. options always came first.

It is only later tools that ignored how a proper program (TM) should
behave ;-)

> "better syntax" does not apply here as the two alternatives have the
> same expressivity, and need the same amount of typing and learning. It
> is a matter of taste.

Indeed. One is more pretty, and the other one is more ugly ;-)

> > Do you have an idea how the "script" would pull out the arguments
> > from that variable?
>
> Rasmus already suggested $1 .. $9 for the positional arguments (where
> counting should not stop at 9).

Fine with me.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The universe does not have laws - it has habits, and  habits  can  be
broken.

[PATCH 0/3] add "call" command

[Tom Rini]

On Sat, Sep 26, 2020 at 04:02:08PM +0200, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <CAPnjgZ27owL439SkG3_V38KQnBkUDL5u2Ay7vmPeJ=mCmvHnzA@mail.gmail.com> you wrote:
> >
> > I would like to see this also. Is the busybox version the latest?
> > There might even be some tests although I can't see any.
> 
> I have never been able to locate any other origin of the housh shell
> source code, so - exept for other ports - this is the only current
> souce I know of.

One of my worries about updating our hush code would be to maintain
compatibility with all of the scripts out there that rely on whatever
odd behavior we have.

If one goes down the path of "give U-Boot a better shell", an opt-in
"hush v2" or whatever naming makes sense for re-porting something from
busybox and having a plan to keep it in sync would be the way I would
like to see it go.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200929/fb47d474/attachment.sig>

[PATCH 0/3] add "call" command

[Wolfgang Denk]

Dear Tom,

In message <20200929174506.GJ14816@bill-the-cat> you wrote:
> 
> One of my worries about updating our hush code would be to maintain
> compatibility with all of the scripts out there that rely on whatever
> odd behavior we have.

Agreed.  Unfortunately we never collected any list of warts and bugs
and possible or recommended workarounds - we really should have done
that.

But then, IIRC, all problems of hush are of the type the specific
things are not working correctly, and the work around was to use
other ways or a combination of other commands / operators.  All
these workarounds should work in a bugfree version of hush still
tehe same, they would just be not necessary any more.

Of course we can't know the zillion of scripts out in the wild, and
where they might rely on broken behaviour.  If there are such, then
for them the upgrade to a newver version would indeed break the
system.

> If one goes down the path of "give U-Boot a better shell", an opt-in
> "hush v2" or whatever naming makes sense for re-porting something from
> busybox and having a plan to keep it in sync would be the way I would
> like to see it go.

I don't understand what you mean?  We probably cannot keep both
versions the hush shell (the current one and a more recent, less
broken one) in parallel in the source tree.  and I don't think we
should.  Fixing bugs is perfectly ok, and we never made any claim
that future versions of U-Boot must be bug-compatible to older ones.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Anything free is worth what you pay for it.

[PATCH v2 0/3] allow positional arguments with "run"

[Rasmus Villemoes]

This enables one to use positional arguments $1..$9 in functions
defined in the environment, rather than having to use global
variables. I.e., whereas

  run foo

requires one to set the (shell or environment) variables referenced
from foo beforehand, with this one can instead do

  run foo -- arg1 arg2 arg3

and use $1... up to $9 in the definition of foo. $# is set so foo can
make decisions based on that, and ${3:-default} works as expected.

In v2, at Heinrich's suggestion, make this available as an extension
of the existing run command rather than being a separate 'call'
command.

Rasmus Villemoes (3):
  cli_hush.c: refactor handle_dollar() to prepare for "run with
    arguments"
  allow positional arguments with "run" command
  ut: add small hush tests

 cmd/Kconfig                 | 10 ++++
 cmd/nvedit.c                |  7 ++-
 common/cli.c                | 44 +++++++++++++++---
 common/cli_hush.c           | 50 ++++++++++++++++----
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 include/cli_hush.h          |  9 ++++
 include/test/suites.h       |  1 +
 test/cmd/Makefile           |  1 +
 test/cmd/hush.c             | 91 +++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c               |  6 +++
 11 files changed, 204 insertions(+), 17 deletions(-)
 create mode 100644 test/cmd/hush.c

-- 
2.23.0

[PATCH v2 1/3] cli_hush.c: refactor handle_dollar() to prepare for "run with arguments"

[Rasmus Villemoes]

A later patch will add handling of $1 through $9 as well as $#, using
the same SPECIAL_VAR_SYMBOL handling as is currently used for $?. So
move that case to an explicit #ifdef __U_BOOT__ branch, and
consolidate a few of the #ifndef __U_BOOT__ cases, making it easier to
see the original hush code.

No functional change.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/cli_hush.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/common/cli_hush.c b/common/cli_hush.c
index 5b1f119074..072b871f1e 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -2863,6 +2863,16 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i
 		advance = 1;
 #endif
 	} else switch (ch) {
+#ifdef __U_BOOT__
+		case '?':
+			ctx->child->sp++;
+			b_addchr(dest, SPECIAL_VAR_SYMBOL);
+			b_addchr(dest, '$');
+			b_addchr(dest, '?');
+			b_addchr(dest, SPECIAL_VAR_SYMBOL);
+			advance = 1;
+			break;
+#endif
 #ifndef __U_BOOT__
 		case '$':
 			b_adduint(dest,getpid());
@@ -2872,20 +2882,10 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i
 			if (last_bg_pid > 0) b_adduint(dest, last_bg_pid);
 			advance = 1;
 			break;
-#endif
 		case '?':
-#ifndef __U_BOOT__
 			b_adduint(dest,last_return_code);
-#else
-			ctx->child->sp++;
-			b_addchr(dest, SPECIAL_VAR_SYMBOL);
-			b_addchr(dest, '$');
-			b_addchr(dest, '?');
-			b_addchr(dest, SPECIAL_VAR_SYMBOL);
-#endif
 			advance = 1;
 			break;
-#ifndef __U_BOOT__
 		case '#':
 			b_adduint(dest,global_argc ? global_argc-1 : 0);
 			advance = 1;
-- 
2.23.0

[PATCH v2 2/3] allow positional arguments with "run" command

[Rasmus Villemoes]

Currently, the only way to emulate functions with arguments in the
U-Boot shell is by doing "foo=arg1; bar=arg2; run func" and having
"func" refer to $foo and $bar. That works, but is a bit clunky, and
also suffers from foo and bar being set globally - if func itself wants
to run other "functions" defined in the environment, those other
functions better not use the same parameter names:

  setenv g 'do_g_stuff $foo'
  setenv f 'do_f_stuff $foo $bar; foo=123; run g; do_more_f_stuff $foo $bar'
  foo=arg1; bar=arg2; run f

Sure, f could do a "saved_foo=$foo; .... foo=$saved_foo" dance, but
that makes everything even more clunky.

In order to increase readability, allow passing positional arguments
to the functions invoked via run: When invoked with a -- separator,
the remaining arguments are use to set the local shell variables $1 through
$9 (and $#). As in a "real" shell, they are local to the current
function, so if f is called with two arguments, and f calls g with one
argument, g sees $2 as unset. Then the above can be written

  setenv g 'do_g_stuff $1'
  setenv f 'do_f_stuff $1 $2; run g -- 123; do_more_f_stuff $1 $2'
  run f -- arg1 arg2

Everything except

-                       b_addchr(dest, '?');
+                       b_addchr(dest, ch);

is under CONFIG_CMD_RUN_ARGS, and when CONFIG_CMD_RUN_ARGS=n, the ch
there can only be '?'. So no functional change when
CONFIG_CMD_RUN_ARGS is not selected.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 cmd/Kconfig        | 10 ++++++++++
 cmd/nvedit.c       |  7 ++++++-
 common/cli.c       | 44 ++++++++++++++++++++++++++++++++++++++------
 common/cli_hush.c  | 32 +++++++++++++++++++++++++++++++-
 include/cli_hush.h |  9 +++++++++
 5 files changed, 94 insertions(+), 8 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 0c984d735d..b8426d19d7 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -443,6 +443,16 @@ config CMD_RUN
 	help
 	  Run the command in the given environment variable.
 
+config CMD_RUN_ARGS
+	bool "allow positional arguments with run command"
+	depends on HUSH_PARSER
+	depends on CMD_RUN
+	help
+	  Allow invoking 'run' as 'run f g -- arg1 arg2 ...', which
+	  will run the commands defined in the environment variables f
+	  and g with positional arguments $1..$9 set to the arguments
+	  following the -- separator.
+
 config CMD_IMI
 	bool "iminfo"
 	default y
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 7fce723800..202139bfb9 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1575,7 +1575,12 @@ U_BOOT_CMD_COMPLETE(
 	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
 	"run commands in an environment variable",
 	"var [...]\n"
-	"    - run the commands in the environment variable(s) 'var'",
+	"    - run the commands in the environment variable(s) 'var'\n"
+	CONFIG_IS_ENABLED(CMD_RUN_ARGS, (
+	"run var [...] -- arg1 arg2 [...]\n"
+	"    - run the commands in the environment variable(s) 'var',\n"
+	"      with shell variables $1, $2, ... set to arg1, arg2, ...\n"
+	)),
 	var_complete
 );
 #endif
diff --git a/common/cli.c b/common/cli.c
index 6635ab2bcf..f970bd1eae 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -131,24 +131,56 @@ int run_command_list(const char *cmd, int len, int flag)
 #if defined(CONFIG_CMD_RUN)
 int do_run(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	int i;
+	struct run_args ra;
+	int i, j;
+	int ret = 0;
+	int cmds = argc;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	for (i = 1; i < argc; ++i) {
+	if (CONFIG_IS_ENABLED(CMD_RUN_ARGS)) {
+		for (i = 1; i < argc; ++i) {
+			if (!strcmp(argv[i], "--")) {
+				cmds = i;
+				++i;
+				break;
+			}
+		}
+		ra.count = argc - i;
+		if (ra.count > MAX_RUN_ARGS) {
+			printf("## Error: At most %d positional arguments allowed\n",
+			       MAX_RUN_ARGS);
+			return 1;
+		}
+		for (j = i; j < argc; ++j)
+			ra.args[j - i] = argv[j];
+
+		ra.prev = current_run_args;
+		current_run_args = &ra;
+	}
+
+	for (i = 1; i < cmds; ++i) {
 		char *arg;
 
 		arg = env_get(argv[i]);
 		if (arg == NULL) {
 			printf("## Error: \"%s\" not defined\n", argv[i]);
-			return 1;
+			ret = 1;
+			goto out;
 		}
 
-		if (run_command(arg, flag | CMD_FLAG_ENV) != 0)
-			return 1;
+		if (run_command(arg, flag | CMD_FLAG_ENV) != 0) {
+			ret = 1;
+			goto out;
+		}
 	}
-	return 0;
+
+out:
+	if (CONFIG_IS_ENABLED(CMD_RUN_ARGS))
+		current_run_args = ra.prev;
+
+	return ret;
 }
 #endif
 
diff --git a/common/cli_hush.c b/common/cli_hush.c
index 072b871f1e..df35c9c8d2 100644
--- a/common/cli_hush.c
+++ b/common/cli_hush.c
@@ -135,6 +135,11 @@ DECLARE_GLOBAL_DATA_PTR;
 #define syntax() syntax_err()
 #define xstrdup strdup
 #define error_msg printf
+
+#ifdef CONFIG_CMD_RUN_ARGS
+struct run_args *current_run_args;
+#endif
+
 #else
 typedef enum {
 	REDIRECT_INPUT     = 1,
@@ -2144,6 +2149,10 @@ char *get_local_var(const char *s)
 #ifdef __U_BOOT__
 	if (*s == '$')
 		return get_dollar_var(s[1]);
+	/* To make ${1:-default} work: */
+	if (IS_ENABLED(CONFIG_CMD_RUN_ARGS) &&
+	    '1' <= s[0] && s[0] <= '9' && !s[1])
+		return get_dollar_var(s[0]);
 #endif
 
 	for (cur = top_vars; cur; cur=cur->next)
@@ -2826,6 +2835,23 @@ static char *get_dollar_var(char ch)
 		case '?':
 			sprintf(buf, "%u", (unsigned int)last_return_code);
 			break;
+#ifdef CONFIG_CMD_RUN_ARGS
+		case '#':
+			if (!current_run_args)
+				return NULL;
+			sprintf(buf, "%u", current_run_args->count);
+			break;
+		case '1' ... '9': {
+			const struct run_args *ra = current_run_args;
+			int i = ch - '1';
+
+			if (!ra)
+				return NULL;
+			if (i >= ra->count)
+				return NULL;
+			return ra->args[i];
+		}
+#endif
 		default:
 			return NULL;
 	}
@@ -2865,10 +2891,14 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i
 	} else switch (ch) {
 #ifdef __U_BOOT__
 		case '?':
+#ifdef CONFIG_CMD_RUN_ARGS
+		case '1' ... '9':
+		case '#':
+#endif
 			ctx->child->sp++;
 			b_addchr(dest, SPECIAL_VAR_SYMBOL);
 			b_addchr(dest, '$');
-			b_addchr(dest, '?');
+			b_addchr(dest, ch);
 			b_addchr(dest, SPECIAL_VAR_SYMBOL);
 			advance = 1;
 			break;
diff --git a/include/cli_hush.h b/include/cli_hush.h
index 2bd35670c7..d6eb7e908d 100644
--- a/include/cli_hush.h
+++ b/include/cli_hush.h
@@ -23,4 +23,13 @@ char *get_local_var(const char *s);
 #if defined(CONFIG_HUSH_INIT_VAR)
 extern int hush_init_var (void);
 #endif
+
+#define MAX_RUN_ARGS 9
+struct run_args {
+	struct run_args *prev;
+	int count;
+	char *args[MAX_RUN_ARGS]; /* [0] holds $1 etc. */
+};
+extern struct run_args *current_run_args;
+
 #endif
-- 
2.23.0

[PATCH v2 3/3] ut: add small hush tests

[Rasmus Villemoes]

This is primarily to add a test of the new "call" command, but we
might as well add some very basic tests as well.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 configs/sandbox64_defconfig |  1 +
 configs/sandbox_defconfig   |  1 +
 include/test/suites.h       |  1 +
 test/cmd/Makefile           |  1 +
 test/cmd/hush.c             | 91 +++++++++++++++++++++++++++++++++++++
 test/cmd_ut.c               |  6 +++
 6 files changed, 101 insertions(+)
 create mode 100644 test/cmd/hush.c

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index c3ca796d51..96e5a37627 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -25,6 +25,7 @@ CONFIG_CMD_LICENSE=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_BOOTEFI_HELLO=y
 # CONFIG_CMD_ELF is not set
+CONFIG_CMD_RUN_ARGS=y
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
 CONFIG_CMD_ERASEENV=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 6e9f029cc9..94c8376837 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -30,6 +30,7 @@ CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_BOOTEFI_HELLO=y
 CONFIG_CMD_ABOOTIMG=y
 # CONFIG_CMD_ELF is not set
+CONFIG_CMD_RUN_ARGS=y
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_GREPENV=y
 CONFIG_CMD_ERASEENV=y
diff --git a/include/test/suites.h b/include/test/suites.h
index ab7b3bd9ca..082b87ce52 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -32,6 +32,7 @@ int do_ut_compression(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[]);
 int do_ut_dm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
+int do_ut_hush(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_lib(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
 int do_ut_log(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_mem(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
diff --git a/test/cmd/Makefile b/test/cmd/Makefile
index 859dcda239..adc5ba0307 100644
--- a/test/cmd/Makefile
+++ b/test/cmd/Makefile
@@ -4,3 +4,4 @@
 
 obj-y += mem.o
 obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
+obj-$(CONFIG_HUSH_PARSER) += hush.o
diff --git a/test/cmd/hush.c b/test/cmd/hush.c
new file mode 100644
index 0000000000..1f5e8afdf2
--- /dev/null
+++ b/test/cmd/hush.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <common.h>
+#include <console.h>
+#include <test/suites.h>
+#include <test/ut.h>
+
+#define HUSH_TEST(_name, _flags) UNIT_TEST(_name, _flags, hush_test)
+
+int do_ut_hush(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
+{
+	struct unit_test *tests = ll_entry_start(struct unit_test, hush_test);
+	const int n_ents = ll_entry_count(struct unit_test, hush_test);
+
+	return cmd_ut_category("cmd_hush", "cmd_hush_", tests, n_ents, argc,
+			       argv);
+}
+
+static int hush_test_basic(struct unit_test_state *uts)
+{
+	run_command("true", 0);
+	ut_assert_console_end();
+
+	run_command("echo $?", 0);
+	ut_assert_nextline("0");
+	ut_assert_console_end();
+
+	run_command("false", 0);
+	ut_assert_console_end();
+
+	run_command("echo $?", 0);
+	ut_assert_nextline("1");
+	ut_assert_console_end();
+
+	return 0;
+}
+HUSH_TEST(hush_test_basic, UT_TESTF_CONSOLE_REC);
+
+static int hush_test_cond(struct unit_test_state *uts)
+{
+	run_command("if true ; then echo yay ; else echo nay ; fi", 0);
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if false ; then echo yay ; else echo nay ; fi", 0);
+	ut_assert_nextline("nay");
+	ut_assert_console_end();
+
+	/* Short-circuiting */
+	run_command("if true || echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if false || echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("x");
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if true && echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("x");
+	ut_assert_nextline("yay");
+	ut_assert_console_end();
+
+	run_command("if false && echo x ; then echo yay; else echo nay ; fi", 0);
+	ut_assert_nextline("nay");
+	ut_assert_console_end();
+
+	return 0;
+}
+HUSH_TEST(hush_test_cond, UT_TESTF_CONSOLE_REC);
+
+#ifdef CONFIG_CMD_RUN_ARGS
+static int hush_test_call(struct unit_test_state *uts)
+{
+	run_command("env set f 'echo f: argc=$#, [$1] [${2}] [${3:-z}]; run g -- $2; echo f: [$1] [${2}]'", 0);
+	ut_assert_console_end();
+
+	run_command("env set g 'echo g: argc=$#, [$1] [$2] [${2:-y}]'", 0);
+	ut_assert_console_end();
+
+	run_command("run f g -- 11 22", 0);
+	ut_assert_nextline("f: argc=2, [11] [22] [z]");
+	ut_assert_nextline("g: argc=1, [22] [] [y]");
+	ut_assert_nextline("f: [11] [22]");
+	ut_assert_nextline("g: argc=2, [11] [22] [22]");
+	ut_assert_console_end();
+
+	return 0;
+}
+HUSH_TEST(hush_test_call, UT_TESTF_CONSOLE_REC);
+#endif
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index 8f0bc688a2..cd903186b9 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -81,6 +81,9 @@ static struct cmd_tbl cmd_ut_sub[] = {
 #if CONFIG_IS_ENABLED(UT_UNICODE) && !defined(API_BUILD)
 	U_BOOT_CMD_MKENT(unicode, CONFIG_SYS_MAXARGS, 1, do_ut_unicode, "", ""),
 #endif
+#ifdef CONFIG_HUSH_PARSER
+	U_BOOT_CMD_MKENT(hush, CONFIG_SYS_MAXARGS, 1, do_ut_hush, "", ""),
+#endif
 #ifdef CONFIG_SANDBOX
 	U_BOOT_CMD_MKENT(compression, CONFIG_SYS_MAXARGS, 1, do_ut_compression,
 			 "", ""),
@@ -140,6 +143,9 @@ static char ut_help_text[] =
 #ifdef CONFIG_UT_ENV
 	"ut env [test-name]\n"
 #endif
+#ifdef CONFIG_HUSH_PARSER
+	"ut hush - test (hush) shell functionality\n"
+#endif
 #ifdef CONFIG_UT_LIB
 	"ut lib [test-name] - test library functions\n"
 #endif
-- 
2.23.0

[PATCH v2 3/3] ut: add small hush tests

[Simon Glass]

On Wed, 7 Oct 2020 at 01:21, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> This is primarily to add a test of the new "call" command, but we
> might as well add some very basic tests as well.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  configs/sandbox64_defconfig |  1 +
>  configs/sandbox_defconfig   |  1 +
>  include/test/suites.h       |  1 +
>  test/cmd/Makefile           |  1 +
>  test/cmd/hush.c             | 91 +++++++++++++++++++++++++++++++++++++
>  test/cmd_ut.c               |  6 +++
>  6 files changed, 101 insertions(+)
>  create mode 100644 test/cmd/hush.c

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 1/3] cli_hush.c: refactor handle_dollar() to prepare for "run with arguments"

[Simon Glass]

On Wed, 7 Oct 2020 at 01:21, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> A later patch will add handling of $1 through $9 as well as $#, using
> the same SPECIAL_VAR_SYMBOL handling as is currently used for $?. So
> move that case to an explicit #ifdef __U_BOOT__ branch, and
> consolidate a few of the #ifndef __U_BOOT__ cases, making it easier to
> see the original hush code.
>
> No functional change.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  common/cli_hush.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 2/3] allow positional arguments with "run" command

[Simon Glass]

On Wed, 7 Oct 2020 at 01:21, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Currently, the only way to emulate functions with arguments in the
> U-Boot shell is by doing "foo=arg1; bar=arg2; run func" and having
> "func" refer to $foo and $bar. That works, but is a bit clunky, and
> also suffers from foo and bar being set globally - if func itself wants
> to run other "functions" defined in the environment, those other
> functions better not use the same parameter names:
>
>   setenv g 'do_g_stuff $foo'
>   setenv f 'do_f_stuff $foo $bar; foo=123; run g; do_more_f_stuff $foo $bar'
>   foo=arg1; bar=arg2; run f
>
> Sure, f could do a "saved_foo=$foo; .... foo=$saved_foo" dance, but
> that makes everything even more clunky.
>
> In order to increase readability, allow passing positional arguments
> to the functions invoked via run: When invoked with a -- separator,
> the remaining arguments are use to set the local shell variables $1 through
> $9 (and $#). As in a "real" shell, they are local to the current
> function, so if f is called with two arguments, and f calls g with one
> argument, g sees $2 as unset. Then the above can be written
>
>   setenv g 'do_g_stuff $1'
>   setenv f 'do_f_stuff $1 $2; run g -- 123; do_more_f_stuff $1 $2'
>   run f -- arg1 arg2
>
> Everything except
>
> -                       b_addchr(dest, '?');
> +                       b_addchr(dest, ch);
>
> is under CONFIG_CMD_RUN_ARGS, and when CONFIG_CMD_RUN_ARGS=n, the ch
> there can only be '?'. So no functional change when
> CONFIG_CMD_RUN_ARGS is not selected.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  cmd/Kconfig        | 10 ++++++++++
>  cmd/nvedit.c       |  7 ++++++-
>  common/cli.c       | 44 ++++++++++++++++++++++++++++++++++++++------
>  common/cli_hush.c  | 32 +++++++++++++++++++++++++++++++-
>  include/cli_hush.h |  9 +++++++++
>  5 files changed, 94 insertions(+), 8 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

I'm not sure where the previous discussion went. But please think
about how we can add some tests here.

- Simon

[PATCH v2 2/3] allow positional arguments with "run" command

[Rasmus Villemoes]

On 12/10/2020 05.34, Simon Glass wrote:
> On Wed, 7 Oct 2020 at 01:21, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>>  cmd/Kconfig        | 10 ++++++++++
>>  cmd/nvedit.c       |  7 ++++++-
>>  common/cli.c       | 44 ++++++++++++++++++++++++++++++++++++++------
>>  common/cli_hush.c  | 32 +++++++++++++++++++++++++++++++-
>>  include/cli_hush.h |  9 +++++++++
>>  5 files changed, 94 insertions(+), 8 deletions(-)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I'm not sure where the previous discussion went. But please think
> about how we can add some tests here.

Isn't that exactly what I do in 3/3? Or are you thinking of something else?

Rasmus

[PATCH v2 2/3] allow positional arguments with "run" command

[Simon Glass]

Hi Rasmus,

On Mon, 12 Oct 2020 at 01:06, Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> On 12/10/2020 05.34, Simon Glass wrote:
> > On Wed, 7 Oct 2020 at 01:21, Rasmus Villemoes
> > <rasmus.villemoes@prevas.dk> wrote:
> >>
> >>  cmd/Kconfig        | 10 ++++++++++
> >>  cmd/nvedit.c       |  7 ++++++-
> >>  common/cli.c       | 44 ++++++++++++++++++++++++++++++++++++++------
> >>  common/cli_hush.c  | 32 +++++++++++++++++++++++++++++++-
> >>  include/cli_hush.h |  9 +++++++++
> >>  5 files changed, 94 insertions(+), 8 deletions(-)
> >>
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > I'm not sure where the previous discussion went. But please think
> > about how we can add some tests here.
>
> Isn't that exactly what I do in 3/3? Or are you thinking of something else?
>

Yes that's good, but is the plan now to take these patches rather than
update to the latest hush? I was wondering is Buzybox has any tests
for hush.

Regards,
Simon

[PATCH v2 2/3] allow positional arguments with "run" command

[Rasmus Villemoes]

On 15/10/2020 17.05, Simon Glass wrote:
> Hi Rasmus,
> 
> On Mon, 12 Oct 2020 at 01:06, Rasmus Villemoes
> <rasmus.villemoes@prevas.dk> wrote:
>>
>> On 12/10/2020 05.34, Simon Glass wrote:
>>> On Wed, 7 Oct 2020 at 01:21, Rasmus Villemoes
>>> <rasmus.villemoes@prevas.dk> wrote:
>>>>
>>>>  cmd/Kconfig        | 10 ++++++++++
>>>>  cmd/nvedit.c       |  7 ++++++-
>>>>  common/cli.c       | 44 ++++++++++++++++++++++++++++++++++++++------
>>>>  common/cli_hush.c  | 32 +++++++++++++++++++++++++++++++-
>>>>  include/cli_hush.h |  9 +++++++++
>>>>  5 files changed, 94 insertions(+), 8 deletions(-)
>>>>
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> I'm not sure where the previous discussion went. But please think
>>> about how we can add some tests here.
>>
>> Isn't that exactly what I do in 3/3? Or are you thinking of something else?
>>
> 
> Yes that's good, but is the plan now to take these patches rather than
> update to the latest hush? I was wondering is Buzybox has any tests
> for hush.

Well, updating the whole hush code is not, as I've said before,
something I can or will take on me ATM, and it's not even clear that
that would automatically provide real shell functions.

Whether "the plan" includes accepting these patches I can't say. I'm
just trying to plug a hole and make the U-Boot shell a little more
usable. It's somewhat similar to the setexpr command; we don't have
$((a+4)) arithmetic, but can achieve the same thing with an extra
command. Or askenv, which takes the place of 'read -p'. Or run, for that
matter, which combined with setenv can do much of what eval in a POSIX
shell could do. And with 3/3, there's a place to put tests of e.g.
setexpr (not that adding the boilerplate is hard, but it is a tedious
first step; once that is in place, adding extra test cases is somewhat
easier and natural).

Rasmus

[PATCH v2 2/3] allow positional arguments with "run" command

[Wolfgang Denk]

Dear Rasmus,

In message <2284dd1d-f20c-6246-805e-55454a581960@prevas.dk> you wrote:
>
> > Yes that's good, but is the plan now to take these patches rather than
> > update to the latest hush? I was wondering is Buzybox has any tests
> > for hush.
>
> Well, updating the whole hush code is not, as I've said before,
> something I can or will take on me ATM, and it's not even clear that
> that would automatically provide real shell functions.

Yes, current versions of busybox hush do implement shell functions;
tested under Fedora 32:

-> rpm -q busybox
busybox-1.31.1-2.fc32.x86_64
-> cd /tmp
-> ln -s /sbin/busybox hush
-> ./hush
-> foo() {
> echo argc=$# arg1=$1 arg2=$2 arg3=$3
> }
-> foo
argc=0 arg1= arg2= arg3=
-> foo aa
argc=1 arg1=aa arg2= arg3=
-> foo aa bb
argc=2 arg1=aa arg2=bb arg3=
-> foo aa bb cc
argc=3 arg1=aa arg2=bb arg3=cc
-> foo aa bb cc dd
argc=4 arg1=aa arg2=bb arg3=cc
-> exit

No, I cannot see any shell / hush related test code in the busybox
repository.

> Whether "the plan" includes accepting these patches I can't say. I'm
> just trying to plug a hole and make the U-Boot shell a little more
> usable. It's somewhat similar to the setexpr command; we don't have
> $((a+4)) arithmetic, but can achieve the same thing with an extra
> command. Or askenv, which takes the place of 'read -p'. Or run, for that
> matter, which combined with setenv can do much of what eval in a POSIX
> shell could do. And with 3/3, there's a place to put tests of e.g.
> setexpr (not that adding the boilerplate is hard, but it is a tedious
> first step; once that is in place, adding extra test cases is somewhat
> easier and natural).

Well, that's note really the same.  For shell functions, ther eis
now a (hopefully) clean implementation in recent versions of hush,
so upgrading to a recent version would not only solve the
problem/task we're discussing here, but also bring a lot of other
bug fixes and improvements.

The examples you mentioned above are moe related to U-Boots concept
of environment handling, which is idependent of the shell we are
using i. e. it's the same with U-Boots own trivial command parser.


My big concern here is that adding bells and whistles to our ancient
version of hush will just make it all the harder to upgrade to a
recent version.  Already now we have the situation that we must be
afraid to break existing code which works around some of the
problems.  Adding more "special code" will not make this better.

And even if we can upgrade to a new version independently, we still
might have to keep this workaround code for compatibility, as people
started using it, and it is not compatible with any existing shell.


So the _much_ better approach would indeed be to upgrade to a recent
version of hush.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I have a very small mind and must live with it.    -- Edsger Dijkstra

[PATCH v2 2/3] allow positional arguments with "run" command

[Rasmus Villemoes]

On 19/10/2020 09.31, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <2284dd1d-f20c-6246-805e-55454a581960@prevas.dk> you wrote:
>>
>>> Yes that's good, but is the plan now to take these patches rather than
>>> update to the latest hush? I was wondering is Buzybox has any tests
>>> for hush.
>>
>> Well, updating the whole hush code is not, as I've said before,
>> something I can or will take on me ATM, and it's not even clear that
>> that would automatically provide real shell functions.
> 
> Yes, current versions of busybox hush do implement shell functions;
> tested under Fedora 32:

Not what I meant, of course busybox hush does that. What I meant is that
it is not at all obvious how that support would actually benefit U-Boot.
The problem is how one would go about getting the functions defined. Putting

define_func='func() { do_stuff $1 $2; do_more_stuff $3; }'

in the environment and then having to say

run define_func; func foo ...

does not really look like an improvement to me. In contrast, the current
way of defining "one-liner" functions and running with, well, run, is
quite ergonomic - but I do miss the ability to provide parameters other
than via global settings. With these patches, the above would just be

func='do_stuff $1 $2; do_more_stuff $3;'

run func -- foo ...

I guess one could have something like CONFIG_DOT_PROFILE and have that
point at a script that gets built into the U-Boot binary and sourced at
shell startup; then one could put one's functions in there, or have the
flexibility of having that file load some stdlib.sh from somewhere.

> 
> My big concern here is that adding bells and whistles to our ancient
> version of hush will just make it all the harder to upgrade to a
> recent version.  Already now we have the situation that we must be
> afraid to break existing code which works around some of the
> problems.  Adding more "special code" will not make this better.
> 
> And even if we can upgrade to a new version independently, we still
> might have to keep this workaround code for compatibility, as people
> started using it, and it is not compatible with any existing shell.
> 
> So the _much_ better approach would indeed be to upgrade to a recent
> version of hush.

I agree in principle - there are other shell features I'm also missing
(though see above, I don't immediately see how an upgrade would make
functions available in a useful way).

Someone speaking up and saying "I'm going to look at an overhaul of hush
within the next year or so" would clearly be a strong argument against
inclusion of these patches. Lacking such a pony promise, this really
boils down to an idealist/pragmatist issue, and we can keep going around
this in circles forever, so I think someone (Tom?) should make a decision.

Rasmus

[PATCH v2 2/3] allow positional arguments with "run" command

[Wolfgang Denk]

Dear Rasmus,

In message <b8e3d765-d0a6-12da-5fc9-3e0da0818cb8@prevas.dk> you wrote:
>
> > Yes, current versions of busybox hush do implement shell functions;
> > tested under Fedora 32:
>
> Not what I meant, of course busybox hush does that. What I meant is that
> it is not at all obvious how that support would actually benefit U-Boot.

um... I think you should define what you want.  You asked for shell
functions; I tell you they are supported; now you ask for something
else....

> The problem is how one would go about getting the functions defined. Putting
>
> define_func='func() { do_stuff $1 $2; do_more_stuff $3; }'
>
> in the environment and then having to say
>
> run define_func; func foo ...
>
> does not really look like an improvement to me. In contrast, the current

Shell functions is something you usually use as part of longer
scripts.  You can either define these as part of one or more
envrionment variables, or you can put these into a file or a U-Boot
image etc. and source it when needed.  There are many ways.

> way of defining "one-liner" functions and running with, well, run, is
> quite ergonomic - but I do miss the ability to provide parameters other
> than via global settings. With these patches, the above would just be
>
> func='do_stuff $1 $2; do_more_stuff $3;'
>
> run func -- foo ...

I can't see why you cannot do the same with shell fnctions?

setenv define_func 'func() { do_stuff $1 $2; do_more_stuff $3; }'
...
run define_func
func foo ...

> I guess one could have something like CONFIG_DOT_PROFILE and have that
> point at a script that gets built into the U-Boot binary and sourced at
> shell startup; then one could put one's functions in there, or have the
> flexibility of having that file load some stdlib.sh from somewhere.

You don't need any new defines for such a thing.   You can define
something like a sequence "test if file .profile exisits in some
stroage device; if yes, then source it" as past of your
CONFIG_USE_PREBOOT settings.

> I agree in principle - there are other shell features I'm also missing
> (though see above, I don't immediately see how an upgrade would make
> functions available in a useful way).

So do you want shell functions or any other standard shell features,
or do you just want to implement your own nonstandard ideas?  The
former I do support, the latter I don't...

> Someone speaking up and saying "I'm going to look at an overhaul of hush
> within the next year or so" would clearly be a strong argument against
> inclusion of these patches. Lacking such a pony promise, this really
> boils down to an idealist/pragmatist issue, and we can keep going around
> this in circles forever, so I think someone (Tom?) should make a decision.

As mentioned - addin more non-standard stuff will just create new
compatibility problems and make an upgrade more difficult and thus
less likely.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I had the rare misfortune of being one of the first people to try and
implement a PL/1 compiler.                             -- T. Cheatham

[PATCH v2 0/3] allow positional arguments with "run"

[Rasmus Villemoes]

On 07/10/2020 09.20, Rasmus Villemoes wrote:
> This enables one to use positional arguments $1..$9 in functions
> defined in the environment, 

Tom, can I ask for a decision on these? I know Wolfgang is opposed, and
if that counts as a veto, fine, I'd just like to know so these are at
least not kept hanging indefinitely.

Thanks,
Rasmus

[PATCH v2 0/3] allow positional arguments with "run"

[Tom Rini]

On Thu, Nov 05, 2020 at 08:25:31AM +0100, Rasmus Villemoes wrote:
> On 07/10/2020 09.20, Rasmus Villemoes wrote:
> > This enables one to use positional arguments $1..$9 in functions
> > defined in the environment, 
> 
> Tom, can I ask for a decision on these? I know Wolfgang is opposed, and
> if that counts as a veto, fine, I'd just like to know so these are at
> least not kept hanging indefinitely.

Sorry for the lack of feedback.  I guess, I just don't know.  There's at
least two series now (this and Simon's setexp) where part of the
feedback has been "our hush is ancient, and we should replace and keep
it in sync, and add features _upstream_".  That position isn't wrong.
But it's not easy to do, either.  I know you already said you didn't
have time and wouldn't step up to do that.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201106/e3f63e02/attachment.sig>

[PATCH v2 0/3] allow positional arguments with "run"

[Wolfgang Denk]

Dear Tom,

In message <20201106205245.GH5340@bill-the-cat> you wrote:
> 
> Sorry for the lack of feedback.  I guess, I just don't know.  There's at
> least two series now (this and Simon's setexp) where part of the
> feedback has been "our hush is ancient, and we should replace and keep
> it in sync, and add features _upstream_".  That position isn't wrong.
> But it's not easy to do, either.  I know you already said you didn't
> have time and wouldn't step up to do that.

This argument is not new, and I can fully understand this position,
too.  I'mm all to often myself in the position where the right Thing
(TM) requires more efforts and/or time than what is available in the
given project.

And from the maintainer's point of view, this has always been the
argument to sneak in code which is a workaround at best, and which
itself cements the original problem even more and makes it more
difficult to solve it at the roots.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q:  Why do mountain climbers rope themselves together?
A:  To prevent the sensible ones from going home.