[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Stefan Brüns]

From: Stefan Br??ns <stefan.bruens@rwth-aachen.de>

The following checks are currently implemented:
1. listing a directory
2. verifying size of a file
3. veryfying md5sum for a file region
4. reading the beginning of a file

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 test/py/tests/test_fs.py | 357 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 357 insertions(+)
 create mode 100644 test/py/tests/test_fs.py

diff --git a/test/py/tests/test_fs.py b/test/py/tests/test_fs.py
new file mode 100644
index 0000000000..7aaf8debaf
--- /dev/null
+++ b/test/py/tests/test_fs.py
@@ -0,0 +1,357 @@
+# Copyright (c) 2016, Stefan Bruens <stefan.bruens@rwth-aachen.de>
+#
+# SPDX-License-Identifier: GPL-2.0
+
+# Test U-Boot's filesystem implementations
+#
+# The tests are currently covering the ext4 and fat filesystem implementations.
+#
+# Following functionality is currently checked:
+# - Listing a directory and checking for a set of files
+# - Verifying the size of a file
+# - Reading sparse and non-sparse files
+# - File content verification using md5sums
+
+
+from distutils.spawn import find_executable
+import hashlib
+import pytest
+import os
+import random
+import re
+import u_boot_utils as util
+
+ at pytest.fixture(scope='session')
+def prereq_commands():
+    """Detect required commands to run file system tests."""
+    for command in ['mkfs', 'mount', 'umount']:
+        if find_executable(command) is None:
+            pytest.skip('Filesystem tests, "{0}" not in PATH'.format(command))
+
+"""
+Scenarios:
+    hostfs: access image contents through the sandbox hostfs
+        facility, using the filesytem implementation of
+        the sandbox host, e.g. Linux kernel
+    generic: test u-boots native filesystem implementations,
+        using the 'generic' command names, e.g. 'load'
+    TODO -
+    fscommands: test u-boots native filesystem implementations,
+        using the fs specific commands, e.g. 'ext4load'
+"""
+ at pytest.fixture(scope='class', params=['generic', 'hostfs'])
+def scenario(request):
+    request.cls.scenario = request.param
+    return request.param
+
+
+"""
+Dictionary of files to use during filesystem tests. The files
+are keyed by the filenames. The value is an array of strides, each tuple
+contains the the start offset (inclusive) and end offset (exclusive).
+"""
+files = {
+    'empty.file' : [(0, 0)],
+    '1MB.file'   : [(0, 1e6)],
+    '1MB.sparse.file'   : [(1e6-1, 1e6)],
+    '32MB.sparse.file'   : [(0, 1e6), (4e6, 5e6), (31e6, 32e6)],
+    # Creating a 2.5 GB file on FAT is exceptionally slow, disable it for now
+    # '2_5GB.sparse.file'   : [(0, 1e6), (1e9, 1e9+1e6), (2.5e9-1e6, 2.5e9)],
+}
+
+"""Options to pass to mkfs."""
+mkfs_opts = {
+	'fat' :'-t vfat',
+	'ext4' : '-t ext4 -F',
+}
+
+class FsImage:
+    def __init__(self, fstype, imagepath, mountpath):
+        """Create a new filesystem image.
+
+        Args:
+            fstype: filesystem type (string)
+            imagepath: full path to image file
+            mountpath: mountpoint directory
+        """
+        self.fstype = fstype
+        self.imagepath = imagepath
+        self.mountpath = mountpath
+        self.md5s = {}
+        with open(self.imagepath, 'w') as fd:
+            fd.truncate(0)
+            fd.seek(3e9)
+            fd.write(bytes([0]))
+
+    def mkfs(self, log):
+        mkfsopts = mkfs_opts.get(self.fstype)
+        util.run_and_log(log,
+            'mkfs {0} {1}'.format(mkfsopts, self.imagepath))
+
+    def create_file(self, log, filename, strides):
+        """Create a single file in the filesystem. Each file
+        is defined by one or more strides, which is filled with
+        random data. For each stride, the md5sum is calculated
+        and stored.
+        """
+        md5sums = []
+        with open(self.mountpath + '/' + filename, 'w') as fd:
+            for stride in strides:
+                length = int(stride[1] - stride[0])
+                data = bytearray(random.getrandbits(8) for _ in xrange(length))
+                md5 = hashlib.md5(data).hexdigest()
+                md5sums.append(md5)
+                log.info('{0}: write {1} bytes @ {2} : {3}'.format(
+                    filename, int(stride[1] - stride[0]),
+                    int(stride[0]), md5))
+                fd.seek(stride[0])
+                fd.write(data);
+        self.md5s[filename] = md5sums
+
+    def create_files(self, log):
+        with log.section('Create initial files'):
+            for filename in files:
+                self.create_file(log, filename, files[filename])
+            log.info('Created test files in "{0}"'.format(self.mountpath))
+            util.run_and_log(log, 'ls -la {0}'.format(self.mountpath))
+            util.run_and_log(log, 'sync {0}'.format(self.mountpath))
+
+    def mount(self, log):
+        if not os.path.exists(self.mountpath):
+            os.mkdir(self.mountpath)
+        log.info('Mounting {0} at {1}'.format(self.imagepath, self.mountpath))
+        if self.fstype == 'ext4':
+            cmd = 'sudo -n mount -o loop,rw {0} {1}'.format(self.imagepath, self.mountpath)
+        else:
+            cmd = 'sudo -n mount -o loop,rw,umask=000 {0} {1}'.format(self.imagepath, self.mountpath)
+        util.run_and_log(log, cmd)
+        if self.fstype == 'ext4':
+            cmd = 'sudo -n chmod og+rw {0}'.format(self.mountpath)
+            return util.run_and_log(log, cmd)
+
+    def unmount(self, log):
+        log.info('Unmounting {0}'.format(self.imagepath))
+        cmd = 'sudo -n umount -l {0}'.format(self.mountpath)
+        util.run_and_log(log, cmd, ignore_errors=True)
+
+
+ at pytest.fixture(scope='module', params=['fat', 'ext4'])
+def fsimage(prereq_commands, u_boot_config, u_boot_log, request):
+    """Filesystem image instance."""
+    datadir = u_boot_config.result_dir + '/'
+    fstype = request.param
+    imagepath = datadir + '3GB.' + fstype + '.img'
+    mountpath = datadir + 'mnt_' + fstype
+
+    with u_boot_log.section('Create image "{0}"'.format(imagepath)):
+        fsimage = FsImage(fstype, imagepath, mountpath)
+        fsimage.mkfs(u_boot_log)
+
+    yield fsimage
+    fsimage.unmount(u_boot_log)
+
+ at pytest.fixture(scope='module')
+def populated_image(fsimage, u_boot_log):
+    """Filesystem populated with files required for tests."""
+    try:
+        fsimage.mount(u_boot_log)
+    except Exception as e:
+        pytest.skip('{0}: could not mount "{1}"'.format(
+            fsimage.fstype, fsimage.imagepath))
+        yield None
+
+    fsimage.create_files(u_boot_log)
+    fsimage.unmount(u_boot_log)
+    yield fsimage
+
+ at pytest.fixture(scope='function')
+def boundimage(populated_image, u_boot_console, request):
+    """Filesystem image instance which is accessible from inside
+    the running U-Boot instance."""
+    image = populated_image
+    request.cls.image = image
+    if request.cls.scenario == 'hostfs':
+        image.mount(u_boot_console.log)
+        image.rootpath = image.mountpath + '/'
+        yield image
+        image.unmount(u_boot_console.log)
+    else:
+        output = u_boot_console.run_command_list(
+            ['host bind 0 {0}'.format(image.imagepath)])
+        image.rootpath = '/'
+        yield image
+        output = u_boot_console.run_command_list(['host bind 0 '])
+
+
+def test_fs_prepare_image(u_boot_config, fsimage, request):
+    """Dummy test to create an image file with filesystem.
+    Useful to isolate fixture setup from actual tests."""
+    if not fsimage:
+        pytest.fail('Failed to create image')
+
+def test_fs_populate_image(populated_image, request):
+    """Dummy test to create initial filesystem contents."""
+    if not populated_image:
+        pytest.fail('Failed create initial image content')
+
+ at pytest.mark.usefixtures('u_boot_console', 'scenario', 'boundimage')
+class TestFilesystems:
+    ignore_cleanup_errors = True
+    filesize_regex = re.compile('^filesize=([A-Fa-f0-9]+)')
+    md5sum_regex = re.compile('^md5 for .* ==> ([A-Fa-f0-9]{32})')
+    dirlist_regex = re.compile('\s+(\d+)\s+(\S+.file\S*)')
+
+    commands = {
+            'fat' : {
+                    'listcmd'   : 'ls',
+                    'readcmd'   : 'load',
+                    'sizecmd'   : 'size',
+                    'writecmd'  : 'size',
+            },
+            'ext4' : {
+                    'listcmd'   : 'ls',
+                    'readcmd'   : 'load',
+                    'sizecmd'   : 'size',
+                    'writecmd'  : 'size',
+            },
+    }
+
+    cmd_parameters = {
+            'hostfs' : {
+                    'prefix'    : 'host ',
+                    'interface' : 'hostfs -',
+            },
+            'generic' : {
+                    'prefix'    : '',
+                    'interface' : 'host 0:0',
+            },
+    }
+
+    def get_filesize(self, filename):
+        """Get the size of the given file."""
+        strides = files[filename]
+        return int(strides[-1][1])
+
+    def check_dirlist(self, string, filenames):
+        """Check if the output string returned by a list command
+        contains all given filenames."""
+        m = self.dirlist_regex.findall(string)
+        assert(m)
+        for i, e in enumerate(m):
+            m[i] = (int(e[0]), e[1].lower())
+        for f in filenames:
+            e = (self.get_filesize(f), f.lower())
+            assert(e in m)
+
+    def check_filesize(self, string, size):
+        """Check if the output string returned by a size command
+        matches the expected file size."""
+        m = self.filesize_regex.match(string)
+        assert(m)
+        assert(int(m.group(1), 16) == size)
+
+    def check_md5sum(self, string, md5):
+        """Check if the output string returned by the md5sum
+        command matches the expected md5."""
+        m = self.md5sum_regex.match(string)
+        assert(m)
+        assert(len(m.group(1)) == 32)
+        assert(m.group(1) == md5)
+
+    def run_listcmd(self, dirname):
+        """Run the scenario and filesystem specific list command
+        for the given directory name."""
+        cmd = '{0}{1} {2} {3}'.format(
+            self.fs_params.get('prefix'),
+            self.fs_commands.get('listcmd'),
+            self.fs_params.get('interface'),
+            self.image.rootpath + dirname)
+        with self.console.log.section('List "{0}"'.format(dirname)):
+            output = self.console.run_command_list([cmd])
+            return output[0]
+
+    def run_readcmd(self, filename, offset, length):
+        """Run the scenario and filesystem specific read command
+        for the given file."""
+        cmd = '{0}{1} {2} {3} {4} 0x{5:x} 0x{6:x}'.format(
+            self.fs_params.get('prefix'),
+            self.fs_commands.get('readcmd'),
+            self.fs_params.get('interface'),
+            '0', # address
+            self.image.rootpath + filename,
+            length, offset)
+        with self.console.log.section('Read file "{0}"'.format(filename)):
+            output = self.console.run_command_list(
+                [cmd, 'env print filesize',
+                 'md5sum 0 $filesize', 'env set filesize'])
+            return output[1:3]
+
+    def run_sizecmd(self, filename):
+        """Run the scenario and filesystem specific size command
+        for the given file."""
+        cmd = '{0}{1} {2} {3}'.format(
+            self.fs_params.get('prefix'),
+            self.fs_commands.get('sizecmd'),
+            self.fs_params.get('interface'),
+            self.image.rootpath + filename)
+        with self.console.log.section('Get size of "{0}"'.format(filename)):
+            output = self.console.run_command_list(
+                [cmd, 'env print filesize', 'env set filesize'])
+            return output[1]
+
+    def setup(self):
+        self.fs_params = self.cmd_parameters.get(self.scenario)
+        self.fs_commands = self.commands.get(self.image.fstype)
+
+    @pytest.mark.parametrize('dirname', ['', './'])
+    def test_fs_ls(self, u_boot_console, dirname):
+        """Check the contents of the given directory."""
+        if self.image.fstype == 'fat' and dirname == './':
+            pytest.skip("FAT has no '.' entry in the root directory")
+        self.console = u_boot_console
+        self.setup()
+
+        output = self.run_listcmd(dirname)
+        self.check_dirlist(output, files.keys())
+
+    @pytest.mark.parametrize('filename', files.keys())
+    def test_fs_filesize(self, u_boot_console, filename):
+        """Check the filesize of the given file."""
+        self.console = u_boot_console
+        self.setup()
+
+        filesize = self.get_filesize(filename)
+
+        output = self.run_sizecmd(filename)
+        self.check_filesize(output, filesize)
+
+    @pytest.mark.parametrize('filename', files.keys())
+    def test_fs_read(self, u_boot_console, filename):
+        """Read all defined strides of the given file and checks
+        its contents."""
+        self.console = u_boot_console
+        self.setup()
+
+        md5s = self.image.md5s[filename]
+
+        for i, stride in enumerate(files[filename]):
+            length = int(stride[1]) - int(stride[0])
+            offset = int(stride[0])
+            output = self.run_readcmd(filename, offset, length)
+            self.check_filesize(output[0], length)
+            self.console.log.info('md5: {0}'.format(md5s[i]))
+            self.check_md5sum(output[1], md5s[i])
+
+    @pytest.mark.parametrize('filename', files.keys())
+    def test_fs_read_head(self, u_boot_console, filename):
+        """Check reading the head of the given file, up to the first
+        4 Megabyte (or less for smaller files). Also reads sparse
+        regions of a file."""
+        self.console = u_boot_console
+        self.setup()
+
+        filesize = self.get_filesize(filename)
+        filesize = min(filesize, int(4e6))
+
+        output = self.run_readcmd(filename, 0, filesize)
+        self.check_filesize(output[0], filesize)
-- 
2.11.0

