[prev in list] [next in list] [prev in thread] [next in thread] 

List:       u-boot
Subject:    Re: [PATCH 2/5] cmd: Add support for (limited) command substitution
From:       Heinrich Schuchardt <xypron.glpk () gmx ! de>
Date:       2021-02-28 23:59:53
Message-ID: 16D657C7-6302-463D-83FE-528309857213 () gmx ! de
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]