[U-Boot] [PATCH v4 1/2] cmd: nvedit: env import can now import only variables passed as parameters

[U-Boot] [PATCH v4 1/2] 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 | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 11489b0..18cc4db 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1001,7 +1001,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
@@ -1015,6 +1015,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[])
@@ -1026,6 +1031,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;
@@ -1074,9 +1080,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 {
@@ -1098,6 +1104,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;
@@ -1112,9 +1121,10 @@ 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("Environment import failed: errno = %d\n", errno);
+
 		return 1;
 	}
 	gd->flags |= GD_FLG_ENV_READY;
@@ -1242,7 +1252,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)

base-commit: b5351a439088dfffd83bfaac81f604344ee2e3bd
--
git-series 0.9.1

[U-Boot] [PATCH v4 2/2] 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>
---

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 | 50 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+)

diff --git a/test/py/tests/test_env.py b/test/py/tests/test_env.py
index bfb5fc0..4da124d 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,52 @@ 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_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')
+
+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')
+
+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 v4 2/2] test/py: add test for whitelist of variables while importing environment

[Stephen Warren]

On 06/04/2018 03:47 AM, Quentin Schulz wrote:
> 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.

Reviewed-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_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')

I could imagine all kinds of other useful tests here, e.g. completely 
missing the size parameter rather than passing it explicitly as - for 
example. But we can add that later if need be.

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

[Quentin Schulz]

Hi Stephen,

On Thu, Jun 07, 2018 at 05:21:19PM -0600, Stephen Warren wrote:
> On 06/04/2018 03:47 AM, Quentin Schulz wrote:
> > 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.
> 
> Reviewed-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_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')
> 
> I could imagine all kinds of other useful tests here, e.g. completely
> missing the size parameter rather than passing it explicitly as - for
> example. But we can add that later if need be.

Indeed. There are a lot of tests to be added to the sandbox for
environment handling. In this patch I focused on what I actually changed
rather than the environment handling as a whole.

That was the idea behind it. Let's get the feature merged with its tests
and then work separately on other tests in sandbox.

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/20180608/1f47bd18/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Tom Rini]

On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:

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

This seems to not be working?

https://travis-ci.org/trini/u-boot/jobs/391504525

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180613/c299ff3f/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Quentin Schulz]

Hi Tom,

On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> 
> > 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>
> 
> This seems to not be working?
> 
> https://travis-ci.org/trini/u-boot/jobs/391504525
> 

I just rebased on top of v2018.07-rc1, ran
make mrproper
./test/py/test.py --bd sandbox --build

and the tests run fine until one error in some tests *after* the env
test suite has passed. See log:
http://code.bulix.org/iojwnp-358559

It's dirty because I had to remove NO_SDL to make sandbox to compile
$ git diff
diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 1a49d1dab5..a76fc384c2 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -74,10 +74,6 @@
 #define CONFIG_BOOTP_SERVERIP
 #define CONFIG_IP_DEFRAG
 
-#ifndef SANDBOX_NO_SDL
-#define CONFIG_SANDBOX_SDL
-#endif
-
 /* LCD and keyboard require SDL support */
 #ifdef CONFIG_SANDBOX_SDL
 #define LCD_BPP                        LCD_COLOR16

Can I have more info on how to reproduce this error please?

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/20180613/345864dc/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Stephen Warren]

On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> Hi Tom,
> 
> On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
>> On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
>>
>>> 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>
>>
>> This seems to not be working?
>>
>> https://travis-ci.org/trini/u-boot/jobs/391504525
>>
> 
> I just rebased on top of v2018.07-rc1, ran
> make mrproper
> ./test/py/test.py --bd sandbox --build
> 
> and the tests run fine ...

Most likely the failure is due to the test relying on some feature that 
isn't enabled on the board being tested (emulated via qemu); you'll need 
to add something like the following to indicate which feature the test 
relies upon:

@pytest.mark.buildconfigspec('cmd_echo')

All the test commands are in .travis.yml in the top-level of the U-Boot 
source tree. You can see the exact error at the very end of the log that 
Tom linked to.

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Tom Rini]

On Wed, Jun 13, 2018 at 08:53:12PM +0200, Quentin Schulz wrote:
> Hi Tom,
> 
> On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > 
> > > 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>
> > 
> > This seems to not be working?
> > 
> > https://travis-ci.org/trini/u-boot/jobs/391504525
> > 
> 
> I just rebased on top of v2018.07-rc1, ran
> make mrproper
> ./test/py/test.py --bd sandbox --build
> 
> and the tests run fine until one error in some tests *after* the env
> test suite has passed. See log:
> http://code.bulix.org/iojwnp-358559
> 
> It's dirty because I had to remove NO_SDL to make sandbox to compile
> $ git diff
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 1a49d1dab5..a76fc384c2 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -74,10 +74,6 @@
>  #define CONFIG_BOOTP_SERVERIP
>  #define CONFIG_IP_DEFRAG
>  
> -#ifndef SANDBOX_NO_SDL
> -#define CONFIG_SANDBOX_SDL
> -#endif
> -
>  /* LCD and keyboard require SDL support */
>  #ifdef CONFIG_SANDBOX_SDL
>  #define LCD_BPP                        LCD_COLOR16
> 
> Can I have more info on how to reproduce this error please?

