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
