| Message ID | 20191018115127.2671-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Skip read-only cases in 118 when run as root | expand |
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 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>
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
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)
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(+)