So, sandbox is clean and doesn't fail.  Looking at the job itself,
https://travis-ci.org/trini/u-boot/builds/391504460, in addition to
failing on xtensa, it fails on integratorcp_cm926ejs, qemu-x86,
qemu-ppce500 and zynq_zc702.  Of those, qemu-x86 is probably the easiest
to reproduce locally for you.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180613/59ef2058/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Quentin Schulz]

Hi all,

On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
> On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> > Hi Tom,
> > 
> > On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > > 
> > > > 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>
> > > 
> > > This seems to not be working?
> > > 
> > > https://travis-ci.org/trini/u-boot/jobs/391504525
> > > 
> > 
> > I just rebased on top of v2018.07-rc1, ran
> > make mrproper
> > ./test/py/test.py --bd sandbox --build
> > 
> > and the tests run fine ...
> 
> Most likely the failure is due to the test relying on some feature that
> isn't enabled on the board being tested (emulated via qemu); you'll need to
> add something like the following to indicate which feature the test relies
> upon:
> 
> @pytest.mark.buildconfigspec('cmd_echo')
> 

OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
that does not make it work any better.

I added my U-Boot repo to Travis and ran the tests. Here is the output
of the job: https://travis-ci.org/QSchulz/u-boot/

Specifically, you have:
https://travis-ci.org/QSchulz/u-boot/jobs/396742661
https://travis-ci.org/QSchulz/u-boot/jobs/396742668
https://travis-ci.org/QSchulz/u-boot/jobs/396742669
https://travis-ci.org/QSchulz/u-boot/jobs/396742670
https://travis-ci.org/QSchulz/u-boot/jobs/396742671

I've dumped the RAM after the `env export` and it looks pretty much
empty compared to what I could see with sandbox tests.

Since all the other tests work, I'm not sure I actually introduced a
regression or if it just never worked. I'll run tests without my patches
that do a `env export` followed by a dump of the memory, a reset of the
environement and a `env import` to see where we stand right now.

Before doing this test (which takes hours), my guess is that either `env
export` is not working for the given configs, or there is something
broken in the test framework (is the RAM address I get with
u_boot_utils.find_ram_base() actually valid?).

I'll let you know the outcome of the tests without my patches.

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/20180626/f033b8bc/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Tom Rini]

On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
> Hi all,
> 
> On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
> > On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> > > Hi Tom,
> > > 
> > > On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > > > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > > > 
> > > > > 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>
> > > > 
> > > > This seems to not be working?
> > > > 
> > > > https://travis-ci.org/trini/u-boot/jobs/391504525
> > > > 
> > > 
> > > I just rebased on top of v2018.07-rc1, ran
> > > make mrproper
> > > ./test/py/test.py --bd sandbox --build
> > > 
> > > and the tests run fine ...
> > 
> > Most likely the failure is due to the test relying on some feature that
> > isn't enabled on the board being tested (emulated via qemu); you'll need to
> > add something like the following to indicate which feature the test relies
> > upon:
> > 
> > @pytest.mark.buildconfigspec('cmd_echo')
> > 
> 
> OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
> that does not make it work any better.
> 
> I added my U-Boot repo to Travis and ran the tests. Here is the output
> of the job: https://travis-ci.org/QSchulz/u-boot/
> 
> Specifically, you have:
> https://travis-ci.org/QSchulz/u-boot/jobs/396742661
> https://travis-ci.org/QSchulz/u-boot/jobs/396742668
> https://travis-ci.org/QSchulz/u-boot/jobs/396742669
> https://travis-ci.org/QSchulz/u-boot/jobs/396742670
> https://travis-ci.org/QSchulz/u-boot/jobs/396742671
> 
> I've dumped the RAM after the `env export` and it looks pretty much
> empty compared to what I could see with sandbox tests.
> 
> Since all the other tests work, I'm not sure I actually introduced a
> regression or if it just never worked. I'll run tests without my patches
> that do a `env export` followed by a dump of the memory, a reset of the
> environement and a `env import` to see where we stand right now.

It's possible you've exposed a latent bug here.  What stood out to me at
the time was that it looked like we were using 0x0 for the address and
that's quite possibly an invalid location to be using on these
platforms.

> Before doing this test (which takes hours), my guess is that either `env
> export` is not working for the given configs, or there is something
> broken in the test framework (is the RAM address I get with
> u_boot_utils.find_ram_base() actually valid?).

