Re: [PATCH] iotests: Skip read-only cases in 118 when run as root

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 10/18/19 4:27 PM, Kevin Wolf wrote:
> Am 18.10.2019 um 14:59 hat Philippe Mathieu-Daudé geschrieben:
>> Hi Kevin,
>>
>> On 10/18/19 1:51 PM, Kevin Wolf wrote:
>>> Some tests in 118 use chmod to remove write permissions from the file
>>> and assume that the image can indeed not be opened read-write
>>> afterwards. This doesn't work when the test is run as root, because root
>>> can still open the file as writable even when the permission bit isn't
>>> set.
>>>
>>> Introduce a @skip_if_root decorator and use it in 118 to skip the tests
>>> in question when the script is run as root.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    tests/qemu-iotests/118        |  3 +++
>>>    tests/qemu-iotests/iotests.py | 10 ++++++++++
>>>    2 files changed, 13 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
>>> index ea0b326ae0..9eff46d189 100755
>>> --- a/tests/qemu-iotests/118
>>> +++ b/tests/qemu-iotests/118
>>> @@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
>>>            self.assert_qmp(result, 'return[0]/inserted/ro', True)
>>>            self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
>>> +    @iotests.skip_if_root
>>
>> Why not have case_notrun() return 'reason' and use:
>>
>> @unittest.skipIf(os.getuid() == 0, case_notrun("cannot be run as root"))
> 
> Because we can't skip test cases using unittest functionality, it
> results in different output (the test is marked as 's' instead of '.'
> and a message '(skipped=n)' is added), which means failure for
> qemu-iotests.

Ah, I see.

> 
> Apart from that, it would duplicate the logic and the error message in
> every place, which wouldn't be very nice anyway. With the necessary
> iotests.case_notrun() the line becomes > 80 characters, too.

OK.

> 
>>>        def test_rw_ro_retain(self):
>>>            os.chmod(new_img, 0o444)
>>>            self.vm.add_drive(old_img, 'media=disk', 'none')
>>> @@ -530,6 +531,7 @@ class TestChangeReadOnly(ChangeBaseClass):
>>>            self.assert_qmp(result, 'return[0]/inserted/ro', True)
>>>            self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
>>> +    @iotests.skip_if_root
>>>        def test_make_ro_rw(self):
>>>            os.chmod(new_img, 0o444)
>>>            self.vm.add_drive(old_img, 'media=disk', 'none')
>>> @@ -571,6 +573,7 @@ class TestChangeReadOnly(ChangeBaseClass):
>>>            self.assert_qmp(result, 'return[0]/inserted/ro', True)
>>>            self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
>>> +    @iotests.skip_if_root
>>>        def test_make_ro_rw_by_retain(self):
>>>            os.chmod(new_img, 0o444)
>>>            self.vm.add_drive(old_img, 'media=disk', 'none')
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3a8f378f90..9c66db613e 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -920,6 +920,16 @@ def skip_if_unsupported(required_formats=[], read_only=False):
>>>            return func_wrapper
>>>        return skip_test_decorator
>>> +def skip_if_root(func):
>>
>> skip_if_user_is_root() is slightly less confuse.
> 
> Ok, I can make this change.

Thanks.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>