| Message ID | 20200925111942.4629-1-rasmus.villemoes@prevas.dk |
|---|---|
| Headers | show |
| Series | add "call" command | expand |
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 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
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
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 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
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
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 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
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
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 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