[U-Boot] [PATCH 00/14] test/py: Convert vboot tests to use the test/py framework

[U-Boot] [PATCH 00/14] test/py: Convert vboot tests to use the test/py framework

[Simon Glass]

There are a number of tests which still exist as shell scripts in U-Boot.
This series converts the vboot test over, adding some functionality to the
test environment as needed.

A few minor changes are made to the vboot test as part of this:
- fit_check_sign is used to check that an invalid signature is detected.
  Previously only the valid case was checked
- Sandbox is adjusted to return normally from a bootm command, instead of
  exiting. This makes the test easier to write and seems more consistent.

One observation about the test/py code. I feel it is confusing that the
classes for running U-Boot commands and shell commands are named in a
similar way. I would suggest renaming u_boot_utils to test_utils, or
similar.


Simon Glass (14):
  test: Add a README
  test: Add a simple script to run tests on sandbox
  sandbox: Don't exit when bootm completes
  test/py: Allow tests to control the sandbox device-tree file
  test/py: Allow RunAndLog() to return the output
  test/py: Provide output from exceptions with RunAndLog()
  test/py: Return output from run_and_log()
  test/py: Add an option to execute a string containing a command
  test/py: Provide a way to check that a command fails
  test/py: Add a helper to run a list of U-Boot commands
  tools: Add an error code when fit_handle_file() fails
  tools: Correct error handling in fit_image_process_hash()
  test/py: Fix up after the rename of CONFIG_SYS_HUSH_PARSER
  test: Convert the vboot test to test/py

 arch/sandbox/lib/bootm.c                          |   2 +-
 common/bootm_os.c                                 |   1 +
 test/README                                       |  92 +++++++++++
 test/py/conftest.py                               |   1 +
 test/py/multiplexed_log.py                        |  10 +-
 test/py/tests/test_hush_if_test.py                |   8 +-
 test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
 test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
 test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
 test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
 test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
 test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
 test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
 test/py/u_boot_console_base.py                    |  16 ++
 test/py/u_boot_console_sandbox.py                 |   2 +-
 test/py/u_boot_utils.py                           |  39 ++++-
 test/run                                          |   4 +
 test/vboot/.gitignore                             |   3 -
 test/vboot/vboot_test.sh                          | 151 ------------------
 tools/fit_image.c                                 |   4 +-
 tools/image-host.c                                |  14 +-
 21 files changed, 361 insertions(+), 171 deletions(-)
 create mode 100644 test/README
 create mode 100644 test/py/tests/test_vboot.py
 rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
 rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
 rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
 rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
 rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
 rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
 create mode 100755 test/run
 delete mode 100644 test/vboot/.gitignore
 delete mode 100755 test/vboot/vboot_test.sh

-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 01/14] test: Add a README

[Simon Glass]

Add a few notes about how testing works in U-Boot.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/README | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 test/README

diff --git a/test/README b/test/README
new file mode 100644
index 0000000..dfd83d6
--- /dev/null
+++ b/test/README
@@ -0,0 +1,82 @@
+Testing in U-Boot
+=================
+
+U-Boot has a large amount of code. This file describes how this code is
+tested and what tests you should write when adding a new feature.
+
+
+Sandbox
+-------
+U-Boot can be built as a user-space application (e.g. for Linux). This
+allows test to be executed without needing target hardware. The 'sandbox'
+target provides this feature and it is widely used in tests.
+
+
+Pytest Suite
+------------
+
+Many tests are available using the pytest suite, in test/py. This can run
+either on sandbox or on real hardware. It relies on the U-Boot console to
+inject test commands and check the result. It is slower to run than C code,
+but provides the ability to unify lots of test and summarise their results.
+
+You can run the tests on sandbox with:
+
+	./test/py/test.py --bd sandbox --build
+
+This will produce HTML output in build-sandbox/test-log.html
+
+See test/py/README.md for more information about the pytest suite.
+
+
+tbot
+----
+
+Tbot provides a way to execute tests on target hardware. It is intended for
+trying out both U-Boot and Linux (and potentially other software) on a
+number of boards automatically. It can be used to create a continuous test
+environment. See tools/tbot/README for more information.
+
+
+Ad-hoc tests
+------------
+
+There are several ad-hoc tests which run outside the pytest environment:
+
+   test/fs	- File system test (shell script)
+   test/image	- FIT and lagacy image tests (shell script and Python)
+   test/stdint	- A test that stdint.h can be used in U-Boot (shell script)
+   trace	- Test for the tracing feature (shell script)
+   vboot	- Test for verified boot (shell script)
+
+The above should be converted to run as part of the pytest suite.
+
+
+When to write tests
+-------------------
+
+If you add code to U-Boot without a test you are taking a risk. Even if you
+perform thorough manual testing at the time of submission, it may break when
+future changes are made to U-Boot. It may even break when applied to mainline,
+if other changes interact with it. A good mindset is that untested code
+probably doesn't work and should be deleted.
+
+You can assume that the Pytest suite will be run before patches are accepted
+to mainline, so this provides protection against future breakage.
+
+On the other hand there is quite a bit of code that is not covered with tests,
+or is covered sparingly. So here are some suggestions:
+
+- If you are adding a new uclass, add a sandbox driver and a test that uses it
+- If you are modifying code covered by an existing test, add a new test case
+  to cover your changes
+- If the code you are modifying has not tests, consider writing one. Even a
+  very basic test is useful, and may be picked up and enhanced by others. It
+  is much easier to add onto a test - writing a new large test can seem
+  daunting to most contributors.
+
+
+Future work
+-----------
+
+Converting existing shell scripts into pytest tests.
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 02/14] test: Add a simple script to run tests on sandbox

[Simon Glass]

A common check before sending patches is to run all available tests on
sandbox. But everytime I do this I have to look up the README. This presents
quite a barrier to actually doing this.

Add a shell script to help. To run the tests, type:

   test/run

in the U-Boot directory, which should be easy to remember.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/README | 11 +++++++++++
 test/run    |  4 ++++
 2 files changed, 15 insertions(+)
 create mode 100755 test/run

diff --git a/test/README b/test/README
index dfd83d6..6cfee05 100644
--- a/test/README
+++ b/test/README
@@ -5,6 +5,17 @@ U-Boot has a large amount of code. This file describes how this code is
 tested and what tests you should write when adding a new feature.
 
 
+Running tests
+-------------
+
+To run most tests on sandbox, type this:
+
+    test/run
+
+in the U-Boot directory. Note that only the pytest suite is run using this
+comment.
+
+
 Sandbox
 -------
 U-Boot can be built as a user-space application (e.g. for Linux). This
diff --git a/test/run b/test/run
new file mode 100755
index 0000000..a6dcf8f
--- /dev/null
+++ b/test/run
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+# Run all tests
+./test/py/test.py --bd sandbox --build
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 03/14] sandbox: Don't exit when bootm completes

[Simon Glass]

At present sandbox exits when the 'bootm' command completes, since it is not
actually able to run the OS that is loaded. Normally 'bootm' failure is
considered a fatal error in U-Boot.

However this is annoying for tests, which may want to examine the state
after a test is complete. In any case there is a 'reset' command which can
be used to exit, if required.

Change the behaviour to return normally from the 'bootm' command on sandbox.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/sandbox/lib/bootm.c | 2 +-
 common/bootm_os.c        | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c
index d49c927..0c9a797 100644
--- a/arch/sandbox/lib/bootm.c
+++ b/arch/sandbox/lib/bootm.c
@@ -56,7 +56,7 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 		bootstage_mark(BOOTSTAGE_ID_RUN_OS);
 		printf("## Transferring control to Linux (at address %08lx)...\n",
 		       images->ep);
-		reset_cpu(0);
+		printf("sandbox: continuing, as we cannot run Linux\n");
 	}
 
 	return 0;
diff --git a/common/bootm_os.c b/common/bootm_os.c
index 9ec84bd..e3f5a46 100644
--- a/common/bootm_os.c
+++ b/common/bootm_os.c
@@ -481,6 +481,7 @@ int boot_selected_os(int argc, char * const argv[], int state,
 
 	/* Stand-alone may return when 'autostart' is 'no' */
 	if (images->os.type == IH_TYPE_STANDALONE ||
+	    IS_ENABLED(CONFIG_SANDBOX) ||
 	    state == BOOTM_STATE_OS_FAKE_GO) /* We expect to return */
 		return 0;
 	bootstage_error(BOOTSTAGE_ID_BOOT_OS_RETURNED);
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 04/14] test/py: Allow tests to control the sandbox device-tree file

[Simon Glass]

Normally tests will run with the test.dtb file designed for this purpose.
However, the verified boot tests need to run with their own device-tree
file, containing a public key.

Make the device-tree file a config option so that it can be adjusted by
tests. The default is to keep the current behaviour.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/conftest.py               | 1 +
 test/py/u_boot_console_sandbox.py | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/py/conftest.py b/test/py/conftest.py
index 449f98b..5b16456 100644
--- a/test/py/conftest.py
+++ b/test/py/conftest.py
@@ -179,6 +179,7 @@ def pytest_configure(config):
     ubconfig.board_type = board_type
     ubconfig.board_identity = board_identity
     ubconfig.gdbserver = gdbserver
+    ubconfig.dtb = build_dir + '/arch/sandbox/dts/test.dtb'
 
     env_vars = (
         'board_type',
diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
index 04654ae..b440497 100644
--- a/test/py/u_boot_console_sandbox.py
+++ b/test/py/u_boot_console_sandbox.py
@@ -46,7 +46,7 @@ class ConsoleSandbox(ConsoleBase):
             self.config.build_dir + '/u-boot',
             '-v',
             '-d',
-            self.config.build_dir + '/arch/sandbox/dts/test.dtb'
+            self.config.dtb
         ]
         return Spawn(cmd, cwd=self.config.source_dir)
 
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 05/14] test/py: Allow RunAndLog() to return the output

[Simon Glass]

Tests may want to look at the output from running a command. Return it so
that this is possible.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/multiplexed_log.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/py/multiplexed_log.py b/test/py/multiplexed_log.py
index 68917eb..02c44df 100644
--- a/test/py/multiplexed_log.py
+++ b/test/py/multiplexed_log.py
@@ -119,7 +119,7 @@ class RunAndLog(object):
                 raised if such problems occur.
 
         Returns:
-            Nothing.
+            The output as a string.
         """
 
         msg = '+' + ' '.join(cmd) + '\n'
@@ -161,6 +161,7 @@ class RunAndLog(object):
             self.chained_file.write(output)
         if exception:
             raise exception
+        return output
 
 class SectionCtxMgr(object):
     """A context manager for Python's "with" statement, which allows a certain
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 06/14] test/py: Provide output from exceptions with RunAndLog()

[Simon Glass]

Tests may want to look at the output from running a command, even if it
fails (e.g. with a non-zero return code). Provide a means to obtain this.

Another approach would be to return a class object containing both the
output and the exception, but I'm not sure if that would result in a lot
of refactoring.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/multiplexed_log.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/test/py/multiplexed_log.py b/test/py/multiplexed_log.py
index 02c44df..35a32fb 100644
--- a/test/py/multiplexed_log.py
+++ b/test/py/multiplexed_log.py
@@ -101,6 +101,7 @@ class RunAndLog(object):
         self.logfile = logfile
         self.name = name
         self.chained_file = chained_file
+        self.output = None
 
     def close(self):
         """Clean up any resources managed by this object."""
@@ -109,6 +110,9 @@ class RunAndLog(object):
     def run(self, cmd, cwd=None, ignore_errors=False):
         """Run a command as a sub-process, and log the results.
 
+        The output is available at self.output which can be useful if there is
+        an exception.
+
         Args:
             cmd: The command to execute.
             cwd: The directory to run the command in. Can be None to use the
@@ -159,6 +163,9 @@ class RunAndLog(object):
         self.logfile.write(self, output)
         if self.chained_file:
             self.chained_file.write(output)
+
+        # Store the output so it can be accessed if we raise an exception.
+        self.output = output
         if exception:
             raise exception
         return output
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 07/14] test/py: Return output from run_and_log()

[Simon Glass]

It is useful to be able to obtain the output from a command. Return it from
this function.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/u_boot_utils.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
index 6a6b2ec..5dc0f71 100644
--- a/test/py/u_boot_utils.py
+++ b/test/py/u_boot_utils.py
@@ -165,12 +165,13 @@ def run_and_log(u_boot_console, cmd, ignore_errors=False):
             problems occur.
 
     Returns:
-        Nothing.
+        The output as a string.
     """
 
     runner = u_boot_console.log.get_runner(cmd[0], sys.stdout)