Note that when debugging these kind of issues (and it's too much of a
hassle to recreate their environment locally via docker) it's quite
normal to start by hacking out all of the other jobs from .travis.yml so
it only runs what you care about.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180626/21ebfb12/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Stephen Warren]

On 06/26/2018 06:41 AM, Quentin Schulz wrote:
> Hi all,
> 
> On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
>> On 06/13/2018 12:53 PM, Quentin Schulz wrote:
>>> Hi Tom,
>>>
>>> On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
>>>> On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
>>>>
>>>>> 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>
>>>>
>>>> This seems to not be working?
>>>>
>>>> https://travis-ci.org/trini/u-boot/jobs/391504525
>>>>
>>>
>>> I just rebased on top of v2018.07-rc1, ran
>>> make mrproper
>>> ./test/py/test.py --bd sandbox --build
>>>
>>> and the tests run fine ...
>>
>> Most likely the failure is due to the test relying on some feature that
>> isn't enabled on the board being tested (emulated via qemu); you'll need to
>> add something like the following to indicate which feature the test relies
>> upon:
>>
>> @pytest.mark.buildconfigspec('cmd_echo')
>>
> 
> OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
> that does not make it work any better.
> 
> I added my U-Boot repo to Travis and ran the tests. Here is the output
> of the job: https://travis-ci.org/QSchulz/u-boot/
> 
> Specifically, you have:
> https://travis-ci.org/QSchulz/u-boot/jobs/396742661
> https://travis-ci.org/QSchulz/u-boot/jobs/396742668
> https://travis-ci.org/QSchulz/u-boot/jobs/396742669
> https://travis-ci.org/QSchulz/u-boot/jobs/396742670
> https://travis-ci.org/QSchulz/u-boot/jobs/396742671
> 
> I've dumped the RAM after the `env export` and it looks pretty much
> empty compared to what I could see with sandbox tests.
> 
> Since all the other tests work, I'm not sure I actually introduced a
> regression or if it just never worked. I'll run tests without my patches
> that do a `env export` followed by a dump of the memory, a reset of the
> environement and a `env import` to see where we stand right now.
> 
> Before doing this test (which takes hours), my guess is that either `env
> export` is not working for the given configs, or there is something
> broken in the test framework (is the RAM address I get with
> u_boot_utils.find_ram_base() actually valid?).

I checked the qemu-x86 job from Tom's original failure:

https://travis-ci.org/trini/u-boot/jobs/391504523

At least two tests use find_ram_base(); test_net (uses a 4MB offset from 
the base) and test_md (uses the base as-is). Both these tests write data 
to the specified address, then check either the actual value, or a CRC32 
of a range of addresses, and both pass. This implies to me that 
find_ram_base() is returning the correct value, and the memory at that 
location works fine.

This doesn't rule out some problem in find_ram_base() on the other 
failing platforms of course; I didn't check those.

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Quentin Schulz]

Hi Tom,

On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
> On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
> > Hi all,
> > 
> > On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
> > > On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> > > > Hi Tom,
> > > > 
> > > > On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > > > > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > > > > 
> > > > > > 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>
> > > > > 
> > > > > This seems to not be working?
> > > > > 
> > > > > https://travis-ci.org/trini/u-boot/jobs/391504525
> > > > > 
> > > > 
> > > > I just rebased on top of v2018.07-rc1, ran
> > > > make mrproper
> > > > ./test/py/test.py --bd sandbox --build
> > > > 
> > > > and the tests run fine ...
> > > 
> > > Most likely the failure is due to the test relying on some feature that
> > > isn't enabled on the board being tested (emulated via qemu); you'll need to
> > > add something like the following to indicate which feature the test relies
> > > upon:
> > > 
> > > @pytest.mark.buildconfigspec('cmd_echo')
> > > 
> > 
> > OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
> > that does not make it work any better.
> > 
> > I added my U-Boot repo to Travis and ran the tests. Here is the output
> > of the job: https://travis-ci.org/QSchulz/u-boot/
> > 
> > Specifically, you have:
> > https://travis-ci.org/QSchulz/u-boot/jobs/396742661
> > https://travis-ci.org/QSchulz/u-boot/jobs/396742668
> > https://travis-ci.org/QSchulz/u-boot/jobs/396742669
> > https://travis-ci.org/QSchulz/u-boot/jobs/396742670
> > https://travis-ci.org/QSchulz/u-boot/jobs/396742671
> > 
> > I've dumped the RAM after the `env export` and it looks pretty much
> > empty compared to what I could see with sandbox tests.
> > 
> > Since all the other tests work, I'm not sure I actually introduced a
> > regression or if it just never worked. I'll run tests without my patches
> > that do a `env export` followed by a dump of the memory, a reset of the
> > environement and a `env import` to see where we stand right now.
> 
> It's possible you've exposed a latent bug here.  What stood out to me at
> the time was that it looked like we were using 0x0 for the address and
> that's quite possibly an invalid location to be using on these
> platforms.
> 

