[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Zhao Gongyi]

In many Embedded system, we need add tmpfs wo filesystem whitelist since
there is only tmpfs can be used to test.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>

---------------
v1->v2:
1)create a unique temporary directory under TMPDIR and use it as a mount
point in the has_kernel_support() function.
2)add tst_umount after mount.
---------------

---
 lib/tst_supported_fs_types.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index 00ede549d..964efb659 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -22,6 +22,7 @@ static const char *const fs_type_whitelist[] = {
 	"vfat",
 	"exfat",
 	"ntfs",
+	"tmpfs",
 	NULL
 };

@@ -34,6 +35,10 @@ static int has_mkfs(const char *fs_type)

 	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);

+	if (strstr(buf, "mkfs.tmpfs")) {
+		return 1;
+	}
+
 	ret = tst_system(buf);

 	if (WEXITSTATUS(ret) == 127) {
@@ -50,17 +55,30 @@ static int has_kernel_support(const char *fs_type, int flags)
 	static int fuse_supported = -1;
 	const char *tmpdir = getenv("TMPDIR");
 	char buf[128];
+	char template[PATH_MAX];
 	int ret;

 	if (!tmpdir)
 		tmpdir = "/tmp";

-	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
-	if (errno != ENODEV) {
+	sprintf(template, "%s/mountXXXXXX", tmpdir);
+	if (mkdtemp(template) == NULL) {
+		tst_res(TWARN | TERRNO , "%s: mkdtemp(%s) failed", __func__, template);
+	}
+
+	ret = mount("/dev/zero", template, fs_type, 0, NULL);
+	if ((ret && errno != ENODEV) || !ret) {
+		if (!ret)
+			tst_umount(template);
 		tst_res(TINFO, "Kernel supports %s", fs_type);
+		if (rmdir(template) == -1)
+			tst_res(TWARN | TERRNO, "rmdir %s failed", template);
 		return 1;
 	}

+	if (rmdir(template) == -1)
+		tst_res(TWARN | TERRNO, "rmdir %s failed", template);
+
 	/* Is FUSE supported by kernel? */
 	if (fuse_supported == -1) {
 		ret = open("/dev/fuse", O_RDWR);
--
2.17.1

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Cyril Hrubis]

Hi!
> @@ -34,6 +35,10 @@ static int has_mkfs(const char *fs_type)
> 
>  	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);
> 
> +	if (strstr(buf, "mkfs.tmpfs")) {
> +		return 1;
> +	}

The curly braces around single line statement are useless here.

Also it would be cleaner if we checked before the sprintf and against
the fs_type instead with if (!strcmp(fs_type, "tmpfs")).

We may also print a TINFO message something along the lines:

	tst_res(TINFO, "mkfs is not needed for tmpfs");

>  	ret = tst_system(buf);
> 
>  	if (WEXITSTATUS(ret) == 127) {
> @@ -50,17 +55,30 @@ static int has_kernel_support(const char *fs_type, int flags)
>  	static int fuse_supported = -1;
>  	const char *tmpdir = getenv("TMPDIR");
>  	char buf[128];
> +	char template[PATH_MAX];
>  	int ret;
> 
>  	if (!tmpdir)
>  		tmpdir = "/tmp";
> 
> -	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
> -	if (errno != ENODEV) {
> +	sprintf(template, "%s/mountXXXXXX", tmpdir);

We should at least use snprintf() with sizeof(template) in order not to
overrun the buffer. Or we can use asprintf() instead and free the buffer
later on.

> +	if (mkdtemp(template) == NULL) {
> +		tst_res(TWARN | TERRNO , "%s: mkdtemp(%s) failed", __func__, template);
> +	}

I guess that we can tst_brk(TBROK, ""); here if mkdtemp() failed,
continuing here we would pass non-existing directory thte mount()
syscall below.

Also please do not include the __func__ in the message here.

> +	ret = mount("/dev/zero", template, fs_type, 0, NULL);
> +	if ((ret && errno != ENODEV) || !ret) {
> +		if (!ret)
> +			tst_umount(template);
>  		tst_res(TINFO, "Kernel supports %s", fs_type);
> +		if (rmdir(template) == -1)
> +			tst_res(TWARN | TERRNO, "rmdir %s failed", template);

We do have SAFE_RMDIR() please use that.

>  		return 1;
>  	}
> 
> +	if (rmdir(template) == -1)
> +		tst_res(TWARN | TERRNO, "rmdir %s failed", template);

Here as well.

>  	/* Is FUSE supported by kernel? */
>  	if (fuse_supported == -1) {
>  		ret = open("/dev/fuse", O_RDWR);
> --
> 2.17.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Cyril Hrubis]

Hi!
Finally pushed, along with two patches that skip sync and O_DIRECT tests
on tmpfs, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Petr Vorel]

Hi Gongyi,

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Thanks!

Kind regards,
Petr

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Cyril Hrubis]

Hi!
> In many embedded system, we need add tmpfs to filesystem whitelist since
> there is only tmpfs can be used to test.
> 
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> 
> ---------------
> v2->v3:
> 1)skipping mkfs for tmpfs gracefully.
> 2)replace sprintf with snprintf.
> 3)remove __func__ in the message
> 4)replace rmdir with SAFE_RMDIR
> ---------------

