[U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
When importing variables allow size to be elided using '-' and then support a list of variables which restricts what will be picked during the import.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com ---
cmd/nvedit.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 4cb25b8..486bb24 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -972,7 +972,7 @@ sep_err:
#ifdef CONFIG_CMD_IMPORTENV /* - * env import [-d] [-t [-r] | -b | -c] addr [size] + * env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] * -d: delete existing environment before importing; * otherwise overwrite / append to existing definitions * -t: assume text format; either "size" must be given or the @@ -985,7 +985,10 @@ sep_err: * -c: assume checksum protected environment format * addr: memory address to read from * size: length of input data; if missing, proper '\0' - * termination is mandatory + * termination is mandatory. If not required and passing + * variables to import use '-' + * var...: List of variable names that get imported. Without arguments, + * all variables are imported */ static int do_env_import(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -1043,11 +1046,20 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, crlf_is_lf = 0;
addr = simple_strtoul(argv[0], NULL, 16); + --argc; + ++argv; ptr = map_sysmem(addr, 0);
- if (argc == 2) { - size = simple_strtoul(argv[1], NULL, 16); - } else if (argc == 1 && chk) { + if (argc >= 1 && !strcmp(argv[0], "-")) { + --argc; + ++argv; + } + + if (argc >= 1) { + size = simple_strtoul(argv[0], NULL, 16); + --argc; + ++argv; + } else if (argc == 0 && chk) { puts("## Error: external checksum format must pass size\n"); return CMD_RET_FAILURE; } else { @@ -1084,7 +1096,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, }
if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, - crlf_is_lf, 0, NULL) == 0) { + crlf_is_lf, argc, argc ? argv : NULL) == 0) { pr_err("Environment import failed: errno = %d\n", errno); return 1; } @@ -1213,7 +1225,7 @@ static char env_help_text[] = #endif #endif #if defined(CONFIG_CMD_IMPORTENV) - "env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n" + "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n" #endif "env print [-a | name ...] - print environment\n" #if defined(CONFIG_CMD_RUN)
Hi Alex,
On Tue, Mar 27, 2018 at 08:43:26AM +0000, Alex Kiernan wrote:
When importing variables allow size to be elided using '-' and then support a list of variables which restricts what will be picked during the import.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
I'm pretty sure it's the same goal as this patch[1] I suggested. Could you answer in the thread telling you need it as well so that we could get it merged or at least reviewed?
Thanks, Quentin
[1] http://patchwork.ozlabs.org/patch/855542/
cmd/nvedit.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 4cb25b8..486bb24 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -972,7 +972,7 @@ sep_err:
#ifdef CONFIG_CMD_IMPORTENV /*
- env import [-d] [-t [-r] | -b | -c] addr [size]
- env import [-d] [-t [-r] | -b | -c] addr [size] [var ...]
- -d: delete existing environment before importing;
otherwise overwrite / append to existing definitions- -t: assume text format; either "size" must be given or the
@@ -985,7 +985,10 @@ sep_err:
- -c: assume checksum protected environment format
- addr: memory address to read from
- size: length of input data; if missing, proper '\0'
termination is mandatory
termination is mandatory. If not required and passing
variables to import use '-'
- var...: List of variable names that get imported. Without arguments,
*/
all variables are importedstatic int do_env_import(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -1043,11 +1046,20 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, crlf_is_lf = 0;
addr = simple_strtoul(argv[0], NULL, 16);
- --argc;
- ++argv; ptr = map_sysmem(addr, 0);
- if (argc == 2) {
size = simple_strtoul(argv[1], NULL, 16);- } else if (argc == 1 && chk) {
- if (argc >= 1 && !strcmp(argv[0], "-")) {
--argc;++argv;- }
- if (argc >= 1) {
size = simple_strtoul(argv[0], NULL, 16);--argc;++argv;- } else if (argc == 0 && chk) { puts("## Error: external checksum format must pass size\n"); return CMD_RET_FAILURE; } else {
@@ -1084,7 +1096,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, }
if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
crlf_is_lf, 0, NULL) == 0) {
crlf_is_lf, argc, argc ? argv : NULL) == 0) {@@ -1213,7 +1225,7 @@ static char env_help_text[] = #endif #endif #if defined(CONFIG_CMD_IMPORTENV)
- "env import [-d] [-t [-r] | -b | -c] addr [size] - import environment\n"
- "env import [-d] [-t [-r] | -b | -c] addr [size] [var ...] - import environment\n"
#endif "env print [-a | name ...] - print environment\n"
#if defined(CONFIG_CMD_RUN)
2.7.4
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Tue, Mar 27, 2018 at 10:28 AM, Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Tue, Mar 27, 2018 at 08:43:26AM +0000, Alex Kiernan wrote:
When importing variables allow size to be elided using '-' and then support a list of variables which restricts what will be picked during the import.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
I'm pretty sure it's the same goal as this patch[1] I suggested.
It is, maybe it was your message I was thinking of when I asked the question the other day:
https://lists.denx.de/pipermail/u-boot/2018-March/323687.html
Could you answer in the thread telling you need it as well so that we could get it merged or at least reviewed?
Assuming I've understood your patch correctly, I think I can replicate your use case with this:
env import ... ${whitelisted_vars}
I've two uses for this right now for this - with different white lists:
# override defaults from uboot.env if fatload mmc ${mmcdev} ${loadaddr} uboot.env; then env import -c ${loadaddr} ${filesize} serial# ethaddr fi
# source OSTree deployments if load ${devtype} ${bootpart} ${loadaddr} /boot/loader/uEnv.txt; then env import -t ${loadaddr} ${filesize} kernel_image kernel_image2 bootargs bootargs2 fi
On Tue, Mar 27, 2018 at 01:39:25PM +0100, Alex Kiernan wrote:
On Tue, Mar 27, 2018 at 10:28 AM, Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Tue, Mar 27, 2018 at 08:43:26AM +0000, Alex Kiernan wrote:
When importing variables allow size to be elided using '-' and then support a list of variables which restricts what will be picked during the import.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
I'm pretty sure it's the same goal as this patch[1] I suggested.
It is, maybe it was your message I was thinking of when I asked the question the other day:
https://lists.denx.de/pipermail/u-boot/2018-March/323687.html
Could you answer in the thread telling you need it as well so that we could get it merged or at least reviewed?
Assuming I've understood your patch correctly, I think I can replicate your use case with this:
env import ... ${whitelisted_vars}
I've two uses for this right now for this - with different white lists:
# override defaults from uboot.env if fatload mmc ${mmcdev} ${loadaddr} uboot.env; then env import -c ${loadaddr} ${filesize} serial# ethaddr fi
# source OSTree deployments if load ${devtype} ${bootpart} ${loadaddr} /boot/loader/uEnv.txt; then env import -t ${loadaddr} ${filesize} kernel_image kernel_image2 bootargs bootargs2 fi
What I don't like with this approach is that it's going to be very hard to read the line if you want to import a lot of variables.
You could do the same with just a
setenv whitelisted_vars my_var0 my_var1 env import -w -t ${loadaddr} ${filesize}
setenv whitelisted_vars my_var2 env import -w -t ${loadaddr} ${filesize}
I find it more readable.
Quentin
On Tue, Mar 27, 2018 at 03:14:59PM +0200, Quentin Schulz wrote:
On Tue, Mar 27, 2018 at 01:39:25PM +0100, Alex Kiernan wrote:
On Tue, Mar 27, 2018 at 10:28 AM, Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Alex,
On Tue, Mar 27, 2018 at 08:43:26AM +0000, Alex Kiernan wrote:
When importing variables allow size to be elided using '-' and then support a list of variables which restricts what will be picked during the import.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
I'm pretty sure it's the same goal as this patch[1] I suggested.
It is, maybe it was your message I was thinking of when I asked the question the other day:
https://lists.denx.de/pipermail/u-boot/2018-March/323687.html
Could you answer in the thread telling you need it as well so that we could get it merged or at least reviewed?
Assuming I've understood your patch correctly, I think I can replicate your use case with this:
env import ... ${whitelisted_vars}
I've two uses for this right now for this - with different white lists:
# override defaults from uboot.env if fatload mmc ${mmcdev} ${loadaddr} uboot.env; then env import -c ${loadaddr} ${filesize} serial# ethaddr fi
# source OSTree deployments if load ${devtype} ${bootpart} ${loadaddr} /boot/loader/uEnv.txt; then env import -t ${loadaddr} ${filesize} kernel_image kernel_image2 bootargs bootargs2 fi
What I don't like with this approach is that it's going to be very hard to read the line if you want to import a lot of variables.
You could do the same with just a
setenv whitelisted_vars my_var0 my_var1 env import -w -t ${loadaddr} ${filesize}
setenv whitelisted_vars my_var2 env import -w -t ${loadaddr} ${filesize}
I find it more readable.
OK, so that was a bad example as you could do the same with your patch :D
What I'm more concerned about is that after we've merged your patch, it isn't possible to pass multiple parameters at the end of the command. There might be (I have no example as of today) some things we want to do in the future that can't be done an other way than passing a list of arguments at the end of the command and your patch prevent doing so in the future.
We can do it now with a single option (-w or whatever letter we want to use) so I guess it's better to leave the option for cases that really require to have a list of parameters passed to the command.
Quentin
On Tue, Mar 27, 2018 at 2:30 PM, Quentin Schulz quentin.schulz@bootlin.com wrote:
What I'm more concerned about is that after we've merged your patch, it isn't possible to pass multiple parameters at the end of the command. There might be (I have no example as of today) some things we want to do in the future that can't be done an other way than passing a list of arguments at the end of the command and your patch prevent doing so in the future.
Fair comment... I was really just following what was already there for env export (so much so I could cut'n'paste the documentation changes).
We can do it now with a single option (-w or whatever letter we want to use) so I guess it's better to leave the option for cases that really require to have a list of parameters passed to the command.
I really don't like magic variables that get used invisibly, though I guess you could pass a variable in.
participants (2)
-
Alex Kiernan -
Quentin Schulz