Yes, looked weird to me as well but can't tell if that's a legit RAM
address. Stephen says other tests interacting with RAM works on those
configs so that should be working.

So, I tested without my patches for whitelisting and it does not work
for the same configs:

https://travis-ci.org/QSchulz/u-boot/jobs/396898590
https://travis-ci.org/QSchulz/u-boot/jobs/396898597
https://travis-ci.org/QSchulz/u-boot/jobs/396898598
https://travis-ci.org/QSchulz/u-boot/jobs/396898599

I'm not introducing a regression with my patch. Do we know if env import
and export is actually supported by those configs? How could we fix
those? Let me know if I can help.

On another subject, I'm worried by these failing jobs:
https://travis-ci.org/QSchulz/u-boot/jobs/396898591
https://travis-ci.org/QSchulz/u-boot/jobs/396898592

Does this mean that we don't have a clean environnement for each and
every test? (env import complains when I'm trying to import an
environnement with ethXaddr in it, so I forcibly removed them from the
environnement before exporting).

> > Before doing this test (which takes hours), my guess is that either `env
> > export` is not working for the given configs, or there is something
> > broken in the test framework (is the RAM address I get with
> > u_boot_utils.find_ram_base() actually valid?).
> 
> Note that when debugging these kind of issues (and it's too much of a
> hassle to recreate their environment locally via docker) it's quite
> normal to start by hacking out all of the other jobs from .travis.yml so
> it only runs what you care about.  Thanks!
> 

Good point, I'll see how I can tweak the Travis YAML to do only the
tests we want.

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/20180627/10a17315/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Tom Rini]

On Wed, Jun 27, 2018 at 09:52:50AM +0200, Quentin Schulz wrote:
> Hi Tom,
> 
> On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
> > On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
> > > Hi all,
> > > 
> > > On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
> > > > On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> > > > > Hi Tom,
> > > > > 
> > > > > On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > > > > > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > > > > > 
> > > > > > > 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>
> > > > > > 
> > > > > > This seems to not be working?
> > > > > > 
> > > > > > https://travis-ci.org/trini/u-boot/jobs/391504525
> > > > > > 
> > > > > 
> > > > > I just rebased on top of v2018.07-rc1, ran
> > > > > make mrproper
> > > > > ./test/py/test.py --bd sandbox --build
> > > > > 
> > > > > and the tests run fine ...
> > > > 
> > > > Most likely the failure is due to the test relying on some feature that
> > > > isn't enabled on the board being tested (emulated via qemu); you'll need to
> > > > add something like the following to indicate which feature the test relies
> > > > upon:
> > > > 
> > > > @pytest.mark.buildconfigspec('cmd_echo')
> > > > 
> > > 
> > > OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
> > > that does not make it work any better.
> > > 
> > > I added my U-Boot repo to Travis and ran the tests. Here is the output
> > > of the job: https://travis-ci.org/QSchulz/u-boot/
> > > 
> > > Specifically, you have:
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396742661
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396742668
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396742669
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396742670
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396742671
> > > 
> > > I've dumped the RAM after the `env export` and it looks pretty much
> > > empty compared to what I could see with sandbox tests.
> > > 
> > > Since all the other tests work, I'm not sure I actually introduced a
> > > regression or if it just never worked. I'll run tests without my patches
> > > that do a `env export` followed by a dump of the memory, a reset of the
> > > environement and a `env import` to see where we stand right now.
> > 
> > It's possible you've exposed a latent bug here.  What stood out to me at
> > the time was that it looked like we were using 0x0 for the address and
> > that's quite possibly an invalid location to be using on these
> > platforms.
> > 
> 
> Yes, looked weird to me as well but can't tell if that's a legit RAM
> address. Stephen says other tests interacting with RAM works on those
> configs so that should be working.
>> 
> So, I tested without my patches for whitelisting and it does not work
> for the same configs:
> 
> https://travis-ci.org/QSchulz/u-boot/jobs/396898590
> https://travis-ci.org/QSchulz/u-boot/jobs/396898597
> https://travis-ci.org/QSchulz/u-boot/jobs/396898598
> https://travis-ci.org/QSchulz/u-boot/jobs/396898599
> 
> I'm not introducing a regression with my patch. Do we know if env import
> and export is actually supported by those configs? How could we fix
> those? Let me know if I can help.

Maybe the tests need to depend on something else too then, that's not
enabled on these platforms?

> On another subject, I'm worried by these failing jobs:
> https://travis-ci.org/QSchulz/u-boot/jobs/396898591
> https://travis-ci.org/QSchulz/u-boot/jobs/396898592
> 
> Does this mean that we don't have a clean environnement for each and
> every test? (env import complains when I'm trying to import an
> environnement with ethXaddr in it, so I forcibly removed them from the
> environnement before exporting).

