All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist
@ 2021-03-09 13:01 Zhao Gongyi
  2021-03-10 12:29 ` Cyril Hrubis
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Zhao Gongyi @ 2021-03-09 13:01 UTC (permalink / raw)
  To: ltp

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist
@ 2021-03-09 13:08 zhaogongyi
  0 siblings, 0 replies; 12+ messages in thread
From: zhaogongyi @ 2021-03-09 13:08 UTC (permalink / raw)
  To: ltp

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist
@ 2021-03-04  2:14 zhaogongyi
  0 siblings, 0 replies; 12+ messages in thread
From: zhaogongyi @ 2021-03-04  2:14 UTC (permalink / raw)
  To: ltp

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist
@ 2021-03-04  2:08 Zhao Gongyi
  2021-03-08 16:11 ` Cyril Hrubis
  0 siblings, 1 reply; 12+ messages in thread
From: Zhao Gongyi @ 2021-03-04  2:08 UTC (permalink / raw)
  To: ltp

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist
@ 2021-02-27  9:33 zhaogongyi
  2021-03-02 10:43 ` Cyril Hrubis
  0 siblings, 1 reply; 12+ messages in thread
From: zhaogongyi @ 2021-02-27  9:33 UTC (permalink / raw)
  To: ltp

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist
@ 2021-02-26  9:06 Zhao Gongyi
  2021-02-26  9:54 ` Cyril Hrubis
  0 siblings, 1 reply; 12+ messages in thread
From: Zhao Gongyi @ 2021-02-26  9:06 UTC (permalink / raw)
  To: ltp

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-04-13 17:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-09 13:01 [LTP] [PATCH] lib/tst_supported_fs_types.c: Add tmpfs to filesystem whitelist Zhao Gongyi
2021-03-10 12:29 ` Cyril Hrubis
2021-03-12 10:46 ` Petr Vorel
2021-04-13 17:28 ` Cyril Hrubis
  -- strict thread matches above, loose matches on Subject: below --
2021-03-09 13:08 zhaogongyi
2021-03-04  2:14 zhaogongyi
2021-03-04  2:08 Zhao Gongyi
2021-03-08 16:11 ` Cyril Hrubis
2021-02-27  9:33 zhaogongyi
2021-03-02 10:43 ` Cyril Hrubis
2021-02-26  9:06 Zhao Gongyi
2021-02-26  9:54 ` Cyril Hrubis

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.