| Message ID | 509a01fb-ba01-33b9-33bc-7323544d290a@prevas.dk |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: nvedit: support doing one (extra) expansion of the value in "env set" | expand |
On 14/02/2020 12.54, Rasmus Villemoes wrote: > Now we can work around the lack of break in the busybox shell by writing ^^^^^^^^^^^^^ > But we still can't translate this to busybox shell, because there's no ^^^^^^^^^^^^^ I of course meant "U-Boot shell", sorry about that. Rasmus
Hi, I chose to implement boot count / selection functionality as a command instead: https://patchwork.ozlabs.org/patch/943549/ I do plan to extend it a bit in the coming weeks though: * move the env-get and -set to weak functions, so that board files can put the info wherever. * add support for multiple lists of boot slots, so that the kernel can be updated independently of the rootfs. // Martin On 14 February 2020 12.54.35 CET, Rasmus Villemoes <rasmus.villemoes at prevas.dk> wrote: >On 13/02/2020 16.55, Wolfgang Denk wrote: >> 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'm sorry, but no, you did not. You used the capability of the print >(or >rather printenv) command to take the name of a variable and print >"name=value" to the terminal. > >In your example, result had the value BOOT_A_LEFT, so doing "print >$result" meant executing the command "print BOOT_A_LEFT", and of course >the output of that was then "BOOT_A_LEFT=boot a left". > >However, what I need is to get that "boot a left" value stored in some >other variable so I can actually do further logic based on that value. > >> 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. > >Assume BOOT_ORDER contains some permutation of "A B C", and for each >letter x, there's a BOOT_x_LEFT counter telling how many boot attempts >that slot has left. Now I want to find the first x in $BOOT_ORDER for >which $BOOT_x_LEFT is positive. If all BOOT_x_LEFT are 0, say I want >the >sentinel value 'none'. > >So in bash, that might be written > >slot=none >for x in $BOOT_ORDER ; do > eval "left=\${BOOT_${x}_LEFT}" > if [ $left -gt 0 ] ; then > slot=$x > break > fi >done > >Now we can work around the lack of break in the busybox shell by >writing >the loop so that the main body is skipped if we've found a valid slot: > >slot=none >for x in $BOOT_ORDER ; do > if [ $slot != 'none' ] ; then > true > else > eval "left=\${BOOT_${x}_LEFT}" > if [ $left -gt 0 ] ; then > slot=$x > fi > fi >done > >But we still can't translate this to busybox shell, because there's no >eval. Now, I can do this with a hypothetical "env get" command which I >just implemented to test that it works, and then the above becomes > >env set slot none; >for x in $BOOT_ORDER ; do > if test $slot != 'none' ; then > true ; > else > env get left BOOT_${x}_LEFT ; # magic > if test $left -gt 0 ; then > env set slot $x ; > fi; > fi; >done; > >Now, if you can implement the line marked #magic with existing >functions, I'm all ears. Or if you can implement the semantics of this >snippet in some other way, which does not open-code explicit references >to BOOT_A_LEFT, BOOT_B_LEFT etc. This is what I meant when I said I'd >prefer not to write the loop like this: > >env set slot none; >for x in $BOOT_ORDER ; do > if test $slot != 'none' ; then > true ; > else > if test $x = A && test $BOOT_A_LEFT -gt 0 ; then > env set slot A > env set left $BOOT_A_LEFT > elif test $x = B && test $BOOT_B_LEFT -gt 0 ; then > env set slot B > env set left $BOOT_B_LEFT > elif test $x = C && test $BOOT_C_LEFT -gt 0 ; then > env set slot C > env set left $BOOT_C_LEFT > fi > fi; >done; > >[yes, I also want left set as a side effect to the current value of the >appropriate BOOT_x_LEFT]. > >>> 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. > >Of course not. But _I'm_ not seeing how one can actually fetch the >value >of one variable into another, just given the first variable's name - >when that name is programmatically generated, e.g. as BOOT_${x}_LEFT or >whatnot. > >PS: Implementation of "env get" is just: > >--- a/cmd/nvedit.c >+++ b/cmd/nvedit.c >@@ -386,6 +386,20 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, >int argc, char * const argv[]) > return _do_env_set(flag, argc, argv, H_INTERACTIVE); > } > >+static int do_env_get(cmd_tbl_t *cmdtp, int flag, int argc, char * >const argv[]) >+{ >+ char *local_args[4]; >+ >+ if (argc != 3) >+ return CMD_RET_USAGE; >+ local_args[0] = argv[0]; >+ local_args[1] = argv[1]; >+ local_args[2] = env_get(argv[2]); >+ local_args[3] = NULL; >+ >+ return _do_env_set(flag, argc, local_args, H_INTERACTIVE); >+} >+ > /* > * Prompt for environment variable > */ >@@ -1334,6 +1348,7 @@ static cmd_tbl_t cmd_env_sub[] = { > #endif > #endif > U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 0, do_env_set, "", ""), >+ U_BOOT_CMD_MKENT(get, 3, 0, do_env_get, "", ""), > #if defined(CONFIG_CMD_ENV_EXISTS) > U_BOOT_CMD_MKENT(exists, 2, 0, do_env_exists, "", ""), > #endif > >So it differs from "env set" (or setenv) in that > > env set foo bar > >sets foo to the value "bar" > >while > > env get foo bar > >"gets" the value of the bar variable and stores it in foo, i.e. this is >essentially > > env set foo "$bar" > >But the power of "env get" comes when bar is not just an explicit bare >word such as bar - namely, when I can use the shell to do one expansion >of the command line > > env get foo BOOT_${x}_LEFT > >and thus choose which variable's value I'm fetching into foo. > >Rasmus
Dear Rasmus, In message <509a01fb-ba01-33b9-33bc-7323544d290a at prevas.dk> you wrote: > > So in bash, that might be written > > slot=none > for x in $BOOT_ORDER ; do > eval "left=\${BOOT_${x}_LEFT}" > if [ $left -gt 0 ] ; then > slot=$x > break > fi > done OK, now I get the context. You mean something like this in U-Boot ? setenv BOOT_ORDER A B C D setenv BOOT_A_LEFT 0 setenv BOOT_B_LEFT 0 setenv BOOT_C_LEFT 2 setenv BOOT_D_LEFT 5 slot=none for i in $BOOT_ORDER ; do setenv tmp_cmd 'setexpr tmp_val sub '^' "" $'BOOT_${i}_LEFT run tmp_cmd test $slot = none && test $tmp_val -gt 0 && slot=$i done echo "## Chosen Slot = $slot" This returns ## Chosen Slot = C as you want. The only inelegance of this approach is that it will also print tmp_val=0 tmp_val=0 tmp_val=2 tmp_val=5 on the console, which is not really nice. I wonder if it's worth adding a "-q" flag to the setexpr sommand? And - I even beat your bash script which has 8 lines, while my script has only 6 :-) :-) > Now we can work around the lack of break in the busybox shell by writing > the loop so that the main body is skipped if we've found a valid slot: Things like this are usually easier done using && and || > But we still can't translate this to busybox shell, because there's no > eval. Now, I can do this with a hypothetical "env get" command which I > just implemented to test that it works, and then the above becomes There is no eval, but there is still a zillion of simple tricks you can pull to get your stuff done :-) > Now, if you can implement the line marked #magic with existing > functions, I'm all ears. Or if you can implement the semantics of this > snippet in some other way, which does not open-code explicit references > to BOOT_A_LEFT, BOOT_B_LEFT etc. This is what I meant when I said I'd > prefer not to write the loop like this: See above... > [yes, I also want left set as a side effect to the current value of the > appropriate BOOT_x_LEFT]. Oh, this is a new requirement... So lets change my little script to add setting "left": slot=none for i in $BOOT_ORDER ; do setenv tmp_cmd 'setexpr tmp_val sub '^' "" $'BOOT_${i}_LEFT run tmp_cmd test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val done echo "## Chosen Slot = $slot ; Left = $left" Result: ## Chosen Slot = C ; Left = 2 Best regards, Wolfgang Denk
Dear Martin, In message <2CD28D15-B5BF-4C5B-A57C-B9A24AD6D49E at geanix.com> you wrote: > > > I chose to implement boot count / selection functionality as a command instead: > https://patchwork.ozlabs.org/patch/943549/ Thanks for pointing out, I think I should NAK this patch, too. It can be implemented by simple scripting, so there is no need to add code that has to be maintained by all but is used by a very small minoryty, if at all. Best regards, Wolfgang Denk
Dear Rasmus, In message <20200216152427.E80C7240036 at gemini.denx.de> I wrote: > > So lets change my little script to add setting "left": > > slot=none > for i in $BOOT_ORDER ; do > setenv tmp_cmd 'setexpr tmp_val sub '^' "" $'BOOT_${i}_LEFT > run tmp_cmd > test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val > done > echo "## Chosen Slot = $slot ; Left = $left" > > Result: > > ## Chosen Slot = C ; Left = 2 Actually I'm stupid. It's much easier this way, and without the ugly printed messages: setenv BOOT_ORDER A B C D setenv BOOT_A_LEFT 0 setenv BOOT_B_LEFT 0 setenv BOOT_C_LEFT 2 setenv BOOT_D_LEFT 5 slot=none for i in $BOOT_ORDER ; do setenv tmp_cmd 'setenv tmp_val $'BOOT_${i}_LEFT run tmp_cmd test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val done echo "## Chosen Slot = $slot ; Left = $left" Best regards, Wolfgang Denk
On 16/02/2020 18.25, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200216152427.E80C7240036 at gemini.denx.de> I wrote: >> >> So lets change my little script to add setting "left": >> >> slot=none >> for i in $BOOT_ORDER ; do >> setenv tmp_cmd 'setexpr tmp_val sub '^' "" $'BOOT_${i}_LEFT >> run tmp_cmd >> test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val >> done >> echo "## Chosen Slot = $slot ; Left = $left" >> >> Result: >> >> ## Chosen Slot = C ; Left = 2 > > Actually I'm stupid. No, but I did notice the above seemed needlessly obfuscated :) It's much easier this way, and without the > ugly printed messages: > > setenv BOOT_ORDER A B C D > setenv BOOT_A_LEFT 0 > setenv BOOT_B_LEFT 0 > setenv BOOT_C_LEFT 2 > setenv BOOT_D_LEFT 5 > > slot=none > for i in $BOOT_ORDER ; do > setenv tmp_cmd 'setenv tmp_val $'BOOT_${i}_LEFT > run tmp_cmd Nice. So the trick I was missing was to get a literal $, followed by the ("computed") name of the env var I wanted, all embedded in a command to be run (to invoke the second expansion). It's a bit tricky, but it does get the job done. There should be some catalogue of things like this, mentioning "U-Boot shell doesn't directly have $that feature, but you can often emulate it with something like $this". > test $slot = none && test $tmp_val -gt 0 && slot=$i && left=$tmp_val If performance matters, one can move the tmp_cmd handling after the slot=none test - then one can also use left directly instead of tmp_val, so the line only grows by one clause: test $slot = none && setenv tmp_cmd 'setenv left $'BOOT_${i}_LEFT && run tmp_cmd && test $left -gt 0 && slot=$i Or as a more readable alternative that still avoids the "run" overhead and saves one line (and the tmp_var) setenv tmp_cmd 'setenv left $'BOOT_${i}_LEFT test $slot = none && run tmp_cmd && test $left -gt 0 && slot=$i Thanks, Wolfgang. Consider both "env set -E" and the alternative "env get" withdrawn. Rasmus
On 18/02/2020 09.11, Rasmus Villemoes wrote: > Thanks, Wolfgang. Consider both "env set -E" and the alternative "env > get" withdrawn. So, if I wanted to change the status of such a patch in patchwork, what would be the appropriate status? There's no "Withdrawn" or "Retracted". So "Not applicable" or "Rejected"? Rasmus
On Fri, Feb 21, 2020 at 11:32:16PM +0000, Rasmus Villemoes wrote: > On 18/02/2020 09.11, Rasmus Villemoes wrote: > > > Thanks, Wolfgang. Consider both "env set -E" and the alternative "env > > get" withdrawn. > > So, if I wanted to change the status of such a patch in patchwork, what > would be the appropriate status? There's no "Withdrawn" or "Retracted". > So "Not applicable" or "Rejected"? No strong preference, you can even just put it at / leave it at RFC. Thanks!
On 14/02/2020 12.54, Rasmus Villemoes wrote: > > Assume BOOT_ORDER contains some permutation of "A B C", and for each > letter x, there's a BOOT_x_LEFT counter telling how many boot attempts > that slot has left. Now I want to find the first x in $BOOT_ORDER for > which $BOOT_x_LEFT is positive. If all BOOT_x_LEFT are 0, say I want the > sentinel value 'none'. > > So in bash, that might be written > > slot=none > for x in $BOOT_ORDER ; do > eval "left=\${BOOT_${x}_LEFT}" > if [ $left -gt 0 ] ; then > slot=$x > break > fi > done > > Now we can work around the lack of break in the [U-Boot] shell by writing > the loop so that the main body is skipped if we've found a valid slot: > > slot=none > for x in $BOOT_ORDER ; do > if [ $slot != 'none' ] ; then > true > else > eval "left=\${BOOT_${x}_LEFT}" > if [ $left -gt 0 ] ; then > slot=$x > fi > fi > done > > But we still can't translate this to [U-Boot] shell, because there's no > eval. Now, I can do this with a hypothetical "env get" command which I > just implemented to test that it works, and then the above becomes > > env set slot none; > for x in $BOOT_ORDER ; do > if test $slot != 'none' ; then > true ; > else > env get left BOOT_${x}_LEFT ; # magic > if test $left -gt 0 ; then > env set slot $x ; > fi; > fi; > done; For the benefit of people stumbling on this thread in the future, that #magic line is actually possible as of v2022.07 and is called "env indirect", guarded by CONFIG_CMD_NVEDIT_INDIRECT . Rasmus
--- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -386,6 +386,20 @@ static int do_env_set(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return _do_env_set(flag, argc, argv, H_INTERACTIVE); } +static int do_env_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + char *local_args[4]; + + if (argc != 3) + return CMD_RET_USAGE; + local_args[0] = argv[0]; + local_args[1] = argv[1]; + local_args[2] = env_get(argv[2]); + local_args[3] = NULL; + + return _do_env_set(flag, argc, local_args, H_INTERACTIVE); +} + /*