[v2,4/4] test: Don't unmount not (yet) mounted system

When test suite tries to create a file for a new filesystem test case and fails,
the clean up of the exception tries to unmount the image, that has not yet been
mounted. When it happens, the fuse_mounted global variable is set to False and
inconveniently the test case tries to use sudo, so without this change the
admin of the machine gets an (annoying) email:

  Subject: *** SECURITY information for example.com ***

  example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt

and second run of the test cases on uncleaned build folder will ask for sudo
which is not what expected.

Besides that there is a double unmount calls during successfully run test case.

All of these due to over engineered Python try-except clause and people didn't
get it properly at all. The rule of thumb is that don't use more keywords than
try-except in the exception handling code. Nevertheless, here we adjust code
to be less intrusive to the initial logic behind that complex and unclear
constructions in the test case, although it adds a lot of lines of the code,
i.e. splits one exception handler to three, so on each step we know what
cleanup shall perform.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 22 deletions(-)

Hi Andy,

On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> When test suite tries to create a file for a new filesystem test case and fails,
> the clean up of the exception tries to unmount the image, that has not yet been
> mounted. When it happens, the fuse_mounted global variable is set to False and
> inconveniently the test case tries to use sudo, so without this change the
> admin of the machine gets an (annoying) email:
>
>   Subject: *** SECURITY information for example.com ***
>
>   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
>
> and second run of the test cases on uncleaned build folder will ask for sudo
> which is not what expected.
>
> Besides that there is a double unmount calls during successfully run test case.
>
> All of these due to over engineered Python try-except clause and people didn't
> get it properly at all. The rule of thumb is that don't use more keywords than
> try-except in the exception handling code. Nevertheless, here we adjust code
> to be less intrusive to the initial logic behind that complex and unclear
> constructions in the test case, although it adds a lot of lines of the code,
> i.e. splits one exception handler to three, so on each step we know what
> cleanup shall perform.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 22 deletions(-)
>

This looks OK to me, but there is a lot of duplication in the code,
isn't there? Perhaps another forray?

Regards,
Simon

On Thu, Feb 18, 2021 at 6:46 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Andy,
>
> On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > When test suite tries to create a file for a new filesystem test case and fails,
> > the clean up of the exception tries to unmount the image, that has not yet been
> > mounted. When it happens, the fuse_mounted global variable is set to False and
> > inconveniently the test case tries to use sudo, so without this change the
> > admin of the machine gets an (annoying) email:
> >
> >   Subject: *** SECURITY information for example.com ***
> >
> >   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
> >
> > and second run of the test cases on uncleaned build folder will ask for sudo
> > which is not what expected.
> >
> > Besides that there is a double unmount calls during successfully run test case.
> >
> > All of these due to over engineered Python try-except clause and people didn't
> > get it properly at all. The rule of thumb is that don't use more keywords than
> > try-except in the exception handling code. Nevertheless, here we adjust code
> > to be less intrusive to the initial logic behind that complex and unclear
> > constructions in the test case, although it adds a lot of lines of the code,
> > i.e. splits one exception handler to three, so on each step we know what
> > cleanup shall perform.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > v2: new patch
> >  test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
> >  1 file changed, 56 insertions(+), 22 deletions(-)
> >
>
> This looks OK to me, but there is a lot of duplication in the code,
> isn't there? Perhaps another forray?

Can we apply this fix as is and think about optimisations later, please?
W/o this I'm really blocked from running tests against U-Boot.

On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Thu, Feb 18, 2021 at 6:46 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Andy,
> >
> > On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > When test suite tries to create a file for a new filesystem test case and fails,
> > > the clean up of the exception tries to unmount the image, that has not yet been
> > > mounted. When it happens, the fuse_mounted global variable is set to False and
> > > inconveniently the test case tries to use sudo, so without this change the
> > > admin of the machine gets an (annoying) email:
> > >
> > >   Subject: *** SECURITY information for example.com ***
> > >
> > >   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
> > >
> > > and second run of the test cases on uncleaned build folder will ask for sudo
> > > which is not what expected.
> > >
> > > Besides that there is a double unmount calls during successfully run test case.
> > >
> > > All of these due to over engineered Python try-except clause and people didn't
> > > get it properly at all. The rule of thumb is that don't use more keywords than
> > > try-except in the exception handling code. Nevertheless, here we adjust code
> > > to be less intrusive to the initial logic behind that complex and unclear
> > > constructions in the test case, although it adds a lot of lines of the code,
> > > i.e. splits one exception handler to three, so on each step we know what
> > > cleanup shall perform.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > v2: new patch
> > >  test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
> > >  1 file changed, 56 insertions(+), 22 deletions(-)
> > >
> >
> > This looks OK to me, but there is a lot of duplication in the code,
> > isn't there? Perhaps another forray?
>
> Can we apply this fix as is and think about optimisations later, please?
> W/o this I'm really blocked from running tests against U-Boot.

