* [PATCH] iotests: Skip read-only cases in 118 when run as root
@ 2019-10-18 11:51 Kevin Wolf
2019-10-18 12:59 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2019-10-18 11:51 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iotests: Skip read-only cases in 118 when run as root
2019-10-18 11:51 [PATCH] iotests: Skip read-only cases in 118 when run as root Kevin Wolf
@ 2019-10-18 12:59 ` Philippe Mathieu-Daudé
2019-10-18 14:27 ` Kevin Wolf
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-18 12:59 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iotests: Skip read-only cases in 118 when run as root
2019-10-18 12:59 ` Philippe Mathieu-Daudé
@ 2019-10-18 14:27 ` Kevin Wolf
2019-10-18 14:46 ` Philippe Mathieu-Daudé
2019-10-18 15:00 ` Max Reitz
0 siblings, 2 replies; 6+ messages in thread
From: Kevin Wolf @ 2019-10-18 14:27 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iotests: Skip read-only cases in 118 when run as root
2019-10-18 14:27 ` Kevin Wolf
@ 2019-10-18 14:46 ` Philippe Mathieu-Daudé
2019-10-18 15:00 ` Max Reitz
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-18 14:46 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iotests: Skip read-only cases in 118 when run as root
2019-10-18 14:27 ` Kevin Wolf
2019-10-18 14:46 ` Philippe Mathieu-Daudé
@ 2019-10-18 15:00 ` Max Reitz
2019-10-18 16:52 ` Kevin Wolf
1 sibling, 1 reply; 6+ messages in thread
From: Max Reitz @ 2019-10-18 15:00 UTC (permalink / raw)
To: Kevin Wolf, Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 1871 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iotests: Skip read-only cases in 118 when run as root
2019-10-18 15:00 ` Max Reitz
@ 2019-10-18 16:52 ` Kevin Wolf
0 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2019-10-18 16:52 UTC (permalink / raw)
To: Max Reitz; +Cc: Philippe Mathieu-Daudé, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2261 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-18 16:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-18 11:51 [PATCH] iotests: Skip read-only cases in 118 when run as root Kevin Wolf
2019-10-18 12:59 ` Philippe Mathieu-Daudé
2019-10-18 14:27 ` Kevin Wolf
2019-10-18 14:46 ` Philippe Mathieu-Daudé
2019-10-18 15:00 ` Max Reitz
2019-10-18 16:52 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.