Right, tests are run serially and don't reset the board for each, iirc.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180627/d78a188e/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Stephen Warren]

On 06/27/2018 09:23 AM, Tom Rini wrote:
> On Wed, Jun 27, 2018 at 09:52:50AM +0200, Quentin Schulz wrote:
>> Hi Tom,
>>
>> On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
>>> On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
>>>> Hi all,
>>>>
>>>> On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
>>>>> On 06/13/2018 12:53 PM, Quentin Schulz wrote:
>>>>>> Hi Tom,
>>>>>>
>>>>>> On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
>>>>>>> On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
>>>>>>>
>>>>>>>> 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>
>>>>>>>
>>>>>>> This seems to not be working?
>>>>>>>
>>>>>>> https://travis-ci.org/trini/u-boot/jobs/391504525
>>>>>>>
>>>>>>
>>>>>> I just rebased on top of v2018.07-rc1, ran
>>>>>> make mrproper
>>>>>> ./test/py/test.py --bd sandbox --build
>>>>>>
>>>>>> and the tests run fine ...
>>>>>
>>>>> Most likely the failure is due to the test relying on some feature that
>>>>> isn't enabled on the board being tested (emulated via qemu); you'll need to
>>>>> add something like the following to indicate which feature the test relies
>>>>> upon:
>>>>>
>>>>> @pytest.mark.buildconfigspec('cmd_echo')
>>>>>
>>>>
>>>> OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
>>>> that does not make it work any better.
>>>>
>>>> I added my U-Boot repo to Travis and ran the tests. Here is the output
>>>> of the job: https://travis-ci.org/QSchulz/u-boot/
>>>>
>>>> Specifically, you have:
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742661
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742668
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742669
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742670
>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742671
>>>>
>>>> I've dumped the RAM after the `env export` and it looks pretty much
>>>> empty compared to what I could see with sandbox tests.
>>>>
>>>> Since all the other tests work, I'm not sure I actually introduced a
>>>> regression or if it just never worked. I'll run tests without my patches
>>>> that do a `env export` followed by a dump of the memory, a reset of the
>>>> environement and a `env import` to see where we stand right now.
>>>
>>> It's possible you've exposed a latent bug here.  What stood out to me at
>>> the time was that it looked like we were using 0x0 for the address and
>>> that's quite possibly an invalid location to be using on these
>>> platforms.
>>>
>>
>> Yes, looked weird to me as well but can't tell if that's a legit RAM
>> address. Stephen says other tests interacting with RAM works on those
>> configs so that should be working.
>>>
>> So, I tested without my patches for whitelisting and it does not work
>> for the same configs:
>>
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898590
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898597
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898598
>> https://travis-ci.org/QSchulz/u-boot/jobs/396898599
>>
>> I'm not introducing a regression with my patch. Do we know if env import
>> and export is actually supported by those configs? How could we fix
>> those? Let me know if I can help.
> 
> Maybe the tests need to depend on something else too then, that's not
> enabled on these platforms?

The bug is in the implementation of hexport_r(); if the provided 
destination pointer is NULL, it allocates a new buffer to write to and 
returns that instead of writing to address 0.

I wonder if some related issue is why test_net adds 4MB to the value 
returned by find_ram_base(); to avoid the NULL pointer.

I'd suggest the following:

1) Enhance find_ram_base() so that if it would return 0, it actually 
returns something else. Adding 2MiB should be safe I think, and return a 
value that's still nicely aligned to any reasonable requirement. (2MiB 
being an ARM LPAE/v8 section size, which makes 2MiB more interesting 
than just 1MiB).

2) Once that's done, perhaps revert the addition of 4MiB in test_net.

3) In the first log you linked above:

https://travis-ci.org/QSchulz/u-boot/jobs/396898590

I see:

Integrator-CP # Integrator-CP # env import 00000000
## Warning: defaulting to text format
## Warning: Input data exceeds 1048576 bytes - truncated
## Info: input data size = 1048578 = 0x100002
Environment import failed: errno = 12

This means "env import" failed, but test/py failed to detect it. Let's 
update the implementation of do_env_export so that its error message is 
formatted like other error messages so that test/py's pattern-based 
error message detection detects it:

u_boot_console_base.py:
pattern_error_notification = re.compile('## Error: ')

Or, update test_env.py to validate that 'Environment import failed' 
isn't in the output of the "env export" command.

After that, hopefully your new test will work on these platforms.

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Quentin Schulz]

Hi Tom,

