| Message ID | 20201007072052.28200-3-rasmus.villemoes@prevas.dk |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | allow positional arguments with "run" | expand |
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
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
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(-)