[PATCH 0/3] add "call" command

classic Classic list List threaded Threaded
38 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] add "call" command

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

  run foo

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

  call foo arg1 arg2 arg3

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

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

  foo arg1 arg2 arg3

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

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

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

--
2.23.0

Reply | Threaded
Open this post in threaded view
|

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

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

No functional change.

Signed-off-by: Rasmus Villemoes <[hidden email]>
---
 common/cli_hush.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

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

Reply | Threaded
Open this post in threaded view
|

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

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

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

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

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

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

Everything except

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

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

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

  f a b c

instead of

  call f a b c

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

Signed-off-by: Rasmus Villemoes <[hidden email]>
---
 cmd/Kconfig       |  8 +++++
 common/cli_hush.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 82 insertions(+), 1 deletion(-)

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

Reply | Threaded
Open this post in threaded view
|

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

Rasmus Villemoes
In reply to this post by Rasmus Villemoes
This is primarily to add a test of the new "call" command, but we
might as well add some very basic tests as well.

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

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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Heinrich Schuchardt
In reply to this post by Rasmus Villemoes
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
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Rasmus Villemoes
On 25/09/2020 13.52, Heinrich Schuchardt wrote:

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

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

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

POSIX(-like) shells have "command":

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

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

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

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

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

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

moving the check:

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

        name=strdup(s);

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

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

we get

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

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

Rasmus
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus,

In message <[hidden email]> you wrote:

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

This is definitely a useful idea.

But...

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

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

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

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
God runs electromagnetics by wave theory on  Monday,  Wednesday,  and
Friday,  and the Devil runs them by quantum theory on Tuesday, Thurs-
day, and Saturday.                                   -- William Bragg
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus,

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

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

Please consider this a soft-NAK ;-)

Best regards,

Wolfgang Denk

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

Re: [PATCH 0/3] add "call" command

Wolfgang Denk
In reply to this post by Heinrich Schuchardt
Dear Heinrich Schuchardt,

In message <[hidden email]> you wrote:
>
> Further we cannot first introduce a command call and then eliminate it
> due to backward compatibility. We should decide on the final version
> beforehand.

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

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

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

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
"Ada is PL/I trying to be Smalltalk.                 - Codoso diBlini
Reply | Threaded
Open this post in threaded view
|

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

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

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

Rasmus
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Rasmus Villemoes
In reply to this post by Wolfgang Denk
On 25/09/2020 15.09, Wolfgang Denk wrote:
> Dear Heinrich Schuchardt,
>
> In message <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Heinrich Schuchardt
In reply to this post by Wolfgang Denk
On 25.09.20 15:09, Wolfgang Denk wrote:

> Dear Heinrich Schuchardt,
>
> In message <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Rasmus Villemoes
On 25/09/2020 15.38, Heinrich Schuchardt wrote:

> On 25.09.20 15:09, Wolfgang Denk wrote:
>> Dear Heinrich Schuchardt,
>>
>> In message <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Simon Glass-3
In reply to this post by Wolfgang Denk
Hi,

On Fri, 25 Sep 2020 at 06:59, Wolfgang Denk <[hidden email]> wrote:

>
> Dear Rasmus,
>
> In message <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus,

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

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

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
What is tolerance? -- it is the consequence of humanity. We  are  all
formed  of frailty and error; let us pardon reciprocally each other's
folly -- that is the first law of nature.                  - Voltaire
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Wolfgang Denk
In reply to this post by Heinrich Schuchardt
Dear Heinrich,

In message <[hidden email]> you wrote:
>
> For me this could be realized by enhancing the run command to allow:
>
> run varname1 varname2 ... varnameN --args argv1 argv2 argv3
>
> Arguments argv1, argv2, ... are passed to the script identified by the
> last variable (varnameN).

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

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

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

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

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
What was sliced bread the greatest thing since?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Wolfgang Denk
In reply to this post by Rasmus Villemoes
Dear Rasmus,

In message <[hidden email]> you wrote:
>
> Though I'd probably make it
>
>   run varname -- arg1 arg2 arg3

Or rather

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


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

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


Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [hidden email]
(null cookie; hope that's ok)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Heinrich Schuchardt
In reply to this post by Wolfgang Denk
On 9/26/20 10:51 AM, Wolfgang Denk wrote:

> Dear Heinrich,
>
> In message <[hidden email]> 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
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] add "call" command

Wolfgang Denk
In reply to this post by Simon Glass-3
Dear Simon,

In message <CAPnjgZ27owL439SkG3_V38KQnBkUDL5u2Ay7vmPeJ=[hidden email]> you wrote:
>
> I would like to see this also. Is the busybox version the latest?
> There might even be some tests although I can't see any.

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

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

ouch...

Best regards,

Wolfgang Denk

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

Re: [PATCH 0/3] add "call" command

Wolfgang Denk
In reply to this post by Heinrich Schuchardt
Dear Heinrich,

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

I was commenting on another suggestion.

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

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

        command_name [ -options ... ] arguments

i. e. options always came first.

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

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

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

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

Fine with me.

Best regards,

Wolfgang Denk

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