[LTP] [PATCH] syscalls/open11.c: Fix the failure of opening device special file

[LTP] [PATCH] syscalls/open11.c: Fix the failure of opening device special file

[Xiao Yang]

1) If temporary directory(TMPDIR/TEMPDIR) is on the mountpoint which
   is mounted with nodev option(or temporary directory is mounted with
   with nodev option directly), open(2) has no permission to open device
   special file in the temporary directory and failed as expected.  as below:
   --------------------------------------------------------------------------
   open11.c:351: Open device special file O_RDONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
   open11.c:351: Open device special file O_WRONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
   open11.c:351: Open device special file O_RDWR - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
   --------------------------------------------------------------------------
   We try to fix this issue by creating device special file in /dev/ instead.

2) Cleanup && Convert to new API.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/open/open11.c | 165 +++++++++-----------------------
 1 file changed, 45 insertions(+), 120 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open11.c b/testcases/kernel/syscalls/open/open11.c
index 78002f3..c540eb8 100644
--- a/testcases/kernel/syscalls/open/open11.c
+++ b/testcases/kernel/syscalls/open/open11.c
@@ -1,28 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (C) 2013 Red Hat, Inc.
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 of the GNU General Public
- * License as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
- *
- * Further, this software is distributed without any warranty that it
- * is free of the rightful claim of any third person regarding
- * infringement or the like.  Any license provided herein, whether
- * implied or otherwise, applies only to this software file.  Patent
- * licenses, if any, provided herein do not apply to combinations of
- * this program with other software, or any other product whatsoever.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
- */
-
-/*
  * Basic tests for open(2) and make sure open(2) works and handles error
  * conditions correctly.
  *
@@ -60,25 +39,15 @@
  */
 
 #define _GNU_SOURCE
-#include "config.h"
+#include <errno.h>
+#include <sys/sysmacros.h>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <sys/sysmacros.h>
-#include <sys/wait.h>
-#include <errno.h>
 #include <fcntl.h>
 #include <unistd.h>
-#include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-
-#include "test.h"
-#include "safe_macros.h"
 
-char *TCID = "open11";
+#include "tst_test.h"
 
-/* Define test files */
 #define T_REG "t_reg"			/* regular file with content */
 #define T_REG_EMPTY "t_reg_empty"	/* empty regular file */
 #define T_LINK_REG "t_link_reg"		/* hard link to T_REG */
@@ -86,24 +55,18 @@ char *TCID = "open11";
 #define T_SYMLINK_REG "t_symlink_reg"	/* symlink to T_REG */
 #define T_DIR "t_dir"			/* test dir */
 #define T_SYMLINK_DIR "t_symlink_dir"	/* symlink to T_DIR */
-#define T_DEV "t_dev"			/* test device special file */
+#define T_DEV "/dev/t_dev"		/* test device special file */
 
 #define T_MSG "this is a test string"
 
