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

[Stephen Warren <swarren@wwwdotorg.org>]

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.