[U-Boot] [PATCH v5 1/5] env: add the same prefix to error messages to make it detectable by tests

[U-Boot] [PATCH v5 1/5] env: add the same prefix to error messages to make it detectable by tests

[Quentin Schulz]

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
-- 
git-series 0.9.1

[U-Boot] [PATCH v5 2/5] test/py: return a RAM address different from 0 as it can be interpreted as NULL

[Quentin Schulz]

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):
-- 
git-series 0.9.1

[U-Boot] [PATCH v5 3/5] test/py: remove hacks for non-zero RAM base address in tests

[Quentin Schulz]

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 $?')
-- 
git-series 0.9.1

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Quentin Schulz]

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)
-- 
git-series 0.9.1

[U-Boot] [PATCH v5 5/5] test/py: add test for whitelist of variables while importing environment

[Quentin Schulz]

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)
+
+ at 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')
+
+ at 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')
+
+ at pytest.mark.buildconfigspec('cmd_exportenv')
+ at 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')
-- 
git-series 0.9.1

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
We want to create puppets that pull their own strings.   - Ann Marion

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Quentin Schulz]

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
-------------- 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/20180629/ddef64d0/attachment.sig>

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are very few personal problems that cannot be solved through  a
suitable application of high explosives.

[U-Boot] [PATCH v5 3/5] test/py: remove hacks for non-zero RAM base address in tests

[Stephen Warren]

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...

[U-Boot] [PATCH v5 3/5] test/py: remove hacks for non-zero RAM base address in tests

[Stephen Warren]

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>

[U-Boot] [PATCH v5 1/5] env: add the same prefix to error messages to make it detectable by tests

[Simon Glass]

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>

[U-Boot] [PATCH v5 2/5] test/py: return a RAM address different from 0 as it can be interpreted as NULL

[Simon Glass]

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>

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Quentin Schulz]

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
-------------- 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/20180702/a7b9bc8c/attachment.sig>

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Perfection is reached, not when there is no longer anything  to  add,
but when there is no longer anything to take away.
                                           - Antoine de Saint-Exupery

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Quentin Schulz]

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
-------------- 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/20180702/12cefd12/attachment.sig>

[U-Boot] [PATCH v5 4/5] cmd: nvedit: env import can now import only variables passed as parameters

[Alex Kiernan]

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:
>
> 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.
>

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