-    runner.run(cmd, ignore_errors=ignore_errors)
+    output = runner.run(cmd, ignore_errors=ignore_errors)
     runner.close()
+    return output
 
 ram_base = None
 def find_ram_base(u_boot_console):
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 08/14] test/py: Add an option to execute a string containing a command

[Simon Glass]

It is sometimes inconvenient to convert a string into a list for execution
with run_and_log(). Provide a helper function to do this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/u_boot_utils.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
index 5dc0f71..5d638d9 100644
--- a/test/py/u_boot_utils.py
+++ b/test/py/u_boot_utils.py
@@ -173,6 +173,18 @@ def run_and_log(u_boot_console, cmd, ignore_errors=False):
     runner.close()
     return output
 
+def cmd(u_boot_console, cmd_str):
+    """Run a single command string and log its output.
+
+    Args:
+        u_boot_console: A console connection to U-Boot.
+        cmd: The command to run, as a string.
+
+    Returns:
+        The output as a string.
+    """
+    return run_and_log(u_boot_console, cmd_str.split())
+
 ram_base = None
 def find_ram_base(u_boot_console):
     """Find the running U-Boot's RAM location.
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 09/14] test/py: Provide a way to check that a command fails

[Simon Glass]

Sometimes we want to run a command and check that it fails. Add a function
to handle this. It can check the return code and also make sure that the
output contains a given error message.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/u_boot_utils.py | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
index 5d638d9..c834e7b 100644
--- a/test/py/u_boot_utils.py
+++ b/test/py/u_boot_utils.py
@@ -185,6 +185,28 @@ def cmd(u_boot_console, cmd_str):
     """
     return run_and_log(u_boot_console, cmd_str.split())
 
+def run_and_log_expect_exception(u_boot_console, cmd, retcode, msg):
+    """Run a command which is expected to fail.
+
+    This runs a command and checks that it fails with the expected return code
+    and exception method. If not, an exception is raised.
+
+    Args:
+        u_boot_console: A console connection to U-Boot.
+        cmd: The command to run, as an array of argv[].
+        retcode: Expected non-zero return code from the command.
+        msg: String which should be contained within the command's output.
+    """
+    try:
+        runner = u_boot_console.log.get_runner(cmd[0], sys.stdout)
+        runner.run(cmd)
+    except Exception as e:
+        assert(msg in runner.output)
+    else:
+        raise Exception('Expected exception, but not raised')
+    finally:
+        runner.close()
+
 ram_base = None
 def find_ram_base(u_boot_console):
     """Find the running U-Boot's RAM location.
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 10/14] test/py: Add a helper to run a list of U-Boot commands

[Simon Glass]

Some tests want to execute a sequence of commands. Add a helper for this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/u_boot_console_base.py | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
index 815fa64..b5aad7c 100644
--- a/test/py/u_boot_console_base.py
+++ b/test/py/u_boot_console_base.py
@@ -216,6 +216,22 @@ class ConsoleBase(object):
             self.cleanup_spawn()
             raise
 
+    def run_command_list(self, cmds):
+        """Run a list of commands.
+
+        This is a helper function to call run_command() with default arguments
+        for each command in a list.
+
+        Args:
+            cmd: List of commands (each a string)
+        Returns:
+            Combined output of all commands, as a string
+        """
+        output = ''
+        for cmd in cmds:
+            output += self.run_command(cmd)
+        return output
+
     def ctrlc(self):
         """Send a CTRL-C character to U-Boot.
 
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 11/14] tools: Add an error code when fit_handle_file() fails

[Simon Glass]

The error code may provide useful information for debugging. Add it to the
error string.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/fit_image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/fit_image.c b/tools/fit_image.c
index 58aa8e2..f471982 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -650,8 +650,8 @@ static int fit_handle_file(struct image_tool_params *params)
 	}
 
 	if (ret) {
-		fprintf(stderr, "%s Can't add hashes to FIT blob\n",
-			params->cmdname);
+		fprintf(stderr, "%s Can't add hashes to FIT blob: %d\n",
+			params->cmdname, ret);
 		goto err_system;
 	}
 
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 12/14] tools: Correct error handling in fit_image_process_hash()

[Simon Glass]

We should not be returning -1 as an error code. This can mask a situation
where we run out of space adding things to the FIT. By returning the correct
error in this case (-ENOSPC) it can be handled by the higher-level code.

This may fix the error reported by Tom Van Deun here:

https://www.mail-archive.com/u-boot at lists.denx.de/msg217417.html

although I am not sure as I cannot actually repeat it.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reported-by: Tom Van Deun <tom.vandeun@wapice.com>
---

 tools/image-host.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/image-host.c b/tools/image-host.c
index 7effb6c..3e14fdc 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -38,7 +38,7 @@ static int fit_set_hash_value(void *fit, int noffset, uint8_t *value,
 		printf("Can't set hash '%s' property for '%s' node(%s)\n",
 		       FIT_VALUE_PROP, fit_get_name(fit, noffset, NULL),
 		       fdt_strerror(ret));
-		return -1;
+		return ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO;
 	}
 
 	return 0;
@@ -64,25 +64,27 @@ static int fit_image_process_hash(void *fit, const char *image_name,
 	const char *node_name;
 	int value_len;
 	char *algo;
+	int ret;
 
 	node_name = fit_get_name(fit, noffset, NULL);
 
 	if (fit_image_hash_get_algo(fit, noffset, &algo)) {
 		printf("Can't get hash algo property for '%s' hash node in '%s' image node\n",
 		       node_name, image_name);
-		return -1;
+		return -ENOENT;
 	}
 
 	if (calculate_hash(data, size, algo, value, &value_len)) {
 		printf("Unsupported hash algorithm (%s) for '%s' hash node in '%s' image node\n",
 		       algo, node_name, image_name);
-		return -1;
+		return -EPROTONOSUPPORT;
 	}
 
-	if (fit_set_hash_value(fit, noffset, value, value_len)) {
+	ret = fit_set_hash_value(fit, noffset, value, value_len);
+	if (ret) {
 		printf("Can't set hash value for '%s' hash node in '%s' image node\n",
 		       node_name, image_name);
-		return -1;
+		return ret;
 	}
 
 	return 0;
@@ -322,7 +324,7 @@ int fit_image_add_verification_data(const char *keydir, void *keydest,
 				comment, require_keys);
 		}
 		if (ret)
-			return -1;
+			return ret;
 	}
 
 	return 0;
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 13/14] test/py: Fix up after the rename of CONFIG_SYS_HUSH_PARSER

[Simon Glass]

At present all the hush tests are skipped on sandbox because the test thinks
that this option is disabled. In fact it has just been renamed.

It might be better to use the full CONFIG_xxx name in tests with
@pytest.mark.buildconfigspec(), since at present it is not really clear that
the options are related.

Fixes: f1f9d4fa (hush: complete renaming CONFIG_SYS_HUSH_PARSER to CONFIG_HUSH_PARSER)

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/tests/test_hush_if_test.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test/py/tests/test_hush_if_test.py b/test/py/tests/test_hush_if_test.py
index 1eeaa5b..b572538 100644
--- a/test/py/tests/test_hush_if_test.py
+++ b/test/py/tests/test_hush_if_test.py
@@ -109,27 +109,27 @@ def exec_hush_if(u_boot_console, expr, result):
     response = u_boot_console.run_command(cmd)
     assert response.strip() == str(result).lower()
 
- at pytest.mark.buildconfigspec('sys_hush_parser')
+ at pytest.mark.buildconfigspec('hush_parser')
 def test_hush_if_test_setup(u_boot_console):
     """Set up environment variables used during the "if" tests."""
 
     u_boot_console.run_command('setenv ut_var_nonexistent')
     u_boot_console.run_command('setenv ut_var_exists 1')
 
- at pytest.mark.buildconfigspec('sys_hush_parser')
+ at pytest.mark.buildconfigspec('hush_parser')
 @pytest.mark.parametrize('expr,result', subtests)
 def test_hush_if_test(u_boot_console, expr, result):
     """Test a single "if test" condition."""
 
     exec_hush_if(u_boot_console, expr, result)
 
- at pytest.mark.buildconfigspec('sys_hush_parser')
+ at pytest.mark.buildconfigspec('hush_parser')
 def test_hush_if_test_teardown(u_boot_console):
     """Clean up environment variables used during the "if" tests."""
 
     u_boot_console.run_command('setenv ut_var_exists')
 
- at pytest.mark.buildconfigspec('sys_hush_parser')
+ at pytest.mark.buildconfigspec('hush_parser')
 # We might test this on real filesystems via UMS, DFU, 'save', etc.
 # Of those, only UMS currently allows file removal though.
 @pytest.mark.boardspec('sandbox')
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 14/14] test: Convert the vboot test to test/py

[Simon Glass]

Now that we have a suitable test framework we should move all tests into it.
The vboot test is a suitable candidate. Rewrite it in Python and move the
data files into an appropriate directory.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/README                                       |   1 -
 test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
 test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
 test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
 test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
 test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
 test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
 test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
 test/vboot/.gitignore                             |   3 -
 test/vboot/vboot_test.sh                          | 151 ------------------
 10 files changed, 185 insertions(+), 155 deletions(-)
 create mode 100644 test/py/tests/test_vboot.py
 rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
 rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
 rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
 rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
 rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
 rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
 delete mode 100644 test/vboot/.gitignore
 delete mode 100755 test/vboot/vboot_test.sh

diff --git a/test/README b/test/README
index 6cfee05..ee55972 100644
--- a/test/README
+++ b/test/README
@@ -58,7 +58,6 @@ There are several ad-hoc tests which run outside the pytest environment:
    test/image	- FIT and lagacy image tests (shell script and Python)
    test/stdint	- A test that stdint.h can be used in U-Boot (shell script)
    trace	- Test for the tracing feature (shell script)
-   vboot	- Test for verified boot (shell script)
 
 The above should be converted to run as part of the pytest suite.
 
diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
new file mode 100644
index 0000000..c779895
--- /dev/null
+++ b/test/py/tests/test_vboot.py
@@ -0,0 +1,185 @@
+# Copyright (c) 2013, Google Inc.
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+# U-Boot Verified Boot Test
+
+"""
+This tests verified boot in the following ways:
+
+For image verification:
+- Create FIT (unsigned) with mkimage
+- Check that verification shows that no keys are verified
+- Sign image
+- Check that verification shows that a key is now verified
+
+For configuration verification:
+- Corrupt signature and check for failure
+- Create FIT (with unsigned configuration) with mkimage
+- Check that image veriication works
+- Sign the FIT and mark the key as 'required' for verification
+- Check that image verification works
+- Corrupt the signature
+- Check that image verification no-longer works
+
+Tests run with both SHA1 and SHA256 hashing.
+"""
+
+import pytest
+import sys
+import u_boot_utils as util
+
+ at pytest.mark.buildconfigspec('fit_signature')
+def test_vboot(u_boot_console):
+    """Test verified boot signing with mkimage and verification with 'bootm'.
+
+    This works using sandbox only as it needs to update the device tree used
+    by U-Boot to hold public keys from the signing process.
+
+    The SHA1 and SHA256 tests are combined into a single test since the
+    key-generation process is quite slow and we want to avoid doing it twice.
+    """
+    def dtc(dts):
+        """Run the device-tree compiler to compile a .dts file
+
+        The output file will be the same as the input file but with a .dtb
+        extension.
+
+        Args:
+            dts: Device tree file to compile.
+        """
+        dtb = dts.replace('.dts', '.dtb')
+        util.cmd(cons, 'dtc %s %s%s -O dtb '
+                       '-o %s%s' % (dtc_args, datadir, dts, tmpdir, dtb))
+
+    def run_bootm(test_type, expect_string):
+        """Run a 'bootm' command U-Boot.
+
+        This always starts a fresh U-Boot instance since the device tree may
+        contain a new public key.
+
+        Args:
+            test_type: A string identifying the test type
+            expect_string: A string which is expected in the output
+        """
+        cons.cleanup_spawn()
+        cons.ensure_spawned()
+        cons.log.action('%s: Test Verified Boot Run: %s' % (algo, test_type))
+        output = cons.run_command_list(
+            ['sb load hostfs - 100 %stest.fit' % tmpdir,
+             'fdt addr 100',
+             'bootm 100'])
+        assert(expect_string in output)
+
+    def make_fit(its):
+        """Make a new FIT from the .its source file
+
+        This runs 'mkimage -f' to create a new FIT.
+
+        Args:
+            its: Filename containing .its source
+        """
+        util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f',
+                                '%s%s' % (datadir, its), fit])
+
+    def sign_fit():
+        """Sign the FIT
+
+        Signs the FIT and writes the signature into it. It also writes the
+        public key into the dtb.
+        """
+        cons.log.action('%s: Sign images' % algo)
+        util.run_and_log(cons, [mkimage, '-F', '-k', tmpdir, '-K', dtb,
+                                '-r', fit])
+
+    def test_with_algo(sha):
+        """Test verified boot with the given hash algorithm
+
+        This is the main part of the test code. The same procedure is followed
+        for both hashing algorithms.
+
+        Args:
+            sha: Either 'sha1' or 'sha256', to select the algorithm to use
+        """
+        global algo
+
+        algo = sha
+
+        # Compile our device tree files for kernel and U-Boot
+        dtc('sandbox-kernel.dts')
+        dtc('sandbox-u-boot.dts')
+
+        # Build the FIT, but don't sign anything yet
+        cons.log.action('%s: Test FIT with signed images' % algo)
+        make_fit('sign-images-%s.its' % algo)
+        run_bootm('unsigned images', 'dev-')
+
+        # Sign images with our dev keys
+        sign_fit()
+        run_bootm('signed images', 'dev+')
+
+        # Create a fresh .dtb without the public keys
+        dtc('sandbox-u-boot.dts')
+
+        cons.log.action('%s: Test FIT with signed configuration' % algo)
+        make_fit('sign-configs-%s.its' % algo)
+        run_bootm('unsigned config', '%s+ OK' % algo)
+
+        # Sign images with our dev keys
+        sign_fit()
+        run_bootm('signed config', 'dev+')
+
+        cons.log.action('%s: Check signed config on the host' % algo)
+
+        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', tmpdir,
+                                '-k', dtb])
+
+        # Increment the first byte of the signature, which should cause failure
+        sig = util.cmd(cons, 'fdtget -t bx %s %s value' % (fit, sig_node))
+        byte_list = sig.split()
+        byte = int(byte_list[0], 16)
+        byte_list = ['%x' % (byte + 1)] + byte_list[1:]
+        sig = ' '.join(byte_list)
+        util.cmd(cons, 'fdtput -t bx %s %s value %s' % (fit, sig_node, sig))
+
+        run_bootm('Signed config with bad hash', 'Bad Data Hash')
+
+        cons.log.action('%s: Check bad config on the host' % algo)
+        util.run_and_log_expect_exception(cons, [fit_check_sign, '-f', fit,
+                '-k', dtb], 1, 'Failed to verify required signature')
+
+    cons = u_boot_console
+    tmpdir = cons.config.result_dir + '/'
+    tmp = tmpdir + 'vboot.tmp'
+    datadir = 'test/py/tests/vboot/'
+    fit = '%stest.fit' % tmpdir
+    mkimage = cons.config.build_dir + '/tools/mkimage'
+    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
+    dtc_args = '-I dts -O dtb -i %s' % tmpdir
+    dtb = '%ssandbox-u-boot.dtb' % tmpdir
+    sig_node = '/configurations/conf at 1/signature at 1'
+
+    # Create an RSA key pair
+    public_exponent = 65537
+    util.cmd(cons, 'openssl genpkey -algorithm RSA -out %sdev.key '
+                   '-pkeyopt rsa_keygen_bits:2048 '
+                   '-pkeyopt rsa_keygen_pubexp:%d '
+                   '2>/dev/null'  % (tmpdir, public_exponent))
+
+    # Create a certificate containing the public key
+    util.cmd(cons, 'openssl req -batch -new -x509 -key %sdev.key -out '
+                   '%sdev.crt' % (tmpdir, tmpdir))
+
+    # Create a number kernel image with zeroes
+    with open('%stest-kernel.bin' % tmpdir, 'w') as fd:
+        fd.write(5000 * chr(0))
+
+    try:
+        # We need to use our own device tree file. Remember to restore it
+        # afterwards.
+        old_dtb = cons.config.dtb
+        cons.config.dtb = dtb
+        test_with_algo('sha1')
+        test_with_algo('sha256')
+    finally:
+        cons.config.dtb = old_dtb
diff --git a/test/vboot/sandbox-kernel.dts b/test/py/tests/vboot/sandbox-kernel.dts
similarity index 100%
rename from test/vboot/sandbox-kernel.dts
rename to test/py/tests/vboot/sandbox-kernel.dts
diff --git a/test/vboot/sandbox-u-boot.dts b/test/py/tests/vboot/sandbox-u-boot.dts
similarity index 100%
rename from test/vboot/sandbox-u-boot.dts
rename to test/py/tests/vboot/sandbox-u-boot.dts
diff --git a/test/vboot/sign-configs-sha1.its b/test/py/tests/vboot/sign-configs-sha1.its
similarity index 100%
rename from test/vboot/sign-configs-sha1.its
rename to test/py/tests/vboot/sign-configs-sha1.its
diff --git a/test/vboot/sign-configs-sha256.its b/test/py/tests/vboot/sign-configs-sha256.its
similarity index 100%
rename from test/vboot/sign-configs-sha256.its
rename to test/py/tests/vboot/sign-configs-sha256.its
diff --git a/test/vboot/sign-images-sha1.its b/test/py/tests/vboot/sign-images-sha1.its
similarity index 100%
rename from test/vboot/sign-images-sha1.its
rename to test/py/tests/vboot/sign-images-sha1.its
diff --git a/test/vboot/sign-images-sha256.its b/test/py/tests/vboot/sign-images-sha256.its
similarity index 100%
rename from test/vboot/sign-images-sha256.its
rename to test/py/tests/vboot/sign-images-sha256.its
diff --git a/test/vboot/.gitignore b/test/vboot/.gitignore
deleted file mode 100644
index 4631242..0000000
--- a/test/vboot/.gitignore
+++ /dev/null
@@ -1,3 +0,0 @@
-/*.dtb
-/test.fit
-/dev-keys
diff --git a/test/vboot/vboot_test.sh b/test/vboot/vboot_test.sh
deleted file mode 100755
index 6d7abb8..0000000
--- a/test/vboot/vboot_test.sh
+++ /dev/null
@@ -1,151 +0,0 @@
-#!/bin/bash
-#
-# Copyright (c) 2013, Google Inc.
-#
-# Simple Verified Boot Test Script
-#
-# SPDX-License-Identifier:	GPL-2.0+
-
-set -e
-
-# Run U-Boot and report the result
-# Args:
-#	$1:	Test message
-run_uboot() {
-	echo -n "Test Verified Boot Run: $1: "
-	${uboot} -d sandbox-u-boot.dtb >${tmp} -c '
-sb load hostfs - 100 test.fit;
-fdt addr 100;
-bootm 100;
-reset'
-	if ! grep -q "$2" ${tmp}; then
-		echo
-		echo "Verified boot key check failed, output follows:"
-		cat ${tmp}
-		false
-	else
-		echo "OK"
-	fi
-}
-
-echo "Simple Verified Boot Test"
-echo "========================="
-echo
-echo "Please see doc/uImage.FIT/verified-boot.txt for more information"
-echo
-
-err=0
-tmp=/tmp/vboot_test.$$
-
-dir=$(dirname $0)
-
-if [ -z ${O} ]; then
-	O=.
-fi
-O=$(readlink -f ${O})
-
-dtc="-I dts -O dtb -p 2000"
-uboot="${O}/u-boot"
-mkimage="${O}/tools/mkimage"
-fit_check_sign="${O}/tools/fit_check_sign"
-keys="${dir}/dev-keys"
-echo ${mkimage} -D "${dtc}"
-
-echo "Build keys"
-mkdir -p ${keys}
-
-PUBLIC_EXPONENT=${1}
-
-if [ -z "${PUBLIC_EXPONENT}" ]; then
-	PUBLIC_EXPONENT=65537
-fi
-
-# Create an RSA key pair
-openssl genpkey -algorithm RSA -out ${keys}/dev.key \
-    -pkeyopt rsa_keygen_bits:2048 \
-    -pkeyopt rsa_keygen_pubexp:${PUBLIC_EXPONENT} 2>/dev/null
-
-# Create a certificate containing the public key
-openssl req -batch -new -x509 -key ${keys}/dev.key -out ${keys}/dev.crt
-
-pushd ${dir} >/dev/null
-
-function do_test {
-	echo do $sha test
-	# Compile our device tree files for kernel and U-Boot
-	dtc -p 0x1000 sandbox-kernel.dts -O dtb -o sandbox-kernel.dtb
-	dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
-
-	# Create a number kernel image with zeroes
-	head -c 5000 /dev/zero >test-kernel.bin
-
-	# Build the FIT, but don't sign anything yet
-	echo Build FIT with signed images
-	${mkimage} -D "${dtc}" -f sign-images-$sha.its test.fit >${tmp}
-
-	run_uboot "unsigned signatures:" "dev-"
-
-	# Sign images with our dev keys
-	echo Sign images
-	${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb \
-		-r test.fit >${tmp}
-
-	run_uboot "signed images" "dev+"
-
-
-	# Create a fresh .dtb without the public keys
-	dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
-
-	echo Build FIT with signed configuration
-	${mkimage} -D "${dtc}" -f sign-configs-$sha.its test.fit >${tmp}
-
-	run_uboot "unsigned config" $sha"+ OK"
-
-	# Sign images with our dev keys
-	echo Sign images
-	${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb \
-		-r test.fit >${tmp}
-
-	run_uboot "signed config" "dev+"
-
-	echo check signed config on the host
-	if ! ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then
-		echo
-		echo "Verified boot key check on host failed, output follows:"
-		cat ${tmp}
-		false
-	else
-		if ! grep -q "dev+" ${tmp}; then
-			echo
-			echo "Verified boot key check failed, output follows:"
-			cat ${tmp}
-			false
-		else
-			echo "OK"
-		fi
-	fi
-
-	run_uboot "signed config" "dev+"
-
-	# Increment the first byte of the signature, which should cause failure
-	sig=$(fdtget -t bx test.fit /configurations/conf at 1/signature at 1 value)
-	newbyte=$(printf %x $((0x${sig:0:2} + 1)))
-	sig="${newbyte} ${sig:2}"
-	fdtput -t bx test.fit /configurations/conf at 1/signature at 1 value ${sig}
-
-	run_uboot "signed config with bad hash" "Bad Data Hash"
-}
-
-sha=sha1
-do_test
-sha=sha256
-do_test
-
-popd >/dev/null
-
-echo
-if ${ok}; then
-	echo "Test passed"
-else
-	echo "Test failed"
-fi
-- 
2.8.0.rc3.226.g39d4020

[U-Boot] [PATCH 11/14] tools: Add an error code when fit_handle_file() fails

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> The error code may provide useful information for debugging. Add it to the
> error string.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  tools/fit_image.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index 58aa8e2..f471982 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -650,8 +650,8 @@ static int fit_handle_file(struct image_tool_params *params)
>         }
>
>         if (ret) {
> -               fprintf(stderr, "%s Can't add hashes to FIT blob\n",
> -                       params->cmdname);
> +               fprintf(stderr, "%s Can't add hashes to FIT blob: %d\n",
> +                       params->cmdname, ret);
>                 goto err_system;
>         }
>
> --
> 2.8.0.rc3.226.g39d4020
>

Simple change, should help debugging/dev!
-Teddy

-- 
Teddy Reed V

[U-Boot] [PATCH 01/14] test: Add a README

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Add a few notes about how testing works in U-Boot.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/README | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 test/README
>
> diff --git a/test/README b/test/README
> new file mode 100644
> index 0000000..dfd83d6
> --- /dev/null
> +++ b/test/README
> @@ -0,0 +1,82 @@
> +Testing in U-Boot
> +=================
> +
> +U-Boot has a large amount of code. This file describes how this code is
> +tested and what tests you should write when adding a new feature.
> +
> +
> +Sandbox
> +-------
> +U-Boot can be built as a user-space application (e.g. for Linux). This
> +allows test to be executed without needing target hardware. The 'sandbox'
> +target provides this feature and it is widely used in tests.
> +
> +
> +Pytest Suite
> +------------
> +
> +Many tests are available using the pytest suite, in test/py. This can run
> +either on sandbox or on real hardware. It relies on the U-Boot console to
> +inject test commands and check the result. It is slower to run than C code,
> +but provides the ability to unify lots of test and summarise their results.

lots of tests

> +
> +You can run the tests on sandbox with:
> +
> +       ./test/py/test.py --bd sandbox --build
> +
> +This will produce HTML output in build-sandbox/test-log.html
> +
> +See test/py/README.md for more information about the pytest suite.
> +
> +
> +tbot
> +----
> +
> +Tbot provides a way to execute tests on target hardware. It is intended for
> +trying out both U-Boot and Linux (and potentially other software) on a
> +number of boards automatically. It can be used to create a continuous test
> +environment. See tools/tbot/README for more information.
> +
> +
> +Ad-hoc tests
> +------------
> +
> +There are several ad-hoc tests which run outside the pytest environment:
> +
> +   test/fs     - File system test (shell script)
> +   test/image  - FIT and lagacy image tests (shell script and Python)

s/lagacy/legacy/

> +   test/stdint - A test that stdint.h can be used in U-Boot (shell script)
> +   trace       - Test for the tracing feature (shell script)
> +   vboot       - Test for verified boot (shell script)
> +
> +The above should be converted to run as part of the pytest suite.

Is this a NOTE or a TODO?

> +
> +
> +When to write tests
> +-------------------
> +
> +If you add code to U-Boot without a test you are taking a risk. Even if you
> +perform thorough manual testing at the time of submission, it may break when
> +future changes are made to U-Boot. It may even break when applied to mainline,
> +if other changes interact with it. A good mindset is that untested code
> +probably doesn't work and should be deleted.
> +
> +You can assume that the Pytest suite will be run before patches are accepted
> +to mainline, so this provides protection against future breakage.
> +
> +On the other hand there is quite a bit of code that is not covered with tests,
> +or is covered sparingly. So here are some suggestions:
> +
> +- If you are adding a new uclass, add a sandbox driver and a test that uses it
> +- If you are modifying code covered by an existing test, add a new test case
> +  to cover your changes
> +- If the code you are modifying has not tests, consider writing one. Even a
> +  very basic test is useful, and may be picked up and enhanced by others. It
> +  is much easier to add onto a test - writing a new large test can seem
> +  daunting to most contributors.
> +
> +
> +Future work
> +-----------
> +
> +Converting existing shell scripts into pytest tests.
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Thank for these docs! I wonder if a clone/mirror of the repo on Github
can be set up to run the sandbox/Pytest tests in TravisCI with minimum
effort? :)

-Teddy

-- 
Teddy Reed V

[U-Boot] [PATCH 04/14] test/py: Allow tests to control the sandbox device-tree file

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Normally tests will run with the test.dtb file designed for this purpose.
> However, the verified boot tests need to run with their own device-tree
> file, containing a public key.
>
> Make the device-tree file a config option so that it can be adjusted by
> tests. The default is to keep the current behaviour.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/conftest.py               | 1 +
>  test/py/u_boot_console_sandbox.py | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/test/py/conftest.py b/test/py/conftest.py
> index 449f98b..5b16456 100644
> --- a/test/py/conftest.py
> +++ b/test/py/conftest.py
> @@ -179,6 +179,7 @@ def pytest_configure(config):
>      ubconfig.board_type = board_type
>      ubconfig.board_identity = board_identity
>      ubconfig.gdbserver = gdbserver
> +    ubconfig.dtb = build_dir + '/arch/sandbox/dts/test.dtb'
>
>      env_vars = (
>          'board_type',
> diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
> index 04654ae..b440497 100644
> --- a/test/py/u_boot_console_sandbox.py
> +++ b/test/py/u_boot_console_sandbox.py
> @@ -46,7 +46,7 @@ class ConsoleSandbox(ConsoleBase):
>              self.config.build_dir + '/u-boot',
>              '-v',
>              '-d',
> -            self.config.build_dir + '/arch/sandbox/dts/test.dtb'
> +            self.config.dtb
>          ]
>          return Spawn(cmd, cwd=self.config.source_dir)
>
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Teddy Reed V

[U-Boot] [PATCH 08/14] test/py: Add an option to execute a string containing a command

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> It is sometimes inconvenient to convert a string into a list for execution
> with run_and_log(). Provide a helper function to do this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/u_boot_utils.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
> index 5dc0f71..5d638d9 100644
> --- a/test/py/u_boot_utils.py
> +++ b/test/py/u_boot_utils.py
> @@ -173,6 +173,18 @@ def run_and_log(u_boot_console, cmd, ignore_errors=False):
>      runner.close()
>      return output
>
> +def cmd(u_boot_console, cmd_str):
> +    """Run a single command string and log its output.
> +
> +    Args:
> +        u_boot_console: A console connection to U-Boot.
> +        cmd: The command to run, as a string.

cmd_str

> +
> +    Returns:
> +        The output as a string.
> +    """
> +    return run_and_log(u_boot_console, cmd_str.split())

The default behavior for `string.split` is to remove most whitespace
tokens and use any, or any consecutive set, as a delimiter.

In the case of:

$ cmd_str = 'here "are three" arguments'
$ cmd_str.split()
['here', '"are', 'three"', 'arguments']

If this is an acceptable side-effect or handled elsewhere, it seems reasonable.

> +
>  ram_base = None
>  def find_ram_base(u_boot_console):
>      """Find the running U-Boot's RAM location.
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Teddy Reed V

[U-Boot] [PATCH 03/14] sandbox: Don't exit when bootm completes

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> At present sandbox exits when the 'bootm' command completes, since it is not
> actually able to run the OS that is loaded. Normally 'bootm' failure is
> considered a fatal error in U-Boot.
>
> However this is annoying for tests, which may want to examine the state
> after a test is complete. In any case there is a 'reset' command which can
> be used to exit, if required.
>
> Change the behaviour to return normally from the 'bootm' command on sandbox.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  arch/sandbox/lib/bootm.c | 2 +-
>  common/bootm_os.c        | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c
> index d49c927..0c9a797 100644
> --- a/arch/sandbox/lib/bootm.c
> +++ b/arch/sandbox/lib/bootm.c
> @@ -56,7 +56,7 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>                 bootstage_mark(BOOTSTAGE_ID_RUN_OS);
>                 printf("## Transferring control to Linux (at address %08lx)...\n",
>                        images->ep);
> -               reset_cpu(0);
> +               printf("sandbox: continuing, as we cannot run Linux\n");
>         }
>
>         return 0;
> diff --git a/common/bootm_os.c b/common/bootm_os.c
> index 9ec84bd..e3f5a46 100644
> --- a/common/bootm_os.c
> +++ b/common/bootm_os.c
> @@ -481,6 +481,7 @@ int boot_selected_os(int argc, char * const argv[], int state,
>
>         /* Stand-alone may return when 'autostart' is 'no' */
>         if (images->os.type == IH_TYPE_STANDALONE ||
> +           IS_ENABLED(CONFIG_SANDBOX) ||
>             state == BOOTM_STATE_OS_FAKE_GO) /* We expect to return */
>                 return 0;
>         bootstage_error(BOOTSTAGE_ID_BOOT_OS_RETURNED);
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