[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Simon Glass]

On 4 December 2016 at 19:52, Stefan Br?ns <stefan.bruens@rwth-aachen.de> wrote:
> From: Stefan Br??ns <stefan.bruens@rwth-aachen.de>
>
> The following checks are currently implemented:
> 1. listing a directory
> 2. verifying size of a file
> 3. veryfying md5sum for a file region
> 4. reading the beginning of a file
>
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  test/py/tests/test_fs.py | 357 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 357 insertions(+)
>  create mode 100644 test/py/tests/test_fs.py

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

Great to get this conversion going, thanks!

[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Stephen Warren]

On 12/04/2016 05:52 PM, Stefan Br?ns wrote:
> From: Stefan Br??ns <stefan.bruens@rwth-aachen.de>
>
> The following checks are currently implemented:
> 1. listing a directory
> 2. verifying size of a file
> 3. veryfying md5sum for a file region
> 4. reading the beginning of a file

General comments:

1) I don't see anywhere that limits this test to sandbox. I assume it 
can't run on non-sandbox targets, even when using the U-Boot native 
filesystem commands, since there's no logic here to transfer the 
filesystem images to the target device.

I think this issue blocks checking in this change as-is, although all 
the other comments below can likely be deferred until later if you want, 
perhaps with the exception of (2) immediately below?