'make qcheck' bypasses this.

+Heinrich Schuchardt

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

On Thu, Feb 18, 2021 at 09:52:12PM -0700, Simon Glass wrote:
> On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Feb 18, 2021 at 6:46 AM Simon Glass <sjg@chromium.org> wrote:
> > > On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > When test suite tries to create a file for a new filesystem test case and fails,
> > > > the clean up of the exception tries to unmount the image, that has not yet been
> > > > mounted. When it happens, the fuse_mounted global variable is set to False and
> > > > inconveniently the test case tries to use sudo, so without this change the
> > > > admin of the machine gets an (annoying) email:
> > > >
> > > >   Subject: *** SECURITY information for example.com ***
> > > >
> > > >   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
> > > >
> > > > and second run of the test cases on uncleaned build folder will ask for sudo
> > > > which is not what expected.
> > > >
> > > > Besides that there is a double unmount calls during successfully run test case.
> > > >
> > > > All of these due to over engineered Python try-except clause and people didn't
> > > > get it properly at all. The rule of thumb is that don't use more keywords than
> > > > try-except in the exception handling code. Nevertheless, here we adjust code
> > > > to be less intrusive to the initial logic behind that complex and unclear
> > > > constructions in the test case, although it adds a lot of lines of the code,
> > > > i.e. splits one exception handler to three, so on each step we know what
> > > > cleanup shall perform.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > > v2: new patch
> > > >  test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
> > > >  1 file changed, 56 insertions(+), 22 deletions(-)
> > > >
> > >
> > > This looks OK to me, but there is a lot of duplication in the code,
> > > isn't there? Perhaps another forray?
> >
> > Can we apply this fix as is and think about optimisations later, please?
> > W/o this I'm really blocked from running tests against U-Boot.
> 
> 'make qcheck' bypasses this.
> 
> +Heinrich Schuchardt
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks!

Tom, I don't see this being applied. Can we actually get it in?

On Tue, Mar 30, 2021 at 10:41:15PM +0300, Andy Shevchenko wrote:
> On Thu, Feb 18, 2021 at 09:52:12PM -0700, Simon Glass wrote:
> > On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Feb 18, 2021 at 6:46 AM Simon Glass <sjg@chromium.org> wrote:
> > > > On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > When test suite tries to create a file for a new filesystem test case and fails,
> > > > > the clean up of the exception tries to unmount the image, that has not yet been
> > > > > mounted. When it happens, the fuse_mounted global variable is set to False and
> > > > > inconveniently the test case tries to use sudo, so without this change the
> > > > > admin of the machine gets an (annoying) email:
> > > > >
> > > > >   Subject: *** SECURITY information for example.com ***
> > > > >
> > > > >   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
> > > > >
> > > > > and second run of the test cases on uncleaned build folder will ask for sudo
> > > > > which is not what expected.
> > > > >
> > > > > Besides that there is a double unmount calls during successfully run test case.
> > > > >
> > > > > All of these due to over engineered Python try-except clause and people didn't
> > > > > get it properly at all. The rule of thumb is that don't use more keywords than
> > > > > try-except in the exception handling code. Nevertheless, here we adjust code
> > > > > to be less intrusive to the initial logic behind that complex and unclear
> > > > > constructions in the test case, although it adds a lot of lines of the code,
> > > > > i.e. splits one exception handler to three, so on each step we know what
> > > > > cleanup shall perform.
> > > > >
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > ---
> > > > > v2: new patch
> > > > >  test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
> > > > >  1 file changed, 56 insertions(+), 22 deletions(-)
> > > > >
> > > >
> > > > This looks OK to me, but there is a lot of duplication in the code,
> > > > isn't there? Perhaps another forray?
> > >
> > > Can we apply this fix as is and think about optimisations later, please?
> > > W/o this I'm really blocked from running tests against U-Boot.
> > 
> > 'make qcheck' bypasses this.
> > 
> > +Heinrich Schuchardt
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Thanks!
> 
> Tom, I don't see this being applied. Can we actually get it in?