This will help a lot with vboot testing, thanks!

This may affect others with existing sandbox tests set up in a CI/CB.
But I feel this is the right approach since it now allows fallback
(corrupted recovery) / secondary boot media testing too.

-- 
Teddy Reed V

[U-Boot] [PATCH 02/14] test: Add a simple script to run tests on sandbox

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> A common check before sending patches is to run all available tests on
> sandbox. But everytime I do this I have to look up the README. This presents
> quite a barrier to actually doing this.
>
> Add a shell script to help. To run the tests, type:
>
>    test/run
>
> in the U-Boot directory, which should be easy to remember.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/README | 11 +++++++++++
>  test/run    |  4 ++++
>  2 files changed, 15 insertions(+)
>  create mode 100755 test/run
>
> diff --git a/test/README b/test/README
> index dfd83d6..6cfee05 100644
> --- a/test/README
> +++ b/test/README
> @@ -5,6 +5,17 @@ U-Boot has a large amount of code. This file describes how this code is
>  tested and what tests you should write when adding a new feature.
>
>
> +Running tests
> +-------------
> +
> +To run most tests on sandbox, type this:

To run most tests on _the_ sandbox?

> +
> +    test/run
> +
> +in the U-Boot directory. Note that only the pytest suite is run using this
> +comment.

s/comment/command/

> +
> +
>  Sandbox
>  -------
>  U-Boot can be built as a user-space application (e.g. for Linux). This
> diff --git a/test/run b/test/run
> new file mode 100755
> index 0000000..a6dcf8f
> --- /dev/null
> +++ b/test/run
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +# Run all tests
> +./test/py/test.py --bd sandbox --build
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

This seems reasonable, but would a make target be easier: make sandbox_test?

-- 
Teddy Reed V

[U-Boot] [PATCH 05/14] test/py: Allow RunAndLog() to return the output

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Tests may want to look at the output from running a command. Return it so
> that this is possible.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/multiplexed_log.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/test/py/multiplexed_log.py b/test/py/multiplexed_log.py
> index 68917eb..02c44df 100644
> --- a/test/py/multiplexed_log.py
> +++ b/test/py/multiplexed_log.py
> @@ -119,7 +119,7 @@ class RunAndLog(object):
>                  raised if such problems occur.
>
>          Returns:
> -            Nothing.
> +            The output as a string.
>          """
>
>          msg = '+' + ' '.join(cmd) + '\n'
> @@ -161,6 +161,7 @@ class RunAndLog(object):
>              self.chained_file.write(output)
>          if exception:
>              raise exception
> +        return output
>
>  class SectionCtxMgr(object):
>      """A context manager for Python's "with" statement, which allows a certain
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Teddy Reed V

[U-Boot] [PATCH 06/14] test/py: Provide output from exceptions with RunAndLog()

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Tests may want to look at the output from running a command, even if it
> fails (e.g. with a non-zero return code). Provide a means to obtain this.
>
> Another approach would be to return a class object containing both the
> output and the exception, but I'm not sure if that would result in a lot
> of refactoring.

In my experience with Pytest/Gtest _not_ using a class to represent
output/exception is the way to go!

Then a test author can write test cases with a flow like:
ASSERT_NOEXCEPT(output = doAction())
EXPECT_EQUAL(2, output)

The test harness can provide much more succinct errors when these cases fail. :)