2) Doesn't mounting/unmounting require root? Nothing else in test/py 
does. It'd be useful to put a comment at the top of the file detailing 
which command one might want to add to /etc/sudoers to avoid having to 
run the whole thing as root, and use sudo within the test where necessary.

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

> +from distutils.spawn import find_executable

Is distutils part of the standard library, i.e. in a default install? If 
not, please add it to the requirements section of test/py/README.md.

> +"""
> +Scenarios:
> +    hostfs: access image contents through the sandbox hostfs
> +        facility, using the filesytem implementation of
> +        the sandbox host, e.g. Linux kernel
> +    generic: test u-boots native filesystem implementations,
> +        using the 'generic' command names, e.g. 'load'
> +    TODO -
> +    fscommands: test u-boots native filesystem implementations,
> +        using the fs specific commands, e.g. 'ext4load'
> +"""
> + at pytest.fixture(scope='class', params=['generic', 'hostfs'])
> +def scenario(request):

Shouldn't that docstring be the first "statement" of the function, not 
right before it? I can't remember if Python picks up docstrings from 
right before objects too. If not, perhaps a comment would be better 
rather than a string.

> +class FsImage:
...
> +    def mount(self, log):
> +        if not os.path.exists(self.mountpath):
> +            os.mkdir(self.mountpath)