Minor nits:

This changelog part should be below the "---" line, since that part gets
cut out automatically and the patch subject should have v3 in it's name
(git format-patch -v3 ...).

As for the rest of the patch, it's nearly perfect, I will push it with
minor changes but we need to apply my patchset[1] first so that we can
skip a few tests on tmpfs.

[1] http://patchwork.ozlabs.org/project/ltp/list/?series=233180

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[zhaogongyi]

Hi Cyril,

I have resubmit the patch according to your review.

Thanks so much!

Best Regards,
Gongyi

-----------------------------------------------------------------------------------------
> 
> Hi!
> > @@ -34,6 +35,10 @@ static int has_mkfs(const char *fs_type)
> >
> >  	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);
> >
> > +	if (strstr(buf, "mkfs.tmpfs")) {
> > +		return 1;
> > +	}
> 
> The curly braces around single line statement are useless here.
> 
> Also it would be cleaner if we checked before the sprintf and against the
> fs_type instead with if (!strcmp(fs_type, "tmpfs")).
> 
> We may also print a TINFO message something along the lines:
> 
> 	tst_res(TINFO, "mkfs is not needed for tmpfs");
> 
> >  	ret = tst_system(buf);
> >
> >  	if (WEXITSTATUS(ret) == 127) {
> > @@ -50,17 +55,30 @@ static int has_kernel_support(const char
> *fs_type, int flags)
> >  	static int fuse_supported = -1;
> >  	const char *tmpdir = getenv("TMPDIR");
> >  	char buf[128];
> > +	char template[PATH_MAX];
> >  	int ret;
> >
> >  	if (!tmpdir)
> >  		tmpdir = "/tmp";
> >
> > -	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
> > -	if (errno != ENODEV) {
> > +	sprintf(template, "%s/mountXXXXXX", tmpdir);
> 
> We should at least use snprintf() with sizeof(template) in order not to
> overrun the buffer. Or we can use asprintf() instead and free the buffer
> later on.
> 
> > +	if (mkdtemp(template) == NULL) {
> > +		tst_res(TWARN | TERRNO , "%s: mkdtemp(%s) failed", __func__,
> template);
> > +	}
> 
> I guess that we can tst_brk(TBROK, ""); here if mkdtemp() failed,
> continuing here we would pass non-existing directory thte mount() syscall
> below.
> 
> Also please do not include the __func__ in the message here.
> 
> > +	ret = mount("/dev/zero", template, fs_type, 0, NULL);
> > +	if ((ret && errno != ENODEV) || !ret) {
> > +		if (!ret)
> > +			tst_umount(template);
> >  		tst_res(TINFO, "Kernel supports %s", fs_type);
> > +		if (rmdir(template) == -1)
> > +			tst_res(TWARN | TERRNO, "rmdir %s failed", template);
> 
> We do have SAFE_RMDIR() please use that.
> 
> >  		return 1;
> >  	}
> >
> > +	if (rmdir(template) == -1)
> > +		tst_res(TWARN | TERRNO, "rmdir %s failed", template);
> 
> Here as well.
> 
> >  	/* Is FUSE supported by kernel? */
> >  	if (fuse_supported == -1) {
> >  		ret = open("/dev/fuse", O_RDWR);
> > --
> > 2.17.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Zhao Gongyi]

In many embedded system, we need add tmpfs to filesystem whitelist since
there is only tmpfs can be used to test.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>

---------------
v2->v3:
1)skipping mkfs for tmpfs gracefully.
2)replace sprintf with snprintf.
3)remove __func__ in the message
4)replace rmdir with SAFE_RMDIR
---------------
---
 lib/tst_supported_fs_types.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index 00ede549d..b5e3cbe85 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -22,6 +22,7 @@ static const char *const fs_type_whitelist[] = {
 	"vfat",
 	"exfat",
 	"ntfs",
+	"tmpfs",
 	NULL
 };

