| Message ID | 1449255744-25787-1-git-send-email-nm@ti.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Nishanth Menon, In message <1449255744-25787-1-git-send-email-nm@ti.com> you wrote: > When we use the following in bootargs: > v1=abc > v2=123-${v1} > echo ${v2} > we get 123-${v1} > when we should have got > 123-abc > This is because we do not recursively check to see if v2 by itself has > a hidden variable. Fix the same with recursive call. > > NOTE: this is a limited implementation as the next level variable > default assignment etc would not function. As I wrote in my comment to your previous versionof this patch, I think your approach is wrong: Current behaviour is what a standard shell would do as well: bash$ v1=abc bash$ v2='123-${v1}' bash$ echo $v2 123-${v1} I think your change would causes non-standard shell behaviour. If you want to evaluate variables, you have to do so as part of a "run" command... Best regards, Wolfgang Denk
Hi, On 4 December 2015 at 13:20, Wolfgang Denk <wd@denx.de> wrote: > Dear Nishanth Menon, > > In message <1449255744-25787-1-git-send-email-nm@ti.com> you wrote: >> When we use the following in bootargs: >> v1=abc >> v2=123-${v1} >> echo ${v2} >> we get 123-${v1} >> when we should have got >> 123-abc >> This is because we do not recursively check to see if v2 by itself has >> a hidden variable. Fix the same with recursive call. >> >> NOTE: this is a limited implementation as the next level variable >> default assignment etc would not function. > > As I wrote in my comment to your previous versionof this patch, I > think your approach is wrong: > > Current behaviour is what a standard shell would do as well: > > bash$ v1=abc > bash$ v2='123-${v1}' > bash$ echo $v2 > 123-${v1} > > I think your change would causes non-standard shell behaviour. > > If you want to evaluate variables, you have to do so as part of a > "run" command... I find the recursive behaviour much more useful. In particular we have to jump through all sorts of hoops to build up a command line. It would be much easier if we could make things recursive. The single quote example above explicitly stops all substitution - do we need a way to do that also? Regards, Simon
Dear Simon, In message <CAPnjgZ1j=yNOSbmDU0Pkq2y1wq-y=v7O2y7zPTNEfc==St3rgQ@mail.gmail.com> you wrote: > > > I think your change would causes non-standard shell behaviour. > > > > If you want to evaluate variables, you have to do so as part of a > > "run" command... > > I find the recursive behaviour much more useful. In particular we have > to jump through all sorts of hoops to build up a command line. It > would be much easier if we could make things recursive. The single > quote example above explicitly stops all substitution - do we need a > way to do that also? Useful it may be - in samoe cases at least, but I think it is a very bad idea to deviate from standard shell behaviour. Best regards, Wolfgang Denk
Hi Wolfgang, On 6 December 2015 at 23:28, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon, > > In message <CAPnjgZ1j=yNOSbmDU0Pkq2y1wq-y=v7O2y7zPTNEfc==St3rgQ@mail.gmail.com> you wrote: >> >> > I think your change would causes non-standard shell behaviour. >> > >> > If you want to evaluate variables, you have to do so as part of a >> > "run" command... >> >> I find the recursive behaviour much more useful. In particular we have >> to jump through all sorts of hoops to build up a command line. It >> would be much easier if we could make things recursive. The single >> quote example above explicitly stops all substitution - do we need a >> way to do that also? > > Useful it may be - in samoe cases at least, but I think it is a very > bad idea to deviate from standard shell behaviour. If you look at this patch [1] you'll see the pain we have to go through to make environment variables work. We have 'run regen_all' which basically does what this patch already supports. It has to use setenv to set up the variables after any of the variables in the expressions change. And this is not so bad. It is much worse when you try to use these sorts of things on the command line with setenv - in fact I believe in some cases it is impossible to make the command line do the right thing. I'd really like to see recursive eval of environment variables. I don't think of everything I enter in U-Boot being in single quotes. Perhaps we should make this an option? Regards, Simon [1] https://patchwork.ozlabs.org/patch/471911/
diff --git a/common/cli_hush.c b/common/cli_hush.c index 5a26af80c758..d56d6cfb131e 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -471,7 +471,7 @@ static int redirect_opt_num(o_string *o); static int process_command_subs(o_string *dest, struct p_context *ctx, struct in_str *input, int subst_end); static int parse_group(o_string *dest, struct p_context *ctx, struct in_str *input, int ch); #endif -static char *lookup_param(char *src); +static char *lookup_param(char *src, bool *b); 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__ @@ -2740,7 +2740,7 @@ static int parse_group(o_string *dest, struct p_context *ctx, /* basically useful version until someone wants to get fancier, * see the bash man page under "Parameter Expansion" */ -static char *lookup_param(char *src) +static char *lookup_param(char *src, bool *b) { char *p; char *sep; @@ -2748,9 +2748,10 @@ static char *lookup_param(char *src) int assign = 0; int expand_empty = 0; - if (!src) + if (!src || !b) return NULL; + *b = false; sep = strchr(src, ':'); if (sep) { @@ -2783,6 +2784,16 @@ static char *lookup_param(char *src) } } else if (expand_empty) { p += strlen(p); + } else { + char f[CONFIG_SYS_CBSIZE]; + + if (strlen(p) < CONFIG_SYS_CBSIZE) { + cli_simple_process_macros(p, f); + if (strncmp(p, f, CONFIG_SYS_CBSIZE)) { + p = strdup(f); + *b = true; + } + } } if (sep) @@ -3501,6 +3512,7 @@ static char *insert_var_value_sub(char *inp, int tag_subst) int len; int done = 0; char *p, *p1, *res_str = NULL; + bool b; while ((p = strchr(inp, SPECIAL_VAR_SYMBOL))) { /* check the beginning of the string for normal characters */ @@ -3516,7 +3528,8 @@ static char *insert_var_value_sub(char *inp, int tag_subst) p = strchr(inp, SPECIAL_VAR_SYMBOL); *p = '\0'; /* look up the value to substitute */ - if ((p1 = lookup_param(inp))) { + p1 = lookup_param(inp, &b); + if (p1) { if (tag_subst) len = res_str_len + strlen(p1) + 2; else @@ -3544,6 +3557,9 @@ static char *insert_var_value_sub(char *inp, int tag_subst) strcpy((res_str + res_str_len), p1); res_str_len = len; + + if (b) + free(p1); } *p = SPECIAL_VAR_SYMBOL; inp = ++p;
When we use the following in bootargs: v1=abc v2=123-${v1} echo ${v2} we get 123-${v1} when we should have got 123-abc This is because we do not recursively check to see if v2 by itself has a hidden variable. Fix the same with recursive call. NOTE: this is a limited implementation as the next level variable default assignment etc would not function. Signed-off-by: Nishanth Menon <nm@ti.com> --- For next level recursion of variables to work, this patch depends on https://patchwork.ozlabs.org/patch/552823/ Tested on Sandbox. common/cli_hush.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)