* [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
@ 2018-03-27 8:43 Alex Kiernan
2018-03-27 9:28 ` Quentin Schulz
0 siblings, 1 reply; 6+ messages in thread
From: Alex Kiernan @ 2018-03-27 8:43 UTC (permalink / raw)
To: u-boot
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)
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
2018-03-27 8:43 [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import Alex Kiernan
@ 2018-03-27 9:28 ` Quentin Schulz
2018-03-27 12:39 ` Alex Kiernan
0 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2018-03-27 9:28 UTC (permalink / raw)
To: u-boot
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 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)
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180327/71f93638/attachment-0001.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
2018-03-27 9:28 ` Quentin Schulz
@ 2018-03-27 12:39 ` Alex Kiernan
2018-03-27 13:14 ` Quentin Schulz
0 siblings, 1 reply; 6+ messages in thread
From: Alex Kiernan @ 2018-03-27 12:39 UTC (permalink / raw)
To: 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
--
Alex Kiernan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
2018-03-27 12:39 ` Alex Kiernan
@ 2018-03-27 13:14 ` Quentin Schulz
2018-03-27 13:30 ` Quentin Schulz
0 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2018-03-27 13:14 UTC (permalink / raw)
To: u-boot
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180327/770d87f4/attachment-0001.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
2018-03-27 13:14 ` Quentin Schulz
@ 2018-03-27 13:30 ` Quentin Schulz
2018-03-27 16:06 ` Alex Kiernan
0 siblings, 1 reply; 6+ messages in thread
From: Quentin Schulz @ 2018-03-27 13:30 UTC (permalink / raw)
To: u-boot
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180327/f552323d/attachment-0001.sig>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import
2018-03-27 13:30 ` Quentin Schulz
@ 2018-03-27 16:06 ` Alex Kiernan
0 siblings, 0 replies; 6+ messages in thread
From: Alex Kiernan @ 2018-03-27 16:06 UTC (permalink / raw)
To: u-boot
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.
--
Alex Kiernan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-27 16:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27 8:43 [U-Boot] [PATCH] cmd: nvedit: Add filtering during env import Alex Kiernan
2018-03-27 9:28 ` Quentin Schulz
2018-03-27 12:39 ` Alex Kiernan
2018-03-27 13:14 ` Quentin Schulz
2018-03-27 13:30 ` Quentin Schulz
2018-03-27 16:06 ` Alex Kiernan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.