On Thu, May 20, 2021 at 10:09:46PM +0300, Alper Nebi Yasak wrote: > Commit 1ba21bb06b08 ("test: Don't unmount not (yet) mounted system") > fixes an issue in the filesystem tests where the test setup may fail > to mount an image and still attempt to unmount it. However, the commit > unintentionally breaks the test setups in two ways. > > The newly created unmounted filesystem images are being immediately > deleted due to some cleanup steps being misplaced into finally blocks, > which makes them always run instead of only on failures. The mount calls > always fail since the images never exist, causing the tests to be always > skipped. This patch moves these cleanup calls into the except blocks to > fix this and makes the tests run again. > > There are also unmount calls misplaced into finally blocks, making them > run after the tests instead of before the tests. These unmount calls > make the filesystem image file consistent with the changes made to it as > part of the test setup, and this misplacement is making a number of > tests fail unexpectedly. > > The unmount calls must be run before the tests use the image, meaning > before the yield call and not in the finally block. They must also be > run as a cleanup step when the filesystem setup fails, so they can't be > placed as the final call in the try blocks since they would be skipped > on such failures. For these reasons, this patch places the unmount calls > both in the except blocks and the else blocks of the final setup step. > This makes the unexpectedly failing tests to succeed again. > > Furthermore, this isolates the mount calls to their own try-except > statement to avoid reintroducing the original issue of unmounting a > not-mounted image while fixing the unmount misplacement. > > After these fixes, running "make tests" with guestmount available results > in two test failures not related to the mentioned commit. If the > guestmount executables are unavailable, the mounts fallback to using > sudo and result in no failures. > > Fixes: 1ba21bb06b08 ("test: Don't unmount not (yet) mounted system") > Signed-off-by: Alper Nebi Yasak <alpernebiya...@gmail.com> > --- > There is more discussion at Heinrich's revert-patch [1] of the mentioned > commit. This patch is an alternative to the revert. > > Andy: I don't think unmounting the failed-to-mount image was the cause > of the sudo call you described in that commit, so I believe this would > end up running sudo on your system. Wanted to warn in advance, but looks > like your issue needs a different solution.
Thanks for doing this! If you think this fix is a right thing to do, go ahead for it, I prefer it over revert. Acked-by: Andy Shevchenko <andriy.shevche...@linux.intel.com> > [1] > https://patchwork.ozlabs.org/project/uboot/patch/20210513114035.177293-1-xypron.g...@gmx.de/ > > test/py/tests/test_fs/conftest.py | 48 +++++++++++++++++++++++-------- > 1 file changed, 36 insertions(+), 12 deletions(-) > > diff --git a/test/py/tests/test_fs/conftest.py > b/test/py/tests/test_fs/conftest.py > index 50af9efcf768..410a675b9714 100644 > --- a/test/py/tests/test_fs/conftest.py > +++ b/test/py/tests/test_fs/conftest.py > @@ -278,14 +278,19 @@ def fs_obj_basic(request, u_boot_config): > check_call('mkdir -p %s' % mount_dir, shell=True) > except CalledProcessError as err: > pytest.skip('Preparing mount folder failed for filesystem: ' + > fs_type + '. {}'.format(err)) > - return > - finally: > call('rm -f %s' % fs_img, shell=True) > + return > > try: > # Mount the image so we can populate it. > mount_fs(fs_type, fs_img, mount_dir) > + except CalledProcessError as err: > + pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + > '. {}'.format(err)) > + call('rmdir %s' % mount_dir, shell=True) > + call('rm -f %s' % fs_img, shell=True) > + return > > + try: > # Create a subdirectory. > check_call('mkdir %s/SUBDIR' % mount_dir, shell=True) > > @@ -348,11 +353,12 @@ def fs_obj_basic(request, u_boot_config): > > except CalledProcessError as err: > pytest.skip('Setup failed for filesystem: ' + fs_type + '. > {}'.format(err)) > + umount_fs(mount_dir) > return > else: > + umount_fs(mount_dir) > yield [fs_ubtype, fs_img, md5val] > finally: > - umount_fs(mount_dir) > call('rmdir %s' % mount_dir, shell=True) > call('rm -f %s' % fs_img, shell=True) > > @@ -394,14 +400,19 @@ def fs_obj_ext(request, u_boot_config): > check_call('mkdir -p %s' % mount_dir, shell=True) > except CalledProcessError as err: > pytest.skip('Preparing mount folder failed for filesystem: ' + > fs_type + '. {}'.format(err)) > - return > - finally: > call('rm -f %s' % fs_img, shell=True) > + return > > try: > # Mount the image so we can populate it. > mount_fs(fs_type, fs_img, mount_dir) > + except CalledProcessError as err: > + pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + > '. {}'.format(err)) > + call('rmdir %s' % mount_dir, shell=True) > + call('rm -f %s' % fs_img, shell=True) > + return > > + try: > # Create a test directory > check_call('mkdir %s/dir1' % mount_dir, shell=True) > > @@ -443,11 +454,12 @@ def fs_obj_ext(request, u_boot_config): > check_call('rm %s' % tmp_file, shell=True) > except CalledProcessError: > pytest.skip('Setup failed for filesystem: ' + fs_type) > + umount_fs(mount_dir) > return > else: > + umount_fs(mount_dir) > yield [fs_ubtype, fs_img, md5val] > finally: > - umount_fs(mount_dir) > call('rmdir %s' % mount_dir, shell=True) > call('rm -f %s' % fs_img, shell=True) > > @@ -517,14 +529,19 @@ def fs_obj_unlink(request, u_boot_config): > check_call('mkdir -p %s' % mount_dir, shell=True) > except CalledProcessError as err: > pytest.skip('Preparing mount folder failed for filesystem: ' + > fs_type + '. {}'.format(err)) > - return > - finally: > call('rm -f %s' % fs_img, shell=True) > + return > > try: > # Mount the image so we can populate it. > mount_fs(fs_type, fs_img, mount_dir) > + except CalledProcessError as err: > + pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + > '. {}'.format(err)) > + call('rmdir %s' % mount_dir, shell=True) > + call('rm -f %s' % fs_img, shell=True) > + return > > + try: > # Test Case 1 & 3 > check_call('mkdir %s/dir1' % mount_dir, shell=True) > check_call('dd if=/dev/urandom of=%s/dir1/file1 bs=1K count=1' > @@ -548,11 +565,12 @@ def fs_obj_unlink(request, u_boot_config): > > except CalledProcessError: > pytest.skip('Setup failed for filesystem: ' + fs_type) > + umount_fs(mount_dir) > return > else: > + umount_fs(mount_dir) > yield [fs_ubtype, fs_img] > finally: > - umount_fs(mount_dir) > call('rmdir %s' % mount_dir, shell=True) > call('rm -f %s' % fs_img, shell=True) > > @@ -594,14 +612,19 @@ def fs_obj_symlink(request, u_boot_config): > check_call('mkdir -p %s' % mount_dir, shell=True) > except CalledProcessError as err: > pytest.skip('Preparing mount folder failed for filesystem: ' + > fs_type + '. {}'.format(err)) > - return > - finally: > call('rm -f %s' % fs_img, shell=True) > + return > > try: > # Mount the image so we can populate it. > mount_fs(fs_type, fs_img, mount_dir) > + except CalledProcessError as err: > + pytest.skip('Mounting to folder failed for filesystem: ' + fs_type + > '. {}'.format(err)) > + call('rmdir %s' % mount_dir, shell=True) > + call('rm -f %s' % fs_img, shell=True) > + return > > + try: > # Create a subdirectory. > check_call('mkdir %s/SUBDIR' % mount_dir, shell=True) > > @@ -625,10 +648,11 @@ def fs_obj_symlink(request, u_boot_config): > > except CalledProcessError: > pytest.skip('Setup failed for filesystem: ' + fs_type) > + umount_fs(mount_dir) > return > else: > + umount_fs(mount_dir) > yield [fs_ubtype, fs_img, md5val] > finally: > - umount_fs(mount_dir) > call('rmdir %s' % mount_dir, shell=True) > call('rm -f %s' % fs_img, shell=True) > -- > 2.31.1 > -- With Best Regards, Andy Shevchenko