Does this apply cleanly to master?  I thought only 1/4 did so I applied
that.

On Tuesday, March 30, 2021, Tom Rini <trini@konsulko.com> wrote:

> On Tue, Mar 30, 2021 at 10:41:15PM +0300, Andy Shevchenko wrote:
> > On Thu, Feb 18, 2021 at 09:52:12PM -0700, Simon Glass wrote:
> > > On Thu, 18 Feb 2021 at 03:56, Andy Shevchenko <
> andy.shevchenko@gmail.com> wrote:
> > > > On Thu, Feb 18, 2021 at 6:46 AM Simon Glass <sjg@chromium.org>
> wrote:
> > > > > On Thu, 11 Feb 2021 at 07:40, Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > >
> > > > > > When test suite tries to create a file for a new filesystem test
> case and fails,
> > > > > > the clean up of the exception tries to unmount the image, that
> has not yet been
> > > > > > mounted. When it happens, the fuse_mounted global variable is
> set to False and
> > > > > > inconveniently the test case tries to use sudo, so without this
> change the
> > > > > > admin of the machine gets an (annoying) email:
> > > > > >
> > > > > >   Subject: *** SECURITY information for example.com ***
> > > > > >
> > > > > >   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount
> .../build-sandbox/persistent-data/mnt
> > > > > >
> > > > > > and second run of the test cases on uncleaned build folder will
> ask for sudo
> > > > > > which is not what expected.
> > > > > >
> > > > > > Besides that there is a double unmount calls during successfully
> run test case.
> > > > > >
> > > > > > All of these due to over engineered Python try-except clause and
> people didn't
> > > > > > get it properly at all. The rule of thumb is that don't use more
> keywords than
> > > > > > try-except in the exception handling code. Nevertheless, here we
> adjust code
> > > > > > to be less intrusive to the initial logic behind that complex
> and unclear
> > > > > > constructions in the test case, although it adds a lot of lines
> of the code,
> > > > > > i.e. splits one exception handler to three, so on each step we
> know what
> > > > > > cleanup shall perform.
> > > > > >
> > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.
> intel.com>
> > > > > > ---
> > > > > > v2: new patch
> > > > > >  test/py/tests/test_fs/conftest.py | 78
> ++++++++++++++++++++++---------
> > > > > >  1 file changed, 56 insertions(+), 22 deletions(-)
> > > > > >
> > > > >
> > > > > This looks OK to me, but there is a lot of duplication in the code,
> > > > > isn't there? Perhaps another forray?
> > > >
> > > > Can we apply this fix as is and think about optimisations later,
> please?
> > > > W/o this I'm really blocked from running tests against U-Boot.
> > >
> > > 'make qcheck' bypasses this.
> > >
> > > +Heinrich Schuchardt
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Thanks!
> >
> > Tom, I don't see this being applied. Can we actually get it in?
>
> Does this apply cleanly to master?  I thought only 1/4 did so I applied
> that.


Patches 2&3 are against Simon’s tree, patches 1&4 are against main branch


>
> --
> Tom
>

On Thu, Feb 11, 2021 at 04:40:12PM +0200, Andy Shevchenko wrote:

> When test suite tries to create a file for a new filesystem test case and fails,
> the clean up of the exception tries to unmount the image, that has not yet been
> mounted. When it happens, the fuse_mounted global variable is set to False and
> inconveniently the test case tries to use sudo, so without this change the
> admin of the machine gets an (annoying) email:
> 
>   Subject: *** SECURITY information for example.com ***
> 
>   example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
> 
> and second run of the test cases on uncleaned build folder will ask for sudo
> which is not what expected.
> 
> Besides that there is a double unmount calls during successfully run test case.
> 
> All of these due to over engineered Python try-except clause and people didn't
> get it properly at all. The rule of thumb is that don't use more keywords than
> try-except in the exception handling code. Nevertheless, here we adjust code
> to be less intrusive to the initial logic behind that complex and unclear
> constructions in the test case, although it adds a lot of lines of the code,
> i.e. splits one exception handler to three, so on each step we know what
> cleanup shall perform.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