@@ -32,6 +33,11 @@ static int has_mkfs(const char *fs_type)
 	char buf[128];
 	int ret;

+	if (strstr(fs_type, "tmpfs")) {
+		tst_res(TINFO, "mkfs is not needed for tmpfs");
+		return 1;
+	}
+
 	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);

 	ret = tst_system(buf);
@@ -50,17 +56,28 @@ static int has_kernel_support(const char *fs_type, int flags)
 	static int fuse_supported = -1;
 	const char *tmpdir = getenv("TMPDIR");
 	char buf[128];
+	char template[PATH_MAX];
 	int ret;

 	if (!tmpdir)
 		tmpdir = "/tmp";

-	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
-	if (errno != ENODEV) {
+	snprintf(template, sizeof(template), "%s/mountXXXXXX", tmpdir);
+	if (mkdtemp(template) == NULL) {
+		tst_brk(TBROK | TERRNO , "mkdtemp(%s) failed", template);
+	}
+
+	ret = mount("/dev/zero", template, fs_type, 0, NULL);
+	if ((ret && errno != ENODEV) || !ret) {
+		if (!ret)
+			tst_umount(template);
 		tst_res(TINFO, "Kernel supports %s", fs_type);
+		SAFE_RMDIR(template);
 		return 1;
 	}

+	SAFE_RMDIR(template);
+
 	/* Is FUSE supported by kernel? */
 	if (fuse_supported == -1) {
 		ret = open("/dev/fuse", O_RDWR);
--
2.17.1

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[zhaogongyi]

Hi Cyril,

I have resubmit the patch according to your review.

Thanks so much!

> 
> Hi!
> > > But there is another problem there, since the code still mounts
> > > tmpfs on tmpdir for a short while, which is temporary directory used
> > > by all LTP tests, which may potentially break tests that runs in parallel.
> >
> > 	In general, when all_filesystems has been set to 1,  the
> needs_tmpdir would be set to 1 and the mntpoint must has been set to
> some path, so the test run in /tmp/tmpxxxxxx/mntpoint and
> > 	other LTP tests would run in another tmpdir. So it has no problem for
> running in parallel.
> >
> > 	I don't know if I understand it right.
> 
> The has_kernel_support() uses TMPDIR as the mount point, which is the
> parent directory for all LTP tests, moreover it often points to just "/tmp". If
> you mount anything over that directory, even for a short while, the whole
> system will get different and empty "/tmp" which will possibly break many
> things.
> 
> We may get over this with using "." instead, in a case that testcase has
> created temporary directory and changed the PWD to it, but it's not that
> simple either, since tst_test.c is not the only place that calls
> has_kernel_support().
> 
> We have testcases/lib/tst_supported_fs.c helper as well that is used by
> shell tests. This is a standalone binary that does not create a temporary
> directory and should work even without TMPDIR being set, which is the
> reason we have the if (!tmpdir) tmpdir = "/tmp"; check in the
> has_kernel_support() function to begin with.
> 
> So all in all I guess that safest option would be to create a unique
> temporary directory under TMPDIR and use it as a mount point in the
> has_kernel_support() function.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Cyril Hrubis]

Hi!
> > But there is another problem there, since the code still mounts tmpfs on
> > tmpdir for a short while, which is temporary directory used by all LTP tests,
> > which may potentially break tests that runs in parallel.
> 
> 	In general, when all_filesystems has been set to 1,  the needs_tmpdir would be set to 1 and the mntpoint must has been set to some path, so the test run in /tmp/tmpxxxxxx/mntpoint and
> 	other LTP tests would run in another tmpdir. So it has no problem for running in parallel.
> 
> 	I don't know if I understand it right.

The has_kernel_support() uses TMPDIR as the mount point, which is the
parent directory for all LTP tests, moreover it often points to just
"/tmp". If you mount anything over that directory, even for a short
while, the whole system will get different and empty "/tmp" which will
possibly break many things.

We may get over this with using "." instead, in a case that testcase has
created temporary directory and changed the PWD to it, but it's not that
simple either, since tst_test.c is not the only place that calls
has_kernel_support().

We have testcases/lib/tst_supported_fs.c helper as well that is used by
shell tests. This is a standalone binary that does not create a
temporary directory and should work even without TMPDIR being set, which
is the reason we have the if (!tmpdir) tmpdir = "/tmp"; check in the
has_kernel_support() function to begin with.

So all in all I guess that safest option would be to create a unique
temporary directory under TMPDIR and use it as a mount point in the
has_kernel_support() function.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[zhaogongyi]

Hi Cyril,

> 
> But there is another problem there, since the code still mounts tmpfs on
> tmpdir for a short while, which is temporary directory used by all LTP tests,
> which may potentially break tests that runs in parallel.

	In general, when all_filesystems has been set to 1,  the needs_tmpdir would be set to 1 and the mntpoint must has been set to some path, so the test run in /tmp/tmpxxxxxx/mntpoint and
	other LTP tests would run in another tmpdir. So it has no problem for running in parallel.

	I don't know if I understand it right.
> 
> So we will have to prepare a temporary directory with mkdtemp() under
> the tmpdir as well and pass that to the mount() syscall instead.
>

Thanks so much!

-------------------------------------------------------------------------------------------------------
> 
> Hi!
> > diff --git a/lib/tst_supported_fs_types.c
> > b/lib/tst_supported_fs_types.c index 00ede549d..696b6731e 100644
> > --- a/lib/tst_supported_fs_types.c
> > +++ b/lib/tst_supported_fs_types.c
> > @@ -22,6 +22,7 @@ static const char *const fs_type_whitelist[] = {
> >  	"vfat",
> >  	"exfat",
> >  	"ntfs",
> > +	"tmpfs",
> >  	NULL
> >  };
> >
> > @@ -34,6 +35,10 @@ static int has_mkfs(const char *fs_type)
> >
> >  	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);
> >
> > +	if (strstr(buf, "mkfs.tmpfs")) {
> > +		return 1;
> > +	}
> > +
> >  	ret = tst_system(buf);
> >
> >  	if (WEXITSTATUS(ret) == 127) {
> > @@ -55,8 +60,8 @@ static int has_kernel_support(const char *fs_type,
> int flags)
> >  	if (!tmpdir)
> >  		tmpdir = "/tmp";
> >
> > -	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
> > -	if (errno != ENODEV) {
> > +	ret = mount("/dev/zero", tmpdir, fs_type, 0, NULL);
> 
> The manual page explicitly says that errno is set to ENODEV if filesystem is
> not supported by kernel. So the check for errno should stay, since the
> statement above will fail to mount any real filesystem since we pass
> "/dev/zero" instead of valid filesystem image there.
> 
> I.e. if we pass a real filesystem there it will either fail with EINVAL (since
> /dev/zero does not have a valid superblock) or ENODEV if there is no
> kernel driver for the filesystem.
> 
> > +	if (!ret) {
> 
> I guess that tmpfs succeeds to mount there. So I guess that we should
> change the condition to:
> 
> 	if ((ret && errno != ENODEV) || !ret) {
> 		if (!ret)
> 			tst_umount(tmpdir);
> 
> 		tst_res(TINFO, "Kernel supports %s", fs_type);
> 		return 1;
> 	}
> 
> 
> But there is another problem there, since the code still mounts tmpfs on
> tmpdir for a short while, which is temporary directory used by all LTP tests,
> which may potentially break tests that runs in parallel.
> 
> So we will have to prepare a temporary directory with mkdtemp() under
> the tmpdir as well and pass that to the mount() syscall instead.
> 
> --
> Cyril Hrubis
> chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Cyril Hrubis]

Hi!
> diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
> index 00ede549d..696b6731e 100644
> --- a/lib/tst_supported_fs_types.c
> +++ b/lib/tst_supported_fs_types.c
> @@ -22,6 +22,7 @@ static const char *const fs_type_whitelist[] = {
>  	"vfat",
>  	"exfat",
>  	"ntfs",
> +	"tmpfs",
>  	NULL
>  };
> 
> @@ -34,6 +35,10 @@ static int has_mkfs(const char *fs_type)
> 
>  	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);
> 
> +	if (strstr(buf, "mkfs.tmpfs")) {
> +		return 1;
> +	}
> +
>  	ret = tst_system(buf);
> 
>  	if (WEXITSTATUS(ret) == 127) {
> @@ -55,8 +60,8 @@ static int has_kernel_support(const char *fs_type, int flags)
>  	if (!tmpdir)
>  		tmpdir = "/tmp";
> 
> -	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
> -	if (errno != ENODEV) {
> +	ret = mount("/dev/zero", tmpdir, fs_type, 0, NULL);

The manual page explicitly says that errno is set to ENODEV if
filesystem is not supported by kernel. So the check for errno should
stay, since the statement above will fail to mount any real filesystem
since we pass "/dev/zero" instead of valid filesystem image there.

I.e. if we pass a real filesystem there it will either fail with EINVAL
(since /dev/zero does not have a valid superblock) or ENODEV if there is
no kernel driver for the filesystem.

> +	if (!ret) {

I guess that tmpfs succeeds to mount there. So I guess that we should
change the condition to:

	if ((ret && errno != ENODEV) || !ret) {
		if (!ret)
			tst_umount(tmpdir);

		tst_res(TINFO, "Kernel supports %s", fs_type);
		return 1;
	}


But there is another problem there, since the code still mounts tmpfs on
tmpdir for a short while, which is temporary directory used by all LTP
tests, which may potentially break tests that runs in parallel.

So we will have to prepare a temporary directory with mkdtemp() under
the tmpdir as well and pass that to the mount() syscall instead.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist

[Zhao Gongyi]

In many Embedded system, we need add tmpfs wo filesystem whitelist since
there is only tmpfs can be used to test.

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 lib/tst_supported_fs_types.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/lib/tst_supported_fs_types.c b/lib/tst_supported_fs_types.c
index 00ede549d..696b6731e 100644
--- a/lib/tst_supported_fs_types.c
+++ b/lib/tst_supported_fs_types.c
@@ -22,6 +22,7 @@ static const char *const fs_type_whitelist[] = {
 	"vfat",
 	"exfat",
 	"ntfs",
+	"tmpfs",
 	NULL
 };

@@ -34,6 +35,10 @@ static int has_mkfs(const char *fs_type)

 	sprintf(buf, "mkfs.%s >/dev/null 2>&1", fs_type);

+	if (strstr(buf, "mkfs.tmpfs")) {
+		return 1;
+	}
+
 	ret = tst_system(buf);

 	if (WEXITSTATUS(ret) == 127) {
@@ -55,8 +60,8 @@ static int has_kernel_support(const char *fs_type, int flags)
 	if (!tmpdir)
 		tmpdir = "/tmp";

-	mount("/dev/zero", tmpdir, fs_type, 0, NULL);
-	if (errno != ENODEV) {
+	ret = mount("/dev/zero", tmpdir, fs_type, 0, NULL);
+	if (!ret) {
 		tst_res(TINFO, "Kernel supports %s", fs_type);
 		return 1;
 	}
--
2.17.1