>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/multiplexed_log.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/test/py/multiplexed_log.py b/test/py/multiplexed_log.py
> index 02c44df..35a32fb 100644
> --- a/test/py/multiplexed_log.py
> +++ b/test/py/multiplexed_log.py
> @@ -101,6 +101,7 @@ class RunAndLog(object):
>          self.logfile = logfile
>          self.name = name
>          self.chained_file = chained_file
> +        self.output = None
>
>      def close(self):
>          """Clean up any resources managed by this object."""
> @@ -109,6 +110,9 @@ class RunAndLog(object):
>      def run(self, cmd, cwd=None, ignore_errors=False):
>          """Run a command as a sub-process, and log the results.
>
> +        The output is available at self.output which can be useful if there is
> +        an exception.
> +
>          Args:
>              cmd: The command to execute.
>              cwd: The directory to run the command in. Can be None to use the
> @@ -159,6 +163,9 @@ class RunAndLog(object):
>          self.logfile.write(self, output)
>          if self.chained_file:
>              self.chained_file.write(output)
> +
> +        # Store the output so it can be accessed if we raise an exception.
> +        self.output = output
>          if exception:
>              raise exception
>          return output
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

-- 
Teddy Reed V

[U-Boot] [PATCH 09/14] test/py: Provide a way to check that a command fails

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Sometimes we want to run a command and check that it fails. Add a function
> to handle this. It can check the return code and also make sure that the
> output contains a given error message.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  test/py/u_boot_utils.py | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
> index 5d638d9..c834e7b 100644
> --- a/test/py/u_boot_utils.py
> +++ b/test/py/u_boot_utils.py
> @@ -185,6 +185,28 @@ def cmd(u_boot_console, cmd_str):
>      """
>      return run_and_log(u_boot_console, cmd_str.split())
>
> +def run_and_log_expect_exception(u_boot_console, cmd, retcode, msg):
> +    """Run a command which is expected to fail.

nit, in the restrictive case, s/which/that/

> +
> +    This runs a command and checks that it fails with the expected return code
> +    and exception method. If not, an exception is raised.
> +
> +    Args:
> +        u_boot_console: A console connection to U-Boot.
> +        cmd: The command to run, as an array of argv[].
> +        retcode: Expected non-zero return code from the command.
> +        msg: String which should be contained within the command's output.

nit, same as above

> +    """
> +    try:
> +        runner = u_boot_console.log.get_runner(cmd[0], sys.stdout)
> +        runner.run(cmd)
> +    except Exception as e:
> +        assert(msg in runner.output)

I understand why this is a partial match, as requiring a complete
match would result in changing several test cases every time debug
output was altered. Not fun.

That said, it seems dangerous. When performing failure tests there are
often several failing side-effects. For example, if you are testing
hash-mismatches in vboot several code paths may complain about
comparison failures, partial output matching may not be able to
isolate the failing logic. You may encounter a false positive if any
side-effect emits the same expected output.

Would optional parameters make sense? They could allow a tester to
restrict matching to the last line of output, or request a complete
string match?

> +    else:
> +        raise Exception('Expected exception, but not raised')

This is a bit vague, does it make sense to include the expected
(failing) retcode too?

> +    finally:
> +        runner.close()
> +
>  ram_base = None
>  def find_ram_base(u_boot_console):
>      """Find the running U-Boot's RAM location.
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Let me know if my comments about output matching are too pedantic.
Since there are few existing test cases, the last thing I want to do
is over-engineer some helpful testing facilities. :)

-- 
Teddy Reed V

[U-Boot] [PATCH 07/14] test/py: Return output from run_and_log()

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> It is useful to be able to obtain the output from a command. Return it from
> this function.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/u_boot_utils.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
> index 6a6b2ec..5dc0f71 100644
> --- a/test/py/u_boot_utils.py
> +++ b/test/py/u_boot_utils.py
> @@ -165,12 +165,13 @@ def run_and_log(u_boot_console, cmd, ignore_errors=False):
>              problems occur.
>
>      Returns:
> -        Nothing.
> +        The output as a string.
>      """
>
>      runner = u_boot_console.log.get_runner(cmd[0], sys.stdout)
> -    runner.run(cmd, ignore_errors=ignore_errors)
> +    output = runner.run(cmd, ignore_errors=ignore_errors)
>      runner.close()
> +    return output
>
>  ram_base = None
>  def find_ram_base(u_boot_console):
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Teddy Reed V

[U-Boot] [PATCH 10/14] test/py: Add a helper to run a list of U-Boot commands

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Some tests want to execute a sequence of commands. Add a helper for this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/u_boot_console_base.py | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
> index 815fa64..b5aad7c 100644
> --- a/test/py/u_boot_console_base.py
> +++ b/test/py/u_boot_console_base.py
> @@ -216,6 +216,22 @@ class ConsoleBase(object):
>              self.cleanup_spawn()
>              raise
>
> +    def run_command_list(self, cmds):
> +        """Run a list of commands.
> +
> +        This is a helper function to call run_command() with default arguments
> +        for each command in a list.
> +
> +        Args:
> +            cmd: List of commands (each a string)

nit, in most other docstrings, these Args and Return lines have punctuation.

> +        Returns:
> +            Combined output of all commands, as a string
> +        """
> +        output = ''
> +        for cmd in cmds:
> +            output += self.run_command(cmd)

Although this fits the prototypes for other run_* functions, a list of
output strings is much easier to test and reason about. :/

> +        return output
> +
>      def ctrlc(self):
>          """Send a CTRL-C character to U-Boot.
>
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Teddy Reed V

[U-Boot] [PATCH 13/14] test/py: Fix up after the rename of CONFIG_SYS_HUSH_PARSER

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> At present all the hush tests are skipped on sandbox because the test thinks
> that this option is disabled. In fact it has just been renamed.
>
> It might be better to use the full CONFIG_xxx name in tests with
> @pytest.mark.buildconfigspec(), since at present it is not really clear that
> the options are related.
>
> Fixes: f1f9d4fa (hush: complete renaming CONFIG_SYS_HUSH_PARSER to CONFIG_HUSH_PARSER)
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  test/py/tests/test_hush_if_test.py | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/test/py/tests/test_hush_if_test.py b/test/py/tests/test_hush_if_test.py
> index 1eeaa5b..b572538 100644
> --- a/test/py/tests/test_hush_if_test.py
> +++ b/test/py/tests/test_hush_if_test.py
> @@ -109,27 +109,27 @@ def exec_hush_if(u_boot_console, expr, result):
>      response = u_boot_console.run_command(cmd)
>      assert response.strip() == str(result).lower()
>
> - at pytest.mark.buildconfigspec('sys_hush_parser')
> + at pytest.mark.buildconfigspec('hush_parser')
>  def test_hush_if_test_setup(u_boot_console):
>      """Set up environment variables used during the "if" tests."""
>
>      u_boot_console.run_command('setenv ut_var_nonexistent')
>      u_boot_console.run_command('setenv ut_var_exists 1')
>
> - at pytest.mark.buildconfigspec('sys_hush_parser')
> + at pytest.mark.buildconfigspec('hush_parser')
>  @pytest.mark.parametrize('expr,result', subtests)
>  def test_hush_if_test(u_boot_console, expr, result):
>      """Test a single "if test" condition."""
>
>      exec_hush_if(u_boot_console, expr, result)
>
> - at pytest.mark.buildconfigspec('sys_hush_parser')
> + at pytest.mark.buildconfigspec('hush_parser')
>  def test_hush_if_test_teardown(u_boot_console):
>      """Clean up environment variables used during the "if" tests."""
>
>      u_boot_console.run_command('setenv ut_var_exists')
>
> - at pytest.mark.buildconfigspec('sys_hush_parser')
> + at pytest.mark.buildconfigspec('hush_parser')
>  # We might test this on real filesystems via UMS, DFU, 'save', etc.
>  # Of those, only UMS currently allows file removal though.
>  @pytest.mark.boardspec('sandbox')
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Awesome, thanks!

-- 
Teddy Reed V

[U-Boot] [PATCH 01/14] test: Add a README

[Simon Glass]

+Roger, for Travis-CI

Hi Teddy,

On 3 July 2016 at 13:17, Teddy Reed <teddy.reed@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
>> Add a few notes about how testing works in U-Boot.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Thanks for the comments. Will tidy up.

>
>> ---
>>
>>  test/README | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>  create mode 100644 test/README
>>
>> diff --git a/test/README b/test/README
>> new file mode 100644
>> index 0000000..dfd83d6
>> --- /dev/null
>> +++ b/test/README
>> @@ -0,0 +1,82 @@
>> +Testing in U-Boot
>> +=================
>> +
>> +U-Boot has a large amount of code. This file describes how this code is
>> +tested and what tests you should write when adding a new feature.
>> +
>> +
>> +Sandbox
>> +-------
>> +U-Boot can be built as a user-space application (e.g. for Linux). This
>> +allows test to be executed without needing target hardware. The 'sandbox'
>> +target provides this feature and it is widely used in tests.
>> +
>> +
>> +Pytest Suite
>> +------------
>> +
>> +Many tests are available using the pytest suite, in test/py. This can run
>> +either on sandbox or on real hardware. It relies on the U-Boot console to
>> +inject test commands and check the result. It is slower to run than C code,
>> +but provides the ability to unify lots of test and summarise their results.
>
> lots of tests
>
>> +
>> +You can run the tests on sandbox with:
>> +
>> +       ./test/py/test.py --bd sandbox --build
>> +
>> +This will produce HTML output in build-sandbox/test-log.html
>> +
>> +See test/py/README.md for more information about the pytest suite.
>> +
>> +
>> +tbot
>> +----
>> +
>> +Tbot provides a way to execute tests on target hardware. It is intended for
>> +trying out both U-Boot and Linux (and potentially other software) on a
>> +number of boards automatically. It can be used to create a continuous test
>> +environment. See tools/tbot/README for more information.
>> +
>> +
>> +Ad-hoc tests
>> +------------
>> +
>> +There are several ad-hoc tests which run outside the pytest environment:
>> +
>> +   test/fs     - File system test (shell script)
>> +   test/image  - FIT and lagacy image tests (shell script and Python)
>
> s/lagacy/legacy/
>
>> +   test/stdint - A test that stdint.h can be used in U-Boot (shell script)
>> +   trace       - Test for the tracing feature (shell script)
>> +   vboot       - Test for verified boot (shell script)
>> +
>> +The above should be converted to run as part of the pytest suite.
>
> Is this a NOTE or a TODO?

TODO - I'll reword it.

>
>> +
>> +
>> +When to write tests
>> +-------------------
>> +
>> +If you add code to U-Boot without a test you are taking a risk. Even if you
>> +perform thorough manual testing at the time of submission, it may break when
>> +future changes are made to U-Boot. It may even break when applied to mainline,
>> +if other changes interact with it. A good mindset is that untested code
>> +probably doesn't work and should be deleted.
>> +
>> +You can assume that the Pytest suite will be run before patches are accepted
>> +to mainline, so this provides protection against future breakage.
>> +
>> +On the other hand there is quite a bit of code that is not covered with tests,
>> +or is covered sparingly. So here are some suggestions:
>> +
>> +- If you are adding a new uclass, add a sandbox driver and a test that uses it
>> +- If you are modifying code covered by an existing test, add a new test case
>> +  to cover your changes
>> +- If the code you are modifying has not tests, consider writing one. Even a
>> +  very basic test is useful, and may be picked up and enhanced by others. It
>> +  is much easier to add onto a test - writing a new large test can seem
>> +  daunting to most contributors.
>> +
>> +
>> +Future work
>> +-----------
>> +
>> +Converting existing shell scripts into pytest tests.
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> Thank for these docs! I wonder if a clone/mirror of the repo on Github
> can be set up to run the sandbox/Pytest tests in TravisCI with minimum
> effort? :)

There is a .travis.yml file - I have not tried this but I believe
people are already doing it somewhere.

Regards.
Simon

[U-Boot] [PATCH 12/14] tools: Correct error handling in fit_image_process_hash()

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> We should not be returning -1 as an error code. This can mask a situation
> where we run out of space adding things to the FIT. By returning the correct
> error in this case (-ENOSPC) it can be handled by the higher-level code.
>
> This may fix the error reported by Tom Van Deun here:
>
> https://www.mail-archive.com/u-boot at lists.denx.de/msg217417.html
>
> although I am not sure as I cannot actually repeat it.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reported-by: Tom Van Deun <tom.vandeun@wapice.com>

Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

> ---
>
>  tools/image-host.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 7effb6c..3e14fdc 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -38,7 +38,7 @@ static int fit_set_hash_value(void *fit, int noffset, uint8_t *value,
>                 printf("Can't set hash '%s' property for '%s' node(%s)\n",
>                        FIT_VALUE_PROP, fit_get_name(fit, noffset, NULL),
>                        fdt_strerror(ret));
> -               return -1;
> +               return ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO;
>         }
>
>         return 0;
> @@ -64,25 +64,27 @@ static int fit_image_process_hash(void *fit, const char *image_name,
>         const char *node_name;
>         int value_len;
>         char *algo;
> +       int ret;
>
>         node_name = fit_get_name(fit, noffset, NULL);
>
>         if (fit_image_hash_get_algo(fit, noffset, &algo)) {
>                 printf("Can't get hash algo property for '%s' hash node in '%s' image node\n",
>                        node_name, image_name);
> -               return -1;
> +               return -ENOENT;
>         }
>
>         if (calculate_hash(data, size, algo, value, &value_len)) {
>                 printf("Unsupported hash algorithm (%s) for '%s' hash node in '%s' image node\n",
>                        algo, node_name, image_name);
> -               return -1;
> +               return -EPROTONOSUPPORT;
>         }
>
> -       if (fit_set_hash_value(fit, noffset, value, value_len)) {
> +       ret = fit_set_hash_value(fit, noffset, value, value_len);
> +       if (ret) {
>                 printf("Can't set hash value for '%s' hash node in '%s' image node\n",
>                        node_name, image_name);
> -               return -1;
> +               return ret;
>         }
>
>         return 0;
> @@ -322,7 +324,7 @@ int fit_image_add_verification_data(const char *keydir, void *keydest,
>                                 comment, require_keys);
>                 }
>                 if (ret)
> -                       return -1;
> +                       return ret;
>         }
>
>         return 0;
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Teddy Reed V

[U-Boot] [PATCH 14/14] test: Convert the vboot test to test/py

[Teddy Reed]

Hi Simon,

On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Now that we have a suitable test framework we should move all tests into it.
> The vboot test is a suitable candidate. Rewrite it in Python and move the
> data files into an appropriate directory.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  test/README                                       |   1 -
>  test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
>  test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
>  test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
>  test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
>  test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
>  test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
>  test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
>  test/vboot/.gitignore                             |   3 -
>  test/vboot/vboot_test.sh                          | 151 ------------------
>  10 files changed, 185 insertions(+), 155 deletions(-)
>  create mode 100644 test/py/tests/test_vboot.py
>  rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
>  rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
>  rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
>  rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
>  rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
>  rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
>  delete mode 100644 test/vboot/.gitignore
>  delete mode 100755 test/vboot/vboot_test.sh
>
> diff --git a/test/README b/test/README
> index 6cfee05..ee55972 100644
> --- a/test/README
> +++ b/test/README
> @@ -58,7 +58,6 @@ There are several ad-hoc tests which run outside the pytest environment:
>     test/image  - FIT and lagacy image tests (shell script and Python)
>     test/stdint - A test that stdint.h can be used in U-Boot (shell script)
>     trace       - Test for the tracing feature (shell script)
> -   vboot       - Test for verified boot (shell script)
>
>  The above should be converted to run as part of the pytest suite.
>
> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> new file mode 100644
> index 0000000..c779895
> --- /dev/null
> +++ b/test/py/tests/test_vboot.py
> @@ -0,0 +1,185 @@
> +# Copyright (c) 2013, Google Inc.
> +#
> +# SPDX-License-Identifier:     GPL-2.0+
> +#
> +# U-Boot Verified Boot Test
> +
> +"""
> +This tests verified boot in the following ways:
> +
> +For image verification:
> +- Create FIT (unsigned) with mkimage
> +- Check that verification shows that no keys are verified
> +- Sign image
> +- Check that verification shows that a key is now verified
> +
> +For configuration verification:
> +- Corrupt signature and check for failure
> +- Create FIT (with unsigned configuration) with mkimage
> +- Check that image veriication works