On 2/11/21 3:40 PM, Andy Shevchenko wrote:
> When test suite tries to create a file for a new filesystem test case and fails,
> the clean up of the exception tries to unmount the image, that has not yet been
> mounted. When it happens, the fuse_mounted global variable is set to False and
> inconveniently the test case tries to use sudo, so without this change the
> admin of the machine gets an (annoying) email:
>
>    Subject: *** SECURITY information for example.com ***
>
>    example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
>
> and second run of the test cases on uncleaned build folder will ask for sudo
> which is not what expected.
>
> Besides that there is a double unmount calls during successfully run test case.
>
> All of these due to over engineered Python try-except clause and people didn't
> get it properly at all. The rule of thumb is that don't use more keywords than
> try-except in the exception handling code. Nevertheless, here we adjust code
> to be less intrusive to the initial logic behind that complex and unclear
> constructions in the test case, although it adds a lot of lines of the code,
> i.e. splits one exception handler to three, so on each step we know what
> cleanup shall perform.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Dear Andy,

with this merged patch the following tests are not executed anymore locally:

test/py/tests/test_fs/test_basic.py
test/py/tests/test_fs/test_ext.py

SKIPPED [13] test/py/tests/test_fs/conftest.py:350: Setup failed for
filesystem: ext4. Command 'guestmount -a
build-sandbox/persistent-data/3GB.ext4.img -m /dev/sda
build-sandbox/persistent-data/mnt' returned non-zero exit status 1.

Please, revert the patch or fix it.

Best regards

Heinrich

> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> v2: new patch
>   test/py/tests/test_fs/conftest.py | 78 ++++++++++++++++++++++---------
>   1 file changed, 56 insertions(+), 22 deletions(-)
>
> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
> index ec70e8c4ef3f..50af9efcf768 100644
> --- a/test/py/tests/test_fs/conftest.py
> +++ b/test/py/tests/test_fs/conftest.py
> @@ -270,9 +270,20 @@ def fs_obj_basic(request, u_boot_config):
>
>           # 3GiB volume
>           fs_img = mk_fs(u_boot_config, fs_type, 0xc0000000, '3GB')
> +    except CalledProcessError as err:
> +        pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        return
>
> -        # Mount the image so we can populate it.
> +    try:
>           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)
> +
> +    try:
> +        # Mount the image so we can populate it.
>           mount_fs(fs_type, fs_img, mount_dir)
>
>           # Create a subdirectory.
> @@ -335,18 +346,15 @@ def fs_obj_basic(request, u_boot_config):
>   	    % big_file, shell=True).decode()
>           md5val.append(out.split()[0])
>
> -        umount_fs(mount_dir)
>       except CalledProcessError as err:
> -        pytest.skip('Setup failed for filesystem: ' + fs_type + \
> -            '. {}'.format(err))
> +        pytest.skip('Setup failed for filesystem: ' + fs_type + '. {}'.format(err))
>           return
>       else:
>           yield [fs_ubtype, fs_img, md5val]
>       finally:
>           umount_fs(mount_dir)
>           call('rmdir %s' % mount_dir, shell=True)
> -        if fs_img:
> -            call('rm -f %s' % fs_img, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
>
>   #
>   # Fixture for extended fs test
> @@ -378,9 +386,20 @@ def fs_obj_ext(request, u_boot_config):
>
>           # 128MiB volume
>           fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB')
> +    except CalledProcessError as err:
> +        pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        return
>
> -        # Mount the image so we can populate it.
> +    try:
>           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)
> +
> +    try:
> +        # Mount the image so we can populate it.
>           mount_fs(fs_type, fs_img, mount_dir)
>
>           # Create a test directory
> @@ -422,7 +441,6 @@ def fs_obj_ext(request, u_boot_config):
>           md5val.append(out.split()[0])
>
>           check_call('rm %s' % tmp_file, shell=True)
> -        umount_fs(mount_dir)
>       except CalledProcessError:
>           pytest.skip('Setup failed for filesystem: ' + fs_type)
>           return
> @@ -431,8 +449,7 @@ def fs_obj_ext(request, u_boot_config):
>       finally:
>           umount_fs(mount_dir)
>           call('rmdir %s' % mount_dir, shell=True)
> -        if fs_img:
> -            call('rm -f %s' % fs_img, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
>
>   #
>   # Fixture for mkdir test
> @@ -460,11 +477,10 @@ def fs_obj_mkdir(request, u_boot_config):
>           fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB')
>       except:
>           pytest.skip('Setup failed for filesystem: ' + fs_type)
> +        return
>       else:
>           yield [fs_ubtype, fs_img]
> -    finally:
> -        if fs_img:
> -            call('rm -f %s' % fs_img, shell=True)
> +    call('rm -f %s' % fs_img, shell=True)
>
>   #
>   # Fixture for unlink test
> @@ -493,9 +509,20 @@ def fs_obj_unlink(request, u_boot_config):
>
>           # 128MiB volume
>           fs_img = mk_fs(u_boot_config, fs_type, 0x8000000, '128MB')
> +    except CalledProcessError as err:
> +        pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        return
>
> -        # Mount the image so we can populate it.
> +    try:
>           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)
> +
> +    try:
> +        # Mount the image so we can populate it.
>           mount_fs(fs_type, fs_img, mount_dir)
>
>           # Test Case 1 & 3
> @@ -519,7 +546,6 @@ def fs_obj_unlink(request, u_boot_config):
>           check_call('dd if=/dev/urandom of=%s/dir5/file1 bs=1K count=1'
>                                       % mount_dir, shell=True)
>
> -        umount_fs(mount_dir)
>       except CalledProcessError:
>           pytest.skip('Setup failed for filesystem: ' + fs_type)
>           return
> @@ -528,8 +554,7 @@ def fs_obj_unlink(request, u_boot_config):
>       finally:
>           umount_fs(mount_dir)
>           call('rmdir %s' % mount_dir, shell=True)
> -        if fs_img:
> -            call('rm -f %s' % fs_img, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
>
>   #
>   # Fixture for symlink fs test
> @@ -559,11 +584,22 @@ def fs_obj_symlink(request, u_boot_config):
>
>       try:
>
> -        # 3GiB volume
> +        # 1GiB volume
>           fs_img = mk_fs(u_boot_config, fs_type, 0x40000000, '1GB')
> +    except CalledProcessError as err:
> +        pytest.skip('Creating failed for filesystem: ' + fs_type + '. {}'.format(err))
> +        return
>
> -        # Mount the image so we can populate it.
> +    try:
>           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)
> +
> +    try:
> +        # Mount the image so we can populate it.
>           mount_fs(fs_type, fs_img, mount_dir)
>
>           # Create a subdirectory.
> @@ -587,7 +623,6 @@ def fs_obj_symlink(request, u_boot_config):
>               % medium_file, shell=True).decode()
>           md5val.extend([out.split()[0]])
>
> -        umount_fs(mount_dir)
>       except CalledProcessError:
>           pytest.skip('Setup failed for filesystem: ' + fs_type)
>           return
> @@ -596,5 +631,4 @@ def fs_obj_symlink(request, u_boot_config):
>       finally:
>           umount_fs(mount_dir)
>           call('rmdir %s' % mount_dir, shell=True)
> -        if fs_img:
> -            call('rm -f %s' % fs_img, shell=True)
> +        call('rm -f %s' % fs_img, shell=True)
>

