[LTP] [PATCH 1/1] lib: Fix fs support detection for non-root

[LTP] [PATCH 1/1] lib: Fix fs support detection for non-root

[Petr Vorel]

grep /proc/filesystems to find kernel support.
But consider only 0 (filesystem found) or 1 (not found),
ignore other results (e.g. 2: /proc/filesystems not available or
no permissions) and fallback to old solution (calling mount()).

Warn when mount() fails due no permission (EPERM).

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_supported_fs_types.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index 00ede549d..66307e09e 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -52,10 +52,29 @@ static int has_kernel_support(const char *fs_type, int flags)
 	char buf[128];
 	int ret;
 
+	const char * const argv[] = { "grep", "-q", "-F", "-w", fs_type, "/proc/filesystems", NULL };
+	ret = tst_cmd_(NULL, argv, "/dev/null", "/dev/null", TST_CMD_PASS_RETVAL);
+
+	if (ret == 0) {
+		tst_res(TINFO, "Kernel supports %s", fs_type);
+		return 1;
+	}
+
+	if (ret == 1) {
+		tst_res(TINFO, "Filesystem %s is not supported", fs_type);
+		return 0;
+	}
+
 	if (!tmpdir)
 		tmpdir = "/tmp";
 
 	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
+
+	if (errno == EPERM) {
+		tst_res(TWARN, "No permission to detect support for %s", fs_type);
+		return 1;
+	}
+
 	if (errno != ENODEV) {
 		tst_res(TINFO, "Kernel supports %s", fs_type);
 		return 1;
-- 
2.30.0

[LTP] [PATCH 1/1] lib: Fix fs support detection for non-root

[Petr Vorel]

Hi,

...
> +++ b/lib/tst_supported_fs_types.c
> @@ -52,10 +52,29 @@ static int has_kernel_support(const char *fs_type, int flags)
>  	char buf[128];
>  	int ret;

> +	const char * const argv[] = { "grep", "-q", "-F", "-w", fs_type, "/proc/filesystems", NULL };
"-F" is probably useless.

Kind regards,
Petr

[LTP] [PATCH 1/1] lib: Fix fs support detection for non-root

[Cyril Hrubis]

Hi!
> grep /proc/filesystems to find kernel support.
> But consider only 0 (filesystem found) or 1 (not found),
> ignore other results (e.g. 2: /proc/filesystems not available or
> no permissions) and fallback to old solution (calling mount()).

Why is this needed?

Also this breaks FUSE detection.

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  lib/tst_supported_fs_types.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> index 00ede549d..66307e09e 100644
> --- a/lib/tst_supported_fs_types.c
> +++ b/lib/tst_supported_fs_types.c
> @@ -52,10 +52,29 @@ static int has_kernel_support(const char *fs_type, int flags)
>  	char buf[128];
>  	int ret;
>  
> +	const char * const argv[] = { "grep", "-q", "-F", "-w", fs_type, "/proc/filesystems", NULL };
> +	ret = tst_cmd_(NULL, argv, "/dev/null", "/dev/null", TST_CMD_PASS_RETVAL);

Can't we just open the file and use fgets() in a loop?

Why do we have to execute a grep binary for something like this?

> +	if (ret == 0) {
> +		tst_res(TINFO, "Kernel supports %s", fs_type);
> +		return 1;
> +	}
> +
> +	if (ret == 1) {
> +		tst_res(TINFO, "Filesystem %s is not supported", fs_type);
> +		return 0;
> +	}
> +
>  	if (!tmpdir)
>  		tmpdir = "/tmp";
>  
>  	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
> +
> +	if (errno == EPERM) {
> +		tst_res(TWARN, "No permission to detect support for %s", fs_type);
> +		return 1;

Maybe we can try to read /proc/filesystems here as a fallback?

Again why do we need this at all?

> +	}
> +
>  	if (errno != ENODEV) {
>  		tst_res(TINFO, "Kernel supports %s", fs_type);
>  		return 1;
> -- 
> 2.30.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/1] lib: Fix fs support detection for non-root

[Petr Vorel]

Hi Cyril,

> Hi!
> > grep /proc/filesystems to find kernel support.
> > But consider only 0 (filesystem found) or 1 (not found),
> > ignore other results (e.g. 2: /proc/filesystems not available or
> > no permissions) and fallback to old solution (calling mount()).

> Why is this needed?
generate_lvm_runfile.sh correctly requires TST_NEEDS_ROOT=1, only for this.
zram0{12}.sh (or maybe better zram_lib.sh, which they use) incorrectly don't
require root, they need it for calling modprobe zram and generally for zram use,
but also for this. I'll fix this in my patchset fixing zram and we can ignore
generate_lvm_runfile.sh.

=> It might not be needed. But generally some test in the future (C or shell)
may want to know filesystem support for other reason which does not require
root. Thus we can drop /proc/filesystems, but at least warning would be nice to
have (although that implementation only prints warning and returns 0, thus
warning will be lost for shell).

Also .all_filesystems in C API have .needs_root = 1. Currently copy_file_range01,
preadv03, pwritev03.
They get obvious hint from tst_acquire_loop_device():
tst_device.c:100: TINFO: Not allowed to open /dev/loop-control. Are you root?: EACCES (13)
=> they need to have .needs_root = 1.
And I wonder if we should have check expecting .needs_root for certain flags
(.all_filesystems, .needs_device, .mntpoint at least).

> Also this breaks FUSE detection.
Sorry for this. Agree with all your suggestions and can send v2 if we don't decide wontfix.

Kind regards,
Petr