libltp: add support for .needs_rofs for EROFS check
v3 cover libltp: add support for .needs_rofs for EROFS check
Some tests go through losetup, create, format and mount filesystems only to run tests for 'EROFS' return value from system calls. The tests end up being flaky depending on the tools available on the platform. e.g. mkfs.<filesystem> tool is required for mounting a device with filesystem.
If the test is only to check for EROFS, this can be achieved by simply doing a 'tmpfs' read-only mount in $tmpdir. So, this series adds a ".needs_rofs" flag to 'struct tst_test' that can be used to indicate that the test only needs a read-only filesystem mount ot test for 'EROFS' return value. If the flag is set, the library will first attempt to mount 'tmpfs' filesystem at tst_test->mntpoint and fallback to the original create, format and mount a loopback device if that fails.
Example of tests that can benefit from this are:
access04, mkdirat02, mknodat02, acct01, fchown04, mknod07 etc.
This also allows for these tests to successfully run on Android.
Sandeep Patil (2):
libltp: add support to mount tmpfs for EROFS testing syscalls/access04: use .needs_rofs flag for EROFS testing
include/tst_test.h | 1 + lib/tst_test.c | 36 ++++++++++++++++++++++++----- testcases/kernel/syscalls/access/access04.c | 3 +-- 3 files changed, 32 insertions(+), 8 deletions(-)
Signature · 1 line
2.14.1.690.gbb1197296e-goog
PATCH RESEND v3 1/2 libltp: add support to mount tmpfs for EROFS testing
Some tests go through losetup, create, format and mount filesystems only to test for 'EROFS' return value from system calls. The tests end up being flaky depending on the tools available on the platform. e.g. mkfs.<filesystem> tool is required for mounting a device with filesystem.
If the test is only to check for EROFS, this can be achieved by simply by the bind mounting (MS_BIND) for '/' on 'mntpoint' and then remount the same as 'read-only'.
Example of tests that can benefit from this are:
access04, mkdirat02, mknodat02, acct01, fchown04, mknod07 etc.
This also allows for these tests to successfully run on Android.
---v2->v3
------
- Added Tested-by from Jan Stancek
v1->v2
------
- Changed the interface to be at a higher level, i.e. instead of making tests specify that they need a read-only tmpfs mount, we do that automatically in the library as a preference and fallback to the old way if that doesn't succeed.
- The read-only bind mount of "/" option was tried and it worked just fine with an Android system, but broke my ubuntu instance badly (I lost root). I could never find out why, since hte patch worked fine on Android and it correctly did the bind mount. However, I figured if there is any chance of possible breakage, we should simply avoid that path for now.
include/tst_test.h | 1 + lib/tst_test.c | 36 ++++++++++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 6 deletions(-)
include/tst_test.h +1 -0
@@ -124,6 +124,7 @@ struct tst_test { int needs_checkpoints:1; int format_device:1; int mount_device:1; + int needs_rofs:1;/* Minimal device size in megabytes */ unsigned int dev_min_size;
lib/tst_test.c +30 -6
@@ -21,6 +21,7 @@#include <string.h> #include <stdlib.h> #include <errno.h> +#include <sys/mount.h> #include <sys/types.h> #include <sys/wait.h> #include <sys/time.h>
@@ -41,7 +42,7 @@ struct tst_test *tst_test;static int iterations =1 ;static float duration =-1 ;static pid_t main_pid, lib_pid; -static int device_mounted; +static int mntpoint_active;struct results {int passed;
@@ -701,7 +702,32 @@ static void do_setup(int argc, char *argv[])if (needs_tmpdir () && !tst_tmpdir_created ())tst_tmpdir (); -if (tst_test->needs_device) { +if (tst_test->mntpoint) +SAFE_MKDIR (tst_test->mntpoint,0777 ); + +if (tst_test->needs_rofs) { +if (!tst_test->mntpoint) { +tst_brk (TBROK , +"tst_test->mntpoint must be set!" ); + } + +/* If we failed to do read-only bind mount for '/'. Fallback to +* using a device with empty read-only filesystem. +*/ +if (mount (NULL , tst_test->mntpoint,"tmpfs" ,MS_RDONLY ,NULL )) { +tst_res (TINFO ,"Cannot mount tmpfs read-only at %s, " +"setting up a device instead\n" , + tst_test->mntpoint); + tst_test->mount_device =1 ; + tst_test->needs_device =1 ; + tst_test->format_device =1 ; + tst_test->mnt_flags =MS_RDONLY ; + }else { + mntpoint_active =1 ; + } + } + +if (tst_test->needs_device && !mntpoint_active) { tdev.dev =tst_acquire_device_ (NULL , tst_test->dev_min_size);if (!tdev.dev)
@@ -721,16 +747,14 @@ static void do_setup(int argc, char *argv[]) }if (tst_test->mount_device) { -if (!tst_test->mntpoint) {tst_brk (TBROK ,"tst_test->mntpoint must be set!" ); } -SAFE_MKDIR (tst_test->mntpoint,0777 );SAFE_MOUNT (tdev.dev, tst_test->mntpoint, tdev.fs_type, tst_test->mnt_flags, tst_test->mnt_data); - device_mounted =1 ; + mntpoint_active =1 ; } }
@@ -751,7 +775,7 @@ static void do_test_setup(void)static void do_cleanup (void ) { -if (device_mounted) +if (mntpoint_active)tst_umount (tst_test->mntpoint);if (tst_test->needs_device && tdev.dev)
Signature · 1 line
2.14.1.690.gbb1197296e-goog
PATCH RESEND v3 1/2 libltp: add support to mount tmpfs for EROFS testing
Hi!
Quoted 13 lines
lib/tst_test.c +1
-0@@ -21,6 +21,7 @@#include <string.h> #include <stdlib.h> #include <errno.h> +#include <sys/mount.h> #include <sys/types.h> #include <sys/wait.h> #include <sys/time.h>
lib/tst_test.c struct tst_test *tst_test; · line 45
Quoted 5 lines
lib/tst_test.c +1
-1@@ -41,7 +42,7 @@ struct tst_test *tst_test;static int iterations =1 ;static float duration =-1 ;static pid_t main_pid, lib_pid; -static int device_mounted; +static int mntpoint_active;
A bit better name would be 'mntpoint_mounted' but that is very minor.
Quoted 5 lines
lib/tst_test.c +0
-0struct results {int passed;@@ -701,7 +702,32 @@ static void do_setup(int argc, char *argv[])if (needs_tmpdir () && !tst_tmpdir_created ())tst_tmpdir ();
lib/tst_test.c static void do_setup(int argc, char *argv[]) · line 712
Quoted 10 lines
lib/tst_test.c +8
-1-if (tst_test->needs_device) { +if (tst_test->mntpoint) +SAFE_MKDIR (tst_test->mntpoint,0777 ); + +if (tst_test->needs_rofs) { +if (!tst_test->mntpoint) { +tst_brk (TBROK , +"tst_test->mntpoint must be set!" ); + }
We may as well move this check from both branches to the top to avoid duplication, i.e. add:
if ((tst_test->needs_rofs || tst_test->mount_device) &&
!tst_test->mntpoint) {
tst_brk(TBROK, "tst_test->mntpoint must be set!");
}lib/tst_test.c static void do_setup(int argc, char *argv[]) · line 718
Quoted 5 lines
lib/tst_test.c +5
-0+/* If we failed to do read-only bind mount for '/'. Fallback to +* using a device with empty read-only filesystem. +*/ +if (mount (NULL , tst_test->mntpoint,"tmpfs" ,MS_RDONLY ,NULL )) { + tst_res(TINFO, "Cannot mount tmpfs read-only at %s, "
^
Maybe we should add | TERRNO here as well so that user gets a hint why this mount() had failed.
Quoted 34 lines
lib/tst_test.c +13
-3+"setting up a device instead\n" , + tst_test->mntpoint); + tst_test->mount_device =1 ; + tst_test->needs_device =1 ; + tst_test->format_device =1 ; + tst_test->mnt_flags =MS_RDONLY ; + }else { + mntpoint_active =1 ; + } + } + +if (tst_test->needs_device && !mntpoint_active) { tdev.dev =tst_acquire_device_ (NULL , tst_test->dev_min_size);if (!tdev.dev)@@ -721,16 +747,14 @@ static void do_setup(int argc, char *argv[]) }if (tst_test->mount_device) { -if (!tst_test->mntpoint) {tst_brk (TBROK ,"tst_test->mntpoint must be set!" ); } -SAFE_MKDIR (tst_test->mntpoint,0777 );SAFE_MOUNT (tdev.dev, tst_test->mntpoint, tdev.fs_type, tst_test->mnt_flags, tst_test->mnt_data); - device_mounted =1 ; + mntpoint_active =1 ; } }
lib/tst_test.c static void do_test_setup(void) · line 781
Quoted 8 lines
lib/tst_test.c +1
-1@@ -751,7 +775,7 @@ static void do_test_setup(void)static void do_cleanup (void ) { -if (device_mounted) +if (mntpoint_active)tst_umount (tst_test->mntpoint);if (tst_test->needs_device && tdev.dev)
Other than the minor nits it looks good to me.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH RESEND v3 1/2 libltp: add support to mount tmpfs for EROFS testing
Quoted 96 lines · Cyril Hrubis
Hi!
Quoted 13 lines
lib/tst_test.c +1
-0@@ -21,6 +21,7 @@#include <string.h> #include <stdlib.h> #include <errno.h> +#include <sys/mount.h> #include <sys/types.h> #include <sys/wait.h> #include <sys/time.h> lib/tst_test.c struct tst_test *tst_test; · line 45
Quoted 5 lines
lib/tst_test.c +1
-1@@ -41,7 +42,7 @@ struct tst_test *tst_test;static int iterations =1 ;static float duration =-1 ;static pid_t main_pid, lib_pid; -static int device_mounted; +static int mntpoint_active;A bit better name would be '
mntpoint_mounted' but that is very minor.Quoted 5 lines
lib/tst_test.c +0
-0struct results {int passed;@@ -701,7 +702,32 @@ static void do_setup(int argc, char *argv[])if (needs_tmpdir () && !tst_tmpdir_created ())tst_tmpdir ();lib/tst_test.c static void do_setup(int argc, char *argv[]) · line 712
Quoted 10 lines
lib/tst_test.c +8
-1-if (tst_test->needs_device) { +if (tst_test->mntpoint) +SAFE_MKDIR (tst_test->mntpoint,0777 ); + +if (tst_test->needs_rofs) { +if (!tst_test->mntpoint) { +tst_brk (TBROK , +"tst_test->mntpoint must be set!" ); + }We may as well move this check from both branches to the top to avoid duplication, i.e. add:
if ((tst_test->needs_rofs || tst_test->mount_device) && !tst_test->mntpoint) { tst_brk(TBROK, "tst_test->mntpoint must be set!"); }lib/tst_test.c static void do_setup(int argc, char *argv[]) · line 718
Quoted 5 lines
lib/tst_test.c +5
-0+/* If we failed to do read-only bind mount for '/'. Fallback to +* using a device with empty read-only filesystem. +*/ +if (mount (NULL , tst_test->mntpoint,"tmpfs" ,MS_RDONLY ,NULL )) { + tst_res(TINFO, "Cannot mount tmpfs read-only at %s, "^
Maybe we should add | TERRNO here as well so that user gets a hint why thismount()had failed.Quoted 41 lines
lib/tst_test.c +14
-4+"setting up a device instead\n" , + tst_test->mntpoint); + tst_test->mount_device =1 ; + tst_test->needs_device =1 ; + tst_test->format_device =1 ; + tst_test->mnt_flags =MS_RDONLY ; + }else { + mntpoint_active =1 ; + } + } + +if (tst_test->needs_device && !mntpoint_active) { tdev.dev =tst_acquire_device_ (NULL , tst_test->dev_min_size);if (!tdev.dev)@@ -721,16 +747,14 @@ static void do_setup(int argc, char *argv[]) }if (tst_test->mount_device) { -if (!tst_test->mntpoint) {tst_brk (TBROK ,"tst_test->mntpoint must be set!" ); } -SAFE_MKDIR (tst_test->mntpoint,0777 );SAFE_MOUNT (tdev.dev, tst_test->mntpoint, tdev.fs_type, tst_test->mnt_flags, tst_test->mnt_data); - device_mounted =1 ; + mntpoint_active =1 ; } }@@ -751,7 +775,7 @@ static void do_test_setup(void)static void do_cleanup (void ) { -if (device_mounted) +if (mntpoint_active)tst_umount (tst_test->mntpoint);
Quoted 3 lines
lib/tst_test.c static void do_test_setup(void) · line 781
Quoted 1 line
lib/tst_test.c +0
-0if (tst_test->needs_device && tdev.dev)Other than the minor nits it looks good to me.
ack, I'll spin v4 with the fixes so they are ready in time for release.
- ssp
PATCH RESEND v3 2/2 syscalls/access04: use .needs_rofs flag for EROFS testing
In reply to Sandeep PatilPrefer to use the read-only bind mount of '/' over create, format and mount a filesystem image for testing 'EROFS' return value from access()
--- testcases/kernel/syscalls/access/access04.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
testcases/kernel/syscalls/access/access04.c +1 -2
@@ -125,9 +125,8 @@ static struct tst_test test = { .needs_tmpdir = 1, .needs_root = 1, .forks_child = 1, - .mount_device = 1, + .needs_rofs = 1, .mntpoint = MNT_POINT, - .mnt_flags = MS_RDONLY, .setup = setup, .test = verify_access, };
Signature · 1 line
2.14.1.690.gbb1197296e-goog
PATCH RESEND v3 2/2 syscalls/access04: use .needs_rofs flag for EROFS testing
Hi!
Quoted 14 lines
Prefer to use the read-only bind mount of '/' over create, format and mount a filesystem image for testing 'EROFS' return value from
access()Signed-off-by Sandeep Patil <[email protected]>--- testcases/kernel/syscalls/access/access04.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)testcases/kernel/syscalls/access/access04.c +0
-0
testcases/kernel/syscalls/access/access04.c static struct tst_test test = { · line 132
Quoted 10 lines
testcases/kernel/syscalls/access/access04.c +1
-2@@ -125,9 +125,8 @@ static struct tst_test test = { .needs_tmpdir = 1, .needs_root = 1, .forks_child = 1, - .mount_device = 1, + .needs_rofs = 1, .mntpoint = MNT_POINT, - .mnt_flags = MS_RDONLY, .setup = setup, .test = verify_access, };
This is obviously correct, how many testcases can use this interface?
Lookint into the git tree, there is at least creat06 that could be converted in the very same way, then there is a quite a bit of testcases that will need to be converted to the new test library first.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH RESEND v3 2/2 syscalls/access04: use .needs_rofs flag for EROFS testing
Quoted 19 lines · Cyril Hrubis
Hi!
Quoted 18 lines
Prefer to use the read-only bind mount of '/' over create, format and mount a filesystem image for testing 'EROFS' return value from
access()Signed-off-by Sandeep Patil <[email protected]>--- testcases/kernel/syscalls/access/access04.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)testcases/kernel/syscalls/access/access04.c +0
-1@@ -125,9 +125,8 @@ static struct tst_test test = { .needs_tmpdir = 1, .needs_root = 1, .forks_child = 1, - .mount_device = 1,
Quoted 8 lines
testcases/kernel/syscalls/access/access04.c static struct tst_test test = { · line 132
Quoted 6 lines
testcases/kernel/syscalls/access/access04.c +1
-1+ .needs_rofs = 1, .mntpoint = MNT_POINT, - .mnt_flags = MS_RDONLY, .setup = setup, .test = verify_access, };This is obviously correct, how many testcases can use this interface?
I had this in the cover letter, but here's the preliminary list of tests that will end up using .need_rofs.
access04, mkdirat02, mknodat02, acct01, fchown04, mknod07 etc.
- ssp
Quoted 4 lines
Lookint into the git tree, there is at least creat06 that could be converted in the very same way, then there is a quite a bit of testcases that will need to be converted to the new test library first.
Ack, I have a list of tests to be converted to new library too. Some of them also will make those tests run in Android environment, so I shall follow up with the conversion patches post release too.
- ssp
Quoted 4 lines
Signature · 3 lines
-- Cyril Hrubis [email protected]