Am 1. März 2021 00:47:14 MEZ schrieb Sean Anderson seanga2@gmail.com:

This prints the filename (rather useless) and line (very useful) whenever a syntax error occurs if DEBUG_SHELL is enabled.

Please, use log_error() instead.

Best regards

Heinrich

Signed-off-by: Sean Anderson seanga2@gmail.com

common/cli_hush.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/common/cli_hush.c b/common/cli_hush.c index 1b9bef64b6..83329763c6 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -372,15 +372,17 @@ static inline void debug_printf(const char *format, ...) { } #endif #define final_printf debug_printf

-#ifdef __U_BOOT__ +#ifdef DEBUG_SHELL +static void __syntax(char *file, int line) +{

• error_msg("syntax error %s:%d\n", file, line);

+}

+#define syntax_err() __syntax(__FILE__, __LINE__) +#else static void syntax_err(void) { printf("syntax error\n"); } -#else -static void __syntax(char *file, int line) {

• error_msg("syntax error %s:%d", file, line);

-} -#define syntax() __syntax(__FILE__, __LINE__) #endif

#ifdef __U_BOOT__

On 2/28/21 6:59 PM, Heinrich Schuchardt wrote:

Am 1. März 2021 00:47:15 MEZ schrieb Sean Anderson seanga2@gmail.com:

...

The traditional way to grab the output from a shell script is to use command substitution. Unfortunately, we don't really have the concept of pipes in U-Boot (at least not without console recording). Even if we did, stdout is severely polluted by informational printfs.

Instead of redirecting stdout, instead use a special global variable. This lets commands set a result without having to modify the function signature of every command, and everything that calls commands. This is a bit of a hack, but seemed like the least-invasive of all options.

Currently, the value of cmd_result is printed if it is set. This makes sense for things like echo, where you can do something like

=> echo a b c d e a b c d e => var=c => echo a $(echo b $var d) e a b c d e

but it makes less sense for some other commands

All callers of cmd_process must

1. Print cmd_result (unless it is being "redirected")
2. free() cmd_result
3. set cmd_result to NULL

Calling cmd_process with a non-NULL value in cmd_result is a bug.

I don't understand what you are changing from the lines above.

=> echo a $(echo b $var d) e

This is a syntax error at the moment, and this series makes it work.

Before extending the hush shell we should write a man-page in doc/usage/ describing what it actually does.

What would it be named? Is there an existing section of the documentation which documents the standard and non-standard features of hush?

Building in new gimmicks without documentation does not make any sense to me.

Yes, this should likely be better-documented. However, I'm not sure that this approach is the best one, so I wanted to get some feedback before writing out documentation. I suppose this makes this an RFC series.

--Sean

Best regards

Heinrich

...

Signed-off-by: Sean Anderson seanga2@gmail.com

common/cli_hush.c | 39 ++++++++++++++++++++++++++++--- common/cli_simple.c | 8 ++++++- common/command.c | 3 +++ include/asm-generic/global_data.h | 4 ++++ 4 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/common/cli_hush.c b/common/cli_hush.c index 83329763c6..8fed7eb14e 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -475,6 +475,8 @@ static char *make_string(char **inp, int *nonnull); static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *input); #ifndef __U_BOOT__ static int parse_string(o_string *dest, struct p_context *ctx, const char *src); +#else +static void update_ifs_map(void); #endif static int parse_stream(o_string *dest, struct p_context *ctx, struct in_str *input0, int end_trigger); /* setup: */ @@ -1673,6 +1675,10 @@ static int run_pipe_real(struct pipe *pi) "'run' command\n", child->argv[i]); return -1; }

• if (gd->cmd_result)
  

• 	puts(gd->cmd_result);
  

• free(gd->cmd_result);
  

• gd->cmd_result = NULL;
  

  /* Process the command */ return cmd_process(flag, child->argc - i, child->argv + i, &flag_repeat, NULL);

@@ -2683,6 +2689,7 @@ FILE *generate_stream_from_list(struct pipe *head) #endif return pf; } +#endif /* __U_BOOT__ */