Use os.makedirs(path) instead; you can avoid the if statement, and it'll 
handle parent directories too.

> + at pytest.fixture(scope='module', params=['fat', 'ext4'])
> +def fsimage(prereq_commands, u_boot_config, u_boot_log, request):
> +    """Filesystem image instance."""
> +    datadir = u_boot_config.result_dir + '/'

Wouldn't it be better to put this into 
u_boot_config.persistent_data_dir, plus avoid creating the image file if 
it already exists? See u_boot_utils.py's PersistentRandomFile() as an 
example. I wonder if that could be expanded to create files not just of 
size n, but with sparse layout specs like this test uses?

> +    fstype = request.param
> +    imagepath = datadir + '3GB.' + fstype + '.img'
> +    mountpath = datadir + 'mnt_' + fstype
> +
> +    with u_boot_log.section('Create image "{0}"'.format(imagepath)):
> +        fsimage = FsImage(fstype, imagepath, mountpath)
> +        fsimage.mkfs(u_boot_log)
> +
> +    yield fsimage
> +    fsimage.unmount(u_boot_log)

Unmounting seems to happen in a lot of different places. Can we isolate 
it to just one place?

Also, what happens if the code throws an exception after obtaining an 
fsimage from this generator; I'm not sure that any cleanup happens in 
that case. Should there be "try: ... finally: unmount()" somewhere to 
clean up even in the case of an error? Alternatively, perhaps class 
FsImage should have a destructor that does the unmount (at least if it 
hasn't happened already)?

> +
> +
> +def test_fs_prepare_image(u_boot_config, fsimage, request):

Nit: Two blank lines there.

> +    """Dummy test to create an image file with filesystem.
> +    Useful to isolate fixture setup from actual tests."""
> +    if not fsimage:
> +        pytest.fail('Failed to create image')
> +
> +def test_fs_populate_image(populated_image, request):
> +    """Dummy test to create initial filesystem contents."""
> +    if not populated_image:
> +        pytest.fail('Failed create initial image content')

Why not just fail (or raise an exception) right where the error occurs?

> +    def run_readcmd(self, filename, offset, length):
> +        """Run the scenario and filesystem specific read command
> +        for the given file."""
> +        cmd = '{0}{1} {2} {3} {4} 0x{5:x} 0x{6:x}'.format(
> +            self.fs_params.get('prefix'),
> +            self.fs_commands.get('readcmd'),
> +            self.fs_params.get('interface'),
> +            '0', # address

It might be a good idea to invoke 
u_boot_utils.find_ram_base(u_boot_console) rather than hard-coding an 
address of 0. I don't know if sandbox will ever change its memory 
layout, but if there's any chance any of this will be ported to real HW, 
that function will be required.

> +            self.image.rootpath + filename,
> +            length, offset)
> +        with self.console.log.section('Read file "{0}"'.format(filename)):
> +            output = self.console.run_command_list(
> +                [cmd, 'env print filesize',
> +                 'md5sum 0 $filesize', 'env set filesize'])

Out of curiosity, why not "echo $filesize"?

> +            return output[1:3]

No error checking for output[0]? I suppose if u_boot_console_base.py's 
bad_pattern_defs[] included error patterns that "readcmd" was expected 
to emit, that'd be fine, but it doesn't currently. Maybe we expect that 
the other command can't possibly succeed if the read doesn't. Similar 
comment for run_sizecmd() below, and perhaps elsewhere.

> +    @pytest.mark.parametrize('dirname', ['', './'])
> +    def test_fs_ls(self, u_boot_console, dirname):
> +        """Check the contents of the given directory."""
> +        if self.image.fstype == 'fat' and dirname == './':
> +            pytest.skip("FAT has no '.' entry in the root directory")

Is there a "/" or "" prefix tested too? Hopefully the root dir gets 
tested on FAT.

[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Stefan Bruens]

On Montag, 12. Dezember 2016 11:04:34 CET you wrote:
> On 12/04/2016 05:52 PM, Stefan Br?ns wrote:
> > From: Stefan Br??ns <stefan.bruens@rwth-aachen.de>
> > 
> > The following checks are currently implemented:
> > 1. listing a directory
> > 2. verifying size of a file
> > 3. veryfying md5sum for a file region
> > 4. reading the beginning of a file
> 
> General comments:
> 
> 1) I don't see anywhere that limits this test to sandbox. I assume it
> can't run on non-sandbox targets, even when using the U-Boot native
> filesystem commands, since there's no logic here to transfer the
> filesystem images to the target device.

