* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
@ 2018-05-18 14:44 Quentin Schulz
2018-05-18 14:45 ` [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment Quentin Schulz
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Quentin Schulz @ 2018-05-18 14:44 UTC (permalink / raw
To: u-boot
While the `env export` can take as parameters variables to be exported,
`env import` does not have such a mechanism of variable selection.
Let's add a `-w` option that asks `env import` to look for the
`whitelisted_vars` env variable for a space-separated list of variables
that are whitelisted.
Every env variable present in env at `addr` and in `whitelisted_vars`
env variable will override the value of the variable in the current env.
All the remaining variables are left untouched.
One of its use case could be to load a secure environment from the
signed U-Boot binary and load only a handful of variables from an
other, unsecure, environment without completely losing control of
U-Boot.
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
v2:
- use strdup instead of malloc + strcpy,
- NULL-check the result of strdup,
- add common exit path for freeing memory in one unique place,
- store token pointer from strtok within the char** array instead of
strdup-ing token within elements of array,
cmd/nvedit.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 68 insertions(+), 11 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 4e79d03856..1e33a26f4c 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -971,7 +971,7 @@ sep_err:
#ifdef CONFIG_CMD_IMPORTENV
/*
- * env import [-d] [-t [-r] | -b | -c] addr [size]
+ * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
* -d: delete existing environment before importing;
* otherwise overwrite / append to existing definitions
* -t: assume text format; either "size" must be given or the
@@ -982,6 +982,10 @@ sep_err:
* for line endings. Only effective in addition to -t.
* -b: assume binary format ('\0' separated, "\0\0" terminated)
* -c: assume checksum protected environment format
+ * -w: specify that whitelisting of variables should be used when
+ * importing environment. The space-separated list of variables
+ * that should override the ones in current environment is stored
+ * in `whitelisted_vars`.
* addr: memory address to read from
* size: length of input data; if missing, proper '\0'
* termination is mandatory
@@ -990,18 +994,22 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
int argc, char * const argv[])
{
ulong addr;
- char *cmd, *ptr;
+ char *cmd, *ptr, *tmp;
+ char **array = NULL;
char sep = '\n';
int chk = 0;
int fmt = 0;
int del = 0;
int crlf_is_lf = 0;
+ int wl = 0;
+ int wl_count = 0;
+ int ret = 0;
size_t size;
cmd = *argv;
while (--argc > 0 && **++argv == '-') {
- char *arg = *argv;
+ char *arg = *argv, *str, *token;
while (*++arg) {
switch (*arg) {
case 'b': /* raw binary format */
@@ -1025,6 +1033,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
break;
case 'd':
del = 1;
+ break;
+ case 'w':
+ wl = 1;
+ wl_count = 1;
+
+ str = env_get("whitelisted_vars");
+ if (!str) {
+ puts("## Error: whitelisted_vars is not set.\n");
+ return CMD_RET_USAGE;
+ }
+
+ tmp = strdup(str);
+ if (!tmp)
+ return CMD_RET_FAILURE;
+
+ token = strchr(tmp, ' ');
+ while (!token) {
+ wl_count++;
+ token = strchr(token + 1, ' ');
+ }
+
+ strcpy(tmp, str);
+
+ wl_count = 0;
+ array = malloc(sizeof(char *) * wl_count);
+ if (!array) {
+ free(tmp);
+ return CMD_RET_FAILURE;
+ }
+
+ token = strtok(tmp, " ");
+ while (token) {
+ array[wl_count] = token;
+ wl_count++;
+ token = strtok(NULL, " ");
+ }
+
break;
default:
return CMD_RET_USAGE;
@@ -1032,8 +1077,10 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
}
}
- if (argc < 1)
- return CMD_RET_USAGE;
+ if (argc < 1) {
+ ret = CMD_RET_USAGE;
+ goto exit;
+ }
if (!fmt)
printf("## Warning: defaulting to text format\n");
@@ -1048,7 +1095,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
size = simple_strtoul(argv[1], NULL, 16);
} else if (argc == 1 && chk) {
puts("## Error: external checksum format must pass size\n");
- return CMD_RET_FAILURE;
+ ret = CMD_RET_FAILURE;
+ goto exit;
} else {
char *s = ptr;
@@ -1077,19 +1125,28 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
if (crc32(0, ep->data, size) != crc) {
puts("## Error: bad CRC, import failed\n");
- return 1;
+ ret = 1;
+ goto exit;
}
ptr = (char *)ep->data;
}
if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR,
- crlf_is_lf, 0, NULL) == 0) {
+ crlf_is_lf, wl ? wl_count : 0, wl ? array : NULL) == 0) {
pr_err("Environment import failed: errno = %d\n", errno);
- return 1;
+
+ ret = 1;
+ goto exit;
}
gd->flags |= GD_FLG_ENV_READY;
- return 0;
+exit:
+ if (wl) {
+ free(tmp);
+ free(array);
+ }
+
+ return ret;
sep_err:
printf("## %s: only one of \"-b\", \"-c\" or \"-t\" allowed\n",
@@ -1212,7 +1269,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] [-w] addr [size] - import environment\n"
#endif
"env print [-a | name ...] - print environment\n"
#if defined(CONFIG_CMD_RUN)
--
2.14.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment
2018-05-18 14:44 [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Quentin Schulz
@ 2018-05-18 14:45 ` Quentin Schulz
2018-05-18 16:04 ` Stephen Warren
2018-05-22 23:29 ` Simon Glass
2018-05-18 16:00 ` [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Stephen Warren
2018-05-20 13:01 ` Lothar Waßmann
2 siblings, 2 replies; 16+ messages in thread
From: Quentin Schulz @ 2018-05-18 14:45 UTC (permalink / raw
To: u-boot
This tests that the importing of an environment with a specified
whitelist works as intended.
If the variable whitelisted_vars is not set, the env importing should
fail with a given message.
For each variable separated by spaces in whitelisted_vars, if
- foo is bar in current env and bar2 in exported env, after importing
exported env, foo shall be bar2,
- foo does not exist in current env and foo is bar2 in exported env,
after importing exported env, foo shall be bar2,
- foo is bar in current env and does not exist in exported env (but is
in the whitelisted_vars), after importing exported env, foo shall be
empty,
Any variable not in whitelisted_vars should be left untouched.
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
added in v2
test/py/tests/test_env.py | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
index b7f960c755..d0a2e36876 100644
--- a/test/py/tests/test_env.py
+++ b/test/py/tests/test_env.py
@@ -6,6 +6,7 @@
# Test operation of shell commands relating to environment variables.
import pytest
+import u_boot_utils
# FIXME: This might be useful for other tests;
# perhaps refactor it into ConsoleBase or some other state object?
@@ -231,3 +232,42 @@ def test_env_expansion_spaces(state_test_env):
unset_var(state_test_env, var_space)
if var_test:
unset_var(state_test_env, var_test)
+
+def test_env_import_whitelist_empty(state_test_env):
+ """Test trying to import 0 variables from an environment"""
+ ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+ addr = '%08x' % ram_base
+
+ set_var(state_test_env, "foo1", "bar1")
+
+ state_test_env.u_boot_console.run_command('env export %s' % addr)
+
+ c = state_test_env.u_boot_console
+ with c.disable_check('error_notification'):
+ response = c.run_command('env import -w %s' % addr)
+ assert(response.startswith('## Error: whitelisted_vars is not set'))
+
+def test_env_import_whitelist(state_test_env):
+ """Test importing only a handful of env variables from an environment"""
+ ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console)
+ addr = '%08x' % ram_base
+
+ #no foo1 in current env, foo2 overridden, foo3 should be of the value
+ #before exporting and foo4 should be deleted
+ set_var(state_test_env, "whitelisted_vars", "foo1 foo2 foo4")
+ set_var(state_test_env, "foo1", "bar1")
+ set_var(state_test_env, "foo2", "bar2")
+ set_var(state_test_env, "foo3", "bar3")
+
+ state_test_env.u_boot_console.run_command('env export %s' % addr)
+
+ unset_var(state_test_env, "foo1")
+ set_var(state_test_env, "foo2", "test2")
+ set_var(state_test_env, "foo4", "bar4")
+
+ state_test_env.u_boot_console.run_command('env import -w %s' % addr)
+
+ validate_set(state_test_env, "foo1", "bar1")
+ validate_set(state_test_env, "foo2", "bar2")
+ validate_set(state_test_env, "foo3", "bar3")
+ validate_empty(state_test_env, "foo4")
--
2.14.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-18 14:44 [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Quentin Schulz
2018-05-18 14:45 ` [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment Quentin Schulz
@ 2018-05-18 16:00 ` Stephen Warren
2018-05-21 8:01 ` Quentin Schulz
2018-05-20 13:01 ` Lothar Waßmann
2 siblings, 1 reply; 16+ messages in thread
From: Stephen Warren @ 2018-05-18 16:00 UTC (permalink / raw
To: u-boot
On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> While the `env export` can take as parameters variables to be exported,
> `env import` does not have such a mechanism of variable selection.
>
> Let's add a `-w` option that asks `env import` to look for the
> `whitelisted_vars` env variable for a space-separated list of variables
> that are whitelisted.
Would it be better for the -w option to accept a variable name rather
than hard-coding it as whitelisted_vars? That way, people could
import/export different sets of variables at different times, and also
could choose a more use-case-specific variable name than
whitelisted_vars in order to describe why those variables are "whitelisted".
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment
2018-05-18 14:45 ` [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment Quentin Schulz
@ 2018-05-18 16:04 ` Stephen Warren
2018-05-21 7:43 ` Quentin Schulz
2018-05-22 23:29 ` Simon Glass
1 sibling, 1 reply; 16+ messages in thread
From: Stephen Warren @ 2018-05-18 16:04 UTC (permalink / raw
To: u-boot
On 05/18/2018 08:45 AM, Quentin Schulz wrote:
> This tests that the importing of an environment with a specified
> whitelist works as intended.
>
> If the variable whitelisted_vars is not set, the env importing should
> fail with a given message.
>
> For each variable separated by spaces in whitelisted_vars, if
> - foo is bar in current env and bar2 in exported env, after importing
> exported env, foo shall be bar2,
> - foo does not exist in current env and foo is bar2 in exported env,
> after importing exported env, foo shall be bar2,
> - foo is bar in current env and does not exist in exported env (but is
> in the whitelisted_vars), after importing exported env, foo shall be
> empty,
>
> Any variable not in whitelisted_vars should be left untouched.
Acked-by: Stephen Warren <swarren@nvidia.com>
> diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
> +def test_env_import_whitelist(state_test_env):
> + state_test_env.u_boot_console.run_command('env import -w %s' % addr)
> +
> + validate_set(state_test_env, "foo1", "bar1")
> + validate_set(state_test_env, "foo2", "bar2")
> + validate_set(state_test_env, "foo3", "bar3")
> + validate_empty(state_test_env, "foo4")
It might make sense to iterate over *all* variables and make sure that
they have the same value before/after the import. But, the current code
seems to cover the basic cases, so it's probably not strictly necessary
to do that.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-18 14:44 [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Quentin Schulz
2018-05-18 14:45 ` [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment Quentin Schulz
2018-05-18 16:00 ` [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Stephen Warren
@ 2018-05-20 13:01 ` Lothar Waßmann
2018-05-21 7:29 ` Quentin Schulz
2 siblings, 1 reply; 16+ messages in thread
From: Lothar Waßmann @ 2018-05-20 13:01 UTC (permalink / raw
To: u-boot
Hi,
On Fri, 18 May 2018 16:44:59 +0200 Quentin Schulz wrote:
> While the `env export` can take as parameters variables to be exported,
> `env import` does not have such a mechanism of variable selection.
>
> Let's add a `-w` option that asks `env import` to look for the
> `whitelisted_vars` env variable for a space-separated list of variables
> that are whitelisted.
>
> Every env variable present in env at `addr` and in `whitelisted_vars`
> env variable will override the value of the variable in the current env.
> All the remaining variables are left untouched.
>
> One of its use case could be to load a secure environment from the
> signed U-Boot binary and load only a handful of variables from an
> other, unsecure, environment without completely losing control of
> U-Boot.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
>
> v2:
> - use strdup instead of malloc + strcpy,
> - NULL-check the result of strdup,
> - add common exit path for freeing memory in one unique place,
> - store token pointer from strtok within the char** array instead of
> strdup-ing token within elements of array,
>
> cmd/nvedit.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 68 insertions(+), 11 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 4e79d03856..1e33a26f4c 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -971,7 +971,7 @@ sep_err:
>
> #ifdef CONFIG_CMD_IMPORTENV
> /*
> - * env import [-d] [-t [-r] | -b | -c] addr [size]
> + * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
> * -d: delete existing environment before importing;
> * otherwise overwrite / append to existing definitions
> * -t: assume text format; either "size" must be given or the
> @@ -982,6 +982,10 @@ sep_err:
> * for line endings. Only effective in addition to -t.
> * -b: assume binary format ('\0' separated, "\0\0" terminated)
> * -c: assume checksum protected environment format
> + * -w: specify that whitelisting of variables should be used when
> + * importing environment. The space-separated list of variables
> + * that should override the ones in current environment is stored
> + * in `whitelisted_vars`.
> * addr: memory address to read from
> * size: length of input data; if missing, proper '\0'
> * termination is mandatory
> @@ -990,18 +994,22 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> int argc, char * const argv[])
> {
> ulong addr;
> - char *cmd, *ptr;
> + char *cmd, *ptr, *tmp;
> + char **array = NULL;
> char sep = '\n';
> int chk = 0;
> int fmt = 0;
> int del = 0;
> int crlf_is_lf = 0;
> + int wl = 0;
> + int wl_count = 0;
> + int ret = 0;
> size_t size;
>
> cmd = *argv;
>
> while (--argc > 0 && **++argv == '-') {
> - char *arg = *argv;
> + char *arg = *argv, *str, *token;
> while (*++arg) {
> switch (*arg) {
> case 'b': /* raw binary format */
> @@ -1025,6 +1033,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> break;
> case 'd':
> del = 1;
> + break;
> + case 'w':
> + wl = 1;
> + wl_count = 1;
> +
> + str = env_get("whitelisted_vars");
> + if (!str) {
> + puts("## Error: whitelisted_vars is not set.\n");
> + return CMD_RET_USAGE;
> + }
> +
> + tmp = strdup(str);
> + if (!tmp)
> + return CMD_RET_FAILURE;
> +
> + token = strchr(tmp, ' ');
> + while (!token) {
> + wl_count++;
> + token = strchr(token + 1, ' ');
> + }
> +
> + strcpy(tmp, str);
>
This is redundant. strchr() doesn't modify the array.
> + wl_count = 0;
> + array = malloc(sizeof(char *) * wl_count);
>
You have juste before reset wl_count to 0 are mallocing a zero sized
array here!
> + if (!array) {
> + free(tmp);
> + return CMD_RET_FAILURE;
> + }
> +
>
wl_count should probably be zeroed here...
But I would keep the predetermined vlaue of wl_count and check against
it in the following loop to be sure not to overflow the allocated
array, even in the improbable case, that strtok() should happen to
return more tokens, than you tested before with the strchr() loop.
Lothar Waßmann
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-20 13:01 ` Lothar Waßmann
@ 2018-05-21 7:29 ` Quentin Schulz
0 siblings, 0 replies; 16+ messages in thread
From: Quentin Schulz @ 2018-05-21 7:29 UTC (permalink / raw
To: u-boot
Hi Lothar,
On Sun, May 20, 2018 at 03:01:22PM +0200, Lothar Waßmann wrote:
> Hi,
>
> On Fri, 18 May 2018 16:44:59 +0200 Quentin Schulz wrote:
> > While the `env export` can take as parameters variables to be exported,
> > `env import` does not have such a mechanism of variable selection.
> >
> > Let's add a `-w` option that asks `env import` to look for the
> > `whitelisted_vars` env variable for a space-separated list of variables
> > that are whitelisted.
> >
> > Every env variable present in env at `addr` and in `whitelisted_vars`
> > env variable will override the value of the variable in the current env.
> > All the remaining variables are left untouched.
> >
> > One of its use case could be to load a secure environment from the
> > signed U-Boot binary and load only a handful of variables from an
> > other, unsecure, environment without completely losing control of
> > U-Boot.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> >
> > v2:
> > - use strdup instead of malloc + strcpy,
> > - NULL-check the result of strdup,
> > - add common exit path for freeing memory in one unique place,
> > - store token pointer from strtok within the char** array instead of
> > strdup-ing token within elements of array,
> >
> > cmd/nvedit.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 68 insertions(+), 11 deletions(-)
> >
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index 4e79d03856..1e33a26f4c 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -971,7 +971,7 @@ sep_err:
> >
> > #ifdef CONFIG_CMD_IMPORTENV
> > /*
> > - * env import [-d] [-t [-r] | -b | -c] addr [size]
> > + * env import [-d] [-t [-r] | -b | -c] [-w] addr [size]
> > * -d: delete existing environment before importing;
> > * otherwise overwrite / append to existing definitions
> > * -t: assume text format; either "size" must be given or the
> > @@ -982,6 +982,10 @@ sep_err:
> > * for line endings. Only effective in addition to -t.
> > * -b: assume binary format ('\0' separated, "\0\0" terminated)
> > * -c: assume checksum protected environment format
> > + * -w: specify that whitelisting of variables should be used when
> > + * importing environment. The space-separated list of variables
> > + * that should override the ones in current environment is stored
> > + * in `whitelisted_vars`.
> > * addr: memory address to read from
> > * size: length of input data; if missing, proper '\0'
> > * termination is mandatory
> > @@ -990,18 +994,22 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > int argc, char * const argv[])
> > {
> > ulong addr;
> > - char *cmd, *ptr;
> > + char *cmd, *ptr, *tmp;
> > + char **array = NULL;
> > char sep = '\n';
> > int chk = 0;
> > int fmt = 0;
> > int del = 0;
> > int crlf_is_lf = 0;
> > + int wl = 0;
> > + int wl_count = 0;
> > + int ret = 0;
> > size_t size;
> >
> > cmd = *argv;
> >
> > while (--argc > 0 && **++argv == '-') {
> > - char *arg = *argv;
> > + char *arg = *argv, *str, *token;
> > while (*++arg) {
> > switch (*arg) {
> > case 'b': /* raw binary format */
> > @@ -1025,6 +1033,43 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag,
> > break;
> > case 'd':
> > del = 1;
> > + break;
> > + case 'w':
> > + wl = 1;
> > + wl_count = 1;
> > +
> > + str = env_get("whitelisted_vars");
> > + if (!str) {
> > + puts("## Error: whitelisted_vars is not set.\n");
> > + return CMD_RET_USAGE;
> > + }
> > +
> > + tmp = strdup(str);
> > + if (!tmp)
> > + return CMD_RET_FAILURE;
> > +
> > + token = strchr(tmp, ' ');
> > + while (!token) {
> > + wl_count++;
> > + token = strchr(token + 1, ' ');
> > + }
> > +
> > + strcpy(tmp, str);
> >
> This is redundant. strchr() doesn't modify the array.
>
ACK.
> > + wl_count = 0;
> > + array = malloc(sizeof(char *) * wl_count);
> >
> You have juste before reset wl_count to 0 are mallocing a zero sized
> array here!
>
Don't know how I could get the sandbox test to pass with this error,
thanks.
> > + if (!array) {
> > + free(tmp);
> > + return CMD_RET_FAILURE;
> > + }
> > +
> >
> wl_count should probably be zeroed here...
Indeed.
> But I would keep the predetermined vlaue of wl_count and check against
> it in the following loop to be sure not to overflow the allocated
> array, even in the improbable case, that strtok() should happen to
> return more tokens, than you tested before with the strchr() loop.
>
Good point, never too safe to double check.
Thanks,
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/20180521/e3325d53/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment
2018-05-18 16:04 ` Stephen Warren
@ 2018-05-21 7:43 ` Quentin Schulz
0 siblings, 0 replies; 16+ messages in thread
From: Quentin Schulz @ 2018-05-21 7:43 UTC (permalink / raw
To: u-boot
Hi Stephen,
On Fri, May 18, 2018 at 10:04:05AM -0600, Stephen Warren wrote:
> On 05/18/2018 08:45 AM, Quentin Schulz wrote:
> > This tests that the importing of an environment with a specified
> > whitelist works as intended.
> >
> > If the variable whitelisted_vars is not set, the env importing should
> > fail with a given message.
> >
> > For each variable separated by spaces in whitelisted_vars, if
> > - foo is bar in current env and bar2 in exported env, after importing
> > exported env, foo shall be bar2,
> > - foo does not exist in current env and foo is bar2 in exported env,
> > after importing exported env, foo shall be bar2,
> > - foo is bar in current env and does not exist in exported env (but is
> > in the whitelisted_vars), after importing exported env, foo shall be
> > empty,
> >
> > Any variable not in whitelisted_vars should be left untouched.
>
> Acked-by: Stephen Warren <swarren@nvidia.com>
>
> > diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
>
> > +def test_env_import_whitelist(state_test_env):
>
> > + state_test_env.u_boot_console.run_command('env import -w %s' % addr)
> > +
> > + validate_set(state_test_env, "foo1", "bar1")
> > + validate_set(state_test_env, "foo2", "bar2")
> > + validate_set(state_test_env, "foo3", "bar3")
> > + validate_empty(state_test_env, "foo4")
>
> It might make sense to iterate over *all* variables and make sure that they
> have the same value before/after the import. But, the current code seems to
> cover the basic cases, so it's probably not strictly necessary to do that.
I would advocate for as many test sets as possible, each one testing a
very specific scenario. That way, it's easy to know which scenario
failed instead of having to find within a big scenario what failed and
why. Here, I just want to test that importing some env variables work.
It would make sense to have a small test that tests that setting an
environment variable works (that what's done with test_env_set* test
functions I think) and others to test the exporting of variables and the
importing of some others with all the possible/impossible arguments of
these cli commands. I wish I could test this importing without manually
exporting an environnement before to reduce "dependencies" of the test
and thus keep the test scenario to the actual command we're testing.
However the more the arguments of the commands, the more functions to
write ("exponentially") as we have to test each and every case. The best
would be to also have tests for expected and wanted failures. I think
it's a good start for now but more needs to be done in the env testing
regarding (at least) env exporting and importing.
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/20180521/92f4b525/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-18 16:00 ` [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Stephen Warren
@ 2018-05-21 8:01 ` Quentin Schulz
2018-05-21 11:56 ` Alex Kiernan
0 siblings, 1 reply; 16+ messages in thread
From: Quentin Schulz @ 2018-05-21 8:01 UTC (permalink / raw
To: u-boot
Hi Stephen,
On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > While the `env export` can take as parameters variables to be exported,
> > `env import` does not have such a mechanism of variable selection.
> >
> > Let's add a `-w` option that asks `env import` to look for the
> > `whitelisted_vars` env variable for a space-separated list of variables
> > that are whitelisted.
>
> Would it be better for the -w option to accept a variable name rather than
> hard-coding it as whitelisted_vars? That way, people could import/export
> different sets of variables at different times, and also could choose a more
> use-case-specific variable name than whitelisted_vars in order to describe
> why those variables are "whitelisted".
This has been raised in the previous version of the patch[1] (of which
you weren't in the mail recipients) and a similar patch[2] made by Alex
Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
though I should have mentioned it in the comments of the patch?
TL;DR:
Proposition 1: Have -w only which "hardcodedly" checks for
"whitelisting_vars",
+: straightforward implementation of the argument parsing,
-: implicit declaration of the list: you have to know to set
whitelisted_vars in the environnement,
Proposition 2: Have -w followed by one string-word which is the name of
the env variable where to look for the list of whitelisted env
variables,
+: explicit var to check where whitelist is looked for,
-: a bit of complexity added to the parsing of the parameters of the env
import function,
Proposition 3: Have -w followed by the list of whitelisted env variable,
+: explicit list
-: the list cannot be separated by comma (valid character for an env
variable) or a space (would not be able to distinguish the last
arguments of the commands which are address and size with size being
optional => how to know if size was passed or not?), what char can be
used to separate env variables in the list?
how does it perform with a very long list of whitelisted variables?
I don't know how many features and their complexity we can add to the
env import/export functions, but keeping their arguments simple is
a way to keep features hopefully easy to implement in the future.
I'm not convinced by any of the propositions but proposition 1 is the
one I implemented for now. I just want a consensus towards a solution
(be it among the given 3 or another one) and I'm good with it.
Thanks,
Quentin
[1] https://www.mail-archive.com/u-boot at lists.denx.de/msg286266.html
[2] http://patchwork.ozlabs.org/patch/891422/
-------------- 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/20180521/963238a5/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 8:01 ` Quentin Schulz
@ 2018-05-21 11:56 ` Alex Kiernan
2018-05-21 12:06 ` Quentin Schulz
0 siblings, 1 reply; 16+ messages in thread
From: Alex Kiernan @ 2018-05-21 11:56 UTC (permalink / raw
To: u-boot
On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <quentin.schulz@bootlin.com>
wrote:
> Hi Stephen,
> On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > While the `env export` can take as parameters variables to be
exported,
> > > `env import` does not have such a mechanism of variable selection.
> > >
> > > Let's add a `-w` option that asks `env import` to look for the
> > > `whitelisted_vars` env variable for a space-separated list of
variables
> > > that are whitelisted.
> >
> > Would it be better for the -w option to accept a variable name rather
than
> > hard-coding it as whitelisted_vars? That way, people could import/export
> > different sets of variables at different times, and also could choose a
more
> > use-case-specific variable name than whitelisted_vars in order to
describe
> > why those variables are "whitelisted".
> This has been raised in the previous version of the patch[1] (of which
> you weren't in the mail recipients) and a similar patch[2] made by Alex
> Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
> though I should have mentioned it in the comments of the patch?
> TL;DR:
> Proposition 1: Have -w only which "hardcodedly" checks for
> "whitelisting_vars",
> +: straightforward implementation of the argument parsing,
> -: implicit declaration of the list: you have to know to set
> whitelisted_vars in the environnement,
> Proposition 2: Have -w followed by one string-word which is the name of
> the env variable where to look for the list of whitelisted env
> variables,
> +: explicit var to check where whitelist is looked for,
> -: a bit of complexity added to the parsing of the parameters of the env
> import function,
> Proposition 3: Have -w followed by the list of whitelisted env variable,
> +: explicit list
> -: the list cannot be separated by comma (valid character for an env
> variable) or a space (would not be able to distinguish the last
> arguments of the commands which are address and size with size being
> optional => how to know if size was passed or not?), what char can be
> used to separate env variables in the list?
> how does it perform with a very long list of whitelisted variables?
Two more thoughts, both of which delegate the separator problem to the
caller (the second being the one I implemented as it's almost no code)
- specify multiple -w options each specifying a whitelisted env variable
- use the remaining arguments approach and eat all the trailing arguments
as the names of env vars you import - needs a sentinel value for the size
argument
--
Alex Kiernan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 11:56 ` Alex Kiernan
@ 2018-05-21 12:06 ` Quentin Schulz
2018-05-21 12:26 ` Alex Kiernan
2018-05-21 12:36 ` Alex Kiernan
0 siblings, 2 replies; 16+ messages in thread
From: Quentin Schulz @ 2018-05-21 12:06 UTC (permalink / raw
To: u-boot
Hi Alex,
On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <quentin.schulz@bootlin.com>
> wrote:
>
> > Hi Stephen,
>
> > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > > While the `env export` can take as parameters variables to be
> exported,
> > > > `env import` does not have such a mechanism of variable selection.
> > > >
> > > > Let's add a `-w` option that asks `env import` to look for the
> > > > `whitelisted_vars` env variable for a space-separated list of
> variables
> > > > that are whitelisted.
> > >
> > > Would it be better for the -w option to accept a variable name rather
> than
> > > hard-coding it as whitelisted_vars? That way, people could import/export
> > > different sets of variables at different times, and also could choose a
> more
> > > use-case-specific variable name than whitelisted_vars in order to
> describe
> > > why those variables are "whitelisted".
>
> > This has been raised in the previous version of the patch[1] (of which
> > you weren't in the mail recipients) and a similar patch[2] made by Alex
> > Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
> > though I should have mentioned it in the comments of the patch?
>
> > TL;DR:
> > Proposition 1: Have -w only which "hardcodedly" checks for
> > "whitelisting_vars",
> > +: straightforward implementation of the argument parsing,
> > -: implicit declaration of the list: you have to know to set
> > whitelisted_vars in the environnement,
>
> > Proposition 2: Have -w followed by one string-word which is the name of
> > the env variable where to look for the list of whitelisted env
> > variables,
> > +: explicit var to check where whitelist is looked for,
> > -: a bit of complexity added to the parsing of the parameters of the env
> > import function,
>
> > Proposition 3: Have -w followed by the list of whitelisted env variable,
> > +: explicit list
> > -: the list cannot be separated by comma (valid character for an env
> > variable) or a space (would not be able to distinguish the last
> > arguments of the commands which are address and size with size being
> > optional => how to know if size was passed or not?), what char can be
> > used to separate env variables in the list?
> > how does it perform with a very long list of whitelisted variables?
>
>
> Two more thoughts, both of which delegate the separator problem to the
> caller (the second being the one I implemented as it's almost no code)
>
> - specify multiple -w options each specifying a whitelisted env variable
You'll hit the maximum number of arguments/length of the command quickly
with this method. Quicker than with the other propositions.
Moreover, this can make the command painfully long, painful to read and
thus cumbersome to find the small typo in your command.
> - use the remaining arguments approach and eat all the trailing arguments
> as the names of env vars you import - needs a sentinel value for the size
> argument
>
That can't work I think.
How do you know if the size argument was passed or not? How'd you know
what string is addr, size or the whitelist (if there is even any)?
env import foo1 foo2 foo3 foo4 addr size
env import foo1 foo2 foo3 addr
env import addr size
env import addr
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/20180521/3b140572/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 12:06 ` Quentin Schulz
@ 2018-05-21 12:26 ` Alex Kiernan
2018-05-21 12:34 ` Quentin Schulz
2018-05-21 12:36 ` Alex Kiernan
1 sibling, 1 reply; 16+ messages in thread
From: Alex Kiernan @ 2018-05-21 12:26 UTC (permalink / raw
To: u-boot
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <quentin.schulz@bootlin.com>
wrote:
> Hi Alex,
> On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> > On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
> > wrote:
> >
> > > Hi Stephen,
> >
> > > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > > > While the `env export` can take as parameters variables to be
> > exported,
> > > > > `env import` does not have such a mechanism of variable selection.
> > > > >
> > > > > Let's add a `-w` option that asks `env import` to look for the
> > > > > `whitelisted_vars` env variable for a space-separated list of
> > variables
> > > > > that are whitelisted.
> > > >
> > > > Would it be better for the -w option to accept a variable name
rather
> > than
> > > > hard-coding it as whitelisted_vars? That way, people could
import/export
> > > > different sets of variables at different times, and also could
choose a
> > more
> > > > use-case-specific variable name than whitelisted_vars in order to
> > describe
> > > > why those variables are "whitelisted".
> >
> > > This has been raised in the previous version of the patch[1] (of which
> > > you weren't in the mail recipients) and a similar patch[2] made by
Alex
> > > Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
> > > though I should have mentioned it in the comments of the patch?
> >
> > > TL;DR:
> > > Proposition 1: Have -w only which "hardcodedly" checks for
> > > "whitelisting_vars",
> > > +: straightforward implementation of the argument parsing,
> > > -: implicit declaration of the list: you have to know to set
> > > whitelisted_vars in the environnement,
> >
> > > Proposition 2: Have -w followed by one string-word which is the name
of
> > > the env variable where to look for the list of whitelisted env
> > > variables,
> > > +: explicit var to check where whitelist is looked for,
> > > -: a bit of complexity added to the parsing of the parameters of the
env
> > > import function,
> >
> > > Proposition 3: Have -w followed by the list of whitelisted env
variable,
> > > +: explicit list
> > > -: the list cannot be separated by comma (valid character for an env
> > > variable) or a space (would not be able to distinguish the last
> > > arguments of the commands which are address and size with size
being
> > > optional => how to know if size was passed or not?), what char
can be
> > > used to separate env variables in the list?
> > > how does it perform with a very long list of whitelisted
variables?
> >
> >
> > Two more thoughts, both of which delegate the separator problem to the
> > caller (the second being the one I implemented as it's almost no code)
> >
> > - specify multiple -w options each specifying a whitelisted env variable
> You'll hit the maximum number of arguments/length of the command quickly
> with this method. Quicker than with the other propositions.
> Moreover, this can make the command painfully long, painful to read and
> thus cumbersome to find the small typo in your command.
> > - use the remaining arguments approach and eat all the trailing
arguments
> > as the names of env vars you import - needs a sentinel value for the
size
> > argument
> >
> That can't work I think.
> How do you know if the size argument was passed or not? How'd you know
> what string is addr, size or the whitelist (if there is even any)?
> env import foo1 foo2 foo3 foo4 addr size
> env import foo1 foo2 foo3 addr
> env import addr size
> env import addr
That's why you need a sentinel for the 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
* text data must be '\0' terminated
* -r: handle CRLF like LF, that means exported variables with
* a content which ends with \r won't get imported. Used
* to import text files created with editors which are using
CRLF
* for line endings. Only effective in addition to -t.
* -b: assume binary format ('\0' separated, "\0\0" terminated)
* -c: assume checksum protected environment format
* addr: memory address to read from
* size: length of input data; if missing, proper '\0'
* 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
Which for your examples translates to:
env import addr size foo1 foo2 foo3 foo4
env import addr - foo1 foo2 foo3
env import addr size
env import addr
--
Alex Kiernan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 12:26 ` Alex Kiernan
@ 2018-05-21 12:34 ` Quentin Schulz
2018-05-21 12:51 ` Alex Kiernan
0 siblings, 1 reply; 16+ messages in thread
From: Quentin Schulz @ 2018-05-21 12:34 UTC (permalink / raw
To: u-boot
On Mon, May 21, 2018 at 01:26:11PM +0100, Alex Kiernan wrote:
> On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <quentin.schulz@bootlin.com>
> wrote:
>
> > Hi Alex,
>
> > On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> > > On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
> quentin.schulz at bootlin.com>
> > > wrote:
> > >
> > > > Hi Stephen,
> > >
> > > > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > > > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > > > > While the `env export` can take as parameters variables to be
> > > exported,
> > > > > > `env import` does not have such a mechanism of variable selection.
> > > > > >
> > > > > > Let's add a `-w` option that asks `env import` to look for the
> > > > > > `whitelisted_vars` env variable for a space-separated list of
> > > variables
> > > > > > that are whitelisted.
> > > > >
> > > > > Would it be better for the -w option to accept a variable name
> rather
> > > than
> > > > > hard-coding it as whitelisted_vars? That way, people could
> import/export
> > > > > different sets of variables at different times, and also could
> choose a
> > > more
> > > > > use-case-specific variable name than whitelisted_vars in order to
> > > describe
> > > > > why those variables are "whitelisted".
> > >
> > > > This has been raised in the previous version of the patch[1] (of which
> > > > you weren't in the mail recipients) and a similar patch[2] made by
> Alex
> > > > Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
> > > > though I should have mentioned it in the comments of the patch?
> > >
> > > > TL;DR:
> > > > Proposition 1: Have -w only which "hardcodedly" checks for
> > > > "whitelisting_vars",
> > > > +: straightforward implementation of the argument parsing,
> > > > -: implicit declaration of the list: you have to know to set
> > > > whitelisted_vars in the environnement,
> > >
> > > > Proposition 2: Have -w followed by one string-word which is the name
> of
> > > > the env variable where to look for the list of whitelisted env
> > > > variables,
> > > > +: explicit var to check where whitelist is looked for,
> > > > -: a bit of complexity added to the parsing of the parameters of the
> env
> > > > import function,
> > >
> > > > Proposition 3: Have -w followed by the list of whitelisted env
> variable,
> > > > +: explicit list
> > > > -: the list cannot be separated by comma (valid character for an env
> > > > variable) or a space (would not be able to distinguish the last
> > > > arguments of the commands which are address and size with size
> being
> > > > optional => how to know if size was passed or not?), what char
> can be
> > > > used to separate env variables in the list?
> > > > how does it perform with a very long list of whitelisted
> variables?
> > >
> > >
> > > Two more thoughts, both of which delegate the separator problem to the
> > > caller (the second being the one I implemented as it's almost no code)
> > >
> > > - specify multiple -w options each specifying a whitelisted env variable
>
> > You'll hit the maximum number of arguments/length of the command quickly
> > with this method. Quicker than with the other propositions.
>
> > Moreover, this can make the command painfully long, painful to read and
> > thus cumbersome to find the small typo in your command.
>
> > > - use the remaining arguments approach and eat all the trailing
> arguments
> > > as the names of env vars you import - needs a sentinel value for the
> size
> > > argument
> > >
>
> > That can't work I think.
>
> > How do you know if the size argument was passed or not? How'd you know
> > what string is addr, size or the whitelist (if there is even any)?
>
> > env import foo1 foo2 foo3 foo4 addr size
> > env import foo1 foo2 foo3 addr
> > env import addr size
> > env import addr
>
>
> That's why you need a sentinel for the 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
> * text data must be '\0' terminated
> * -r: handle CRLF like LF, that means exported variables with
> * a content which ends with \r won't get imported. Used
> * to import text files created with editors which are using
> CRLF
> * for line endings. Only effective in addition to -t.
> * -b: assume binary format ('\0' separated, "\0\0" terminated)
> * -c: assume checksum protected environment format
> * addr: memory address to read from
> * size: length of input data; if missing, proper '\0'
> * 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
>
> Which for your examples translates to:
>
> env import addr size foo1 foo2 foo3 foo4
> env import addr - foo1 foo2 foo3
> env import addr size
> env import addr
>
Ah, I misunderstood the word sentinel then :)
Would have been an even better idea if we had some consistency between
env export and env import. For specifying the env export size, we have
to use the -s argument but it's a positional argument for env export. We
then have a list of variables to export, so it would make sense to have
the same for env import I guess?
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/20180521/83f30249/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 12:06 ` Quentin Schulz
2018-05-21 12:26 ` Alex Kiernan
@ 2018-05-21 12:36 ` Alex Kiernan
2018-05-21 12:41 ` Quentin Schulz
1 sibling, 1 reply; 16+ messages in thread
From: Alex Kiernan @ 2018-05-21 12:36 UTC (permalink / raw
To: u-boot
On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <quentin.schulz@bootlin.com>
wrote:
> Hi Alex,
> On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> > On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
quentin.schulz@bootlin.com>
> > wrote:
> >
> > > Hi Stephen,
> >
> > > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > > > While the `env export` can take as parameters variables to be
> > exported,
> > > > > `env import` does not have such a mechanism of variable selection.
> > > > >
> > > > > Let's add a `-w` option that asks `env import` to look for the
> > > > > `whitelisted_vars` env variable for a space-separated list of
> > variables
> > > > > that are whitelisted.
> > > >
> > > > Would it be better for the -w option to accept a variable name
rather
> > than
> > > > hard-coding it as whitelisted_vars? That way, people could
import/export
> > > > different sets of variables at different times, and also could
choose a
> > more
> > > > use-case-specific variable name than whitelisted_vars in order to
> > describe
> > > > why those variables are "whitelisted".
> >
> > > This has been raised in the previous version of the patch[1] (of which
> > > you weren't in the mail recipients) and a similar patch[2] made by
Alex
> > > Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
> > > though I should have mentioned it in the comments of the patch?
> >
> > > TL;DR:
> > > Proposition 1: Have -w only which "hardcodedly" checks for
> > > "whitelisting_vars",
> > > +: straightforward implementation of the argument parsing,
> > > -: implicit declaration of the list: you have to know to set
> > > whitelisted_vars in the environnement,
> >
> > > Proposition 2: Have -w followed by one string-word which is the name
of
> > > the env variable where to look for the list of whitelisted env
> > > variables,
> > > +: explicit var to check where whitelist is looked for,
> > > -: a bit of complexity added to the parsing of the parameters of the
env
> > > import function,
> >
> > > Proposition 3: Have -w followed by the list of whitelisted env
variable,
> > > +: explicit list
> > > -: the list cannot be separated by comma (valid character for an env
> > > variable) or a space (would not be able to distinguish the last
> > > arguments of the commands which are address and size with size
being
> > > optional => how to know if size was passed or not?), what char
can be
> > > used to separate env variables in the list?
> > > how does it perform with a very long list of whitelisted
variables?
> >
> >
> > Two more thoughts, both of which delegate the separator problem to the
> > caller (the second being the one I implemented as it's almost no code)
> >
> > - specify multiple -w options each specifying a whitelisted env variable
> You'll hit the maximum number of arguments/length of the command quickly
> with this method. Quicker than with the other propositions.
So you just made me go and read the definition for CONFIG_SYS_MAXARGS, the
default's clearly pretty small (16). I guess I don't have a feeling for how
many args you want to import - my use case is for 4, so even with max args
at
16 I'm within that limit.
--
Alex Kiernan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 12:36 ` Alex Kiernan
@ 2018-05-21 12:41 ` Quentin Schulz
0 siblings, 0 replies; 16+ messages in thread
From: Quentin Schulz @ 2018-05-21 12:41 UTC (permalink / raw
To: u-boot
On Mon, May 21, 2018 at 01:36:49PM +0100, Alex Kiernan wrote:
> On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <quentin.schulz@bootlin.com>
> wrote:
>
> > Hi Alex,
>
> > On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> > > On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
> quentin.schulz at bootlin.com>
> > > wrote:
> > >
> > > > Hi Stephen,
> > >
> > > > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > > > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > > > > While the `env export` can take as parameters variables to be
> > > exported,
> > > > > > `env import` does not have such a mechanism of variable selection.
> > > > > >
> > > > > > Let's add a `-w` option that asks `env import` to look for the
> > > > > > `whitelisted_vars` env variable for a space-separated list of
> > > variables
> > > > > > that are whitelisted.
> > > > >
> > > > > Would it be better for the -w option to accept a variable name
> rather
> > > than
> > > > > hard-coding it as whitelisted_vars? That way, people could
> import/export
> > > > > different sets of variables at different times, and also could
> choose a
> > > more
> > > > > use-case-specific variable name than whitelisted_vars in order to
> > > describe
> > > > > why those variables are "whitelisted".
> > >
> > > > This has been raised in the previous version of the patch[1] (of which
> > > > you weren't in the mail recipients) and a similar patch[2] made by
> Alex
> > > > Kiernan (Cc of this patch series). I'd say it's an ongoing discussion,
> > > > though I should have mentioned it in the comments of the patch?
> > >
> > > > TL;DR:
> > > > Proposition 1: Have -w only which "hardcodedly" checks for
> > > > "whitelisting_vars",
> > > > +: straightforward implementation of the argument parsing,
> > > > -: implicit declaration of the list: you have to know to set
> > > > whitelisted_vars in the environnement,
> > >
> > > > Proposition 2: Have -w followed by one string-word which is the name
> of
> > > > the env variable where to look for the list of whitelisted env
> > > > variables,
> > > > +: explicit var to check where whitelist is looked for,
> > > > -: a bit of complexity added to the parsing of the parameters of the
> env
> > > > import function,
> > >
> > > > Proposition 3: Have -w followed by the list of whitelisted env
> variable,
> > > > +: explicit list
> > > > -: the list cannot be separated by comma (valid character for an env
> > > > variable) or a space (would not be able to distinguish the last
> > > > arguments of the commands which are address and size with size
> being
> > > > optional => how to know if size was passed or not?), what char
> can be
> > > > used to separate env variables in the list?
> > > > how does it perform with a very long list of whitelisted
> variables?
> > >
> > >
> > > Two more thoughts, both of which delegate the separator problem to the
> > > caller (the second being the one I implemented as it's almost no code)
> > >
> > > - specify multiple -w options each specifying a whitelisted env variable
>
> > You'll hit the maximum number of arguments/length of the command quickly
> > with this method. Quicker than with the other propositions.
>
>
> So you just made me go and read the definition for CONFIG_SYS_MAXARGS, the
> default's clearly pretty small (16). I guess I don't have a feeling for how
> many args you want to import - my use case is for 4, so even with max args
> at
> 16 I'm within that limit.
>
Same for me, a few variables and that's it. I'm trying not to have a
solution that's working only for me/us with a pretty low limitation
though. Once we've settled for a solution, IMHO, we've to stick with it
and thus if the solution can't be used by others, that's bad design from
us.
I guess at worst, we could call multiple times the env import command
with different whitelists but that's not really user-friendly.
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/20180521/4cf6c844/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import
2018-05-21 12:34 ` Quentin Schulz
@ 2018-05-21 12:51 ` Alex Kiernan
0 siblings, 0 replies; 16+ messages in thread
From: Alex Kiernan @ 2018-05-21 12:51 UTC (permalink / raw
To: u-boot
On Mon, May 21, 2018 at 1:34 PM Quentin Schulz <quentin.schulz@bootlin.com>
wrote:
> On Mon, May 21, 2018 at 01:26:11PM +0100, Alex Kiernan wrote:
> > On Mon, May 21, 2018 at 1:06 PM Quentin Schulz <
quentin.schulz@bootlin.com>
> > wrote:
> >
> > > Hi Alex,
> >
> > > On Mon, May 21, 2018 at 12:56:04PM +0100, Alex Kiernan wrote:
> > > > On Mon, May 21, 2018 at 9:02 AM Quentin Schulz <
> > quentin.schulz at bootlin.com>
> > > > wrote:
> > > >
> > > > > Hi Stephen,
> > > >
> > > > > On Fri, May 18, 2018 at 10:00:27AM -0600, Stephen Warren wrote:
> > > > > > On 05/18/2018 08:44 AM, Quentin Schulz wrote:
> > > > > > > While the `env export` can take as parameters variables to be
> > > > exported,
> > > > > > > `env import` does not have such a mechanism of variable
selection.
> > > > > > >
> > > > > > > Let's add a `-w` option that asks `env import` to look for the
> > > > > > > `whitelisted_vars` env variable for a space-separated list of
> > > > variables
> > > > > > > that are whitelisted.
> > > > > >
> > > > > > Would it be better for the -w option to accept a variable name
> > rather
> > > > than
> > > > > > hard-coding it as whitelisted_vars? That way, people could
> > import/export
> > > > > > different sets of variables at different times, and also could
> > choose a
> > > > more
> > > > > > use-case-specific variable name than whitelisted_vars in order
to
> > > > describe
> > > > > > why those variables are "whitelisted".
> > > >
> > > > > This has been raised in the previous version of the patch[1] (of
which
> > > > > you weren't in the mail recipients) and a similar patch[2] made by
> > Alex
> > > > > Kiernan (Cc of this patch series). I'd say it's an ongoing
discussion,
> > > > > though I should have mentioned it in the comments of the patch?
> > > >
> > > > > TL;DR:
> > > > > Proposition 1: Have -w only which "hardcodedly" checks for
> > > > > "whitelisting_vars",
> > > > > +: straightforward implementation of the argument parsing,
> > > > > -: implicit declaration of the list: you have to know to set
> > > > > whitelisted_vars in the environnement,
> > > >
> > > > > Proposition 2: Have -w followed by one string-word which is the
name
> > of
> > > > > the env variable where to look for the list of whitelisted env
> > > > > variables,
> > > > > +: explicit var to check where whitelist is looked for,
> > > > > -: a bit of complexity added to the parsing of the parameters of
the
> > env
> > > > > import function,
> > > >
> > > > > Proposition 3: Have -w followed by the list of whitelisted env
> > variable,
> > > > > +: explicit list
> > > > > -: the list cannot be separated by comma (valid character for an
env
> > > > > variable) or a space (would not be able to distinguish the
last
> > > > > arguments of the commands which are address and size with size
> > being
> > > > > optional => how to know if size was passed or not?), what char
> > can be
> > > > > used to separate env variables in the list?
> > > > > how does it perform with a very long list of whitelisted
> > variables?
> > > >
> > > >
> > > > Two more thoughts, both of which delegate the separator problem to
the
> > > > caller (the second being the one I implemented as it's almost no
code)
> > > >
> > > > - specify multiple -w options each specifying a whitelisted env
variable
> >
> > > You'll hit the maximum number of arguments/length of the command
quickly
> > > with this method. Quicker than with the other propositions.
> >
> > > Moreover, this can make the command painfully long, painful to read
and
> > > thus cumbersome to find the small typo in your command.
> >
> > > > - use the remaining arguments approach and eat all the trailing
> > arguments
> > > > as the names of env vars you import - needs a sentinel value for the
> > size
> > > > argument
> > > >
> >
> > > That can't work I think.
> >
> > > How do you know if the size argument was passed or not? How'd you know
> > > what string is addr, size or the whitelist (if there is even any)?
> >
> > > env import foo1 foo2 foo3 foo4 addr size
> > > env import foo1 foo2 foo3 addr
> > > env import addr size
> > > env import addr
> >
> >
> > That's why you need a sentinel for the 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
> > * text data must be '\0' terminated
> > * -r: handle CRLF like LF, that means exported variables with
> > * a content which ends with \r won't get imported. Used
> > * to import text files created with editors which are
using
> > CRLF
> > * for line endings. Only effective in addition to -t.
> > * -b: assume binary format ('\0' separated, "\0\0"
terminated)
> > * -c: assume checksum protected environment format
> > * addr: memory address to read from
> > * size: length of input data; if missing, proper '\0'
> > * 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
> >
> > Which for your examples translates to:
> >
> > env import addr size foo1 foo2 foo3 foo4
> > env import addr - foo1 foo2 foo3
> > env import addr size
> > env import addr
> >
> Ah, I misunderstood the word sentinel then :)
> Would have been an even better idea if we had some consistency between
> env export and env import. For specifying the env export size, we have
> to use the -s argument but it's a positional argument for env export. We
> then have a list of variables to export, so it would make sense to have
> the same for env import I guess?
I agree, but I can't immediately think of a good way of doing that which is
backward compatible.
--
Alex Kiernan
^ permalink raw reply [flat|nested] 16+ messages in thread
* [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment
2018-05-18 14:45 ` [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment Quentin Schulz
2018-05-18 16:04 ` Stephen Warren
@ 2018-05-22 23:29 ` Simon Glass
1 sibling, 0 replies; 16+ messages in thread
From: Simon Glass @ 2018-05-22 23:29 UTC (permalink / raw
To: u-boot
On 18 May 2018 at 08:45, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> This tests that the importing of an environment with a specified
> whitelist works as intended.
>
> If the variable whitelisted_vars is not set, the env importing should
> fail with a given message.
>
> For each variable separated by spaces in whitelisted_vars, if
> - foo is bar in current env and bar2 in exported env, after importing
> exported env, foo shall be bar2,
> - foo does not exist in current env and foo is bar2 in exported env,
> after importing exported env, foo shall be bar2,
> - foo is bar in current env and does not exist in exported env (but is
> in the whitelisted_vars), after importing exported env, foo shall be
> empty,
>
> Any variable not in whitelisted_vars should be left untouched.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
>
> added in v2
>
> test/py/tests/test_env.py | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-05-22 23:29 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-18 14:44 [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Quentin Schulz
2018-05-18 14:45 ` [U-Boot] [PATCH v2 2/2] test/py: add test for whitelist of variables while importing environment Quentin Schulz
2018-05-18 16:04 ` Stephen Warren
2018-05-21 7:43 ` Quentin Schulz
2018-05-22 23:29 ` Simon Glass
2018-05-18 16:00 ` [U-Boot] [PATCH v2 1/2] cmd: nvedit: add whitelist option for env import Stephen Warren
2018-05-21 8:01 ` Quentin Schulz
2018-05-21 11:56 ` Alex Kiernan
2018-05-21 12:06 ` Quentin Schulz
2018-05-21 12:26 ` Alex Kiernan
2018-05-21 12:34 ` Quentin Schulz
2018-05-21 12:51 ` Alex Kiernan
2018-05-21 12:36 ` Alex Kiernan
2018-05-21 12:41 ` Quentin Schulz
2018-05-20 13:01 ` Lothar Waßmann
2018-05-21 7:29 ` Quentin Schulz
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.