/* this version hacked for testing purposes */ /* return code is exit status of the process that is run. */ @@ -2691,7 +2698,11 @@ static int process_command_subs(o_string *dest, struct p_context *ctx, struct in int retcode; o_string result=NULL_O_STRING; struct p_context inner; +#ifdef __U_BOOT__

• int list_retcode;

+#else FILE *p; +#endif struct in_str pipe_str; initialize_context(&inner);

@@ -2702,13 +2713,21 @@ static int process_command_subs(o_string *dest, struct p_context *ctx, struct in done_pipe(&inner, PIPE_SEQ); b_free(&result);

+#ifdef __U_BOOT__

• list_retcode = run_list_real(inner.list_head);
• setup_string_in_str(&pipe_str, gd->cmd_result ?: "");
• /* Restore the original map as best we can */
• update_ifs_map();

+#else p=generate_stream_from_list(inner.list_head); if (p==NULL) return 1; mark_open(fileno(p)); setup_file_in_str(&pipe_str, p); +#endif

/* now send results of command back into original context */ retcode = parse_stream(dest, ctx, &pipe_str, '\0'); +#ifndef __U_BOOT__ /* XXX In case of a syntax error, should we try to kill the child? * That would be tough to do right, so just read until EOF. */ if (retcode == 1) { @@ -2723,12 +2742,18 @@ static int process_command_subs(o_string *dest, struct p_context *ctx, struct in * to the KISS philosophy of this program. */ mark_closed(fileno(p)); retcode=pclose(p); +#else

• free(gd->cmd_result);
• gd->cmd_result = NULL;
• retcode = list_retcode;

+#endif free_pipe_list(inner.list_head,0); debug_printf("pclosed, retcode=%d\n",retcode); /* XXX this process fails to trim a single trailing newline */ return retcode; }

+#ifndef __U_BOOT__ static int parse_group(o_string *dest, struct p_context *ctx, struct in_str *input, int ch) { @@ -2896,11 +2921,11 @@ static int handle_dollar(o_string *dest, struct p_context *ctx, struct in_str *i } b_addchr(dest, SPECIAL_VAR_SYMBOL); break; -#ifndef __U_BOOT__ case '(': b_getch(input); process_command_subs(dest, ctx, input, ')'); break; +#ifndef __U_BOOT__ case '*': sep[0]=ifs[0]; for (i=1; i<global_argc; i++) { @@ -3165,7 +3190,7 @@ static void update_ifs_map(void) mapset(subst, 3); /* never flow through */ } mapset((uchar *)"\$'"", 3); /* never flow through */

• mapset((uchar *)";&|#", 1); /* flow through if quoted */

• mapset((uchar *)";&|()#", 1); /* flow through if quoted */

#endif mapset(ifs, 2); /* also flow through if quoted */ } @@ -3185,7 +3210,8 @@ static int parse_stream_outer(struct in_str *inp, int flag) ctx.type = flag; initialize_context(&ctx); update_ifs_map();

• if (!(flag & FLAG_PARSE_SEMICOLON) || (flag & FLAG_REPARSING))
  

mapset((uchar *)";$&|", 0);

• if (!(flag & FLAG_PARSE_SEMICOLON) || (flag & FLAG_REPARSING))
  

• 	mapset((uchar *)";$&|()", 0);
  

  inp->promptmode=1; rcode = parse_stream(&temp, &ctx, inp, flag & FLAG_CONT_ON_NEWLINE ? -1 : '\n');

@@ -3205,6 +3231,13 @@ static int parse_stream_outer(struct in_str *inp, int flag) run_list(ctx.list_head); #else code = run_list(ctx.list_head);

• 	if (!(flag & FLAG_REPARSING) && gd->cmd_result) {
  

• 		puts(gd->cmd_result);
  

• 		free(gd->cmd_result);
  

• 		gd->cmd_result = NULL;
  

• 	}
  

• if (code == -2) {	/* exit */
  	b_free(&temp);
  	code = 0;
  

diff --git a/common/cli_simple.c b/common/cli_simple.c index e80ba488a5..5df30d964f 100644 --- a/common/cli_simple.c +++ b/common/cli_simple.c @@ -15,14 +15,16 @@ #include <console.h> #include <env.h> #include <log.h> +#include <malloc.h> #include <linux/ctype.h>

+DECLARE_GLOBAL_DATA_PTR;

#define DEBUG_PARSER 0 /* set to 1 to debug */

#define debug_parser(fmt, args...) \ debug_cond(DEBUG_PARSER, fmt, ##args)

int cli_simple_parse_line(char *line, char *argv[]) { int nargs = 0; @@ -257,6 +259,10 @@ int cli_simple_run_command(const char *cmd, int flag)

if (cmd_process(flag, argc, argv, &repeatable, NULL))
	rc = -1;

• if (gd->cmd_result)
  

• 	puts(gd->cmd_result);
  

• free(gd->cmd_result);
  

• gd->cmd_result = NULL;
  
  

  /* Did the user stop this? */ if (had_ctrlc())

diff --git a/common/command.c b/common/command.c index 3fe6791eda..952a8f00eb 100644 --- a/common/command.c +++ b/common/command.c @@ -588,6 +588,9 @@ enum command_ret_t cmd_process(int flag, int argc, char *const argv[], enum command_ret_t rc = CMD_RET_SUCCESS; struct cmd_tbl *cmdtp;

• /* Clear previous result */
• assert(!gd->cmd_result);

#if defined(CONFIG_SYS_XTRACE) char *xtrace;

diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index b6a9991fc9..85262d9566 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -453,6 +453,10 @@ struct global_data { */ char *smbios_version; #endif

• /**

• * @cmd_result: Result of the current command
  

• */
  

• char *cmd_result;

};

/**

Am 1. März 2021 00:47:16 MEZ schrieb Sean Anderson seanga2@gmail.com:

This adds some complexity, since we need to know how big the arguments are ahead of time instead of finding out when we print them.

Why do you want to add complexity?

Where is the documentation update?

Best regards

Heinrich

Signed-off-by: Sean Anderson seanga2@gmail.com

cmd/echo.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/cmd/echo.c b/cmd/echo.c index fda844ee9d..8f20e635ce 100644 --- a/cmd/echo.c +++ b/cmd/echo.c @@ -6,11 +6,16 @@

#include <common.h> #include <command.h> +#include <malloc.h>

+DECLARE_GLOBAL_DATA_PTR;

static int do_echo(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {

• int i = 1;

• char *result;
• int j, i = 1;
• size_t result_size, arglens[CONFIG_SYS_MAXARGS]; bool space = false; bool newline = true;

@@ -21,18 +26,32 @@ static int do_echo(struct cmd_tbl *cmdtp, int flag, int argc, } }

• result_size = 1 + newline; /* \0 + \n */
• result_size += argc - i - 1; /* spaces */
• for (j = i; j < argc; ++j) {

• arglens[j] = strlen(argv[j]);
  

• result_size += arglens[j];
  

• }
• result = malloc(result_size);
• if (!result)

• return CMD_RET_FAILURE;
  

• gd->cmd_result = result;
• for (; i < argc; ++i) {

• if (space) {
  

• 	putc(' ');
  

• }
  

• puts(argv[i]);
  

• if (space)
  

• 	*result++ = ' ';
  

• memcpy(result, argv[i], arglens[i]);
  

• result += arglens[i];
  

  space = true; }

  if (newline)

• putc('\n');
  

• *result++ = '\n';
  

• *result = '\0';

• return 0;

• return CMD_RET_SUCCESS;

}

U_BOOT_CMD(

Am 1. März 2021 01:03:43 MEZ schrieb Heinrich Schuchardt xypron.glpk@gmx.de:

Am 1. März 2021 00:47:18 MEZ schrieb Sean Anderson seanga2@gmail.com:

...

This is fairly straightforward. This allows part uuid mmc 0 foo To be rewritten as env set foo $(part uuid mmc 0) or even (if the variable is not required to be environmental) foo=$(part uuid mmc 0)

Who needs this? Why?

Where do you document it?

Where are the tests?

Best regards

Heinrich

...

Signed-off-by: Sean Anderson seanga2@gmail.com

cmd/part.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/cmd/part.c b/cmd/part.c index 3395c17b89..97e70d79ff 100644 --- a/cmd/part.c +++ b/cmd/part.c @@ -19,9 +19,12 @@ #include <config.h> #include <command.h> #include <env.h> +#include <malloc.h> #include <part.h> #include <vsprintf.h>

+DECLARE_GLOBAL_DATA_PTR;

enum cmd_part_info { CMD_PART_INFO_START = 0, CMD_PART_INFO_SIZE, @@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const argv[]) if (part < 0) return 1;

• if (argc > 2)

• if (argc > 2) { env_set(argv[2], info.uuid);

• else

• printf("%s\n", info.uuid);
  

• } else {

• size_t result_size = sizeof(info.uuid) + 1;
  
  

• return 0;

• gd->cmd_result = malloc(result_size);
  

This is a memory leak. How about realloc?

...

• if (!gd->cmd_result)
  

• 	return CMD_RET_FAILURE;
  

• snprintf(gd->cmd_result, result_size, "%s\n", info.uuid);
  

• }
• return CMD_RET_SUCCESS;

}

static int do_part_list(int argc, char *const argv[])

Hi Sean,

On Sun, 28 Feb 2021 at 16:47, Sean Anderson seanga2@gmail.com wrote:

This is fairly straightforward. This allows part uuid mmc 0 foo To be rewritten as env set foo $(part uuid mmc 0) or even (if the variable is not required to be environmental) foo=$(part uuid mmc 0)

Signed-off-by: Sean Anderson seanga2@gmail.com

cmd/part.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/cmd/part.c b/cmd/part.c index 3395c17b89..97e70d79ff 100644 --- a/cmd/part.c +++ b/cmd/part.c @@ -19,9 +19,12 @@ #include <config.h> #include <command.h> #include <env.h> +#include <malloc.h> #include <part.h> #include <vsprintf.h>

+DECLARE_GLOBAL_DATA_PTR;

enum cmd_part_info { CMD_PART_INFO_START = 0, CMD_PART_INFO_SIZE, @@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const argv[]) if (part < 0) return 1;

•   if (argc > 2)
  

•   if (argc > 2) {
            env_set(argv[2], info.uuid);
  

•   else
  

•           printf("%s\n", info.uuid);
  

•   } else {
  

•           size_t result_size = sizeof(info.uuid) + 1;
  
  

•   return 0;
  

•           gd->cmd_result = malloc(result_size);
  

•           if (!gd->cmd_result)
  

•                   return CMD_RET_FAILURE;
  

•           snprintf(gd->cmd_result, result_size, "%s\n", info.uuid);
  

•   }
  

•   return CMD_RET_SUCCESS;
  

I suppose I prefer 0 to CMD_RET_SUCCESS, since 0 is the universal success code in U-Boot, and using the enum obscures that a bit. But I don't feel strongly about it.

Regards, Simon