I have added Sandbox only for next patch version.

Running it from qemu should not be to difficult, "transferring" should be 
possible by adding the image to the qemu command.

Ideally, the FS image would be created once and then be reused. This had 
several benefits:
1. Reducing time for creation of the FAT image(s). Creation of the 2.5GB test 
file is time consuming, as FAT has no sparse files. Storage requirement is 
quite low if the FAT image is "resparsed" e.g. using "copy --sparse=always".
2. Sudo would be only (mostly) needed for the initial image creation.
3. If the image were created from a template, write tests (to be added later) 
could always start with a pristine image.

 
> I think this issue blocks checking in this change as-is, although all
> the other comments below can likely be deferred until later if you want,
> perhaps with the exception of (2) immediately below?
> 
> 2) Doesn't mounting/unmounting require root? Nothing else in test/py
> does. It'd be useful to put a comment at the top of the file detailing
> which command one might want to add to /etc/sudoers to avoid having to
> run the whole thing as root, and use sudo within the test where necessary.

It already uses sudo, the run command is visible from the log. Currently, I 
just run "sudo true" prior to executing the test, the password is cached for 5 
minutes.

If the image were created once, there would be no need for sudo for later runs 
(save hostfs).
 
> > diff --git a/test/py/tests/test_fs.py b/test/py/tests/test_fs.py
> > 
> > +from distutils.spawn import find_executable
> 
> Is distutils part of the standard library, i.e. in a default install? If
> not, please add it to the requirements section of test/py/README.md.

Don't know for other distros, for openSUSE it is.
 
> > +"""
> > +Scenarios:
> > +    hostfs: access image contents through the sandbox hostfs
> > +        facility, using the filesytem implementation of
> > +        the sandbox host, e.g. Linux kernel
> > +    generic: test u-boots native filesystem implementations,
> > +        using the 'generic' command names, e.g. 'load'
> > +    TODO -
> > +    fscommands: test u-boots native filesystem implementations,
> > +        using the fs specific commands, e.g. 'ext4load'
> > +"""
> > + at pytest.fixture(scope='class', params=['generic', 'hostfs'])
> 
> > +def scenario(request):
> Shouldn't that docstring be the first "statement" of the function, not
> right before it? I can't remember if Python picks up docstrings from
> right before objects too. If not, perhaps a comment would be better
> rather than a string.

For attributes, the docstring should be in front of the initial assignment, 
and may be picked up by the doc generator. For functions, it should be the 
first line(s) of the function.
 
> > +class FsImage:
> ...
> 
> > +    def mount(self, log):
> > +        if not os.path.exists(self.mountpath):
> > +            os.mkdir(self.mountpath)
> 
> Use os.makedirs(path) instead; you can avoid the if statement, and it'll
> handle parent directories too.

exist_ok only exists in python >= 3.2.
 
> > + at pytest.fixture(scope='module', params=['fat', 'ext4'])
> > +def fsimage(prereq_commands, u_boot_config, u_boot_log, request):
> > +    """Filesystem image instance."""
> > +    datadir = u_boot_config.result_dir + '/'
> 
> Wouldn't it be better to put this into
> u_boot_config.persistent_data_dir, plus avoid creating the image file if
> it already exists? See u_boot_utils.py's PersistentRandomFile() as an
> example. I wonder if that could be expanded to create files not just of
> size n, but with sparse layout specs like this test uses?

See above. I don't think PersistentRandomFile() does fit here, as the files 
are created *inside* an image. Maybe it could be used after the image and 
filesystem  is created and mounted ...
 
> > +    fstype = request.param
> > +    imagepath = datadir + '3GB.' + fstype + '.img'
> > +    mountpath = datadir + 'mnt_' + fstype
> > +
> > +    with u_boot_log.section('Create image "{0}"'.format(imagepath)):
> > +        fsimage = FsImage(fstype, imagepath, mountpath)
> > +        fsimage.mkfs(u_boot_log)
> > +
> > +    yield fsimage
> > +    fsimage.unmount(u_boot_log)
> 
> Unmounting seems to happen in a lot of different places. Can we isolate
> it to just one place?

