| Message ID | 20200205010812.20373-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: nvedit: support doing one (extra) expansion of the value in "env set" | expand |
Hi Rasmus, On Tue, 4 Feb 2020 at 18:08, Rasmus Villemoes <rasmus.villemoes at prevas.dk> wrote: > > Currently, there's no way to fetch the value of an environment > variable whose name is stored in some other variable, or generated from > such - in non-working pseudo-code, > > ${${varname}} > ${array${index}} > > This forces some scripts to needlessly duplicate logic and hardcode > assumptions. For example, in an A/B scheme with three variables > > BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated > BOOT_A_LEFT # 0..3 > BOOT_B_LEFT # 0..3 > > when one needs to determine the slot to boot from, one does something > like > > setenv found > for slot in $BOOT_ORDER ; do > if test "x$found" != "x" ; then > # work around lack of break > elif test "x$slot" = "xA" ; then > if test $BOOT_A_LEFT -gt 0 ; then > setexpr BOOT_A_LEFT $BOOT_A_LEFT - 1 > setenv found A > setenv bootargs ${bootargs_A} > setenv ubivol ${ubivol_A} > # more setup based on A > fi > elif test "x$slot" = "xB" ; then > if test $BOOT_B_LEFT -gt 0 ; then > # the same ... > fi > fi > done > > This is already bad enough, but extending that to A/B/C is tedious and > prone to copy-pastos. > > So this is an attempt at allowing one to do "env set -E var value1 value2" > with the effect that, of course, normal variable expansion happens on > the command line, the valueX are joined with spaces as usual, and then > one more pass is done over that string replacing occurrences of > ${foo}. > > The above would become > > setenv found > for slot in $BOOT_ORDER ; do > if test "x$found" != "x" ; then > # work around lack of break > else > env set -E boot_left "\${BOOT_${slot}_LEFT}" > if test $boot_left -gt 0 ; then > setexpr BOOT_${slot}_LEFT $boot_left - 1 > env set found $slot > env set -E bootargs "\${bootargs_${slot}}" > env set -E ubivol "\${ubivol_${slot}}" > fi > fi > done > > I'm pleasantly surprised it was that easy to implement, but of course > I'm cheating a bit (cli_simple_process_macros is only available if > CONFIG_CMDLINE, though I think cli_simple.o could be unconditionally > built and then link-time GC should get rid of the excess > functions). > > This has been lightly tested in the sandbox. I'll add some proper unit > tests, update the help texts and try to handle the Kconfig issue if > this is something that might be accepted. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> > --- > cmd/nvedit.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) Seems OK to me. I suppose we don't want to implement bash's nested expansion? ${var${suffix}} Regards, Simon
On 05/02/2020 18.59, Simon Glass wrote: > Hi Rasmus, > >> This has been lightly tested in the sandbox. I'll add some proper unit >> tests, update the help texts and try to handle the Kconfig issue if >> this is something that might be accepted. >> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> >> --- >> cmd/nvedit.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) > > Seems OK to me. Thanks. I'll go ahead and write some tests. > I suppose we don't want to implement bash's nested > expansion? ${var${suffix}} It's not that easy to implement inside-out expansion, one needs to juggle a lot of temporary buffers. So I went with the rather simple one-extra-pass, which can mostly achieve the same things (although perhaps the script needs to do a few extra steps). Out of curiosity, what bash version supports the above? $ echo $BASH_VERSION 4.4.20(1)-release $ foo_a=3 $ foo_b=7 $ x=a $ echo ${foo_${x}} bash: ${foo_${x}}: bad substitution however, this variant is supported: $ var=foo_$x $ echo ${var} foo_a $ echo ${!var} 3 > Regards, > Simon >
Dear Rasmus Villemoes, In message <20200205010812.20373-1-rasmus.villemoes at prevas.dk> you wrote: > Currently, there's no way to fetch the value of an environment > variable whose name is stored in some other variable, or generated from > such - in non-working pseudo-code, > > ${${varname}} > ${array${index}} HUSH does not support arrays anyway... > This forces some scripts to needlessly duplicate logic and hardcode > assumptions. For example, in an A/B scheme with three variables > > BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated > BOOT_A_LEFT # 0..3 > BOOT_B_LEFT # 0..3 > > when one needs to determine the slot to boot from, one does something > like Well, there _are_ other ways... > This has been lightly tested in the sandbox. I'll add some proper unit > tests, update the help texts and try to handle the Kconfig issue if > this is something that might be accepted. I'm pretty sure this will blow up in your face in real life, because if side effects on existing shell scripts even if you don't expect this. This needs _lots_ of testing with existing code / scripts. Best regards, Wolfgang Denk
Hi Rasmus, On Tue, 11 Feb 2020 at 08:38, Rasmus Villemoes <rasmus.villemoes at prevas.dk> wrote: > > On 05/02/2020 18.59, Simon Glass wrote: > > Hi Rasmus, > > > > >> This has been lightly tested in the sandbox. I'll add some proper unit > >> tests, update the help texts and try to handle the Kconfig issue if > >> this is something that might be accepted. > >> > >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> > >> --- > >> cmd/nvedit.c | 17 ++++++++++++++++- > >> 1 file changed, 16 insertions(+), 1 deletion(-) > > > > Seems OK to me. > > Thanks. I'll go ahead and write some tests. > > > I suppose we don't want to implement bash's nested > > expansion? ${var${suffix}} > > It's not that easy to implement inside-out expansion, one needs to > juggle a lot of temporary buffers. So I went with the rather simple > one-extra-pass, which can mostly achieve the same things (although > perhaps the script needs to do a few extra steps). > > Out of curiosity, what bash version supports the above? > > $ echo $BASH_VERSION > 4.4.20(1)-release > $ foo_a=3 > $ foo_b=7 > $ x=a > $ echo ${foo_${x}} > bash: ${foo_${x}}: bad substitution > > however, this variant is supported: > > $ var=foo_$x > $ echo ${var} > foo_a > $ echo ${!var} > 3 Er, yes, sorry I was thinking of Makefiles. Regards, Simon
On 11/02/2020 17.30, Wolfgang Denk wrote: > Dear Rasmus Villemoes, > > In message <20200205010812.20373-1-rasmus.villemoes at prevas.dk> you wrote: >> Currently, there's no way to fetch the value of an environment >> variable whose name is stored in some other variable, or generated from >> such - in non-working pseudo-code, >> >> ${${varname}} >> ${array${index}} > > HUSH does not support arrays anyway... Of course not, but they can be emulated by having variables foo0, foo1, foo2 and programmatically accessing the variable foo$index, if only there's a way to do that... In a sense, my BOOT_A_LEFT/BOOT_B_LEFT is also just an array with keys "A" and "B". >> This forces some scripts to needlessly duplicate logic and hardcode >> assumptions. For example, in an A/B scheme with three variables >> >> BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated >> BOOT_A_LEFT # 0..3 >> BOOT_B_LEFT # 0..3 >> >> when one needs to determine the slot to boot from, one does something >> like > > Well, there _are_ other ways... Please do tell. How can I avoid code duplication and access a variable whose name I generate by string concatenation/variable interpolation? I.e., I don't want anything like "if test $slot = "A" ; then setenv result BOOT_A_LEFT ; elif test $slot = "B" ; then setenv result BOOT_B_LEFT ; fi", because that doesn't scale. >> This has been lightly tested in the sandbox. I'll add some proper unit >> tests, update the help texts and try to handle the Kconfig issue if >> this is something that might be accepted. > > I'm pretty sure this will blow up in your face in real life, because > if side effects on existing shell scripts even if you don't > expect this. How so? The behaviour is completely opt-in per "env set", so nothing at all changes for existing scripts, and it's only supposed to be used where one would otherwise use some eval-like construct in a "normal" shell. So env set -E result "\${BOOT_${x}_LEFT}" corresponds to eval "result=\${BOOT_${x}_LEFT}" And just as "eval" is used sparingly in shell scripts, I expect "env set -E" to be used only when necessary. > This needs _lots_ of testing with existing code / > scripts. I'm not proposing changing semantics of existing scripts at all. Thanks, Rasmus
On 11/02/2020 22.20, Rasmus Villemoes wrote: > On 11/02/2020 17.30, Wolfgang Denk wrote: >>> This forces some scripts to needlessly duplicate logic and hardcode >>> assumptions. For example, in an A/B scheme with three variables >>> >>> BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated >>> BOOT_A_LEFT # 0..3 >>> BOOT_B_LEFT # 0..3 >>> >>> when one needs to determine the slot to boot from, one does something >>> like >> >> Well, there _are_ other ways... > > Please do tell. How can I avoid code duplication and access a variable > whose name I generate by string concatenation/variable interpolation? Btw., I also considered implementing this as a separate subcommand "env get <dest> <varname>", then I could do "env get result BOOT_${slot}_LEFT". That wouldn't be quite as powerful as "env set -E", but I think sufficient for my use case(s). Rasmus
Dear Rasmus, In message <c48f9de9-ad02-3b45-ae2e-50c3086d6a68 at prevas.dk> you wrote: > > > HUSH does not support arrays anyway... > > Of course not, but they can be emulated by having variables foo0, foo1, > foo2 and programmatically accessing the variable foo$index, if only > there's a way to do that... In a sense, my BOOT_A_LEFT/BOOT_B_LEFT is > also just an array with keys "A" and "B". Actually the port to U-Boot cripples HUSH in many more aspects. I've always hoped someone would some day volunteer and (1)( update HUSH to a more recent (and less buggy) version and address a few of the missing parts, like Command Substitution, which would be really handy in many cases - here as well. > > Well, there _are_ other ways... > > Please do tell. How can I avoid code duplication and access a variable > whose name I generate by string concatenation/variable interpolation? > I.e., I don't want anything like "if test $slot = "A" ; then setenv > result BOOT_A_LEFT ; elif test $slot = "B" ; then setenv result > BOOT_B_LEFT ; fi", because that doesn't scale. => slot=A => setenv result BOOT_${slot}_LEFT => printenv result result=BOOT_A_LEFT => setenv foo 'setenv result BOOT_${slot}_LEFT; printenv result' => slot=B => run foo result=BOOT_B_LEFT => slot=X => run foo result=BOOT_X_LEFT What exactly is your question? > env set -E result "\${BOOT_${x}_LEFT}" > > corresponds to > > eval "result=\${BOOT_${x}_LEFT}" ...and things become already very tricky here, as you can see from the need of having "\$" and "$" mixed. Now assume you have more of this in the embedded variables... I consider this change a bad idea. Instead of adding such a hack we should add what you really want... Best regards, Wolfgang Denk
Dear Rasmus, In message <2e076ba8-6ffe-66dc-ecb9-ebb62e47f414 at prevas.dk> you wrote: > > >> Well, there _are_ other ways... > > > > Please do tell. How can I avoid code duplication and access a variable > > whose name I generate by string concatenation/variable interpolation? > > Btw., I also considered implementing this as a separate subcommand "env > get <dest> <varname>", then I could do "env get result > BOOT_${slot}_LEFT". That wouldn't be quite as powerful as "env set -E", > but I think sufficient for my use case(s). Sorry, but I still don;t understand what exactly your problem is. Se previous message, or, slightly extended: => setenv BOOT_A_LEFT boot a left => setenv BOOT_B_LEFT boot b left => setenv BOOT_X_LEFT boot x left => setenv foo 'setenv result BOOT_${slot}_LEFT; printenv result; print $result' => slot=B => run foo result=BOOT_B_LEFT BOOT_B_LEFT=boot b left => slot=X => run foo result=BOOT_X_LEFT BOOT_X_LEFT=boot x left => slot=A => run foo result=BOOT_A_LEFT BOOT_A_LEFT=boot a left Best regards, Wolfgang Denk
On 12/02/2020 12.38, Wolfgang Denk wrote: > Dear Rasmus, > > In message <c48f9de9-ad02-3b45-ae2e-50c3086d6a68 at prevas.dk> you wrote: >> >>> HUSH does not support arrays anyway... >> >> Of course not, but they can be emulated by having variables foo0, foo1, >> foo2 and programmatically accessing the variable foo$index, if only >> there's a way to do that... In a sense, my BOOT_A_LEFT/BOOT_B_LEFT is >> also just an array with keys "A" and "B". > > Actually the port to U-Boot cripples HUSH in many more aspects. > I've always hoped someone would some day volunteer and (1)( update > HUSH to a more recent (and less buggy) version and address a few of > the missing parts, like Command Substitution, which would be really > handy in many cases - here as well. I'm certainly also missing break (and to a lesser extent continue) in loops. >>> Well, there _are_ other ways... >> >> Please do tell. How can I avoid code duplication and access a variable >> whose name I generate by string concatenation/variable interpolation? >> I.e., I don't want anything like "if test $slot = "A" ; then setenv >> result BOOT_A_LEFT ; elif test $slot = "B" ; then setenv result >> BOOT_B_LEFT ; fi", because that doesn't scale. > > => slot=A > => setenv result BOOT_${slot}_LEFT > => printenv result > result=BOOT_A_LEFT > => setenv foo 'setenv result BOOT_${slot}_LEFT; printenv result' > => slot=B > => run foo > result=BOOT_B_LEFT > => slot=X > => run foo > result=BOOT_X_LEFT > > What exactly is your question? I'm sorry, I see I mistyped in my example above, it should have been if test $slot = "A" ; setenv result $BOOT_A_LEFT ... as should hopefully be clear from the original post and the eval examples. So to reiterate, the problem is to get the contents (or value, if you will) of the BOOT_A_LEFT variable into the result variable, not setting result to the string "BOOT_A_LEFT" - but with the wrinkle that BOOT_A_LEFT is generated programmatically, so the code cannot literally mention BOOT_A_LEFT anywhere. >> env set -E result "\${BOOT_${x}_LEFT}" >> >> corresponds to >> >> eval "result=\${BOOT_${x}_LEFT}" > > ...and things become already very tricky here, as you can see from > the need of having "\$" and "$" mixed. Now assume you have more of > this in the embedded variables... Eh, yes, exactly as one needs to do in ordinary shells when using the eval construct. I don't see how one can on the one hand trust programmers to have arbitrary read and write access to all of physical memory but not trust them to get such rather basic escaping right (and testing should immediately show if one got it wrong). I also proposed an escape-less solution, namely "env get". That would be slightly less powerful, since env get dest whatever could be implemented in terms of "env set -E" as env set -E dest "\${whatever}" but as I said, I think that would be sufficient for my purposes. So just as print[env] takes the name of a variable and shows the name=value string, and one can thus say "printenv BOOT_${slot}_LEFT" as you did in your extended example, I could do env get result BOOT_${slot}_LEFT and get the value of the BOOT_${slot}_LEFT variable into result. Would you be ok with adding such an "env get" with less foot-gun potential? Rasmus
Dear Rasmus, In message <f9aa247b-4b9b-9196-4de7-b352d25766fe at prevas.dk> you wrote: > > I'm sorry, I see I mistyped in my example above, it should have been > > if test $slot = "A" ; setenv result $BOOT_A_LEFT ... > > as should hopefully be clear from the original post and the eval > examples. So to reiterate, the problem is to get the contents (or value, > if you will) of the BOOT_A_LEFT variable into the result variable, not > setting result to the string "BOOT_A_LEFT" - but with the wrinkle that > BOOT_A_LEFT is generated programmatically, so the code cannot literally > mention BOOT_A_LEFT anywhere. Didn't I show this in my second, expanded example? I suggest you provide a working example of shell code (say, bash, if you like) to demonstrate what you really have in mind. It seems I have problems understanding your verbal description. > So just as print[env] takes the name of a variable and shows the > name=value string, and one can thus say "printenv BOOT_${slot}_LEFT" as > you did in your extended example, I could do > > env get result BOOT_${slot}_LEFT > > and get the value of the BOOT_${slot}_LEFT variable into result. I still fail to see why you think this cannot be done with just the already existing code. Just use setenv instead of printenv in my example? > Would you be ok with adding such an "env get" with less foot-gun potential? I'm not OK with adding any special-purpose code which can easily be implemented with existing scripting capabilites. And so far I don't see the limitation you are running into. Best regards, Wolfgang Denk
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 81d94cd193..ff6ffcb674 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -224,7 +224,7 @@ DONE: */ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) { - int i, len; + int i, len, expand = 0; char *name, *value, *s; struct env_entry e, *ep; @@ -244,6 +244,9 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) case 'f': /* force */ env_flag |= H_FORCE; break; + case 'E': + expand = 1; + break; default: return CMD_RET_USAGE; } @@ -287,6 +290,18 @@ static int _do_env_set(int flag, int argc, char * const argv[], int env_flag) if (s != value) *--s = '\0'; + if (expand) { + char *expanded = malloc(CONFIG_SYS_CBSIZE); + + if (expanded == NULL) { + printf("## Can't malloc %d bytes\n", CONFIG_SYS_CBSIZE); + free(value); + return 1; + } + cli_simple_process_macros(value, expanded); + free(value); + value = expanded; + } e.key = name; e.data = value; hsearch_r(e, ENV_ENTER, &ep, &env_htab, env_flag);
Currently, there's no way to fetch the value of an environment variable whose name is stored in some other variable, or generated from such - in non-working pseudo-code, ${${varname}} ${array${index}} This forces some scripts to needlessly duplicate logic and hardcode assumptions. For example, in an A/B scheme with three variables BOOT_ORDER # Either "A B" or "B A" depending on which slot was last updated BOOT_A_LEFT # 0..3 BOOT_B_LEFT # 0..3 when one needs to determine the slot to boot from, one does something like setenv found for slot in $BOOT_ORDER ; do if test "x$found" != "x" ; then # work around lack of break elif test "x$slot" = "xA" ; then if test $BOOT_A_LEFT -gt 0 ; then setexpr BOOT_A_LEFT $BOOT_A_LEFT - 1 setenv found A setenv bootargs ${bootargs_A} setenv ubivol ${ubivol_A} # more setup based on A fi elif test "x$slot" = "xB" ; then if test $BOOT_B_LEFT -gt 0 ; then # the same ... fi fi done This is already bad enough, but extending that to A/B/C is tedious and prone to copy-pastos. So this is an attempt at allowing one to do "env set -E var value1 value2" with the effect that, of course, normal variable expansion happens on the command line, the valueX are joined with spaces as usual, and then one more pass is done over that string replacing occurrences of ${foo}. The above would become setenv found for slot in $BOOT_ORDER ; do if test "x$found" != "x" ; then # work around lack of break else env set -E boot_left "\${BOOT_${slot}_LEFT}" if test $boot_left -gt 0 ; then setexpr BOOT_${slot}_LEFT $boot_left - 1 env set found $slot env set -E bootargs "\${bootargs_${slot}}" env set -E ubivol "\${ubivol_${slot}}" fi fi done I'm pleasantly surprised it was that easy to implement, but of course I'm cheating a bit (cli_simple_process_macros is only available if CONFIG_CMDLINE, though I think cli_simple.o could be unconditionally built and then link-time GC should get rid of the excess functions). This has been lightly tested in the sandbox. I'll add some proper unit tests, update the help texts and try to handle the Kconfig issue if this is something that might be accepted. Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk> --- cmd/nvedit.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)