On Wed, Jun 27, 2018 at 11:23:38AM -0400, Tom Rini wrote:
> On Wed, Jun 27, 2018 at 09:52:50AM +0200, Quentin Schulz wrote:
> > Hi Tom,
> > 
> > On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
> > > On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
> > > > Hi all,
> > > > 
> > > > On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
> > > > > On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> > > > > > Hi Tom,
> > > > > > 
> > > > > > On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > > > > > > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > > > > > > 
> > > > > > > > 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>
> > > > > > > 
> > > > > > > This seems to not be working?
> > > > > > > 
> > > > > > > https://travis-ci.org/trini/u-boot/jobs/391504525
> > > > > > > 
> > > > > > 
> > > > > > I just rebased on top of v2018.07-rc1, ran
> > > > > > make mrproper
> > > > > > ./test/py/test.py --bd sandbox --build
> > > > > > 
> > > > > > and the tests run fine ...
> > > > > 
> > > > > Most likely the failure is due to the test relying on some feature that
> > > > > isn't enabled on the board being tested (emulated via qemu); you'll need to
> > > > > add something like the following to indicate which feature the test relies
> > > > > upon:
> > > > > 
> > > > > @pytest.mark.buildconfigspec('cmd_echo')
> > > > > 
> > > > 
> > > > OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
> > > > that does not make it work any better.
> > > > 
> > > > I added my U-Boot repo to Travis and ran the tests. Here is the output
> > > > of the job: https://travis-ci.org/QSchulz/u-boot/
> > > > 
> > > > Specifically, you have:
> > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742661
> > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742668
> > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742669
> > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742670
> > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742671
> > > > 
> > > > I've dumped the RAM after the `env export` and it looks pretty much
> > > > empty compared to what I could see with sandbox tests.
> > > > 
> > > > Since all the other tests work, I'm not sure I actually introduced a
> > > > regression or if it just never worked. I'll run tests without my patches
> > > > that do a `env export` followed by a dump of the memory, a reset of the
> > > > environement and a `env import` to see where we stand right now.
> > > 
> > > It's possible you've exposed a latent bug here.  What stood out to me at
> > > the time was that it looked like we were using 0x0 for the address and
> > > that's quite possibly an invalid location to be using on these
> > > platforms.
> > > 
> > 
> > Yes, looked weird to me as well but can't tell if that's a legit RAM
> > address. Stephen says other tests interacting with RAM works on those
> > configs so that should be working.
> >> 
> > So, I tested without my patches for whitelisting and it does not work
> > for the same configs:
> > 
> > https://travis-ci.org/QSchulz/u-boot/jobs/396898590
> > https://travis-ci.org/QSchulz/u-boot/jobs/396898597
> > https://travis-ci.org/QSchulz/u-boot/jobs/396898598
> > https://travis-ci.org/QSchulz/u-boot/jobs/396898599
> > 
> > I'm not introducing a regression with my patch. Do we know if env import
> > and export is actually supported by those configs? How could we fix
> > those? Let me know if I can help.
> 
> Maybe the tests need to depend on something else too then, that's not
> enabled on these platforms?
> 
> > On another subject, I'm worried by these failing jobs:
> > https://travis-ci.org/QSchulz/u-boot/jobs/396898591
> > https://travis-ci.org/QSchulz/u-boot/jobs/396898592
> > 
> > Does this mean that we don't have a clean environnement for each and
> > every test? (env import complains when I'm trying to import an
> > environnement with ethXaddr in it, so I forcibly removed them from the
> > environnement before exporting).
> 
> Right, tests are run serially and don't reset the board for each, iirc.
> 

Is there a reason behind this behaviour?

We should fix this somewhere in the future, the tests are basically
meaningless otherwise. If we don't start from scratch for each and every
test (I'm not even talking about test series (e.g. all env tests) but
each test that is called), we basically test that this test works when
all the other tests before have behaved as they did. At best, you get
false positives, at worst you don't catch bugs.

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/20180628/501f93ad/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Quentin Schulz]

Hi Stephen,