The image is mounted/unmounted for two different reasons - creating/populating 
the image, and when accessing it using the hostfs commands.
 
> Also, what happens if the code throws an exception after obtaining an
> fsimage from this generator; I'm not sure that any cleanup happens in
> that case. Should there be "try: ... finally: unmount()" somewhere to
> clean up even in the case of an error? Alternatively, perhaps class
> FsImage should have a destructor that does the unmount (at least if it
> hasn't happened already)?

Its a fixture and will be torn down by pytest.
 
> > +
> > +
> 
> > +def test_fs_prepare_image(u_boot_config, fsimage, request):
> Nit: Two blank lines there.
> 
> > +    """Dummy test to create an image file with filesystem.
> > +    Useful to isolate fixture setup from actual tests."""
> > +    if not fsimage:
> > +        pytest.fail('Failed to create image')
> > +
> > +def test_fs_populate_image(populated_image, request):
> > +    """Dummy test to create initial filesystem contents."""
> > +    if not populated_image:
> > +        pytest.fail('Failed create initial image content')
> 
> Why not just fail (or raise an exception) right where the error occurs?
> 
> > +    def run_readcmd(self, filename, offset, length):
> > +        """Run the scenario and filesystem specific read command
> > +        for the given file."""
> > +        cmd = '{0}{1} {2} {3} {4} 0x{5:x} 0x{6:x}'.format(
> > +            self.fs_params.get('prefix'),
> > +            self.fs_commands.get('readcmd'),
> > +            self.fs_params.get('interface'),
> > +            '0', # address
> 
> It might be a good idea to invoke
> u_boot_utils.find_ram_base(u_boot_console) rather than hard-coding an
> address of 0. I don't know if sandbox will ever change its memory
> layout, but if there's any chance any of this will be ported to real HW,
> that function will be required.
> 
> > +            self.image.rootpath + filename,
> > +            length, offset)
> > +        with self.console.log.section('Read file
> > "{0}"'.format(filename)):
> > +            output = self.console.run_command_list(
> > +                [cmd, 'env print filesize',
> > +                 'md5sum 0 $filesize', 'env set filesize'])
> 
> Out of curiosity, why not "echo $filesize"?

Because "filesize=1234" is nicer to match
 
> > +            return output[1:3]
> 
> No error checking for output[0]? I suppose if u_boot_console_base.py's
> bad_pattern_defs[] included error patterns that "readcmd" was expected
> to emit, that'd be fine, but it doesn't currently. Maybe we expect that
> the other command can't possibly succeed if the read doesn't. Similar
> comment for run_sizecmd() below, and perhaps elsewhere.

filesize is only set if the read succeeds, and "env print filesize" matches 
bad_pattern_defs in case of an error.
 
> > +    @pytest.mark.parametrize('dirname', ['', './'])
> > +    def test_fs_ls(self, u_boot_console, dirname):
> > +        """Check the contents of the given directory."""
> > +        if self.image.fstype == 'fat' and dirname == './':
> > +            pytest.skip("FAT has no '.' entry in the root directory")
> 
> Is there a "/" or "" prefix tested too? Hopefully the root dir gets
> tested on FAT.

See first parametrize entry.

Kind regards,

Stefan


-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Stephen Warren]

On 01/01/2017 02:48 PM, Stefan Bruens wrote:
> On Montag, 12. Dezember 2016 11:04:34 CET you wrote:
>> On 12/04/2016 05:52 PM, Stefan Br?ns wrote:
>>> From: Stefan Br??ns <stefan.bruens@rwth-aachen.de>
>>>
>>> The following checks are currently implemented:
>>> 1. listing a directory
>>> 2. verifying size of a file
>>> 3. veryfying md5sum for a file region
>>> 4. reading the beginning of a file

>> 2) Doesn't mounting/unmounting require root? Nothing else in test/py
>> does. It'd be useful to put a comment at the top of the file detailing
>> which command one might want to add to /etc/sudoers to avoid having to
>> run the whole thing as root, and use sudo within the test where necessary.
>
> It already uses sudo, the run command is visible from the log. Currently, I
> just run "sudo true" prior to executing the test, the password is cached for 5
> minutes.

I don't see sudo used anywhere in test/py; can you point out where you 
see it using sudo at present?

>>> +class FsImage:
>> ...
>>
>>> +    def mount(self, log):
>>> +        if not os.path.exists(self.mountpath):
>>> +            os.mkdir(self.mountpath)
>>
>> Use os.makedirs(path) instead; you can avoid the if statement, and it'll
>> handle parent directories too.
>
> exist_ok only exists in python >= 3.2.

There's no need to use exist_ok; do this (example from conftest.py):

     try:
         os.makedirs(path)
     except OSError as exc:
         if exc.errno == errno.EEXIST and os.path.isdir(path):
             pass
         else:
             raise