verification

> +- Sign the FIT and mark the key as 'required' for verification
> +- Check that image verification works
> +- Corrupt the signature
> +- Check that image verification no-longer works
> +
> +Tests run with both SHA1 and SHA256 hashing.
> +"""
> +
> +import pytest
> +import sys
> +import u_boot_utils as util
> +
> + at pytest.mark.buildconfigspec('fit_signature')
> +def test_vboot(u_boot_console):
> +    """Test verified boot signing with mkimage and verification with 'bootm'.
> +
> +    This works using sandbox only as it needs to update the device tree used
> +    by U-Boot to hold public keys from the signing process.
> +
> +    The SHA1 and SHA256 tests are combined into a single test since the
> +    key-generation process is quite slow and we want to avoid doing it twice.
> +    """
> +    def dtc(dts):
> +        """Run the device-tree compiler to compile a .dts file

In a few other places it reads: "device tree" without a dash. ;)

> +
> +        The output file will be the same as the input file but with a .dtb
> +        extension.
> +
> +        Args:
> +            dts: Device tree file to compile.
> +        """
> +        dtb = dts.replace('.dts', '.dtb')
> +        util.cmd(cons, 'dtc %s %s%s -O dtb '
> +                       '-o %s%s' % (dtc_args, datadir, dts, tmpdir, dtb))
> +
> +    def run_bootm(test_type, expect_string):
> +        """Run a 'bootm' command U-Boot.
> +
> +        This always starts a fresh U-Boot instance since the device tree may
> +        contain a new public key.
> +
> +        Args:
> +            test_type: A string identifying the test type
> +            expect_string: A string which is expected in the output

nit, punctuation.

> +        """
> +        cons.cleanup_spawn()
> +        cons.ensure_spawned()
> +        cons.log.action('%s: Test Verified Boot Run: %s' % (algo, test_type))
> +        output = cons.run_command_list(
> +            ['sb load hostfs - 100 %stest.fit' % tmpdir,
> +             'fdt addr 100',
> +             'bootm 100'])
> +        assert(expect_string in output)
> +
> +    def make_fit(its):
> +        """Make a new FIT from the .its source file
> +
> +        This runs 'mkimage -f' to create a new FIT.
> +
> +        Args:
> +            its: Filename containing .its source

nit, same

> +        """
> +        util.run_and_log(cons, [mkimage, '-D', dtc_args, '-f',
> +                                '%s%s' % (datadir, its), fit])
> +
> +    def sign_fit():
> +        """Sign the FIT
> +
> +        Signs the FIT and writes the signature into it. It also writes the
> +        public key into the dtb.
> +        """
> +        cons.log.action('%s: Sign images' % algo)
> +        util.run_and_log(cons, [mkimage, '-F', '-k', tmpdir, '-K', dtb,
> +                                '-r', fit])
> +
> +    def test_with_algo(sha):

nit, maybe sha_algo?

> +        """Test verified boot with the given hash algorithm

nit, same