On Wed, Jun 27, 2018 at 10:07:04AM -0600, Stephen Warren wrote:
> On 06/27/2018 09:23 AM, Tom Rini wrote:
> > On Wed, Jun 27, 2018 at 09:52:50AM +0200, Quentin Schulz wrote:
> > > Hi Tom,
> > > 
> > > On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
> > > > On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
> > > > > Hi all,
> > > > > 
> > > > > On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
> > > > > > On 06/13/2018 12:53 PM, Quentin Schulz wrote:
> > > > > > > Hi Tom,
> > > > > > > 
> > > > > > > On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
> > > > > > > > On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
> > > > > > > > 
> > > > > > > > > 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>
> > > > > > > > 
> > > > > > > > This seems to not be working?
> > > > > > > > 
> > > > > > > > https://travis-ci.org/trini/u-boot/jobs/391504525
> > > > > > > > 
> > > > > > > 
> > > > > > > I just rebased on top of v2018.07-rc1, ran
> > > > > > > make mrproper
> > > > > > > ./test/py/test.py --bd sandbox --build
> > > > > > > 
> > > > > > > and the tests run fine ...
> > > > > > 
> > > > > > Most likely the failure is due to the test relying on some feature that
> > > > > > isn't enabled on the board being tested (emulated via qemu); you'll need to
> > > > > > add something like the following to indicate which feature the test relies
> > > > > > upon:
> > > > > > 
> > > > > > @pytest.mark.buildconfigspec('cmd_echo')
> > > > > > 
> > > > > 
> > > > > OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
> > > > > that does not make it work any better.
> > > > > 
> > > > > I added my U-Boot repo to Travis and ran the tests. Here is the output
> > > > > of the job: https://travis-ci.org/QSchulz/u-boot/
> > > > > 
> > > > > Specifically, you have:
> > > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742661
> > > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742668
> > > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742669
> > > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742670
> > > > > https://travis-ci.org/QSchulz/u-boot/jobs/396742671
> > > > > 
> > > > > I've dumped the RAM after the `env export` and it looks pretty much
> > > > > empty compared to what I could see with sandbox tests.
> > > > > 
> > > > > Since all the other tests work, I'm not sure I actually introduced a
> > > > > regression or if it just never worked. I'll run tests without my patches
> > > > > that do a `env export` followed by a dump of the memory, a reset of the
> > > > > environement and a `env import` to see where we stand right now.
> > > > 
> > > > It's possible you've exposed a latent bug here.  What stood out to me at
> > > > the time was that it looked like we were using 0x0 for the address and
> > > > that's quite possibly an invalid location to be using on these
> > > > platforms.
> > > > 
> > > 
> > > Yes, looked weird to me as well but can't tell if that's a legit RAM
> > > address. Stephen says other tests interacting with RAM works on those
> > > configs so that should be working.
> > > > 
> > > So, I tested without my patches for whitelisting and it does not work
> > > for the same configs:
> > > 
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396898590
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396898597
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396898598
> > > https://travis-ci.org/QSchulz/u-boot/jobs/396898599
> > > 
> > > I'm not introducing a regression with my patch. Do we know if env import
> > > and export is actually supported by those configs? How could we fix
> > > those? Let me know if I can help.
> > 
> > Maybe the tests need to depend on something else too then, that's not
> > enabled on these platforms?
> 
> The bug is in the implementation of hexport_r(); if the provided destination
> pointer is NULL, it allocates a new buffer to write to and returns that
> instead of writing to address 0.
> 
> I wonder if some related issue is why test_net adds 4MB to the value
> returned by find_ram_base(); to avoid the NULL pointer.
> 

Thanks for the explanation, hopefully that's the only "bug".

> I'd suggest the following:
> 
> 1) Enhance find_ram_base() so that if it would return 0, it actually returns
> something else. Adding 2MiB should be safe I think, and return a value
> that's still nicely aligned to any reasonable requirement. (2MiB being an
> ARM LPAE/v8 section size, which makes 2MiB more interesting than just 1MiB).
> 

I took 2MiB as told.

> 2) Once that's done, perhaps revert the addition of 4MiB in test_net.
> 

Done.

> 3) In the first log you linked above:
> 
> https://travis-ci.org/QSchulz/u-boot/jobs/396898590
> 
> I see:
> 
> Integrator-CP # Integrator-CP # env import 00000000
> ## Warning: defaulting to text format
> ## Warning: Input data exceeds 1048576 bytes - truncated
> ## Info: input data size = 1048578 = 0x100002
> Environment import failed: errno = 12
> 
> This means "env import" failed, but test/py failed to detect it. Let's
> update the implementation of do_env_export so that its error message is
> formatted like other error messages so that test/py's pattern-based error
> message detection detects it:
> 

Done.

> u_boot_console_base.py:
> pattern_error_notification = re.compile('## Error: ')
> 
> Or, update test_env.py to validate that 'Environment import failed' isn't in
> the output of the "env export" command.
> 
> After that, hopefully your new test will work on these platforms.

I'm running all tests with and without my patch series for environment
variable whitelisting, let's see the outcome tomorrow.

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/20180628/7dad134d/attachment.sig>

[U-Boot] [U-Boot, v4, 2/2] test/py: add test for whitelist of variables while importing environment

[Stephen Warren]