On Thu, May 13, 2021 at 2:32 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 2/11/21 3:40 PM, Andy Shevchenko wrote:
> > When test suite tries to create a file for a new filesystem test case and fails,
> > the clean up of the exception tries to unmount the image, that has not yet been
> > mounted. When it happens, the fuse_mounted global variable is set to False and
> > inconveniently the test case tries to use sudo, so without this change the
> > admin of the machine gets an (annoying) email:
> >
> >    Subject: *** SECURITY information for example.com ***
> >
> >    example.com : Feb  5 19:43:47 : ... COMMAND=/bin/umount .../build-sandbox/persistent-data/mnt
> >
> > and second run of the test cases on uncleaned build folder will ask for sudo
> > which is not what expected.
> >
> > Besides that there is a double unmount calls during successfully run test case.
> >
> > All of these due to over engineered Python try-except clause and people didn't
> > get it properly at all. The rule of thumb is that don't use more keywords than
> > try-except in the exception handling code. Nevertheless, here we adjust code
> > to be less intrusive to the initial logic behind that complex and unclear
> > constructions in the test case, although it adds a lot of lines of the code,
> > i.e. splits one exception handler to three, so on each step we know what
> > cleanup shall perform.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Dear Andy,
>
> with this merged patch the following tests are not executed anymore locally:
>
> test/py/tests/test_fs/test_basic.py
> test/py/tests/test_fs/test_ext.py
>
> SKIPPED [13] test/py/tests/test_fs/conftest.py:350: Setup failed for
> filesystem: ext4. Command 'guestmount -a
> build-sandbox/persistent-data/3GB.ext4.img -m /dev/sda
> build-sandbox/persistent-data/mnt' returned non-zero exit status 1.
>
> Please, revert the patch or fix it.

Thanks for the report, let's investigate it.
And for the consistency let's continue this under the revert thread,