> +
> +        This is the main part of the test code. The same procedure is followed
> +        for both hashing algorithms.
> +
> +        Args:
> +            sha: Either 'sha1' or 'sha256', to select the algorithm to use
> +        """
> +        global algo
> +
> +        algo = sha
> +
> +        # Compile our device tree files for kernel and U-Boot
> +        dtc('sandbox-kernel.dts')
> +        dtc('sandbox-u-boot.dts')
> +
> +        # Build the FIT, but don't sign anything yet
> +        cons.log.action('%s: Test FIT with signed images' % algo)
> +        make_fit('sign-images-%s.its' % algo)
> +        run_bootm('unsigned images', 'dev-')
> +
> +        # Sign images with our dev keys
> +        sign_fit()
> +        run_bootm('signed images', 'dev+')
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')
> +
> +        cons.log.action('%s: Test FIT with signed configuration' % algo)
> +        make_fit('sign-configs-%s.its' % algo)
> +        run_bootm('unsigned config', '%s+ OK' % algo)
> +
> +        # Sign images with our dev keys
> +        sign_fit()
> +        run_bootm('signed config', 'dev+')
> +
> +        cons.log.action('%s: Check signed config on the host' % algo)
> +
> +        util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', tmpdir,
> +                                '-k', dtb])
> +
> +        # Increment the first byte of the signature, which should cause failure
> +        sig = util.cmd(cons, 'fdtget -t bx %s %s value' % (fit, sig_node))
> +        byte_list = sig.split()
> +        byte = int(byte_list[0], 16)
> +        byte_list = ['%x' % (byte + 1)] + byte_list[1:]
> +        sig = ' '.join(byte_list)
> +        util.cmd(cons, 'fdtput -t bx %s %s value %s' % (fit, sig_node, sig))
> +
> +        run_bootm('Signed config with bad hash', 'Bad Data Hash')
> +
> +        cons.log.action('%s: Check bad config on the host' % algo)
> +        util.run_and_log_expect_exception(cons, [fit_check_sign, '-f', fit,
> +                '-k', dtb], 1, 'Failed to verify required signature')
> +
> +    cons = u_boot_console
> +    tmpdir = cons.config.result_dir + '/'
> +    tmp = tmpdir + 'vboot.tmp'

Is there a need to clean these up afterward?

Python's tempfile might be helpful, you can supply the result_dir as
the directory prefix.

> +    datadir = 'test/py/tests/vboot/'
> +    fit = '%stest.fit' % tmpdir
> +    mkimage = cons.config.build_dir + '/tools/mkimage'
> +    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
> +    dtc_args = '-I dts -O dtb -i %s' % tmpdir
> +    dtb = '%ssandbox-u-boot.dtb' % tmpdir
> +    sig_node = '/configurations/conf at 1/signature at 1'

If these variables are used throughout the tests like globals, should
they be DATADIR, MKIMAGE, etc?

> +
> +    # Create an RSA key pair
> +    public_exponent = 65537
> +    util.cmd(cons, 'openssl genpkey -algorithm RSA -out %sdev.key '
> +                   '-pkeyopt rsa_keygen_bits:2048 '
> +                   '-pkeyopt rsa_keygen_pubexp:%d '
> +                   '2>/dev/null'  % (tmpdir, public_exponent))
> +
> +    # Create a certificate containing the public key
> +    util.cmd(cons, 'openssl req -batch -new -x509 -key %sdev.key -out '
> +                   '%sdev.crt' % (tmpdir, tmpdir))
> +
> +    # Create a number kernel image with zeroes
> +    with open('%stest-kernel.bin' % tmpdir, 'w') as fd:
> +        fd.write(5000 * chr(0))
> +

Any need to clean these up afterward or place them into
test-run-unique directories? Is the expectation that subsequent tests
will overwrite existing test data (also that vboot tests cannot run
concurrently)?

> +    try:
> +        # We need to use our own device tree file. Remember to restore it
> +        # afterwards.
> +        old_dtb = cons.config.dtb
> +        cons.config.dtb = dtb
> +        test_with_algo('sha1')
> +        test_with_algo('sha256')
> +    finally:
> +        cons.config.dtb = old_dtb
> diff --git a/test/vboot/sandbox-kernel.dts b/test/py/tests/vboot/sandbox-kernel.dts
> similarity index 100%
> rename from test/vboot/sandbox-kernel.dts
> rename to test/py/tests/vboot/sandbox-kernel.dts
> diff --git a/test/vboot/sandbox-u-boot.dts b/test/py/tests/vboot/sandbox-u-boot.dts
> similarity index 100%
> rename from test/vboot/sandbox-u-boot.dts
> rename to test/py/tests/vboot/sandbox-u-boot.dts
> diff --git a/test/vboot/sign-configs-sha1.its b/test/py/tests/vboot/sign-configs-sha1.its
> similarity index 100%
> rename from test/vboot/sign-configs-sha1.its
> rename to test/py/tests/vboot/sign-configs-sha1.its
> diff --git a/test/vboot/sign-configs-sha256.its b/test/py/tests/vboot/sign-configs-sha256.its
> similarity index 100%
> rename from test/vboot/sign-configs-sha256.its
> rename to test/py/tests/vboot/sign-configs-sha256.its
> diff --git a/test/vboot/sign-images-sha1.its b/test/py/tests/vboot/sign-images-sha1.its
> similarity index 100%
> rename from test/vboot/sign-images-sha1.its
> rename to test/py/tests/vboot/sign-images-sha1.its
> diff --git a/test/vboot/sign-images-sha256.its b/test/py/tests/vboot/sign-images-sha256.its
> similarity index 100%
> rename from test/vboot/sign-images-sha256.its
> rename to test/py/tests/vboot/sign-images-sha256.its
> diff --git a/test/vboot/.gitignore b/test/vboot/.gitignore
> deleted file mode 100644
> index 4631242..0000000
> --- a/test/vboot/.gitignore
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -/*.dtb
> -/test.fit
> -/dev-keys
> diff --git a/test/vboot/vboot_test.sh b/test/vboot/vboot_test.sh
> deleted file mode 100755
> index 6d7abb8..0000000
> --- a/test/vboot/vboot_test.sh
> +++ /dev/null
> @@ -1,151 +0,0 @@
> -#!/bin/bash
> -#
> -# Copyright (c) 2013, Google Inc.
> -#
> -# Simple Verified Boot Test Script
> -#
> -# SPDX-License-Identifier:     GPL-2.0+
> -
> -set -e
> -
> -# Run U-Boot and report the result
> -# Args:
> -#      $1:     Test message
> -run_uboot() {
> -       echo -n "Test Verified Boot Run: $1: "
> -       ${uboot} -d sandbox-u-boot.dtb >${tmp} -c '
> -sb load hostfs - 100 test.fit;
> -fdt addr 100;
> -bootm 100;
> -reset'
> -       if ! grep -q "$2" ${tmp}; then
> -               echo
> -               echo "Verified boot key check failed, output follows:"
> -               cat ${tmp}
> -               false
> -       else
> -               echo "OK"
> -       fi
> -}
> -
> -echo "Simple Verified Boot Test"
> -echo "========================="
> -echo
> -echo "Please see doc/uImage.FIT/verified-boot.txt for more information"
> -echo
> -
> -err=0
> -tmp=/tmp/vboot_test.$$
> -
> -dir=$(dirname $0)
> -
> -if [ -z ${O} ]; then
> -       O=.
> -fi
> -O=$(readlink -f ${O})
> -
> -dtc="-I dts -O dtb -p 2000"
> -uboot="${O}/u-boot"
> -mkimage="${O}/tools/mkimage"
> -fit_check_sign="${O}/tools/fit_check_sign"
> -keys="${dir}/dev-keys"
> -echo ${mkimage} -D "${dtc}"
> -
> -echo "Build keys"
> -mkdir -p ${keys}
> -
> -PUBLIC_EXPONENT=${1}
> -
> -if [ -z "${PUBLIC_EXPONENT}" ]; then
> -       PUBLIC_EXPONENT=65537
> -fi
> -
> -# Create an RSA key pair
> -openssl genpkey -algorithm RSA -out ${keys}/dev.key \
> -    -pkeyopt rsa_keygen_bits:2048 \
> -    -pkeyopt rsa_keygen_pubexp:${PUBLIC_EXPONENT} 2>/dev/null
> -
> -# Create a certificate containing the public key
> -openssl req -batch -new -x509 -key ${keys}/dev.key -out ${keys}/dev.crt
> -
> -pushd ${dir} >/dev/null
> -
> -function do_test {
> -       echo do $sha test
> -       # Compile our device tree files for kernel and U-Boot
> -       dtc -p 0x1000 sandbox-kernel.dts -O dtb -o sandbox-kernel.dtb
> -       dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
> -
> -       # Create a number kernel image with zeroes
> -       head -c 5000 /dev/zero >test-kernel.bin
> -
> -       # Build the FIT, but don't sign anything yet
> -       echo Build FIT with signed images
> -       ${mkimage} -D "${dtc}" -f sign-images-$sha.its test.fit >${tmp}
> -
> -       run_uboot "unsigned signatures:" "dev-"
> -
> -       # Sign images with our dev keys
> -       echo Sign images
> -       ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb \
> -               -r test.fit >${tmp}
> -
> -       run_uboot "signed images" "dev+"
> -
> -
> -       # Create a fresh .dtb without the public keys
> -       dtc -p 0x1000 sandbox-u-boot.dts -O dtb -o sandbox-u-boot.dtb
> -
> -       echo Build FIT with signed configuration
> -       ${mkimage} -D "${dtc}" -f sign-configs-$sha.its test.fit >${tmp}
> -
> -       run_uboot "unsigned config" $sha"+ OK"
> -
> -       # Sign images with our dev keys
> -       echo Sign images
> -       ${mkimage} -D "${dtc}" -F -k dev-keys -K sandbox-u-boot.dtb \
> -               -r test.fit >${tmp}
> -
> -       run_uboot "signed config" "dev+"
> -
> -       echo check signed config on the host
> -       if ! ${fit_check_sign} -f test.fit -k sandbox-u-boot.dtb >${tmp}; then
> -               echo
> -               echo "Verified boot key check on host failed, output follows:"
> -               cat ${tmp}
> -               false
> -       else
> -               if ! grep -q "dev+" ${tmp}; then
> -                       echo
> -                       echo "Verified boot key check failed, output follows:"
> -                       cat ${tmp}
> -                       false
> -               else
> -                       echo "OK"
> -               fi
> -       fi
> -
> -       run_uboot "signed config" "dev+"
> -
> -       # Increment the first byte of the signature, which should cause failure
> -       sig=$(fdtget -t bx test.fit /configurations/conf at 1/signature at 1 value)
> -       newbyte=$(printf %x $((0x${sig:0:2} + 1)))
> -       sig="${newbyte} ${sig:2}"
> -       fdtput -t bx test.fit /configurations/conf at 1/signature at 1 value ${sig}
> -
> -       run_uboot "signed config with bad hash" "Bad Data Hash"
> -}
> -
> -sha=sha1
> -do_test
> -sha=sha256
> -do_test
> -
> -popd >/dev/null
> -
> -echo
> -if ${ok}; then
> -       echo "Test passed"
> -else
> -       echo "Test failed"
> -fi
> --
> 2.8.0.rc3.226.g39d4020
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Thanks for this refactor! If the comments related to the sh to Python
are too nit-picky we can certainly change and expand the test harness
within additional patches later.

-- 
Teddy Reed V

[U-Boot] [PATCH 14/14] test: Convert the vboot test to test/py

[Simon Glass]

Hi Teddy,

On 3 July 2016 at 15:38, Teddy Reed <teddy.reed@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
>> Now that we have a suitable test framework we should move all tests into it.
>> The vboot test is a suitable candidate. Rewrite it in Python and move the
>> data files into an appropriate directory.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  test/README                                       |   1 -
>>  test/py/tests/test_vboot.py                       | 185 ++++++++++++++++++++++
>>  test/{ => py/tests}/vboot/sandbox-kernel.dts      |   0
>>  test/{ => py/tests}/vboot/sandbox-u-boot.dts      |   0
>>  test/{ => py/tests}/vboot/sign-configs-sha1.its   |   0
>>  test/{ => py/tests}/vboot/sign-configs-sha256.its |   0
>>  test/{ => py/tests}/vboot/sign-images-sha1.its    |   0
>>  test/{ => py/tests}/vboot/sign-images-sha256.its  |   0
>>  test/vboot/.gitignore                             |   3 -
>>  test/vboot/vboot_test.sh                          | 151 ------------------
>>  10 files changed, 185 insertions(+), 155 deletions(-)
>>  create mode 100644 test/py/tests/test_vboot.py
>>  rename test/{ => py/tests}/vboot/sandbox-kernel.dts (100%)
>>  rename test/{ => py/tests}/vboot/sandbox-u-boot.dts (100%)
>>  rename test/{ => py/tests}/vboot/sign-configs-sha1.its (100%)
>>  rename test/{ => py/tests}/vboot/sign-configs-sha256.its (100%)
>>  rename test/{ => py/tests}/vboot/sign-images-sha1.its (100%)
>>  rename test/{ => py/tests}/vboot/sign-images-sha256.its (100%)
>>  delete mode 100644 test/vboot/.gitignore
>>  delete mode 100755 test/vboot/vboot_test.sh

[snip]

>
> Thanks for this refactor! If the comments related to the sh to Python
> are too nit-picky we can certainly change and expand the test harness
> within additional patches later.

Thanks for the high-quality review... I'm expecting that Stephen will
have a few things to say about how best to fit things into the pytest
stuff too. So I'll hold off a bit before respinning. But I want to
avoid any more expansion of the vboot shell script.

Regards,
Simon

[U-Boot] [PATCH 08/14] test/py: Add an option to execute a string containing a command

[Stephen Warren]

On 07/03/2016 09:40 AM, Simon Glass wrote:
> It is sometimes inconvenient to convert a string into a list for execution
> with run_and_log(). Provide a helper function to do this.

> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py

> +def cmd(u_boot_console, cmd_str):

"cmd" seems very generic and doesn't give any clue what it does. How 
about extending the existing function name to e.g. "run_and_log_str"?

[U-Boot] [PATCH 09/14] test/py: Provide a way to check that a command fails

[Stephen Warren]

On 07/03/2016 09:40 AM, Simon Glass wrote:
> Sometimes we want to run a command and check that it fails. Add a function
> to handle this. It can check the return code and also make sure that the
> output contains a given error message.

> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py

> +def run_and_log_expect_exception(u_boot_console, cmd, retcode, msg):
> +    """Run a command which is expected to fail.
> +
> +    This runs a command and checks that it fails with the expected return code
> +    and exception method. If not, an exception is raised.
> +
> +    Args:
> +        u_boot_console: A console connection to U-Boot.
> +        cmd: The command to run, as an array of argv[].
> +        retcode: Expected non-zero return code from the command.
> +        msg: String which should be contained within the command's output.
> +    """

retcode isn't used anywhere. Do we care what the return code is, so long 
as it's something non-zero, and the desired exception message appears?

[U-Boot] [PATCH 08/14] test/py: Add an option to execute a string containing a command

[Stephen Warren]

On 07/03/2016 09:40 AM, Simon Glass wrote:
> It is sometimes inconvenient to convert a string into a list for execution
> with run_and_log(). Provide a helper function to do this.

> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py

> +def cmd(u_boot_console, cmd_str):
> +    """Run a single command string and log its output.
> +
> +    Args:
> +        u_boot_console: A console connection to U-Boot.
> +        cmd: The command to run, as a string.
> +
> +    Returns:
> +        The output as a string.
> +    """

Thinking about this more: I believe the Pythonic way to do this would be 
to extend the existing run_and_log() to support the cmd parameter being 
either an array, or a string; I think something like just adding the 
following at the start of run_and_log():

if isinstance(cmd, str):
     cmd = str.split()

This would also allow other higher-order functions like your later 
run_command_list() to take either a list of argv[] or a list of strings 
(or even a mixture), without having to code multiple versions of the 
higher level functions for the different cases.

[U-Boot] [PATCH 13/14] test/py: Fix up after the rename of CONFIG_SYS_HUSH_PARSER

[Stephen Warren]

On 07/03/2016 09:40 AM, Simon Glass wrote:
> At present all the hush tests are skipped on sandbox because the test thinks
> that this option is disabled. In fact it has just been renamed.
>
> It might be better to use the full CONFIG_xxx name in tests with
> @pytest.mark.buildconfigspec(), since at present it is not really clear that
> the options are related.
>
> Fixes: f1f9d4fa (hush: complete renaming CONFIG_SYS_HUSH_PARSER to CONFIG_HUSH_PARSER)

Oh, I sent almost a duplicate of this later:

https://patchwork.ozlabs.org/patch/645376/

That one also fixes a similar issue due to the reset -> sysreset rename.

[U-Boot] [PATCH 14/14] test: Convert the vboot test to test/py

[Stephen Warren]

On 07/03/2016 09:40 AM, Simon Glass wrote:
> Now that we have a suitable test framework we should move all tests into it.
> The vboot test is a suitable candidate. Rewrite it in Python and move the
> data files into an appropriate directory.

> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py

> +# Copyright (c) 2013, Google Inc.

2013-2016?

> + at pytest.mark.buildconfigspec('fit_signature')
> +def test_vboot(u_boot_console):
> +    """Test verified boot signing with mkimage and verification with 'bootm'.
> +
> +    This works using sandbox only as it needs to update the device tree used
> +    by U-Boot to hold public keys from the signing process.