>>> + at pytest.fixture(scope='module', params=['fat', 'ext4'])
>>> +def fsimage(prereq_commands, u_boot_config, u_boot_log, request):
>>> +    """Filesystem image instance."""
>>> +    datadir = u_boot_config.result_dir + '/'
>>
>> Wouldn't it be better to put this into
>> u_boot_config.persistent_data_dir, plus avoid creating the image file if
>> it already exists? See u_boot_utils.py's PersistentRandomFile() as an
>> example. I wonder if that could be expanded to create files not just of
>> size n, but with sparse layout specs like this test uses?
>
> See above. I don't think PersistentRandomFile() does fit here, as the files
> are created *inside* an image. Maybe it could be used after the image and
> filesystem  is created and mounted ...

You missed my point. PersistentRandomFile() is existing code that 
creates a persistent data file. You can create a new function/... that 
uses the same techniques to create the data file, but make it create a 
filesystem image rather than random data.

>>> +    fstype = request.param
>>> +    imagepath = datadir + '3GB.' + fstype + '.img'
>>> +    mountpath = datadir + 'mnt_' + fstype
>>> +
>>> +    with u_boot_log.section('Create image "{0}"'.format(imagepath)):
>>> +        fsimage = FsImage(fstype, imagepath, mountpath)
>>> +        fsimage.mkfs(u_boot_log)
>>> +
>>> +    yield fsimage
>>> +    fsimage.unmount(u_boot_log)
>>
>> Unmounting seems to happen in a lot of different places. Can we isolate
>> it to just one place?
>
> The image is mounted/unmounted for two different reasons - creating/populating
> the image, and when accessing it using the hostfs commands.

IIRC, there's more duplication than that, but I'll look again when this 
is reposted.

>> Also, what happens if the code throws an exception after obtaining an
>> fsimage from this generator; I'm not sure that any cleanup happens in
>> that case. Should there be "try: ... finally: unmount()" somewhere to
>> clean up even in the case of an error? Alternatively, perhaps class
>> FsImage should have a destructor that does the unmount (at least if it
>> hasn't happened already)?
>
> Its a fixture and will be torn down by pytest.

How does that work? Once the fixture function has yielded the image, 
surely if an exception is thrown, it'll be "thrown through" the 
generator function and hence prevent the rest of the function body from 
running? Or is there some special magic that lets the generator 
complete, even if the yield effectively threw an exception?

>>> +            return output[1:3]
>>
>> No error checking for output[0]? I suppose if u_boot_console_base.py's
>> bad_pattern_defs[] included error patterns that "readcmd" was expected
>> to emit, that'd be fine, but it doesn't currently. Maybe we expect that
>> the other command can't possibly succeed if the read doesn't. Similar
>> comment for run_sizecmd() below, and perhaps elsewhere.
>
> filesize is only set if the read succeeds, and "env print filesize" matches
> bad_pattern_defs in case of an error.

What pattern is printed that matches bad_pattern_defs? Nothing in 
bad_pattern_defs obviously would get printed if $filesize wasn't set.

Also, what if $filesize is still set from an earlier test (e.g. in a 
different Python file), and a later test fails, yet this isn't detected 
since that relies on $filesize not being set? I see this script doing 
"env set filesize" /after/ its own tests which should prevent this, but 
not /before/ the first test.

[U-Boot] [PATCH v2 2/2] test/py: Create tests for ext4 and fat testing on sandbox

[Stefan Bruens]

On Donnerstag, 5. Januar 2017 17:15:02 CET you wrote:
> On 01/01/2017 02:48 PM, Stefan Bruens wrote:
> > On Montag, 12. Dezember 2016 11:04:34 CET you wrote:
> >> On 12/04/2016 05:52 PM, Stefan Br?ns wrote:
> >>> From: Stefan Br??ns <stefan.bruens@rwth-aachen.de>
> >>> 
> >>> The following checks are currently implemented:
> >>> 1. listing a directory
> >>> 2. verifying size of a file
> >>> 3. veryfying md5sum for a file region
> >>> 4. reading the beginning of a file
> >> 
> >> 2) Doesn't mounting/unmounting require root? Nothing else in test/py
> >> does. It'd be useful to put a comment at the top of the file detailing
> >> which command one might want to add to /etc/sudoers to avoid having to
> >> run the whole thing as root, and use sudo within the test where
> >> necessary.
> > 
> > It already uses sudo, the run command is visible from the log. Currently,
> > I
> > just run "sudo true" prior to executing the test, the password is cached
> > for 5 minutes.
> 
> I don't see sudo used anywhere in test/py; can you point out where you
> see it using sudo at present?

it == the fs test
 
> >>> +class FsImage:
> >> ...
> >> 
> >>> +    def mount(self, log):
> >>> +        if not os.path.exists(self.mountpath):
> >>> +            os.mkdir(self.mountpath)
> >> 
> >> Use os.makedirs(path) instead; you can avoid the if statement, and it'll
> >> handle parent directories too.
> > 
> > exist_ok only exists in python >= 3.2.
> 
> There's no need to use exist_ok; do this (example from conftest.py):
> 
>      try:
>          os.makedirs(path)
>      except OSError as exc:
>          if exc.errno == errno.EEXIST and os.path.isdir(path):
>              pass
>          else:
>              raise

