[U-Boot] [PATCH v5 1/5] env: add the same prefix to error messages to make it detectable by tests
The error message should start with `## Error: ` so that it's easily detectable by tests without needing to have a complex regexp for matching all possible error message patterns.
Let's add the `## Error: ` prefix to the error messages since it's the one already in use.
Suggested-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com ---
added in v5
cmd/nvedit.c | 12 ++++++++---- env/common.c | 3 ++- 2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index ddc888a..70d7068 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -959,7 +959,8 @@ NXTARG: ; H_MATCH_KEY | H_MATCH_IDENT, &ptr, size, argc, argv); if (len < 0) { - pr_err("Cannot export environment: errno = %d\n", errno); + pr_err("## Error: Cannot export environment: errno = %d\n", + errno); return 1; } sprintf(buf, "%zX", (size_t)len); @@ -979,7 +980,8 @@ NXTARG: ; H_MATCH_KEY | H_MATCH_IDENT, &res, ENV_SIZE, argc, argv); if (len < 0) { - pr_err("Cannot export environment: errno = %d\n", errno); + pr_err("## Error: Cannot export environment: errno = %d\n", + errno); return 1; }
@@ -994,7 +996,8 @@ NXTARG: ; return 0;
sep_err: - printf("## %s: only one of "-b", "-c" or "-t" allowed\n", cmd); + printf("## Error: %s: only one of "-b", "-c" or "-t" allowed\n", + cmd); return 1; } #endif @@ -1114,7 +1117,8 @@ 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) { - pr_err("Environment import failed: errno = %d\n", errno); + pr_err("## Error: Environment import failed: errno = %d\n", + errno); return 1; } gd->flags |= GD_FLG_ENV_READY; diff --git a/env/common.c b/env/common.c index dc8a14f..7bd2790 100644 --- a/env/common.c +++ b/env/common.c @@ -83,7 +83,8 @@ void set_default_env(const char *s) if (himport_r(&env_htab, (char *)default_environment, sizeof(default_environment), '\0', flags, 0, 0, NULL) == 0) - pr_err("Environment import failed: errno = %d\n", errno); + pr_err("## Error: Environment import failed: errno = %d\n", + errno);
gd->flags |= GD_FLG_ENV_READY; gd->flags |= GD_FLG_ENV_DEFAULT;
base-commit: fb77a9e3537039664ad42992bef6688869eda7c1
Some functions test that the given address is not NULL (0) and fail or have a different behaviour if that's the case (e.g. hexport_r).
Let's make the RAM base address to be not zero by setting it to 2MiB if that's the case.
2MiB is chosen because it represents the size of an ARM LPAE/v8 section.
Suggested-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com ---
added in v5
test/py/u_boot_utils.py | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py index bb31e57..07520ac 100644 --- a/test/py/u_boot_utils.py +++ b/test/py/u_boot_utils.py @@ -236,6 +236,12 @@ def find_ram_base(u_boot_console): ram_base = -1 raise Exception('Failed to find RAM bank start in `bdinfo`')
+ # We don't want ram_base to be zero as some functions test if the given + # address is NULL (0). Let's add 2MiB then (size of an ARM LPAE/v8 section). + + if ram_base == 0: + ram_base += 1024 * 1024 * 2 + return ram_base
class PersistentFileHelperCtxMgr(object):
On 29 June 2018 at 00:41, Quentin Schulz quentin.schulz@bootlin.com wrote:
Some functions test that the given address is not NULL (0) and fail or have a different behaviour if that's the case (e.g. hexport_r).
Let's make the RAM base address to be not zero by setting it to 2MiB if that's the case.
2MiB is chosen because it represents the size of an ARM LPAE/v8 section.
Suggested-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com
added in v5
test/py/u_boot_utils.py | 6 ++++++ 1 file changed, 6 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Some functions have different behaviour when the given address is 0 (assumed to be NULL by the function).
find_ram_base() does not return 0 anymore so it's safe to remove those offsets.
Suggested-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com ---
added in v5
test/py/tests/test_efi_loader.py | 2 +- test/py/tests/test_net.py | 4 ++-- test/py/tests/test_tpm2.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/test/py/tests/test_efi_loader.py b/test/py/tests/test_efi_loader.py index 35bd419..a66c6e6 100644 --- a/test/py/tests/test_efi_loader.py +++ b/test/py/tests/test_efi_loader.py @@ -118,7 +118,7 @@ def fetch_tftp_file(u_boot_console, env_conf):
addr = f.get('addr', None) if not addr: - addr = u_boot_utils.find_ram_base(u_boot_console) + (1024 * 1024 * 4) + addr = u_boot_utils.find_ram_base(u_boot_console)
fn = f['fn'] output = u_boot_console.run_command('tftpboot %x %s' % (addr, fn)) diff --git a/test/py/tests/test_net.py b/test/py/tests/test_net.py index f2e432b..2821ce6 100644 --- a/test/py/tests/test_net.py +++ b/test/py/tests/test_net.py @@ -146,7 +146,7 @@ def test_net_tftpboot(u_boot_console):
addr = f.get('addr', None) if not addr: - addr = u_boot_utils.find_ram_base(u_boot_console) + (1024 * 1024 * 4) + addr = u_boot_utils.find_ram_base(u_boot_console)
fn = f['fn'] output = u_boot_console.run_command('tftpboot %x %s' % (addr, fn)) @@ -186,7 +186,7 @@ def test_net_nfs(u_boot_console):
addr = f.get('addr', None) if not addr: - addr = u_boot_utils.find_ram_base(u_boot_console) + (1024 * 1024 * 4) + addr = u_boot_utils.find_ram_base(u_boot_console)
fn = f['fn'] output = u_boot_console.run_command('nfs %x %s' % (addr, fn)) diff --git a/test/py/tests/test_tpm2.py b/test/py/tests/test_tpm2.py index 01ffb31..ef7b86a 100644 --- a/test/py/tests/test_tpm2.py +++ b/test/py/tests/test_tpm2.py @@ -183,7 +183,7 @@ def test_tpm2_pcr_read(u_boot_console): """
force_init(u_boot_console) - ram = u_boot_utils.find_ram_base(u_boot_console) + 1024 + ram = u_boot_utils.find_ram_base(u_boot_console)
read_pcr = u_boot_console.run_command('tpm pcr_read 0 0x%x' % ram) output = u_boot_console.run_command('echo $?') @@ -210,7 +210,7 @@ def test_tpm2_pcr_extend(u_boot_console): """
force_init(u_boot_console) - ram = u_boot_utils.find_ram_base(u_boot_console) + 1024 + ram = u_boot_utils.find_ram_base(u_boot_console)
u_boot_console.run_command('tpm pcr_extend 0 0x%x' % ram) output = u_boot_console.run_command('echo $?')
On 06/29/2018 01:41 AM, Quentin Schulz wrote:
Some functions have different behaviour when the given address is 0 (assumed to be NULL by the function).
find_ram_base() does not return 0 anymore so it's safe to remove those offsets.
Patches 1-3, Reviewed-by: Stephen Warren swarren@nvidia.com
I assume you tested this in Travis with everything enabled and it all passed? I should test it locally on Tegra too...
On 06/29/2018 09:25 AM, Stephen Warren wrote:
On 06/29/2018 01:41 AM, Quentin Schulz wrote:
Some functions have different behaviour when the given address is 0 (assumed to be NULL by the function).
find_ram_base() does not return 0 anymore so it's safe to remove those offsets.
Patches 1-3, Reviewed-by: Stephen Warren swarren@nvidia.com
I assume you tested this in Travis with everything enabled and it all passed? I should test it locally on Tegra too...
The series,
Tested-by: Stephen Warren swarren@nvidia.com
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 the ability to add parameters at the end of the command for variables to be imported.
Every env variable from the env to be imported passed by parameter to this command will override the value of the variable in the current env.
If a variable exists in the current env but not in the imported env, if this variable is passed as a parameter to env import, the variable will be unset.
If a variable exists in the imported env, the variable in the current env will be set to the value of the one from the imported env.
All the remaining variables are left untouched.
As the size parameter of env import is positional but optional, let's add the possibility to use the sentinel '-' for when we don't want to give the size parameter (when the env is '\0' terminated) but we pass a list of variables at the end of the command.
env import addr env import addr - env import addr size env import addr - foo1 foo2 env import addr size foo1 foo2
are all valid.
env import -c addr env import -c addr - env import -c addr - foo1 foo2
are all invalid because they don't pass the size parameter required for checking, while the following are valid.
env import addr size env import addr size foo1 foo2
Nothing's changed for the other parameters or the overall behaviour.
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 Tested-by: Alex Kiernan alex.kiernan@gmail.com ---
v4: - add tested-by by Alex,
v3: - migrate to env import addr size var... instead of env import -w addr size so that the list of variables to load is more explicit and the behaviour of env import is closer to the one of env export,
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 | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/cmd/nvedit.c b/cmd/nvedit.c index 70d7068..5000517 100644 --- a/cmd/nvedit.c +++ b/cmd/nvedit.c @@ -1004,7 +1004,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 @@ -1018,6 +1018,11 @@ sep_err: * addr: memory address to read from * size: length of input data; if missing, proper '\0' * termination is mandatory + * if var is set and size should be missing (i.e. '\0' + * termination), set size to '-' + * var... List of the names of the only variables that get imported from + * the environment at address 'addr'. Without arguments, the whole + * environment gets imported. */ static int do_env_import(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) @@ -1029,6 +1034,7 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, int fmt = 0; int del = 0; int crlf_is_lf = 0; + int wl = 0; size_t size;
cmd = *argv; @@ -1077,9 +1083,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, addr = simple_strtoul(argv[0], NULL, 16); ptr = map_sysmem(addr, 0);
- if (argc == 2) { + if (argc >= 2 && strcmp(argv[1], "-")) { size = simple_strtoul(argv[1], NULL, 16); - } else if (argc == 1 && chk) { + } else if (chk) { puts("## Error: external checksum format must pass size\n"); return CMD_RET_FAILURE; } else { @@ -1101,6 +1107,9 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, printf("## Info: input data size = %zu = 0x%zX\n", size, size); }
+ if (argc > 2) + wl = 1; + if (chk) { uint32_t crc; env_t *ep = (env_t *)ptr; @@ -1115,8 +1124,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, ptr = (char *)ep->data; }
- if (himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, - crlf_is_lf, 0, NULL) == 0) { + if (!himport_r(&env_htab, ptr, size, sep, del ? 0 : H_NOCLEAR, + crlf_is_lf, wl ? argc - 2 : 0, wl ? &argv[2] : NULL)) { pr_err("## Error: Environment import failed: errno = %d\n", errno); return 1; @@ -1246,7 +1255,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)
Dear Quentin,
In message 322e6866c43f4515240ddca9456ee390b6f334c7.1530257385.git-series.quentin.schulz@bootlin.com you 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 the ability to add parameters at the end of the command for variables to be imported.
Thanks!
If a variable exists in the current env but not in the imported env, if this variable is passed as a parameter to env import, the variable will be unset.
I suggest that this isNOT the default behaviour. I think most peaople would expect that this command only adds/replaces environment settings, so omitting one value should not cause unexpected behaviour.
But I agree that such behaviour may also be conveniend - I suggest that we use an additional option ("-d" ?) to avtiavte it explicitly.
Thanks.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Fri, Jun 29, 2018 at 01:03:25PM +0200, Wolfgang Denk wrote:
Dear Quentin,
In message 322e6866c43f4515240ddca9456ee390b6f334c7.1530257385.git-series.quentin.schulz@bootlin.com you 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 the ability to add parameters at the end of the command for variables to be imported.
Thanks!
If a variable exists in the current env but not in the imported env, if this variable is passed as a parameter to env import, the variable will be unset.
I suggest that this isNOT the default behaviour. I think most peaople would expect that this command only adds/replaces environment settings, so omitting one value should not cause unexpected behaviour.
It's the default behaviour of himport_r.
However, there's already a flag parameter so maybe I could just add an H_KEEP_VAR flag to keep the current default behaviour of himport_r (which is in a library) but add the feature you requested in the env import cmd, that should work I guess.
But I agree that such behaviour may also be conveniend - I suggest that we use an additional option ("-d" ?) to avtiavte it explicitly.
There's already a -d option which basically, if I understood correctly, tells himport_r to erase the current env and use only the imported env.
Let's add a -k (for H_KEEP_VAR) to tell himport_r we want to keep variables that aren't in the imported env but requested to be loaded (or present in the imported env in one of the forms `var` or `var=`).
Does this make sense? Is it the way you see it implemented?
Quentin
Dear Quentin,
In message 20180629121942.lm4qbfmm5ya7fsx2@qschulz you wrote:
I suggest that this isNOT the default behaviour. I think most peaople would expect that this command only adds/replaces environment settings, so omitting one value should not cause unexpected behaviour.
It's the default behaviour of himport_r.
Hm.... I don't see what you mean. himport_r imports a set of new name=value pairs (in different formats); however, ther eis no way to specify a name without a value, so himport_r by itself will not delete any variables - it only overrides or adds.
However, there's already a flag parameter so maybe I could just add an H_KEEP_VAR flag to keep the current default behaviour of himport_r (which is in a library) but add the feature you requested in the env import cmd, that should work I guess.
There's already a -d option which basically, if I understood correctly, tells himport_r to erase the current env and use only the imported env.
Um... right - only with the -d option h_import_r will erase any variables - without any name list, it process _all_ variables from the import, and erase _any_ other. With a list of specified names, it seems logical to me that only this list will be processed - i. e. only these will be impoted or erased.
That was what I had in mind with consistent behaviour.
Let's add a -k (for H_KEEP_VAR) to tell himport_r we want to keep variables that aren't in the imported env but requested to be loaded (or present in the imported env in one of the forms `var` or `var=`).
Does this make sense? Is it the way you see it implemented?
No. I think it should be done as I explained before. Only with "-d" existing variables shall be deleted, if they are not in the imported blob. Then the behaviour is all the same, only the selection of the names is different: without an argument list, it's all names, and with an argument list it's only the names in the list.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Fri, Jun 29, 2018 at 02:52:29PM +0200, Wolfgang Denk wrote:
Dear Quentin,
In message 20180629121942.lm4qbfmm5ya7fsx2@qschulz you wrote:
I suggest that this isNOT the default behaviour. I think most peaople would expect that this command only adds/replaces environment settings, so omitting one value should not cause unexpected behaviour.
It's the default behaviour of himport_r.
Hm.... I don't see what you mean. himport_r imports a set of new name=value pairs (in different formats); however, ther eis no way to specify a name without a value, so himport_r by itself will not delete any variables - it only overrides or adds.
That's not what this comment[1] says. Maybe it's not possible to specify a value but there seems to be code to handle this case in himport_r.
[1] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L881
However, there's already a flag parameter so maybe I could just add an H_KEEP_VAR flag to keep the current default behaviour of himport_r (which is in a library) but add the feature you requested in the env import cmd, that should work I guess.
There's already a -d option which basically, if I understood correctly, tells himport_r to erase the current env and use only the imported env.
Um... right - only with the -d option h_import_r will erase any variables - without any name list, it process _all_ variables from the import, and erase _any_ other. With a list of specified names, it seems logical to me that only this list will be processed - i. e. only these will be impoted or erased.
That was what I had in mind with consistent behaviour.
When there's a list of vars passed to himport_r, the ones that are not found in the imported env are removed from the current environment. This is done here: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L936
What about skipping this part if the H_NOCLEAR flag is not set in flags and add a condition here[2] to not delete the htable if there's vars passed to the function?
i.e. something like:
--- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -749,8 +749,11 @@ static int drop_var_from_set(const char *name, int nvars, char * vars[]) * * The "flag" argument can be used to control the behaviour: when the * H_NOCLEAR bit is set, then an existing hash table will kept, i. e. - * new data will be added to an existing hash table; otherwise, old - * data will be discarded and a new hash table will be created. + * new data will be added to an existing hash table; otherwise, if no + * vars are passed, old data will be discarded and a new hash table + * will be created. If vars are passed, passed vars that are not in + * the linear list of "name=value" pairs will be removed from the + * current hash table. * * The separator character for the "name=value" pairs can be selected, * so we both support importing from externally stored environment @@ -801,7 +804,7 @@ int himport_r(struct hsearch_data *htab, if (nvars) memcpy(localvars, vars, sizeof(vars[0]) * nvars);
- if ((flag & H_NOCLEAR) == 0) { + if ((flag & H_NOCLEAR) == 0 && !nvars) { /* Destroy old hash table if one exists */ debug("Destroy Hash Table: %p table = %p\n", htab, htab->table); @@ -933,6 +936,9 @@ int himport_r(struct hsearch_data *htab, debug("INSERT: free(data = %p)\n", data); free(data);
+ if (flag & H_NOCLEAR) + goto end; + /* process variables which were not considered */ for (i = 0; i < nvars; i++) { if (localvars[i] == NULL) @@ -951,6 +957,7 @@ int himport_r(struct hsearch_data *htab, printf("WARNING: '%s' not in imported env, deleting it!\n", localvars[i]); }
+end: debug("INSERT: done\n"); return 1; /* everything OK */ }
[2] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L804
The only thing is that we change the behaviour for the people that passed variables and NOT H_NOCLEAR. Now the whole hash table won't be deleted. There is no user that uses himport_r that way but since the code we're discussing is in lib/ I just want to make sure that's fine.
Let's add a -k (for H_KEEP_VAR) to tell himport_r we want to keep variables that aren't in the imported env but requested to be loaded (or present in the imported env in one of the forms `var` or `var=`).
Does this make sense? Is it the way you see it implemented?
No. I think it should be done as I explained before. Only with "-d" existing variables shall be deleted, if they are not in the imported blob. Then the behaviour is all the same, only the selection of the names is different: without an argument list, it's all names, and with an argument list it's only the names in the list.
This makes sense. Let's agree on the implementation now :)
Thanks, Quentin
Dear Quentin,
In message 20180702092548.bciqnfyd7d2hv26x@qschulz you wrote:
Hm.... I don't see what you mean. himport_r imports a set of new name=value pairs (in different formats); however, there is no way to specify a name without a value, so himport_r by itself will not delete any variables - it only overrides or adds.
That's not what this comment[1] says. Maybe it's not possible to specify a value but there seems to be code to handle this case in himport_r.
[1] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L881
You are right. When importing plain text format (-t) then you can include lines "name=" which will case deletion of this variable.
When there's a list of vars passed to himport_r, the ones that are not found in the imported env are removed from the current environment. This is done here: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L936
Again, you are right. I have to admit that I was not aware of this. This is not from my original design - it was added later by this commit d5370febbcbcee3f554df13ed72b7e2b91e5f66c Author: Gerlando Falauto gerlando.falauto@keymile.com Date: Sun Aug 26 21:53:00 2012 +0000
env: delete selected vars not present in imported env
When variables explicitly specified on the command line are not present in the imported env, delete them from the running env. If the variable is also missing from the running env, issue a warning.
Signed-off-by: Gerlando Falauto gerlando.falauto@keymile.com Reviewed-by: Marek Vasut marex@denx.de
In my opinion this is inconsistent, but it seems I missed that patch when it was posted. As this happened a looong time ago and noone since complained, we probably should accept this behaviour as status quo.
What about skipping this part if the H_NOCLEAR flag is not set in flags and add a condition here[2] to not delete the htable if there's vars passed to the function?
Uh... I'm not sure ... How many users will actually understand such behaviour? Just reading the sentence "skip this then that flag is not set" is difficult to parse...
IMO it does not help if the user has to make specific flags settings.
The only thing is that we change the behaviour for the people that passed variables and NOT H_NOCLEAR. Now the whole hash table won't be deleted. There is no user that uses himport_r that way but since the code we're discussing is in lib/ I just want to make sure that's fine.
I an really a big fan of the Principle of Least Surprise [1], and my experience tells me that most users will expect to be able to use such a command for the purpose to pick a few selected environment settings from an existing environment blob - say, adjust the nework settings by picking "ipaddr", "netmask" and "serverip". And to me this is also the most useful use case for such a command.
I fear, a large lot of such users will be very badly surprised when they realize they just blew away all of their other environment.
So such a default is just dangerous, and (IMO) must be avoided.
[1] https://en.wikipedia.org/wiki/Principle_of_least_astonishment
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Mon, Jul 02, 2018 at 01:21:09PM +0200, Wolfgang Denk wrote:
Dear Quentin,
In message 20180702092548.bciqnfyd7d2hv26x@qschulz you wrote:
Hm.... I don't see what you mean. himport_r imports a set of new name=value pairs (in different formats); however, there is no way to specify a name without a value, so himport_r by itself will not delete any variables - it only overrides or adds.
That's not what this comment[1] says. Maybe it's not possible to specify a value but there seems to be code to handle this case in himport_r.
[1] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L881
You are right. When importing plain text format (-t) then you can include lines "name=" which will case deletion of this variable.
OK, good to know.
When there's a list of vars passed to himport_r, the ones that are not found in the imported env are removed from the current environment. This is done here: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L936
Again, you are right. I have to admit that I was not aware of this. This is not from my original design - it was added later by this commit d5370febbcbcee3f554df13ed72b7e2b91e5f66c Author: Gerlando Falauto gerlando.falauto@keymile.com Date: Sun Aug 26 21:53:00 2012 +0000
env: delete selected vars not present in imported env When variables explicitly specified on the command line are not present in the imported env, delete them from the running env. If the variable is also missing from the running env, issue a warning. Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> Reviewed-by: Marek Vasut <marex@denx.de>In my opinion this is inconsistent, but it seems I missed that patch when it was posted. As this happened a looong time ago and noone since complained, we probably should accept this behaviour as status quo.
What about skipping this part if the H_NOCLEAR flag is not set in flags and add a condition here[2] to not delete the htable if there's vars passed to the function?
Uh... I'm not sure ... How many users will actually understand such behaviour? Just reading the sentence "skip this then that flag is not set" is difficult to parse...
IMO it does not help if the user has to make specific flags settings.
It's the flag that is used already for saying to himport_r to NOT delete the whole hash table.
See: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L804
The snippet I sent is actually making use of this flag for something other than NOT deleting the whole hash table. The two uses for the flag are pretty similar though.
The only thing is that we change the behaviour for the people that passed variables and NOT H_NOCLEAR. Now the whole hash table won't be deleted. There is no user that uses himport_r that way but since the code we're discussing is in lib/ I just want to make sure that's fine.
I an really a big fan of the Principle of Least Surprise [1], and my experience tells me that most users will expect to be able to use such a command for the purpose to pick a few selected environment settings from an existing environment blob - say, adjust the nework settings by picking "ipaddr", "netmask" and "serverip". And to me this is also the most useful use case for such a command.
Agreed.
I fear, a large lot of such users will be very badly surprised when they realize they just blew away all of their other environment.
Well, if today you do himport_r with a list of vars and the flag argument is 0, you actually delete the whole environment.
Today, within the env_import command, there's never a list of vars that is passed to himport_r as we can only import the whole environment in the given RAM address, so the -d option is pretty safe.
Now that I add the list of vars to env_import, with the patch series I sent, if I pass -d and a list of vars to import, the whole environment is deleted and only the vars in the list of vars are imported in the environnement. This is what you fear and I agree this isn't a very nice design.
So such a default is just dangerous, and (IMO) must be avoided.
I think we misunderstood each other on the proposed patch last mail.
Let me recapitulate what is the current behaviour (correct me if I'm wrong).
The current behaviour is the following:
1) himport_r withOUT a list of variables (vars=NULL) and with flag = 0: = delete the current hash table and then import the environnement, the example for this is basically: env import -d 0xaddr 2) himport_r withOUT a list of variables (vars=NULL) and with flag = H_NOCLEAR: = do NOT delete the current hash table and then import the environnement, the example for this is basically: env import 0xaddr 3) himport_r WITH a list of variables (vars!=NULL) and with flag = 0: = delete the current hash table and then import the variables vars from the environnement to be imported, the example for this is basically: env import -d 0xaddr varA varB varC 4) himport_r WITH a list of variables (vars!=NULL) and with flag = H_NOCLEAR: = do NOT delete the current hash table and then import the variables vars from the environnement to be imported, IF a var A in vars is not present in the environnement to be imported, var A is removed from the current environnement. the example for this is basically: env import 0xaddr varA varB varC
What I suggested is to modify 3 and 4) to the following: 3) himport_r WITH a list of variables (vars!=NULL) and with flag = 0: = import the variables vars from the environnement to be imported, IF a var A in vars is not present in the environnement to be imported, var A is removed from the current environnement. the example for this is basically: env import -d 0xaddr varA varB varC 4) himport_r WITH a list of variables (vars!=NULL) and with flag = H_NOCLEAR: = import the variables vars from the environnement to be imported, the example for this is basically: env import 0xaddr varA varB varC
This is what the proposed snippet in last mail is supposed to do.
Hopefully, I better explained it. Let me know.
Quentin
On Mon, Jul 2, 2018 at 1:21 PM Quentin Schulz quentin.schulz@bootlin.com wrote:
Hi Wolfgang,
On Mon, Jul 02, 2018 at 01:21:09PM +0200, Wolfgang Denk wrote:
Dear Quentin,
In message 20180702092548.bciqnfyd7d2hv26x@qschulz you wrote:
Hm.... I don't see what you mean. himport_r imports a set of new name=value pairs (in different formats); however, there is no way to specify a name without a value, so himport_r by itself will not delete any variables - it only overrides or adds.
That's not what this comment[1] says. Maybe it's not possible to specify a value but there seems to be code to handle this case in himport_r.
[1] https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L881
You are right. When importing plain text format (-t) then you can include lines "name=" which will case deletion of this variable.
OK, good to know.
When there's a list of vars passed to himport_r, the ones that are not found in the imported env are removed from the current environment. This is done here: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L936
Again, you are right. I have to admit that I was not aware of this. This is not from my original design - it was added later by this commit d5370febbcbcee3f554df13ed72b7e2b91e5f66c Author: Gerlando Falauto gerlando.falauto@keymile.com Date: Sun Aug 26 21:53:00 2012 +0000
env: delete selected vars not present in imported env When variables explicitly specified on the command line are not present in the imported env, delete them from the running env. If the variable is also missing from the running env, issue a warning. Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> Reviewed-by: Marek Vasut <marex@denx.de>In my opinion this is inconsistent, but it seems I missed that patch when it was posted. As this happened a looong time ago and noone since complained, we probably should accept this behaviour as status quo.
FWIW I found this surprising too. I ended up redesigning my uses to either save and restore existing values, or to avoid the requirement for them altogether.
What about skipping this part if the H_NOCLEAR flag is not set in flags and add a condition here[2] to not delete the htable if there's vars passed to the function?
Uh... I'm not sure ... How many users will actually understand such behaviour? Just reading the sentence "skip this then that flag is not set" is difficult to parse...
IMO it does not help if the user has to make specific flags settings.
It's the flag that is used already for saying to himport_r to NOT delete the whole hash table.
See: https://elixir.bootlin.com/u-boot/latest/source/lib/hashtable.c#L804
The snippet I sent is actually making use of this flag for something other than NOT deleting the whole hash table. The two uses for the flag are pretty similar though.
The only thing is that we change the behaviour for the people that passed variables and NOT H_NOCLEAR. Now the whole hash table won't be deleted. There is no user that uses himport_r that way but since the code we're discussing is in lib/ I just want to make sure that's fine.
I an really a big fan of the Principle of Least Surprise [1], and my experience tells me that most users will expect to be able to use such a command for the purpose to pick a few selected environment settings from an existing environment blob - say, adjust the nework settings by picking "ipaddr", "netmask" and "serverip". And to me this is also the most useful use case for such a command.
Agreed.
I fear, a large lot of such users will be very badly surprised when they realize they just blew away all of their other environment.
Well, if today you do himport_r with a list of vars and the flag argument is 0, you actually delete the whole environment.
Today, within the env_import command, there's never a list of vars that is passed to himport_r as we can only import the whole environment in the given RAM address, so the -d option is pretty safe.
Now that I add the list of vars to env_import, with the patch series I sent, if I pass -d and a list of vars to import, the whole environment is deleted and only the vars in the list of vars are imported in the environnement. This is what you fear and I agree this isn't a very nice design.
So such a default is just dangerous, and (IMO) must be avoided.
I think we misunderstood each other on the proposed patch last mail.
Let me recapitulate what is the current behaviour (correct me if I'm wrong).
The current behaviour is the following:
- himport_r withOUT a list of variables (vars=NULL) and with flag = 0:
= delete the current hash table and then import the environnement, the example for this is basically: env import -d 0xaddr 2) himport_r withOUT a list of variables (vars=NULL) and with flag = H_NOCLEAR: = do NOT delete the current hash table and then import the environnement, the example for this is basically: env import 0xaddr 3) himport_r WITH a list of variables (vars!=NULL) and with flag = 0: = delete the current hash table and then import the variables vars from the environnement to be imported, the example for this is basically: env import -d 0xaddr varA varB varC 4) himport_r WITH a list of variables (vars!=NULL) and with flag = H_NOCLEAR: = do NOT delete the current hash table and then import the variables vars from the environnement to be imported, IF a var A in vars is not present in the environnement to be imported, var A is removed from the current environnement. the example for this is basically: env import 0xaddr varA varB varC
What I suggested is to modify 3 and 4) to the following: 3) himport_r WITH a list of variables (vars!=NULL) and with flag = 0: = import the variables vars from the environnement to be imported, IF a var A in vars is not present in the environnement to be imported, var A is removed from the current environnement. the example for this is basically: env import -d 0xaddr varA varB varC 4) himport_r WITH a list of variables (vars!=NULL) and with flag = H_NOCLEAR: = import the variables vars from the environnement to be imported, the example for this is basically: env import 0xaddr varA varB varC
This is what the proposed snippet in last mail is supposed to do.
Hopefully, I better explained it. Let me know.
Certainly an option to leave existing values there and only overwrite if them if there are new values in the imported env would be very useful.
-- Alex Kiernan
This tests that the importing of an environment with a specified whitelist works as intended.
If there are variables passed as parameter to the env import command, those only should be imported in the current environment.
For each variable passed as parameter, 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 passed as parameter), after importing exported env, foo shall be empty,
Any variable not passed as parameter should be left untouched.
Two other tests are made to test that size cannot be '-' if the checksum protection is enabled.
Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Stephen Warren swarren@nvidia.com ---
v5: - add reviewed-by by Stephen,
v4: - add reviewed-by by Simon, - fix double quotes instead of simple ones for strings, - fix missing space after # starting a comment,
v3: - update whitelist test to reflect changes made in patch 1, - add two tests for no size parameter passed but checksum protection is enabled because I added the possibility to have a sentinel in place of size parameter, - I intentionally didn't put the Reviewed and Acked-by of Simon and Stephen since the code changed since v2,
added in v2
test/py/tests/test_env.py | 54 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+)
diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py index bfb5fc0..61b3bc9 100644 --- a/test/py/tests/test_env.py +++ b/test/py/tests/test_env.py @@ -5,6 +5,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? @@ -239,3 +240,56 @@ 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) + +@pytest.mark.buildconfigspec('cmd_importenv') +def test_env_import_checksum_no_size(state_test_env): + """Test that omitted ('-') size parameter with checksum validation fails the + env import function. + """ + c = state_test_env.u_boot_console + ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console) + addr = '%08x' % ram_base + + with c.disable_check('error_notification'): + response = c.run_command('env import -c %s -' % addr) + assert(response == '## Error: external checksum format must pass size') + +@pytest.mark.buildconfigspec('cmd_importenv') +def test_env_import_whitelist_checksum_no_size(state_test_env): + """Test that omitted ('-') size parameter with checksum validation fails the + env import function when variables are passed as parameters. + """ + c = state_test_env.u_boot_console + ram_base = u_boot_utils.find_ram_base(state_test_env.u_boot_console) + addr = '%08x' % ram_base + + with c.disable_check('error_notification'): + response = c.run_command('env import -c %s - foo1 foo2 foo4' % addr) + assert(response == '## Error: external checksum format must pass size') + +@pytest.mark.buildconfigspec('cmd_exportenv') +@pytest.mark.buildconfigspec('cmd_importenv') +def test_env_import_whitelist(state_test_env): + """Test importing only a handful of env variables from an environment.""" + c = state_test_env.u_boot_console + 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') + set_var(state_test_env, 'foo2', 'bar2') + set_var(state_test_env, 'foo3', 'bar3') + + c.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') + + # no foo1 in current env, foo2 overridden, foo3 should be of the value + # before exporting and foo4 should be deleted + c.run_command('env import %s - foo1 foo2 foo4' % 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')
On 29 June 2018 at 00:41, Quentin Schulz quentin.schulz@bootlin.com wrote:
The error message should start with `## Error: ` so that it's easily detectable by tests without needing to have a complex regexp for matching all possible error message patterns.
Let's add the `## Error: ` prefix to the error messages since it's the one already in use.
Suggested-by: Stephen Warren swarren@wwwdotorg.org Signed-off-by: Quentin Schulz quentin.schulz@bootlin.com
added in v5
cmd/nvedit.c | 12 ++++++++---- env/common.c | 3 ++- 2 files changed, 10 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
participants (5)
-
Alex Kiernan -
Quentin Schulz -
Simon Glass -
Stephen Warren -
Wolfgang Denk