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
     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 Test Decorator
+       Runs the test only without root permissions'''
+    def func_wrapper(*args, **kwargs):
+        if os.getuid() == 0:
+            case_notrun('{}: cannot be run as root'.format(args[0]))
+        else:
+            return func(*args, **kwargs)
+    return func_wrapper
+
 def execute_unittest(output, verbosity, debug):
     runner = unittest.TextTestRunner(stream=output, descriptions=True,
                                      verbosity=verbosity)
-- 
2.20.1

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"))


>       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.

> +    '''Skip Test Decorator
> +       Runs the test only without root permissions'''
> +    def func_wrapper(*args, **kwargs):
> +        if os.getuid() == 0:
> +            case_notrun('{}: cannot be run as root'.format(args[0]))
> +        else:
> +            return func(*args, **kwargs)
> +    return func_wrapper
> +
>   def execute_unittest(output, verbosity, debug):
>       runner = unittest.TextTestRunner(stream=output, descriptions=True,
>                                        verbosity=verbosity)
> 

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.

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.

> >       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.

Kevin

On 18.10.19 16:27, 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.

Not arguing that we should use unittest skipping here, but my “Selfish
patches” series allows it:

https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg03423.html

The advantage is that using unittest skipping works in setUp, too.

Max

Am 18.10.2019 um 17:00 hat Max Reitz geschrieben:
> On 18.10.19 16:27, 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.
> 
> Not arguing that we should use unittest skipping here, but my “Selfish
> patches” series allows it:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg03423.html
> 
> The advantage is that using unittest skipping works in setUp, too.

Ah, good to know. If this had already been in master, I might have
chosen a simple function call iotests.skip_if_root() inside the test
function instead of using a decorator. But in the end, I don't think it
makes a big difference in this case.

Kevin

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>