On 06/28/2018 07:39 AM, Quentin Schulz wrote:
> Hi Tom,
> 
> On Wed, Jun 27, 2018 at 11:23:38AM -0400, Tom Rini wrote:
>> On Wed, Jun 27, 2018 at 09:52:50AM +0200, Quentin Schulz wrote:
>>> Hi Tom,
>>>
>>> On Tue, Jun 26, 2018 at 10:44:59AM -0400, Tom Rini wrote:
>>>> On Tue, Jun 26, 2018 at 02:41:40PM +0200, Quentin Schulz wrote:
>>>>> Hi all,
>>>>>
>>>>> On Wed, Jun 13, 2018 at 01:02:06PM -0600, Stephen Warren wrote:
>>>>>> On 06/13/2018 12:53 PM, Quentin Schulz wrote:
>>>>>>> Hi Tom,
>>>>>>>
>>>>>>> On Wed, Jun 13, 2018 at 11:43:32AM -0400, Tom Rini wrote:
>>>>>>>> On Mon, Jun 04, 2018 at 11:47:30AM +0200, Quentin Schulz wrote:
>>>>>>>>
>>>>>>>>> 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>
>>>>>>>>
>>>>>>>> This seems to not be working?
>>>>>>>>
>>>>>>>> https://travis-ci.org/trini/u-boot/jobs/391504525
>>>>>>>>
>>>>>>>
>>>>>>> I just rebased on top of v2018.07-rc1, ran
>>>>>>> make mrproper
>>>>>>> ./test/py/test.py --bd sandbox --build
>>>>>>>
>>>>>>> and the tests run fine ...
>>>>>>
>>>>>> Most likely the failure is due to the test relying on some feature that
>>>>>> isn't enabled on the board being tested (emulated via qemu); you'll need to
>>>>>> add something like the following to indicate which feature the test relies
>>>>>> upon:
>>>>>>
>>>>>> @pytest.mark.buildconfigspec('cmd_echo')
>>>>>>
>>>>>
>>>>> OK, I've added the dependency on cmd_importenv and cmd_exportenv, but
>>>>> that does not make it work any better.
>>>>>
>>>>> I added my U-Boot repo to Travis and ran the tests. Here is the output
>>>>> of the job: https://travis-ci.org/QSchulz/u-boot/
>>>>>
>>>>> Specifically, you have:
>>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742661
>>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742668
>>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742669
>>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742670
>>>>> https://travis-ci.org/QSchulz/u-boot/jobs/396742671
>>>>>
>>>>> I've dumped the RAM after the `env export` and it looks pretty much
>>>>> empty compared to what I could see with sandbox tests.
>>>>>
>>>>> Since all the other tests work, I'm not sure I actually introduced a
>>>>> regression or if it just never worked. I'll run tests without my patches
>>>>> that do a `env export` followed by a dump of the memory, a reset of the
>>>>> environement and a `env import` to see where we stand right now.
>>>>
>>>> It's possible you've exposed a latent bug here.  What stood out to me at
>>>> the time was that it looked like we were using 0x0 for the address and
>>>> that's quite possibly an invalid location to be using on these
>>>> platforms.
>>>>
>>>
>>> Yes, looked weird to me as well but can't tell if that's a legit RAM
>>> address. Stephen says other tests interacting with RAM works on those
>>> configs so that should be working.
>>>>
>>> So, I tested without my patches for whitelisting and it does not work
>>> for the same configs:
>>>
>>> https://travis-ci.org/QSchulz/u-boot/jobs/396898590
>>> https://travis-ci.org/QSchulz/u-boot/jobs/396898597
>>> https://travis-ci.org/QSchulz/u-boot/jobs/396898598
>>> https://travis-ci.org/QSchulz/u-boot/jobs/396898599
>>>
>>> I'm not introducing a regression with my patch. Do we know if env import
>>> and export is actually supported by those configs? How could we fix
>>> those? Let me know if I can help.
>>
>> Maybe the tests need to depend on something else too then, that's not
>> enabled on these platforms?
>>
>>> On another subject, I'm worried by these failing jobs:
>>> https://travis-ci.org/QSchulz/u-boot/jobs/396898591
>>> https://travis-ci.org/QSchulz/u-boot/jobs/396898592
>>>
>>> Does this mean that we don't have a clean environnement for each and
>>> every test? (env import complains when I'm trying to import an
>>> environnement with ethXaddr in it, so I forcibly removed them from the
>>> environnement before exporting).
>>
>> Right, tests are run serially and don't reset the board for each, iirc.
>>
> 
> Is there a reason behind this behaviour?
> 
> We should fix this somewhere in the future, the tests are basically
> meaningless otherwise. If we don't start from scratch for each and every
> test (I'm not even talking about test series (e.g. all env tests) but
> each test that is called), we basically test that this test works when
> all the other tests before have behaved as they did. At best, you get
> false positives, at worst you don't catch bugs.

This is deliberate for a couple of reasons:

1) In order to catch general stability problems such as memory 
corruption, we do as much as possible in a single "instance" (boot) of 
U-Boot, so that any such problem has a maximal chance of negatively 
affecting future tests. Yes, this can cause false positives as you 
describe, but in my experience that has happened rarely in practice; I 
vaguely recall one instance of a test working when run after another 
test (something in DFU on one of the targets I use?) and one instance of 
a test failing only when run after another test (some unit test in 
sandbox IIRC), so it balanced out nicely.

2) It's quicker not to reboot the system for each test, which reduces 
test time, which allows for more testing (more test cases).