Thats not what you wrote. You said "you can avoid the if statement". What you 
meant is "you can replace the single line if statement with a 6 line exception 
handling statement". The latter is *only* better iff you have to handle 
concurrent creation of the wanted directory.
 
> >>> + at pytest.fixture(scope='module', params=['fat', 'ext4'])
> >>> +def fsimage(prereq_commands, u_boot_config, u_boot_log, request):
> >>> +    """Filesystem image instance."""
> >>> +    datadir = u_boot_config.result_dir + '/'
> >> 
> >> Wouldn't it be better to put this into
> >> u_boot_config.persistent_data_dir, plus avoid creating the image file if
> >> it already exists? See u_boot_utils.py's PersistentRandomFile() as an
> >> example. I wonder if that could be expanded to create files not just of
> >> size n, but with sparse layout specs like this test uses?
> > 
> > See above. I don't think PersistentRandomFile() does fit here, as the
> > files
> > are created *inside* an image. Maybe it could be used after the image and
> > filesystem  is created and mounted ...
> 
> You missed my point. PersistentRandomFile() is existing code that
> creates a persistent data file. You can create a new function/... that
> uses the same techniques to create the data file, but make it create a
> filesystem image rather than random data.

The disk image is a fixture, so it should be created as a fixture. The usage 
of PersistentRandomFile as currently done is wrong, it is a prerequisite of 
the test, not part of the test itself.

The correct use of PersistentRandomFile might be as a helper function used by 
different fixtures.

1. persistent_data_dir may be the better choice, but only if the image is 
created as a template there and then copied to a different location for each 
test run - the idea is to modify the image by write tests, and a clean image 
is needed here. Feel free to implement this yourself.

2. PersistentRandomFile creates single files, the only parameter is the size 
(although extending it to sparse files is quite possible). Disk images with 
filesystems have a lot more parameters - filesystem itself, filesystem 
options, filesystem contents.

Actually you missed my point here - PersistentRandomFile() is existing code, 
but it is useless here. Shoehorning it in here or extending it until it suits 
is a bad idea. It does one thing, and it does it fine. Creation of disk images 
is a different thing it does and should not do.

> >>> +    fstype = request.param
> >>> +    imagepath = datadir + '3GB.' + fstype + '.img'
> >>> +    mountpath = datadir + 'mnt_' + fstype
> >>> +
> >>> +    with u_boot_log.section('Create image "{0}"'.format(imagepath)):
> >>> +        fsimage = FsImage(fstype, imagepath, mountpath)
> >>> +        fsimage.mkfs(u_boot_log)
> >>> +
> >>> +    yield fsimage
> >>> +    fsimage.unmount(u_boot_log)
> >> 
> >> Unmounting seems to happen in a lot of different places. Can we isolate
> >> it to just one place?
> > 
> > The image is mounted/unmounted for two different reasons -
> > creating/populating the image, and when accessing it using the hostfs
> > commands.
> 
> IIRC, there's more duplication than that, but I'll look again when this
> is reposted.
> 
> >> Also, what happens if the code throws an exception after obtaining an
> >> fsimage from this generator; I'm not sure that any cleanup happens in
> >> that case. Should there be "try: ... finally: unmount()" somewhere to
> >> clean up even in the case of an error? Alternatively, perhaps class
> >> FsImage should have a destructor that does the unmount (at least if it
> >> hasn't happened already)?
> > 
> > Its a fixture and will be torn down by pytest.
> 
> How does that work? Once the fixture function has yielded the image,
> surely if an exception is thrown, it'll be "thrown through" the
> generator function and hence prevent the rest of the function body from
> running? Or is there some special magic that lets the generator
> complete, even if the yield effectively threw an exception?

Please read up on pytest:
http://pytest.org/dev/yieldfixture.html

> >>> +            return output[1:3]
> >> 
> >> No error checking for output[0]? I suppose if u_boot_console_base.py's
> >> bad_pattern_defs[] included error patterns that "readcmd" was expected
> >> to emit, that'd be fine, but it doesn't currently. Maybe we expect that
> >> the other command can't possibly succeed if the read doesn't. Similar
> >> comment for run_sizecmd() below, and perhaps elsewhere.
> > 
> > filesize is only set if the read succeeds, and "env print filesize"
> > matches
> > bad_pattern_defs in case of an error.
> 
> What pattern is printed that matches bad_pattern_defs? Nothing in
> bad_pattern_defs obviously would get printed if $filesize wasn't set.

=> env print foo              
## Error: "foo" not defined

> Also, what if $filesize is still set from an earlier test (e.g. in a
> different Python file), and a later test fails, yet this isn't detected
> since that relies on $filesize not being set? I see this script doing
> "env set filesize" /after/ its own tests which should prevent this, but
> not /before/ the first test.

There you have a point. *If* filesize where set, *and* the previously set 
value matches the expected value *and* this is the very first test to check 
$filesize, an error would have gone unnoticed ...

Regards,

Stefan


-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424