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?
+@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