-static void setup(void);
-static void cleanup(void);
-
-struct test_case {
+static struct test_case {
 	char *desc;
 	char *path;
 	int flags;
 	mode_t mode;
 	int err;
-};
-struct test_case tc[] = {
-	/*
-	 * Test open(2) regular file
-	 */
+} tc[] = {
+	/* Test open(2) regular file */
 	{	/* open regular file O_RDONLY */
 		.desc = "Open regular file O_RDONLY",
 		.path = T_REG_EMPTY,
@@ -139,9 +102,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = 0,
 	},
-	/*
-	 * Test open(2) directory
-	 */
+	/* Test open(2) directory */
 	{	/* open dir O_RDONLY */
 		.desc = "Open dir O_RDONLY",
 		.path = T_DIR,
@@ -163,9 +124,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = ENOTDIR,
 	},
-	/*
-	 * Test open(2) hard link
-	 */
+	/* Test open(2) hard link */
 	{	/* open hard link file O_RDONLY */
 		.desc = "Open hard link file O_RDONLY",
 		.path = T_LINK_REG,
@@ -187,9 +146,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = 0,
 	},
-	/*
-	 * Test open(2) sym link
-	 */
+	/* Test open(2) sym link */
 	{	/* open sym link file O_RDONLY */
 		.desc = "Open sym link file O_RDONLY",
 		.path = T_SYMLINK_REG,
@@ -232,9 +189,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = EISDIR,
 	},
-	/*
-	 * Test open(2) device special
-	 */
+	/* * Test open(2) device special */
 	{	/* open device special file O_RDONLY */
 		.desc = "Open device special file O_RDONLY",
 		.path = T_DEV,
@@ -256,9 +211,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = 0,
 	},
-	/*
-	 * Test open(2) non-existing file
-	 */
+	/* * Test open(2) non-existing file */
 	{	/* open non-existing regular file in existing dir */
 		.desc = "Open non-existing regular file in existing dir",
 		.path = T_DIR"/"T_NEW_REG,
@@ -266,9 +219,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = 0,
 	},
-	/*
-	 * test open(2) with O_CREAT
-	 */
+	/* test open(2) with O_CREAT */
 	{	/* open hard link file O_RDONLY | O_CREAT */
 		.desc = "Open link file O_RDONLY | O_CREAT",
 		.path = T_LINK_REG,
@@ -304,9 +255,7 @@ struct test_case tc[] = {
 		.mode = 0644,
 		.err = EISDIR,
 	},
-	/*
-	 * Other random open(2) tests
-	 */
+	/* Other random open(2) tests */
 	{	/* open regular file O_RDONLY | O_TRUNC */
 		.desc = "Open regular file O_RDONLY | O_TRUNC, "
 			"behaviour is undefined but should not oops or hang",
@@ -325,42 +274,22 @@ struct test_case tc[] = {
 	},
 };
 
-int TST_TOTAL = sizeof(tc) / sizeof(tc[0]);
-
-int main(int argc, char *argv[])
+static void verify_open(unsigned int n)
 {
-	int lc;
-	int i;
 	int fd;
-	int ret;
 
-	tst_parse_opts(argc, argv, NULL, NULL);
+	TEST(open(tc[n].path, tc[n].flags, tc[n].mode));
+	fd = TEST_RETURN;
 
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		for (i = 0; i < TST_TOTAL; i++) {
-			TEST(open(tc[i].path, tc[i].flags, tc[i].mode));
-			fd = TEST_RETURN;
-
-			if (tc[i].err == -1 || TEST_ERRNO == tc[i].err) {
-				tst_resm(TPASS, "%s", tc[i].desc);
-			} else {
-				tst_resm(TFAIL | TTERRNO,
-					 "%s - expected errno %d - Got",
-					 tc[i].desc, tc[i].err);
-			}
-			if (fd > 0) {
-				ret = close(fd);
-				if (ret < 0)
-					tst_resm(TWARN, "%s - close failed: %s",
-						 tc[i].desc, strerror(errno));
-			}
-		}
+	if (tc[n].err == -1 || TEST_ERRNO == tc[n].err) {
+		tst_res(TPASS, "%s", tc[n].desc);
+	} else {
+		tst_res(TFAIL | TTERRNO, "%s - expected errno %d - Got",
+			tc[n].desc, tc[n].err);
 	}
 
-	cleanup();
-	tst_exit();
+	if (fd > 0)
+		SAFE_CLOSE(fd);
 }
 
 static void setup(void)
@@ -368,36 +297,32 @@ static void setup(void)
 	int fd;
 	int ret;
 
-	tst_require_root();
-
-	tst_tmpdir();
-
-	/* Create test files */
-	fd = SAFE_OPEN(cleanup, T_REG, O_WRONLY | O_CREAT, 0644);
+	fd = SAFE_OPEN(T_REG, O_WRONLY | O_CREAT, 0644);
 	ret = write(fd, T_MSG, sizeof(T_MSG));
 	if (ret == -1) {
-		close(fd);
-		tst_brkm(TBROK | TERRNO, cleanup, "Write %s failed", T_REG);
+		SAFE_CLOSE(fd);
+		tst_brk(TBROK | TERRNO, "Write %s failed", T_REG);
 	}
-	close(fd);
-
-	fd = SAFE_CREAT(cleanup, T_REG_EMPTY, 0644);
-	close(fd);
-
-	SAFE_LINK(cleanup, T_REG, T_LINK_REG);
-	SAFE_SYMLINK(cleanup, T_REG, T_SYMLINK_REG);
-	SAFE_MKDIR(cleanup, T_DIR, 0755);
-	SAFE_SYMLINK(cleanup, T_DIR, T_SYMLINK_DIR);
+	SAFE_CLOSE(fd);
 
-	ret = mknod(T_DEV, S_IFCHR, makedev(1, 5));
-	if (ret == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "Create char dev %s failed",
-			 T_DEV);
-
-	TEST_PAUSE;
+	SAFE_TOUCH(T_REG_EMPTY, 0644, NULL);
+	SAFE_LINK(T_REG, T_LINK_REG);
+	SAFE_SYMLINK(T_REG, T_SYMLINK_REG);
+	SAFE_MKDIR(T_DIR, 0755);
+	SAFE_SYMLINK(T_DIR, T_SYMLINK_DIR);
+	SAFE_MKNOD(T_DEV, S_IFCHR, makedev(1, 5));
 }
 
 static void cleanup(void)
 {
-	tst_rmdir();
+	if (!access(T_DEV, F_OK))
+		SAFE_UNLINK(T_DEV);
 }
+
+static struct tst_test test = {
+	.tcnt = ARRAY_SIZE(tc),
+	.setup = setup,
+	.cleanup = cleanup,
+	.test = verify_open,
+	.needs_tmpdir = 1,
+};
-- 
1.8.3.1

[LTP] [PATCH] syscalls/open11.c: Fix the failure of opening device special file

[Xiao Yang]

Hi,

Ping :-)

Thanks,
Xiao Yang
On 2018/07/04 16:00, Xiao Yang wrote:
> 1) If temporary directory(TMPDIR/TEMPDIR) is on the mountpoint which
>    is mounted with nodev option(or temporary directory is mounted with
>    with nodev option directly), open(2) has no permission to open device
>    special file in the temporary directory and failed as expected.  as below:
>    --------------------------------------------------------------------------
>    open11.c:351: Open device special file O_RDONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
>    open11.c:351: Open device special file O_WRONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
>    open11.c:351: Open device special file O_RDWR - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
>    --------------------------------------------------------------------------
>    We try to fix this issue by creating device special file in /dev/ instead.
>
> 2) Cleanup && Convert to new API.
>
> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  testcases/kernel/syscalls/open/open11.c | 165 +++++++++-----------------------
>  1 file changed, 45 insertions(+), 120 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/open/open11.c b/testcases/kernel/syscalls/open/open11.c
> index 78002f3..c540eb8 100644
> --- a/testcases/kernel/syscalls/open/open11.c
> +++ b/testcases/kernel/syscalls/open/open11.c
> @@ -1,28 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (C) 2013 Red Hat, Inc.
>   *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of version 2 of the GNU General Public
> - * License as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * Further, this software is distributed without any warranty that it
> - * is free of the rightful claim of any third person regarding
> - * infringement or the like.  Any license provided herein, whether
> - * implied or otherwise, applies only to this software file.  Patent
> - * licenses, if any, provided herein do not apply to combinations of
> - * this program with other software, or any other product whatsoever.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> - * 02110-1301, USA.
> - */
> -
> -/*
>   * Basic tests for open(2) and make sure open(2) works and handles error
>   * conditions correctly.
>   *
> @@ -60,25 +39,15 @@
>   */
>  
>  #define _GNU_SOURCE
> -#include "config.h"
> +#include <errno.h>
> +#include <sys/sysmacros.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> -#include <sys/sysmacros.h>
> -#include <sys/wait.h>
> -#include <errno.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> -#include <signal.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -
> -#include "test.h"
> -#include "safe_macros.h"
>  
> -char *TCID = "open11";
> +#include "tst_test.h"
>  
> -/* Define test files */
>  #define T_REG "t_reg"			/* regular file with content */
>  #define T_REG_EMPTY "t_reg_empty"	/* empty regular file */
>  #define T_LINK_REG "t_link_reg"		/* hard link to T_REG */
> @@ -86,24 +55,18 @@ char *TCID = "open11";
>  #define T_SYMLINK_REG "t_symlink_reg"	/* symlink to T_REG */
>  #define T_DIR "t_dir"			/* test dir */
>  #define T_SYMLINK_DIR "t_symlink_dir"	/* symlink to T_DIR */
> -#define T_DEV "t_dev"			/* test device special file */
> +#define T_DEV "/dev/t_dev"		/* test device special file */
>  
>  #define T_MSG "this is a test string"
>  
> -static void setup(void);
> -static void cleanup(void);
> -
> -struct test_case {
> +static struct test_case {
>  	char *desc;
>  	char *path;
>  	int flags;
>  	mode_t mode;
>  	int err;
> -};
> -struct test_case tc[] = {
> -	/*
> -	 * Test open(2) regular file
> -	 */
> +} tc[] = {
> +	/* Test open(2) regular file */
>  	{	/* open regular file O_RDONLY */
>  		.desc = "Open regular file O_RDONLY",
>  		.path = T_REG_EMPTY,
> @@ -139,9 +102,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * Test open(2) directory
> -	 */
> +	/* Test open(2) directory */
>  	{	/* open dir O_RDONLY */
>  		.desc = "Open dir O_RDONLY",
>  		.path = T_DIR,
> @@ -163,9 +124,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = ENOTDIR,
>  	},
> -	/*
> -	 * Test open(2) hard link
> -	 */
> +	/* Test open(2) hard link */
>  	{	/* open hard link file O_RDONLY */
>  		.desc = "Open hard link file O_RDONLY",
>  		.path = T_LINK_REG,
> @@ -187,9 +146,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * Test open(2) sym link
> -	 */
> +	/* Test open(2) sym link */
>  	{	/* open sym link file O_RDONLY */
>  		.desc = "Open sym link file O_RDONLY",
>  		.path = T_SYMLINK_REG,
> @@ -232,9 +189,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = EISDIR,
>  	},
> -	/*
> -	 * Test open(2) device special
> -	 */
> +	/* * Test open(2) device special */
>  	{	/* open device special file O_RDONLY */
>  		.desc = "Open device special file O_RDONLY",
>  		.path = T_DEV,
> @@ -256,9 +211,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * Test open(2) non-existing file
> -	 */
> +	/* * Test open(2) non-existing file */
>  	{	/* open non-existing regular file in existing dir */
>  		.desc = "Open non-existing regular file in existing dir",
>  		.path = T_DIR"/"T_NEW_REG,
> @@ -266,9 +219,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * test open(2) with O_CREAT
> -	 */
> +	/* test open(2) with O_CREAT */
>  	{	/* open hard link file O_RDONLY | O_CREAT */
>  		.desc = "Open link file O_RDONLY | O_CREAT",
>  		.path = T_LINK_REG,
> @@ -304,9 +255,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = EISDIR,
>  	},
> -	/*
> -	 * Other random open(2) tests
> -	 */
> +	/* Other random open(2) tests */
>  	{	/* open regular file O_RDONLY | O_TRUNC */
>  		.desc = "Open regular file O_RDONLY | O_TRUNC, "
>  			"behaviour is undefined but should not oops or hang",
> @@ -325,42 +274,22 @@ struct test_case tc[] = {
>  	},
>  };
>  
> -int TST_TOTAL = sizeof(tc) / sizeof(tc[0]);
> -
> -int main(int argc, char *argv[])
> +static void verify_open(unsigned int n)
>  {
> -	int lc;
> -	int i;
>  	int fd;
> -	int ret;
>  
> -	tst_parse_opts(argc, argv, NULL, NULL);
> +	TEST(open(tc[n].path, tc[n].flags, tc[n].mode));
> +	fd = TEST_RETURN;
>  
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		for (i = 0; i < TST_TOTAL; i++) {
> -			TEST(open(tc[i].path, tc[i].flags, tc[i].mode));
> -			fd = TEST_RETURN;
> -
> -			if (tc[i].err == -1 || TEST_ERRNO == tc[i].err) {
> -				tst_resm(TPASS, "%s", tc[i].desc);
> -			} else {
> -				tst_resm(TFAIL | TTERRNO,
> -					 "%s - expected errno %d - Got",
> -					 tc[i].desc, tc[i].err);
> -			}
> -			if (fd > 0) {
> -				ret = close(fd);
> -				if (ret < 0)
> -					tst_resm(TWARN, "%s - close failed: %s",
> -						 tc[i].desc, strerror(errno));
> -			}
> -		}
> +	if (tc[n].err == -1 || TEST_ERRNO == tc[n].err) {
> +		tst_res(TPASS, "%s", tc[n].desc);
> +	} else {
> +		tst_res(TFAIL | TTERRNO, "%s - expected errno %d - Got",
> +			tc[n].desc, tc[n].err);
>  	}
>  
> -	cleanup();
> -	tst_exit();
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
>  }
>  
>  static void setup(void)
> @@ -368,36 +297,32 @@ static void setup(void)
>  	int fd;
>  	int ret;
>  
> -	tst_require_root();
> -
> -	tst_tmpdir();
> -
> -	/* Create test files */
> -	fd = SAFE_OPEN(cleanup, T_REG, O_WRONLY | O_CREAT, 0644);
> +	fd = SAFE_OPEN(T_REG, O_WRONLY | O_CREAT, 0644);
>  	ret = write(fd, T_MSG, sizeof(T_MSG));
>  	if (ret == -1) {
> -		close(fd);
> -		tst_brkm(TBROK | TERRNO, cleanup, "Write %s failed", T_REG);
> +		SAFE_CLOSE(fd);
> +		tst_brk(TBROK | TERRNO, "Write %s failed", T_REG);
>  	}
> -	close(fd);
> -
> -	fd = SAFE_CREAT(cleanup, T_REG_EMPTY, 0644);
> -	close(fd);
> -
> -	SAFE_LINK(cleanup, T_REG, T_LINK_REG);
> -	SAFE_SYMLINK(cleanup, T_REG, T_SYMLINK_REG);
> -	SAFE_MKDIR(cleanup, T_DIR, 0755);
> -	SAFE_SYMLINK(cleanup, T_DIR, T_SYMLINK_DIR);
> +	SAFE_CLOSE(fd);
>  
> -	ret = mknod(T_DEV, S_IFCHR, makedev(1, 5));
> -	if (ret == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "Create char dev %s failed",
> -			 T_DEV);
> -
> -	TEST_PAUSE;
> +	SAFE_TOUCH(T_REG_EMPTY, 0644, NULL);
> +	SAFE_LINK(T_REG, T_LINK_REG);
> +	SAFE_SYMLINK(T_REG, T_SYMLINK_REG);
> +	SAFE_MKDIR(T_DIR, 0755);
> +	SAFE_SYMLINK(T_DIR, T_SYMLINK_DIR);
> +	SAFE_MKNOD(T_DEV, S_IFCHR, makedev(1, 5));
>  }
>  
>  static void cleanup(void)
>  {
> -	tst_rmdir();
> +	if (!access(T_DEV, F_OK))
> +		SAFE_UNLINK(T_DEV);
>  }
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tc),
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test = verify_open,
> +	.needs_tmpdir = 1,
> +};

[LTP] [PATCH] syscalls/open11.c: Fix the failure of opening device special file

[Cyril Hrubis]

Hi!
> 1) If temporary directory(TMPDIR/TEMPDIR) is on the mountpoint which
>    is mounted with nodev option(or temporary directory is mounted with
>    with nodev option directly), open(2) has no permission to open device
>    special file in the temporary directory and failed as expected.  as below:
>    --------------------------------------------------------------------------
>    open11.c:351: Open device special file O_RDONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
>    open11.c:351: Open device special file O_WRONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
>    open11.c:351: Open device special file O_RDWR - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
>    --------------------------------------------------------------------------
>    We try to fix this issue by creating device special file in /dev/ instead.

As I said earlier, I do not like much that we are messing up with in the
/dev/ directory. Maybe we need to add a test library flag, something as
.needs_devfs into the test library that would prepare a suitable
filesystem in a defined place for us. That way we can solve the issue in
a single place and once for all. I can look into that later on.

> 2) Cleanup && Convert to new API.

This should have gone in a separate patch anyway, so I've removed the
/dev/ related bit and did a few changes so that it compiles and also
added .needs_root flag which was missing after the conversion and
pushed, thanks.

> Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
> ---
>  testcases/kernel/syscalls/open/open11.c | 165 +++++++++-----------------------
>  1 file changed, 45 insertions(+), 120 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/open/open11.c b/testcases/kernel/syscalls/open/open11.c
> index 78002f3..c540eb8 100644
> --- a/testcases/kernel/syscalls/open/open11.c
> +++ b/testcases/kernel/syscalls/open/open11.c
> @@ -1,28 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (C) 2013 Red Hat, Inc.
>   *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of version 2 of the GNU General Public
> - * License as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it would be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> - *
> - * Further, this software is distributed without any warranty that it
> - * is free of the rightful claim of any third person regarding
> - * infringement or the like.  Any license provided herein, whether
> - * implied or otherwise, applies only to this software file.  Patent
> - * licenses, if any, provided herein do not apply to combinations of
> - * this program with other software, or any other product whatsoever.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> - * 02110-1301, USA.
> - */
> -
> -/*
>   * Basic tests for open(2) and make sure open(2) works and handles error
>   * conditions correctly.
>   *
> @@ -60,25 +39,15 @@
>   */
>  
>  #define _GNU_SOURCE
> -#include "config.h"
> +#include <errno.h>
> +#include <sys/sysmacros.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> -#include <sys/sysmacros.h>
> -#include <sys/wait.h>
> -#include <errno.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> -#include <signal.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -
> -#include "test.h"
> -#include "safe_macros.h"
>  
> -char *TCID = "open11";
> +#include "tst_test.h"
>  
> -/* Define test files */
>  #define T_REG "t_reg"			/* regular file with content */
>  #define T_REG_EMPTY "t_reg_empty"	/* empty regular file */
>  #define T_LINK_REG "t_link_reg"		/* hard link to T_REG */
> @@ -86,24 +55,18 @@ char *TCID = "open11";
>  #define T_SYMLINK_REG "t_symlink_reg"	/* symlink to T_REG */
>  #define T_DIR "t_dir"			/* test dir */
>  #define T_SYMLINK_DIR "t_symlink_dir"	/* symlink to T_DIR */
> -#define T_DEV "t_dev"			/* test device special file */
> +#define T_DEV "/dev/t_dev"		/* test device special file */
>  
>  #define T_MSG "this is a test string"
>  
> -static void setup(void);
> -static void cleanup(void);
> -
> -struct test_case {
> +static struct test_case {
>  	char *desc;
>  	char *path;
>  	int flags;
>  	mode_t mode;
>  	int err;
> -};
> -struct test_case tc[] = {
> -	/*
> -	 * Test open(2) regular file
> -	 */
> +} tc[] = {
> +	/* Test open(2) regular file */
>  	{	/* open regular file O_RDONLY */
>  		.desc = "Open regular file O_RDONLY",
>  		.path = T_REG_EMPTY,
> @@ -139,9 +102,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * Test open(2) directory
> -	 */
> +	/* Test open(2) directory */
>  	{	/* open dir O_RDONLY */
>  		.desc = "Open dir O_RDONLY",
>  		.path = T_DIR,
> @@ -163,9 +124,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = ENOTDIR,
>  	},
> -	/*
> -	 * Test open(2) hard link
> -	 */
> +	/* Test open(2) hard link */
>  	{	/* open hard link file O_RDONLY */
>  		.desc = "Open hard link file O_RDONLY",
>  		.path = T_LINK_REG,
> @@ -187,9 +146,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * Test open(2) sym link
> -	 */
> +	/* Test open(2) sym link */
>  	{	/* open sym link file O_RDONLY */
>  		.desc = "Open sym link file O_RDONLY",
>  		.path = T_SYMLINK_REG,
> @@ -232,9 +189,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = EISDIR,
>  	},
> -	/*
> -	 * Test open(2) device special
> -	 */
> +	/* * Test open(2) device special */
>  	{	/* open device special file O_RDONLY */
>  		.desc = "Open device special file O_RDONLY",
>  		.path = T_DEV,
> @@ -256,9 +211,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * Test open(2) non-existing file
> -	 */
> +	/* * Test open(2) non-existing file */
>  	{	/* open non-existing regular file in existing dir */
>  		.desc = "Open non-existing regular file in existing dir",
>  		.path = T_DIR"/"T_NEW_REG,
> @@ -266,9 +219,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = 0,
>  	},
> -	/*
> -	 * test open(2) with O_CREAT
> -	 */
> +	/* test open(2) with O_CREAT */
>  	{	/* open hard link file O_RDONLY | O_CREAT */
>  		.desc = "Open link file O_RDONLY | O_CREAT",
>  		.path = T_LINK_REG,
> @@ -304,9 +255,7 @@ struct test_case tc[] = {
>  		.mode = 0644,
>  		.err = EISDIR,
>  	},
> -	/*
> -	 * Other random open(2) tests
> -	 */
> +	/* Other random open(2) tests */
>  	{	/* open regular file O_RDONLY | O_TRUNC */
>  		.desc = "Open regular file O_RDONLY | O_TRUNC, "
>  			"behaviour is undefined but should not oops or hang",
> @@ -325,42 +274,22 @@ struct test_case tc[] = {
>  	},
>  };
>  
> -int TST_TOTAL = sizeof(tc) / sizeof(tc[0]);
> -
> -int main(int argc, char *argv[])
> +static void verify_open(unsigned int n)
>  {
> -	int lc;
> -	int i;
>  	int fd;
> -	int ret;
>  
> -	tst_parse_opts(argc, argv, NULL, NULL);
> +	TEST(open(tc[n].path, tc[n].flags, tc[n].mode));
> +	fd = TEST_RETURN;
>  
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		for (i = 0; i < TST_TOTAL; i++) {
> -			TEST(open(tc[i].path, tc[i].flags, tc[i].mode));
> -			fd = TEST_RETURN;
> -
> -			if (tc[i].err == -1 || TEST_ERRNO == tc[i].err) {
> -				tst_resm(TPASS, "%s", tc[i].desc);
> -			} else {
> -				tst_resm(TFAIL | TTERRNO,
> -					 "%s - expected errno %d - Got",
> -					 tc[i].desc, tc[i].err);
> -			}
> -			if (fd > 0) {
> -				ret = close(fd);
> -				if (ret < 0)
> -					tst_resm(TWARN, "%s - close failed: %s",
> -						 tc[i].desc, strerror(errno));
> -			}
> -		}
> +	if (tc[n].err == -1 || TEST_ERRNO == tc[n].err) {
> +		tst_res(TPASS, "%s", tc[n].desc);
> +	} else {
> +		tst_res(TFAIL | TTERRNO, "%s - expected errno %d - Got",
> +			tc[n].desc, tc[n].err);
>  	}
>  
> -	cleanup();
> -	tst_exit();
> +	if (fd > 0)
> +		SAFE_CLOSE(fd);
>  }
>  
>  static void setup(void)
> @@ -368,36 +297,32 @@ static void setup(void)
>  	int fd;
>  	int ret;
>  
> -	tst_require_root();
> -
> -	tst_tmpdir();
> -
> -	/* Create test files */
> -	fd = SAFE_OPEN(cleanup, T_REG, O_WRONLY | O_CREAT, 0644);
> +	fd = SAFE_OPEN(T_REG, O_WRONLY | O_CREAT, 0644);
>  	ret = write(fd, T_MSG, sizeof(T_MSG));
>  	if (ret == -1) {
> -		close(fd);
> -		tst_brkm(TBROK | TERRNO, cleanup, "Write %s failed", T_REG);
> +		SAFE_CLOSE(fd);
> +		tst_brk(TBROK | TERRNO, "Write %s failed", T_REG);
>  	}
> -	close(fd);
> -
> -	fd = SAFE_CREAT(cleanup, T_REG_EMPTY, 0644);
> -	close(fd);
> -
> -	SAFE_LINK(cleanup, T_REG, T_LINK_REG);
> -	SAFE_SYMLINK(cleanup, T_REG, T_SYMLINK_REG);
> -	SAFE_MKDIR(cleanup, T_DIR, 0755);
> -	SAFE_SYMLINK(cleanup, T_DIR, T_SYMLINK_DIR);
> +	SAFE_CLOSE(fd);
>  
> -	ret = mknod(T_DEV, S_IFCHR, makedev(1, 5));
> -	if (ret == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "Create char dev %s failed",
> -			 T_DEV);
> -
> -	TEST_PAUSE;
> +	SAFE_TOUCH(T_REG_EMPTY, 0644, NULL);
> +	SAFE_LINK(T_REG, T_LINK_REG);
> +	SAFE_SYMLINK(T_REG, T_SYMLINK_REG);
> +	SAFE_MKDIR(T_DIR, 0755);
> +	SAFE_SYMLINK(T_DIR, T_SYMLINK_DIR);
> +	SAFE_MKNOD(T_DEV, S_IFCHR, makedev(1, 5));
>  }
>  
>  static void cleanup(void)
>  {
> -	tst_rmdir();
> +	if (!access(T_DEV, F_OK))
> +		SAFE_UNLINK(T_DEV);
>  }
> +
> +static struct tst_test test = {
> +	.tcnt = ARRAY_SIZE(tc),
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test = verify_open,
> +	.needs_tmpdir = 1,
> +};
> -- 
> 1.8.3.1
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 1/4] lib/tst_path_has_mnt_flags.c: Factor out tst_path_has_mnt_flags() for new API

[Xiao Yang]

We can use existed tst_path_has_mnt_flags() to check whether a path is
on a filesystem that is mounted with specified flags, so ensure that the
function can also be used by new API.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/old/test.h               | 13 +------------
 include/tst_path_has_mnt_flags.h | 22 ++++++++++++++++++++++
 include/tst_test.h               |  1 +
 3 files changed, 24 insertions(+), 12 deletions(-)
 create mode 100644 include/tst_path_has_mnt_flags.h

diff --git a/include/old/test.h b/include/old/test.h
index 5eef304..25d7152 100644
--- a/include/old/test.h
+++ b/include/old/test.h
@@ -64,6 +64,7 @@
 #include "old_tmpdir.h"
 #include "tst_minmax.h"
 #include "tst_get_bad_addr.h"
+#include "tst_path_has_mnt_flags.h"
 
 /*
  * Ensure that NUMSIGS is defined.
@@ -236,18 +237,6 @@ unsigned short tst_get_unused_port(void (cleanup_fn)(void),
 const char *tst_strsig(int sig);
 const char *tst_strerrno(int err);
 
-/* lib/tst_path_has_mnt_flags.c
- *
- * Check whether a path is on a filesystem that is mounted with
- * specified flags
- * @path: path to file, if path is NULL tst_tmpdir is used.
- * @flags: NULL or NULL terminated array of mount flags
- *
- * Return: 0..n - number of flags matched
- */
-int tst_path_has_mnt_flags(void (cleanup_fn)(void),
-		const char *path, const char *flags[]);
-
 #ifdef TST_USE_COMPAT16_SYSCALL
 #define TCID_BIT_SUFFIX "_16"
 #elif  TST_USE_NEWER64_SYSCALL
diff --git a/include/tst_path_has_mnt_flags.h b/include/tst_path_has_mnt_flags.h
new file mode 100644
index 0000000..1b16f1b
--- /dev/null
+++ b/include/tst_path_has_mnt_flags.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2018 FUJITSU LIMITED. All rights reserved.
+ * Author: Xiao Yang <yangx.jy@cn.fujitsu.com>
+ */
+
+#ifndef TST_PATH_HAS_MNT_FLAGS_H__
+#define TST_PATH_HAS_MNT_FLAGS_H__
+
+/* lib/tst_path_has_mnt_flags.c
+ *
+ * Check whether a path is on a filesystem that is mounted with
+ * specified flags
+ * @path: path to file, if path is NULL tst_tmpdir is used.
+ * @flags: NULL or NULL terminated array of mount flags
+ *
+ * Return: 0..n - number of flags matched
+ */
+int tst_path_has_mnt_flags(void (*cleanup_fn)(void),
+		const char *path, const char *flags[]);
+
+#endif	/* TST_PATH_HAS_MNT_FLAGS_H__ */
diff --git a/include/tst_test.h b/include/tst_test.h
index 98dacf3..f7d109a 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -41,6 +41,7 @@
 #include "tst_kernel.h"
 #include "tst_minmax.h"
 #include "tst_get_bad_addr.h"
+#include "tst_path_has_mnt_flags.h"
 
 /*
  * Reports testcase result.
-- 
1.8.3.1

[LTP] [PATCH v2 2/4] lib/tst_test.c: add .needs_devfs flag

[Xiao Yang]

We add .needs_devfs flag to prepare a suitable filesystem to test
device special files and use ext2 instead of tmpfs(because tmpfs
doesn't support extended attributes) as default filesystem if
.dev_fs_type is not specified.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/tst_test.h |  1 +
 lib/tst_test.c     | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/tst_test.h b/include/tst_test.h
index f7d109a..4cc6202 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -129,6 +129,7 @@ struct tst_test {
 	int mount_device:1;
 	int needs_rofs:1;
 	int child_needs_reinit:1;
+	int needs_devfs:1;
 	/*
 	 * If set the test function will be executed for all available
 	 * filesystems and the current filesytem type would be set in the
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 2f3d357..039c78f 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -724,6 +724,22 @@ static int prepare_and_mount_ro_fs(const char *dev,
 	return 0;
 }
 
+static void prepare_and_mount_dev_fs(const char *dev,
+				     const char *mntpoint,
+				     const char *fs_type)
+{
+	const char *flags[] = {"nodev", NULL};
+	char abs_path[PATH_MAX];
+
+	sprintf(abs_path, "%s/%s", tst_get_tmpdir(), mntpoint);
+
+	SAFE_MOUNT(dev, mntpoint, fs_type, MS_MGC_VAL, NULL);
+	mntpoint_mounted = 1;
+
+	if (tst_path_has_mnt_flags(NULL, abs_path, flags))
+		tst_brk(TBROK, "%s is mounted with nodev", fs_type);
+}
+
 static void prepare_device(void)
 {
 	if (tst_test->format_device) {
@@ -737,6 +753,12 @@ static void prepare_device(void)
 		return;
 	}
 
+	if (tst_test->needs_devfs) {
+		prepare_and_mount_dev_fs(tdev.dev, tst_test->mntpoint,
+					 tdev.fs_type);
+		return;
+	}
+
 	if (tst_test->mount_device) {
 		SAFE_MOUNT(tdev.dev, tst_test->mntpoint, tdev.fs_type,
 			   tst_test->mnt_flags, tst_test->mnt_data);
@@ -778,6 +800,11 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->all_filesystems)
 		tst_test->needs_device = 1;
 
+	if (tst_test->needs_devfs) {
+		tst_test->needs_device = 1;
+		tst_test->format_device = 1;
+	}
+
 	setup_ipc();
 
 	if (needs_tmpdir() && !tst_tmpdir_created())
@@ -786,8 +813,9 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->mntpoint)
 		SAFE_MKDIR(tst_test->mntpoint, 0777);
 
-	if ((tst_test->needs_rofs || tst_test->mount_device ||
-	     tst_test->all_filesystems) && !tst_test->mntpoint) {
+	if ((tst_test->needs_devfs || tst_test->needs_rofs ||
+	     tst_test->mount_device || tst_test->all_filesystems) &&
+	     !tst_test->mntpoint) {
 		tst_brk(TBROK, "tst_test->mntpoint must be set!");
 	}
 
-- 
1.8.3.1

[LTP] [PATCH v2 3/4] syscalls/open11.c: Fix the failure of opening device special files

[Xiao Yang]

If temporary directory is on the mountpoint which is mounted with nodev
option(or it is mounted with with nodev option directly), open(2) has no
permission to open device special file in the temporary directory and
failed as expected.  as below:
--------------------------------------------------------------------------
open11.c:351: Open device special file O_RDONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
open11.c:351: Open device special file O_WRONLY - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
open11.c:351: Open device special file O_RDWR - expected errno 0 - Got: TEST_ERRNO=EACCES(13): Permission denied
--------------------------------------------------------------------------

We prepare a suitable filesystem to test device special files by setting
.needs_devfs flag.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/open/open11.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open11.c b/testcases/kernel/syscalls/open/open11.c
index 0469a0a..cfd04fd 100644
--- a/testcases/kernel/syscalls/open/open11.c
+++ b/testcases/kernel/syscalls/open/open11.c
@@ -48,6 +48,7 @@
 
 #include "tst_test.h"
 
+#define MNTPOINT "mntpoint"
 #define T_REG "t_reg"			/* regular file with content */
 #define T_REG_EMPTY "t_reg_empty"	/* empty regular file */
 #define T_LINK_REG "t_link_reg"		/* hard link to T_REG */
@@ -55,7 +56,7 @@
 #define T_SYMLINK_REG "t_symlink_reg"	/* symlink to T_REG */
 #define T_DIR "t_dir"			/* test dir */
 #define T_SYMLINK_DIR "t_symlink_dir"	/* symlink to T_DIR */
-#define T_DEV "t_dev"			/* test device special file */
+#define T_DEV MNTPOINT"/t_dev"		/* test device special file */
 
 #define T_MSG "this is a test string"
 
@@ -318,6 +319,7 @@ static struct tst_test test = {
 	.tcnt = ARRAY_SIZE(tc),
 	.setup = setup,
 	.test = verify_open,
-	.needs_tmpdir = 1,
+	.needs_devfs = 1,
+	.mntpoint = MNTPOINT,
 	.needs_root = 1,
 };
-- 
1.8.3.1

[LTP] [PATCH v2 4/4] syscalls/fsetxattr02.c: Fix the failure of opening device files

[Xiao Yang]

If temporary directory is on the mountpoint which is mounted with nodev
option(or it is mounted with with nodev option directly), open(2) has no
permission to open device files in the temporary directory and returns
EACCES.  as below:
--------------------------------------------------------------------
safe_macros.c:225: BROK: fsetxattr02.c:215: open(fsetxattr02chr,0,00) failed: EACCES
--------------------------------------------------------------------

We prepare a suitable filesystem to test device special files by setting
.needs_devfs flag.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 testcases/kernel/syscalls/fsetxattr/fsetxattr02.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c b/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
index a1be652..0f769ce 100644
--- a/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
+++ b/testcases/kernel/syscalls/fsetxattr/fsetxattr02.c
@@ -48,13 +48,14 @@
 #define XATTR_TEST_VALUE "this is a test value"
 #define XATTR_TEST_VALUE_SIZE 20
 
+#define MNTPOINT "mntpoint"
 #define OFFSET    11
 #define FILENAME "fsetxattr02testfile"
 #define DIRNAME  "fsetxattr02testdir"
 #define SYMLINK  "fsetxattr02symlink"
-#define FIFO     "fsetxattr02fifo"
-#define CHR      "fsetxattr02chr"
-#define BLK      "fsetxattr02blk"
+#define FIFO     MNTPOINT"/fsetxattr02fifo"
+#define CHR      MNTPOINT"/fsetxattr02chr"
+#define BLK      MNTPOINT"/fsetxattr02blk"
 #define SOCK     "fsetxattr02sock"
 
 struct test_case {
@@ -139,6 +140,9 @@ static struct test_case tc[] = {
 
 static void verify_fsetxattr(unsigned int i)
 {
+	/* acquire basename to avoid breaking OFFSET logic */
+	tc[i].fname = strstr(tc[i].fname, "fsetxattr02");
+
 	/* some tests might require existing keys for each iteration */
 	if (tc[i].needskeyset) {
 		SAFE_FSETXATTR(tc[i].fd, tc[i].key, tc[i].value, tc[i].size,
@@ -247,7 +251,8 @@ static struct tst_test test = {
 	.test = verify_fsetxattr,
 	.cleanup = cleanup,
 	.tcnt = ARRAY_SIZE(tc),
-	.needs_tmpdir = 1,
+	.needs_devfs = 1,
+	.mntpoint = MNTPOINT,
 	.needs_root = 1,
 };
 
-- 
1.8.3.1

[LTP] [PATCH v2 2/4] lib/tst_test.c: add .needs_devfs flag

[Cyril Hrubis]

Hi!
> We add .needs_devfs flag to prepare a suitable filesystem to test
> device special files and use ext2 instead of tmpfs(because tmpfs
> doesn't support extended attributes) as default filesystem if
> .dev_fs_type is not specified.

Can we do that only if the test temporary directory is not suitable for
creating devices?

There are distributions out there that have /tmp residing on pretty much
normal filesystems, also user can override TMPDIR to point to a path
backed up by a regular fs. In these cases this will slow down these
tests for no good reason.

So I would rather go for:

if tst_test->needs_devfs is set:

* mkdir tst_test->mntpoint if it does not exits
* check if tst_test->mntpoint is suitable for creating devices
* if not, mount some filesystem over it
  - mounting tmpfs without nodev flag should be more than enough
    and if that fails for you, we can fall back to a regular filesystem

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/4] lib/tst_test.c: add .needs_devfs flag

[Xiao Yang]

On 2018/08/16 21:28, Cyril Hrubis wrote:
> Hi!
>> We add .needs_devfs flag to prepare a suitable filesystem to test
>> device special files and use ext2 instead of tmpfs(because tmpfs
>> doesn't support extended attributes) as default filesystem if
>> .dev_fs_type is not specified.
> Can we do that only if the test temporary directory is not suitable for
> creating devices?
>
> There are distributions out there that have /tmp residing on pretty much
> normal filesystems, also user can override TMPDIR to point to a path
> backed up by a regular fs. In these cases this will slow down these
> tests for no good reason.
>
> So I would rather go for:
>
> if tst_test->needs_devfs is set:
>
> * mkdir tst_test->mntpoint if it does not exits
> * check if tst_test->mntpoint is suitable for creating devices
> * if not, mount some filesystem over it
>    - mounting tmpfs without nodev flag should be more than enough
>      and if that fails for you, we can fall back to a regular filesystem
Hi Cyril,

Thanks for your review and suggestion.

Mounting tmpfs without nodev over tst_test->mntpoint doesn't fail for 
me, so it isn't
necessary to mount a regular filesystem instead of tmpfs  over 
tst_test->mntpoint.

Thanks,
Xiao Yang

[LTP] [PATCH v3 2/2] lib/tst_test.c: Add .needs_devfs flag

[Xiao Yang]

We add .needs_devfs flag to prepare a suitable directory for creating devices.
1) If tst_test->needs_devfs is set, tst_test->mntpoint must be set as well.
2) If tst_test->needs_devfs is set and tst_test->mntpoint is on a temporary
   directory that is mounted with nodev, we try to mount tmpfs without nodev
   over tst_test->mntpoint.
3) If tst_test->needs_devfs is set and tst_test->mntpoint is on a temporary
   directory that is mounted without nodev, we just create devices on
   unmounted tst_test->mntpoint.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/tst_test.h |  1 +
 lib/tst_test.c     | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/tst_test.h b/include/tst_test.h
index f7d109a..4cc6202 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -129,6 +129,7 @@ struct tst_test {
 	int mount_device:1;
 	int needs_rofs:1;
 	int child_needs_reinit:1;
+	int needs_devfs:1;
 	/*
 	 * If set the test function will be executed for all available
 	 * filesystems and the current filesytem type would be set in the
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 2f3d357..329bf8d 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -724,6 +724,21 @@ static int prepare_and_mount_ro_fs(const char *dev,
 	return 0;
 }
 
+static void prepare_and_mount_dev_fs(const char *mntpoint)
+{
+	const char *flags[] = {"nodev", NULL};
+	char abs_path[PATH_MAX];
+
+	sprintf(abs_path, "%s/%s", tst_get_tmpdir(), mntpoint);
+
+	if (tst_path_has_mnt_flags(NULL, abs_path, flags)) {
+		tst_res(TINFO, "%s isn't suitable for creating devices, "
+			"so mount tmpfs without nodev over it", abs_path);
+		SAFE_MOUNT(NULL, mntpoint, "tmpfs", 0, NULL);
+		mntpoint_mounted = 1;
+	}
+}
+
 static void prepare_device(void)
 {
 	if (tst_test->format_device) {
@@ -786,11 +801,15 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->mntpoint)
 		SAFE_MKDIR(tst_test->mntpoint, 0777);
 
-	if ((tst_test->needs_rofs || tst_test->mount_device ||
-	     tst_test->all_filesystems) && !tst_test->mntpoint) {
+	if ((tst_test->needs_devfs || tst_test->needs_rofs ||
+	     tst_test->mount_device || tst_test->all_filesystems) &&
+	     !tst_test->mntpoint) {
 		tst_brk(TBROK, "tst_test->mntpoint must be set!");
 	}
 
+	if (tst_test->needs_devfs)
+		prepare_and_mount_dev_fs(tst_test->mntpoint);
+
 	if (tst_test->needs_rofs) {
 		/* If we failed to mount read-only tmpfs. Fallback to
 		 * using a device with read-only filesystem.
-- 
1.8.3.1

[LTP] [PATCH v3 2/2] lib/tst_test.c: Add .needs_devfs flag

[Cyril Hrubis]

Hi!
> +static void prepare_and_mount_dev_fs(const char *mntpoint)
> +{
> +	const char *flags[] = {"nodev", NULL};
> +	char abs_path[PATH_MAX];
> +
> +	sprintf(abs_path, "%s/%s", tst_get_tmpdir(), mntpoint);

The tst_get_tmpdir() allocates memory and also we are sure that mntpoint
is on the same filesystem as the path returned by tst_get_tmpdir().

So I would suggest something like this:

	char *tmpdir;
	int mounted_nodev;

	tmpdir = tst_get_tmpdir();
	mounted_nodev = tst_path_has_mnt_flags(NULL, tmpdir, flags);
	free(tmpdir);

	if (mounted_nodev) {
		...
	}

> +	if (tst_path_has_mnt_flags(NULL, abs_path, flags)) {
> +		tst_res(TINFO, "%s isn't suitable for creating devices, "
> +			"so mount tmpfs without nodev over it", abs_path);
> +		SAFE_MOUNT(NULL, mntpoint, "tmpfs", 0, NULL);
> +		mntpoint_mounted = 1;
> +	}
> +}
> +
>  static void prepare_device(void)
>  {
>  	if (tst_test->format_device) {
> @@ -786,11 +801,15 @@ static void do_setup(int argc, char *argv[])
>  	if (tst_test->mntpoint)
>  		SAFE_MKDIR(tst_test->mntpoint, 0777);
>  
> -	if ((tst_test->needs_rofs || tst_test->mount_device ||
> -	     tst_test->all_filesystems) && !tst_test->mntpoint) {
> +	if ((tst_test->needs_devfs || tst_test->needs_rofs ||
> +	     tst_test->mount_device || tst_test->all_filesystems) &&
> +	     !tst_test->mntpoint) {
>  		tst_brk(TBROK, "tst_test->mntpoint must be set!");
>  	}

I guess that we should also make sure only one of needs_rofs,
needs_devfs or needs_device is set, because otherwise we would attempt
to mount multiple filesystems over the mntpoint.

something as:

	if (!!tst_test->needs_rofs +
	    !!tst_test->needs_devfs +
	    !!tst_test->needs_device > 1) {
		tst_brk(TBROK, "Two or more of needs_{rofs, devfs, device} are set");
	}

> +	if (tst_test->needs_devfs)
> +		prepare_and_mount_dev_fs(tst_test->mntpoint);
> +
>  	if (tst_test->needs_rofs) {
>  		/* If we failed to mount read-only tmpfs. Fallback to
>  		 * using a device with read-only filesystem.

Other than the minor nits this version looks fine.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3 2/2] lib/tst_test.c: Add .needs_devfs flag

[Xiao Yang]

On 2018/08/30 22:49, Cyril Hrubis wrote:
> Hi!
>> +static void prepare_and_mount_dev_fs(const char *mntpoint)
>> +{
>> +	const char *flags[] = {"nodev", NULL};
>> +	char abs_path[PATH_MAX];
>> +
>> +	sprintf(abs_path, "%s/%s", tst_get_tmpdir(), mntpoint);
> The tst_get_tmpdir() allocates memory and also we are sure that mntpoint
> is on the same filesystem as the path returned by tst_get_tmpdir().
>
> So I would suggest something like this:
>
> 	char *tmpdir;
> 	int mounted_nodev;
>
> 	tmpdir = tst_get_tmpdir();
> 	mounted_nodev = tst_path_has_mnt_flags(NULL, tmpdir, flags);
> 	free(tmpdir);
>
> 	if (mounted_nodev) {
> 		...
> 	}
>
Hi Cyril,

Thanks for your review.

By default, the path returned by tst_get_tmpdir() will be used in 
tst_path_has_mnt_flags() if we
pass NULL as a path parameter, so i will use 
tst_path_has_mnt_flags(NULL, NULL, flags) directly.
>> +	if (tst_path_has_mnt_flags(NULL, abs_path, flags)) {
>> +		tst_res(TINFO, "%s isn't suitable for creating devices, "
>> +			"so mount tmpfs without nodev over it", abs_path);
>> +		SAFE_MOUNT(NULL, mntpoint, "tmpfs", 0, NULL);
>> +		mntpoint_mounted = 1;
>> +	}
>> +}
>> +
>>   static void prepare_device(void)
>>   {
>>   	if (tst_test->format_device) {
>> @@ -786,11 +801,15 @@ static void do_setup(int argc, char *argv[])
>>   	if (tst_test->mntpoint)
>>   		SAFE_MKDIR(tst_test->mntpoint, 0777);
>>
>> -	if ((tst_test->needs_rofs || tst_test->mount_device ||
>> -	     tst_test->all_filesystems)&&  !tst_test->mntpoint) {
>> +	if ((tst_test->needs_devfs || tst_test->needs_rofs ||
>> +	     tst_test->mount_device || tst_test->all_filesystems)&&
>> +	     !tst_test->mntpoint) {
>>   		tst_brk(TBROK, "tst_test->mntpoint must be set!");
>>   	}
> I guess that we should also make sure only one of needs_rofs,
> needs_devfs or needs_device is set, because otherwise we would attempt
> to mount multiple filesystems over the mntpoint.
>
> something as:
>
> 	if (!!tst_test->needs_rofs +
> 	    !!tst_test->needs_devfs +
> 	    !!tst_test->needs_device>  1) {
> 		tst_brk(TBROK, "Two or more of needs_{rofs, devfs, device} are set");
> 	}
OK, i will add it as you said.

Thanks,
Xiao Yang
>> +	if (tst_test->needs_devfs)
>> +		prepare_and_mount_dev_fs(tst_test->mntpoint);
>> +
>>   	if (tst_test->needs_rofs) {
>>   		/* If we failed to mount read-only tmpfs. Fallback to
>>   		 * using a device with read-only filesystem.
> Other than the minor nits this version looks fine.
>

[LTP] [PATCH v4 2/2] lib/tst_test.c: Add .needs_devfs flag

[Cyril Hrubis]

Hi!
> +static void prepare_and_mount_dev_fs(const char *mntpoint)
> +{
> +	const char *flags[] = {"nodev", NULL};
> +	int mounted_nodev;
> +
> +	mounted_nodev = tst_path_has_mnt_flags(NULL, NULL, flags);
> +	if (mounted_nodev) {
> +		tst_res(TINFO, "tmpdir isn't suitable for creating devices, "
> +			"so mount tmpfs without nodev on %s", mntpoint);
> +		SAFE_MOUNT(NULL, mntpoint, "tmpfs", 0, NULL);
> +		mntpoint_mounted = 1;
> +	}
> +}

That is even better than my version, nice :-).

There is a last nit to solve, the problem is that
tst_path_has_mnt_flags() is defined in old/test.h, we have to move the
definition to a separate header file (in a separate patch) so that it
could be included in test.h, tst_path_has_mnt_flags.c, and tst_test.c.

I can do that or you can sent v5, your choice.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v4 2/2] lib/tst_test.c: Add .needs_devfs flag

[Xiao Yang]

We add .needs_devfs flag to prepare a suitable directory for creating devices.
1) If tst_test->needs_devfs is set, tst_test->mntpoint must be set as well.
2) If tst_test->needs_devfs is set and the temporary directory is mounted
   with nodev, we try to mount tmpfs without nodev on tst_test->mntpoint.
3) If tst_test->needs_devfs is set and the temporary directory is mounted
   without nodev, we just create devices on unmounted tst_test->mntpoint.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 include/tst_test.h |  1 +
 lib/tst_test.c     | 28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/include/tst_test.h b/include/tst_test.h
index f7d109a..4cc6202 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -129,6 +129,7 @@ struct tst_test {
 	int mount_device:1;
 	int needs_rofs:1;
 	int child_needs_reinit:1;
+	int needs_devfs:1;
 	/*
 	 * If set the test function will be executed for all available
 	 * filesystems and the current filesytem type would be set in the
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 2f3d357..36eb8d5 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -724,6 +724,20 @@ static int prepare_and_mount_ro_fs(const char *dev,
 	return 0;
 }
 
+static void prepare_and_mount_dev_fs(const char *mntpoint)
+{
+	const char *flags[] = {"nodev", NULL};
+	int mounted_nodev;
+
+	mounted_nodev = tst_path_has_mnt_flags(NULL, NULL, flags);
+	if (mounted_nodev) {
+		tst_res(TINFO, "tmpdir isn't suitable for creating devices, "
+			"so mount tmpfs without nodev on %s", mntpoint);
+		SAFE_MOUNT(NULL, mntpoint, "tmpfs", 0, NULL);
+		mntpoint_mounted = 1;
+	}
+}
+
 static void prepare_device(void)
 {
 	if (tst_test->format_device) {
@@ -786,11 +800,21 @@ static void do_setup(int argc, char *argv[])
 	if (tst_test->mntpoint)
 		SAFE_MKDIR(tst_test->mntpoint, 0777);
 
-	if ((tst_test->needs_rofs || tst_test->mount_device ||
-	     tst_test->all_filesystems) && !tst_test->mntpoint) {
+	if ((tst_test->needs_devfs || tst_test->needs_rofs ||
+	     tst_test->mount_device || tst_test->all_filesystems) &&
+	     !tst_test->mntpoint) {
 		tst_brk(TBROK, "tst_test->mntpoint must be set!");
 	}
 
+	if (!!tst_test->needs_rofs + !!tst_test->needs_devfs +
+	    !!tst_test->needs_device > 1) {
+		tst_brk(TBROK,
+			"Two or more of needs_{rofs, devfs, device} are set");
+	}
+
+	if (tst_test->needs_devfs)
+		prepare_and_mount_dev_fs(tst_test->mntpoint);
+
 	if (tst_test->needs_rofs) {
 		/* If we failed to mount read-only tmpfs. Fallback to
 		 * using a device with read-only filesystem.
-- 
1.8.3.1

[LTP] [PATCH v4 2/2] lib/tst_test.c: Add .needs_devfs flag

[Xiao Yang]

On 2018/08/31 20:31, Cyril Hrubis wrote:
> Hi!
>> +static void prepare_and_mount_dev_fs(const char *mntpoint)
>> +{
>> +	const char *flags[] = {"nodev", NULL};
>> +	int mounted_nodev;
>> +
>> +	mounted_nodev = tst_path_has_mnt_flags(NULL, NULL, flags);
>> +	if (mounted_nodev) {
>> +		tst_res(TINFO, "tmpdir isn't suitable for creating devices, "
>> +			"so mount tmpfs without nodev on %s", mntpoint);
>> +		SAFE_MOUNT(NULL, mntpoint, "tmpfs", 0, NULL);
>> +		mntpoint_mounted = 1;
>> +	}
>> +}
> That is even better than my version, nice :-).
>
> There is a last nit to solve, the problem is that
> tst_path_has_mnt_flags() is defined in old/test.h, we have to move the
> definition to a separate header file (in a separate patch) so that it
> could be included in test.h, tst_path_has_mnt_flags.c, and tst_test.c.
Hi Cyril,

This is a patch set, and i have factored out tst_path_has_mnt_flags() by 
the first patch:
http://lists.linux.it/pipermail/ltp/2018-August/009013.html

Thanks,
Xiao Yang
> I can do that or you can sent v5, your choice.
>

[LTP] [PATCH v4 2/2] lib/tst_test.c: Add .needs_devfs flag

[Cyril Hrubis]

Hi!
> This is a patch set, and i have factored out tst_path_has_mnt_flags() by 
> the first patch:
> http://lists.linux.it/pipermail/ltp/2018-August/009013.html

Ah, right, my bad, the rest of the patchset still applies.

I've modified that part so that we do not pass the NULL cleanup argument
in newlib tests (so that it's easier to remove the cleanup callback
completely it once the two oldlib tests that use the API are converted).

I've also cleaned up the last patch in the series so that we do not have
to pass the OFFSET to each tst_res() call, now we calculate the right
pointer at the start of the test.

And pushed the whole patchset, thanks!

-- 
Cyril Hrubis
chrubis@suse.cz