Since this works on sandbox, the function should be marked:

@pytest.mark.boardspec('sandbox')

> +    def dtc(dts):

> +        util.cmd(cons, 'dtc %s %s%s -O dtb '
> +                       '-o %s%s' % (dtc_args, datadir, dts, tmpdir, dtb))

For all the instances of util.cmd() in this file, it looks pretty easy 
to represent them as arrays rather than strings. Is implementing/using 
cmd() really necessary?

> +    def run_bootm(test_type, expect_string):

> +        cons.cleanup_spawn()
> +        cons.ensure_spawned()

That feels a bit too much like relying on internal details, especially 
as the docstring for cleanup_spawn() says "This is an internal function 
and should not be called directly." Can we introduce a new public 
function cons.restart_uboot() that's intended for public use? The 
implementation would just be the two lines above, but it would provide 
some encapsulation of the details.

> +        cons.log.action('%s: Test Verified Boot Run: %s' % (algo, test_type))
 > +        output = cons.run_command_list(
 > +            ['sb load hostfs - 100 %stest.fit' % tmpdir,
 > +             'fdt addr 100',
 > +             'bootm 100'])
 > +        assert(expect_string in output)

Would it make sense to do this instead:

with cons.log.section("Verified boot %s %s" % (algo, test_type)):
     output = ...
     assert ...

? That would nest each invocation of that command list into a separate 
collapsible section of the HTML log file.

> +    def test_with_algo(sha):
> +        """Test verified boot with the given hash algorithm
> +
> +        This is the main part of the test code. The same procedure is followed
> +        for both hashing algorithms.
> +
> +        Args:
> +            sha: Either 'sha1' or 'sha256', to select the algorithm to use
> +        """
> +        global algo
> +
> +        algo = sha

Why not just pass that parameter to the couple of functions that need it?

Certainly, having the various utility functions rely on variables from 
outer scopes is consistent with some other existing tests, but if you're 
going to do that, I'd suggest having this function not take the sha 
parameter, but instead pick up "algo" from an outer scope too?

> +        # Compile our device tree files for kernel and U-Boot
> +        dtc('sandbox-kernel.dts')
> +        dtc('sandbox-u-boot.dts')

I think that happens twice, and ends up doing an identical operation. 
Should those commands (and perhaps some others below too?) be moved 
outside the function into top-level setup code?

Also, is it necessary to repeat those commands if a previous test run 
already ran dtc? Here, dtc is an external utility so I don't think 
running it over and over is worthwhile. However, for some/all of the 
mkimage below, since mkimage presumably comes from the current U-Boot 
build, re-running it each time would actually test something about the 
current U-Boot source tree.

> +        # Build the FIT, but don't sign anything yet
> +        cons.log.action('%s: Test FIT with signed images' % algo)
> +        make_fit('sign-images-%s.its' % algo)
> +        run_bootm('unsigned images', 'dev-')

Perhaps rather than run_bootm() creating its own log section, the 
section should be created here, and wrap all 3 of those lines above?

> +        # Sign images with our dev keys
> +        sign_fit()
> +        run_bootm('signed images', 'dev+')
> +
> +        # Create a fresh .dtb without the public keys
> +        dtc('sandbox-u-boot.dts')

Doesn't this generate the same DTB as above? I don't see any 
FIT/binary/... include statements etc. in that .dts file.

> +        byte_list = ['%x' % (byte + 1)] + byte_list[1:]

byte_list[0] = '%x' % (byte + 1)

?

> +    cons = u_boot_console
> +    tmpdir = cons.config.result_dir + '/'
> +    tmp = tmpdir + 'vboot.tmp'
> +    datadir = 'test/py/tests/vboot/'

I suspect some of the files might usefully be placed into 
ubconfig.persistent_data_dir?

> +    fit = '%stest.fit' % tmpdir
> +    mkimage = cons.config.build_dir + '/tools/mkimage'
> +    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
> +    dtc_args = '-I dts -O dtb -i %s' % tmpdir
> +    dtb = '%ssandbox-u-boot.dtb' % tmpdir

I think all those filename concatenations would be clearer as '%s/%s' 
rather than placing the / into one of the strings.

> +    # Create an RSA key pair
> +    public_exponent = 65537
> +    util.cmd(cons, 'openssl genpkey -algorithm RSA -out %sdev.key '
> +                   '-pkeyopt rsa_keygen_bits:2048 '
> +                   '-pkeyopt rsa_keygen_pubexp:%d '
> +                   '2>/dev/null'  % (tmpdir, public_exponent))
> +
> +    # Create a certificate containing the public key
> +    util.cmd(cons, 'openssl req -batch -new -x509 -key %sdev.key -out '
> +                   '%sdev.crt' % (tmpdir, tmpdir))
> +
> +    # Create a number kernel image with zeroes
> +    with open('%stest-kernel.bin' % tmpdir, 'w') as fd:
> +        fd.write(5000 * chr(0))

Again, I suspect placing those into ubconfig.persistent_data_dir, and 
skipping those commands if the files already exist, would be beneficial. 
See u_boot_utils.py:PersistentRandomFile for a similar case.

> +    try:
> +        # We need to use our own device tree file. Remember to restore it
> +        # afterwards.
> +        old_dtb = cons.config.dtb
> +        cons.config.dtb = dtb
> +        test_with_algo('sha1')
> +        test_with_algo('sha256')
> +    finally:
> +        cons.config.dtb = old_dtb

I think that needs to call cons.restart_uboot() at the end of the 
finally: block, in order to switch back to the old DTB.

Better still would be to only mark the existing U-Boot instance as being 
in an error state, and defer restarting U-Boot to the next test that 
gets run. That way, U-Boot won't be needlessly respawned if this is the 
only/last test to be run.

[U-Boot] [PATCH 14/14] test: Convert the vboot test to test/py

[Stephen Warren]

On 07/03/2016 03:38 PM, Teddy Reed wrote:
> Hi Simon,
>
> On Sun, Jul 3, 2016 at 8:40 AM, Simon Glass <sjg@chromium.org> wrote:
>> Now that we have a suitable test framework we should move all tests into it.
>> The vboot test is a suitable candidate. Rewrite it in Python and move the
>> data files into an appropriate directory.

>> +    datadir = 'test/py/tests/vboot/'
>> +    fit = '%stest.fit' % tmpdir
>> +    mkimage = cons.config.build_dir + '/tools/mkimage'
>> +    fit_check_sign = cons.config.build_dir + '/tools/fit_check_sign'
>> +    dtc_args = '-I dts -O dtb -i %s' % tmpdir
>> +    dtb = '%ssandbox-u-boot.dtb' % tmpdir
>> +    sig_node = '/configurations/conf at 1/signature at 1'
>
> If these variables are used throughout the tests like globals, should
> they be DATADIR, MKIMAGE, etc?

I'd prefer not to use all-caps variable names.

[U-Boot] [U-Boot,01/14] test: Add a README

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:33AM -0600, Simon Glass wrote:

> Add a few notes about how testing works in U-Boot.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 02/14] test: Add a simple script to run tests on sandbox

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:34AM -0600, Simon Glass wrote:

> A common check before sending patches is to run all available tests on
> sandbox. But everytime I do this I have to look up the README. This presents
> quite a barrier to actually doing this.
> 
> Add a shell script to help. To run the tests, type:
> 
>    test/run
> 
> in the U-Boot directory, which should be easy to remember.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 03/14] sandbox: Don't exit when bootm completes

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:35AM -0600, Simon Glass wrote:

> At present sandbox exits when the 'bootm' command completes, since it is not
> actually able to run the OS that is loaded. Normally 'bootm' failure is
> considered a fatal error in U-Boot.
> 
> However this is annoying for tests, which may want to examine the state
> after a test is complete. In any case there is a 'reset' command which can
> be used to exit, if required.
> 
> Change the behaviour to return normally from the 'bootm' command on sandbox.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 04/14] test/py: Allow tests to control the sandbox device-tree file

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:36AM -0600, Simon Glass wrote:

> Normally tests will run with the test.dtb file designed for this purpose.
> However, the verified boot tests need to run with their own device-tree
> file, containing a public key.
> 
> Make the device-tree file a config option so that it can be adjusted by
> tests. The default is to keep the current behaviour.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 05/14] test/py: Allow RunAndLog() to return the output

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:37AM -0600, Simon Glass wrote:

> Tests may want to look at the output from running a command. Return it so
> that this is possible.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 06/14] test/py: Provide output from exceptions with RunAndLog()

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:38AM -0600, Simon Glass wrote:

> Tests may want to look at the output from running a command, even if it
> fails (e.g. with a non-zero return code). Provide a means to obtain this.
> 
> Another approach would be to return a class object containing both the
> output and the exception, but I'm not sure if that would result in a lot
> of refactoring.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 07/14] test/py: Return output from run_and_log()

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:39AM -0600, Simon Glass wrote:

> It is useful to be able to obtain the output from a command. Return it from
> this function.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 08/14] test/py: Add an option to execute a string containing a command

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:40AM -0600, Simon Glass wrote:

> It is sometimes inconvenient to convert a string into a list for execution
> with run_and_log(). Provide a helper function to do this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 09/14] test/py: Provide a way to check that a command fails

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:41AM -0600, Simon Glass wrote:

> Sometimes we want to run a command and check that it fails. Add a function
> to handle this. It can check the return code and also make sure that the
> output contains a given error message.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 10/14] test/py: Add a helper to run a list of U-Boot commands

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:42AM -0600, Simon Glass wrote:

> Some tests want to execute a sequence of commands. Add a helper for this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 10/14] test/py: Add a helper to run a list of U-Boot commands

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:42AM -0600, Simon Glass wrote:

> Some tests want to execute a sequence of commands. Add a helper for this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 11/14] tools: Add an error code when fit_handle_file() fails

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:43AM -0600, Simon Glass wrote:

> The error code may provide useful information for debugging. Add it to the
> error string.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 12/14] tools: Correct error handling in fit_image_process_hash()

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:44AM -0600, Simon Glass wrote:

> We should not be returning -1 as an error code. This can mask a situation
> where we run out of space adding things to the FIT. By returning the correct
> error in this case (-ENOSPC) it can be handled by the higher-level code.
> 
> This may fix the error reported by Tom Van Deun here:
> 
> https://www.mail-archive.com/u-boot at lists.denx.de/msg217417.html
> 
> although I am not sure as I cannot actually repeat it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reported-by: Tom Van Deun <tom.vandeun@wapice.com>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 13/14] test/py: Fix up after the rename of CONFIG_SYS_HUSH_PARSER

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:45AM -0600, Simon Glass wrote:

> At present all the hush tests are skipped on sandbox because the test thinks
> that this option is disabled. In fact it has just been renamed.
> 
> It might be better to use the full CONFIG_xxx name in tests with
> @pytest.mark.buildconfigspec(), since at present it is not really clear that
> the options are related.
> 
> Fixes: f1f9d4fa (hush: complete renaming CONFIG_SYS_HUSH_PARSER to CONFIG_HUSH_PARSER)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Teddy Reed <teddy.reed@gmail.com>

Applied to u-boot/master, thanks!

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

[U-Boot] [U-Boot, 14/14] test: Convert the vboot test to test/py

[Tom Rini]

On Sun, Jul 03, 2016 at 09:40:46AM -0600, Simon Glass wrote:

> Now that we have a suitable test framework we should move all tests into it.
> The vboot test is a suitable candidate. Rewrite it in Python and move the
> data files into an appropriate directory.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

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