[PATCH] test: Have test_fs work with non-functional guestmount tools

[Stephen Warren]

On 7/7/20 12:46 PM, Tom Rini wrote:
> On Tue, Jul 07, 2020 at 12:34:19PM -0600, Stephen Warren wrote:
>> On 7/7/20 9:53 AM, Tom Rini wrote:
>>> Since 2011 Ubuntu has intentionally broken support for guestmount[1] by
>>> default and requires sysadmin intervention to re-enable support.  This
>>> in turn exposed that in our tests if guestmount is available but fails
>>> we do not fall back to trying to use sudo.  Restructure our code to try
>>> sudo if guestmount fails rather than only when it is not in our path.
>>> Further, only note that we are using fuse on success of the call.
>>>
>>> [1]: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/759725
>>
>> That is ... an interesting bug!
>>
>> This change looks conceptually fine.
>>
>>> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
>> ...
>>> @@ -206,10 +206,11 @@ def mount_fs(fs_type, device, mount_point):
>> ...
> +        try:
>>>              mount_opt = 'loop,rw'
>>>              if re.match('fat', fs_type):
>>>                  mount_opt += ',umask=0000'
>> ...
>>>  
>>>              # may not be effective for some file systems
>>>              check_call('sudo chmod a+rw %s' % mount_point, shell=True)
>>> +        except CalledProcessError:
>>> +            raise
>>
>> That last/inner try...except/raise clause doesn't seem useful. Perhaps
>> just remote try...except and keep the body?
> 
> I don't quite follow, sorry.  What we have today is if/else on
> guestmount, but since check_call() throws CalledProcessError when it
> fails to run we never try sudo'ing.  That's why I put the inner
> try/except/raise in.

It's the outer try/except that switches between the two options, not the
inner one.

It makes sense to have one try/except to catch errors running guestmount
and switch to sudo mount. However, the second try/except block that
solely wraps the sudo mount call doesn't do anything; it simply catches
an exception and then immediately re-raises it. This same issue is
actually present in the original code, except that there is applies to a
larger region of code.

So, wherever the code does this:

    try:
        something
    except CalledProcessError:
        raise

Just replace that with this:

    something

So...

     fuse_mounted = False
     try:
         if tool_is_in_path('guestmount'):
             check_call('guestmount -a %s -m /dev/sda %s'
                 % (device, mount_point), shell=True)
            fuse_mounted = True
    except CalledProcessError:
        # TODO: Remove the try here
        # TODO: Remove 1 indent level from the rest of this code
             mount_opt = 'loop,rw'
             if re.match('fat', fs_type):
                 mount_opt += ',umask=0000'
@@ -219,8 +220,8 @@ def mount_fs(fs_type, device, mount_point):

             # may not be effective for some file systems
             check_call('sudo chmod a+rw %s' % mount_point, shell=True)
        # TODO: Remove the except/raise here