[PATCH 0/3] add "call" command
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
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;
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
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 /****************************************************************************/
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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;
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
participants (5)
-
Heinrich Schuchardt -
Rasmus Villemoes -
Simon Glass -
Tom Rini -
Wolfgang Denk