[PATCH 0/5] cmd: Add support for command substitution

[PATCH 0/5] cmd: Add support for command substitution

[Sean Anderson]

This series adds support for command substitution using $(). The motivation
for this is that we have many commands which have a version which prints
some information, and one which sets an environmental variable with that
info. This is a bit clunky, since every time someone wants to grab info
from a command (e.g. [1], [2]), they have to add a version which sets a
variable. Every command also has subtle differences in the syntax of how
the environmental is specified.

The classic way to do this is to redirect the output of the command into a
variable using command substitution. Unfortunately, this approach will not
work in U-Boot for a few reasons. First, we don't have any jobs, so we
can't use pipelines to filter output. In addition, the command output tends
to be very "human-oriented" and would be hard to parse, even if something
like console recording were used. Instead, commands just set
gd->cmd_result. This result is then printed by the caller of cmd_process.

Unfortunately, this means that to support command substitution, commands
must be modified to set cmd_result. However, they already would have needed
modification to support setting an environmental variable.

Printing out the result of the command may not be the best approach here.
If a command which prints out many messages already returns a result, just
printing the result on its own does not clearly convey what the result
means. Perhaps a prefix should be added to printed results? Though this
would require echo to be changed.

This initial series just converts two commands: echo and part uuid. This
showcases the manner in which this new functionality could be implemented.
I think that some helper functions like for env may be useful so that
commands don't have to do some much bookkeeping (like in the current echo
command).

This series depends on [3].

[1] https://patchwork.ozlabs.org/project/uboot/patch/20210226181733.19307-1-farhan.ali at broadcom.com/
[2] https://patchwork.ozlabs.org/project/uboot/patch/20201215165439.13165-1-m.szyprowski at samsung.com/
[3] https://patchwork.ozlabs.org/project/uboot/patch/20210228212951.1175231-1-seanga2 at gmail.com/


Sean Anderson (5):
  hush: Print syntax error line with DEBUG_SHELL
  cmd: Add support for (limited) command substitution
  cmd: Convert echo to use cmd_result
  test: hush: Add test for command substitution
  cmd: Convert part uuid to use cmd_result

 cmd/echo.c                        | 33 +++++++++++++++----
 cmd/part.c                        | 18 ++++++++---
 common/cli_hush.c                 | 53 +++++++++++++++++++++++++------
 common/cli_simple.c               |  8 ++++-
 common/command.c                  |  3 ++
 include/asm-generic/global_data.h |  4 +++
 test/cmd/test_echo.c              | 10 +++++-
 7 files changed, 107 insertions(+), 22 deletions(-)

-- 
2.30.1

[PATCH 1/5] hush: Print syntax error line with DEBUG_SHELL

[Sean Anderson]

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

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__
-- 
2.30.1

[PATCH 2/5] cmd: Add support for (limited) command substitution

[Sean Anderson]

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.

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;
 };
 
 /**
-- 
2.30.1

[PATCH 3/5] cmd: Convert echo to use cmd_result

[Sean Anderson]

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.

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(
-- 
2.30.1

[PATCH 4/5] test: hush: Add test for command substitution

[Sean Anderson]

This adds a basic test for command substitution. A more advanced test
currently fails, and is left commented-out for now. A basic example of the
difference is that in bash

	$ a=b; echo a=$(echo $a)
	b

but in hush

	=> a=b; echo a=$(echo $a)
	a=

This is because command substitution takes place during parsing, before any
of the rest of the command has been executed.

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

 test/cmd/test_echo.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/test/cmd/test_echo.c b/test/cmd/test_echo.c
index 13e1fb7c82..f3d44d840d 100644
--- a/test/cmd/test_echo.c
+++ b/test/cmd/test_echo.c
@@ -31,13 +31,21 @@ static struct test_data echo_data[] = {
 	 * j, q, x are among the least frequent letters in English.
 	 * Hence no collision for the variable name jQx is expected.
 	 */
-	{"setenv jQx X; echo \"a)\" ${jQx} 'b)' '${jQx}' c) ${jQx}; setenv jQx",
+	{"setenv jQx X; echo \"a)\" ${jQx} 'b)' '${jQx}' c\\) ${jQx}; setenv jQx",
 	 "a) X b) ${jQx} c) X"},
 	/* Test shell variable assignments without substitutions */
 	{"foo=bar echo baz", "baz"},
 	/* Test handling of shell variables. */
 	{"setenv jQx; for jQx in 1 2 3; do echo -n \"${jQx}, \"; done; echo;",
 	 "1, 2, 3, "},
+	/* Test basic command substitution */
+	{"jQx=3 echo 1 $(echo 2) $jQx", "1 2 3"},
+	/*
+	 * The following test fails because the subshell is evaluated before
+	 * anything else, but the jQx2 assignment should happen beforehand
+	 */
+	//{"jQx2=2; echo 1 $(echo ${jQx2}) 3",
+	//"1 2 3"},
 };
 
 static int lib_test_hush_echo(struct unit_test_state *uts)
-- 
2.30.1

[PATCH 5/5] cmd: Convert part uuid to use cmd_result

[Sean Anderson]

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;
 }
 
 static int do_part_list(int argc, char *const argv[])
-- 
2.30.1

[PATCH 1/5] hush: Print syntax error line with DEBUG_SHELL

[Heinrich Schuchardt]

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__

[PATCH 1/5] hush: Print syntax error line with DEBUG_SHELL

[Sean Anderson]

On 2/28/21 6:51 PM, Heinrich Schuchardt wrote:
> 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.

The rest of this file uses DEBUG_SHELL already. This is done to reduce
the (torrent) of debugs which are only useful for someone debugging the
shell. If anything, this should be CONFIG_DEBUG_SHELL (and debug_printf
defined to log_debug with CONFIG_DEBUG_SHELL).

--Sean

> 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__
> 

[PATCH 2/5] cmd: Add support for (limited) command substitution

[Heinrich Schuchardt]

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.

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

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

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;
> };
> 
> /**

[PATCH 3/5] cmd: Convert echo to use cmd_result

[Heinrich Schuchardt]

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(

[PATCH 5/5] cmd: Convert part uuid to use cmd_result

[Heinrich Schuchardt]

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);
>+		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[])

[PATCH 2/5] cmd: Add support for (limited) command substitution

[Sean Anderson]

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;
>> };
>>
>> /**
> 

[PATCH 5/5] cmd: Convert part uuid to use cmd_result

[Heinrich Schuchardt]

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[])

[PATCH 5/5] cmd: Convert part uuid to use cmd_result

[Sean Anderson]

On 2/28/21 7:07 PM, Heinrich Schuchardt wrote:
> 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?

This is a consistent (and familiar) syntax for the many commands which
set environmental variables.

>>
>> Where do you document it?

It's undocumented at the moment. Will be fixed in v2.

>>
>> Where are the tests?

There's (a) test in patch 3. Though I forgot to add a test for this
command. Will be done in v2.

>>
>> 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?

This is not a memory leak. See patch 2 for semantics.

--Sean

> 
> 
>>> +		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[])
> 

[PATCH 5/5] cmd: Convert part uuid to use cmd_result

[Simon Glass]

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