[LTP] [PATCH 0/5] Misc cleanups (new lib) and fixes for Android

[LTP] [PATCH 0/5] Misc cleanups (new lib) and fixes for Android

[Sandeep Patil]

The following series contains patches to cleanup some of the old tests
and make use of the new library in them. That came with additional
benefit of getting easy ways to make those tests run on Android systems.

Note that some of these tests were made to run by hard coding for
ANDROID in them and that now goes away too.[1]

Sandeep Patil (5):
  chroot01: Use the 'tmpdir' we create.
  syscalls/chmod07: Use new ltp library
  syscalls/fchmod02: fall back to use "daemon" group if "users" is
    absent.
  syscalls/chmod05: Use new ltp library
  syscalls/posix_fadvise0[13]: Start using new library.

 testcases/kernel/syscalls/chmod/chmod05.c     | 187 ++++--------------
 testcases/kernel/syscalls/chmod/chmod07.c     | 174 ++++------------
 testcases/kernel/syscalls/chroot/chroot01.c   |   4 +-
 .../kernel/syscalls/fadvise/posix_fadvise01.c | 135 +++----------
 .../kernel/syscalls/fadvise/posix_fadvise03.c | 163 ++++-----------
 testcases/kernel/syscalls/fchmod/fchmod02.c   |   7 +-
 6 files changed, 148 insertions(+), 522 deletions(-)

1] https://android-review.googlesource.com/c/platform/external/ltp/+/813818
   https://android-review.googlesource.com/c/platform/external/ltp/+/813822

-- 
2.19.1.930.g4563a0d9d0-goog

[LTP] [PATCH 1/5] chroot01: Use the 'tmpdir' we create.

[Sandeep Patil]

Remove hard coded use of /tmp that breaks this test for Android and use
the tmpdir() we create in the setup() instead.

Signed-off-by: Sandeep Patil <sspatil@google.com>
---
 testcases/kernel/syscalls/chroot/chroot01.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/chroot/chroot01.c b/testcases/kernel/syscalls/chroot/chroot01.c
index 9b0c30cf1..a1db5e157 100644
--- a/testcases/kernel/syscalls/chroot/chroot01.c
+++ b/testcases/kernel/syscalls/chroot/chroot01.c
@@ -54,7 +54,7 @@ char *TCID = "chroot01";
 int TST_TOTAL = 1;
 int fail;
 
-char path[] = "/tmp";
+char *path;
 
 char nobody_uid[] = "nobody";
 struct passwd *ltpuser;
@@ -94,6 +94,7 @@ void setup(void)
 	tst_require_root();
 
 	tst_tmpdir();
+	path = tst_get_tmpdir();
 
 	if ((ltpuser = getpwnam(nobody_uid)) == NULL)
 		tst_brkm(TBROK | TERRNO, cleanup,
@@ -110,5 +111,6 @@ void cleanup(void)
 {
 	SAFE_SETEUID(NULL, 0);
 
+	free(path);
 	tst_rmdir();
 }
-- 
2.19.1.930.g4563a0d9d0-goog

[LTP] [PATCH 1/5] chroot01: Use the 'tmpdir' we create.

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/5] syscalls/chmod07: Use new ltp library

[Sandeep Patil]

Use SPDX-Licence-Identifier and delete dead comments / code.
Side benefit of making sure the test runs on Android when using
SAFE_GETGRNAM_FALLBACK()

Signed-off-by: Sandeep Patil <sspatil@google.com>
---
 testcases/kernel/syscalls/chmod/chmod07.c | 174 ++++++----------------
 1 file changed, 42 insertions(+), 132 deletions(-)

diff --git a/testcases/kernel/syscalls/chmod/chmod07.c b/testcases/kernel/syscalls/chmod/chmod07.c
index 6a3938840..4e43f2e32 100644
--- a/testcases/kernel/syscalls/chmod/chmod07.c
+++ b/testcases/kernel/syscalls/chmod/chmod07.c
@@ -1,20 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- *
  *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 /*
@@ -31,43 +17,6 @@
  * Expected Result:
  *  chmod() should return value 0 on success and succeeds to set sticky bit
  *  on the specified file.
- *
- * Algorithm:
- *  Setup:
- *   Setup signal handling.
- *   Create temporary directory.
- *   Pause for SIGUSR1 if option specified.
- *
- *  Test:
- *   Loop if the proper options are given.
- *   Execute system call
- *   Check return code, if system call failed (return=-1)
- *   	Log the errno and Issue a FAIL message.
- *   Otherwise,
- *   	Verify the Functionality of system call
- *      if successful,
- *      	Issue Functionality-Pass message.
- *      Otherwise,
- *		Issue Functionality-Fail message.
- *  Cleanup:
- *   Print errno log and/or timing stats if options given
- *   Delete the temporary directory created.
- *
- * Usage:  <for command-line>
- *  chmod07 [-c n] [-f] [-i n] [-I x] [-P x] [-t]
- *     where,  -c n : Run n copies concurrently.
- *             -f   : Turn off functionality Testing.
- *	       -i n : Execute test n times.
- *	       -I x : Execute test for x seconds.
- *	       -P x : Pause for x seconds between iterations.
- *	       -t   : Turn on syscall timing.
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *
- * RESTRICTIONS:
- *  This test should be run by 'super-user' (root) only.
- *
  */
 
 #include <stdio.h>
@@ -80,75 +29,46 @@
 #include <grp.h>
 #include <pwd.h>
 
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 
-#define LTPUSER		"nobody"
-#define LTPGRP		"users"
-#define FILE_MODE 	(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH)
-#define PERMS		01777	/*
-				 * Mode permissions of test file with sticky
-				 * bit set.
-				 */
+#define FILE_MODE 	(S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)
+#define PERMS		01777	/* Permissions with sticky bit set. */
 #define TESTFILE	"testfile"
 
-char *TCID = "chmod07";
-int TST_TOTAL = 1;
-
-void setup();			/* Main setup function for the test */
-void cleanup();			/* Main cleanup function for the test */
-
-int main(int ac, char **av)
+void test_chmod(void)
 {
 	struct stat stat_buf;	/* stat(2) struct contents */
-	int lc;
-
-	tst_parse_opts(ac, av, NULL, NULL);
 
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		/*
-		 * Call chmod(2) with specified mode argument
-		 * (sticky-bit set) on testfile.
-		 */
-		TEST(chmod(TESTFILE, PERMS));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL | TTERRNO, "chmod(%s, %#o) failed",
-				 TESTFILE, PERMS);
-			continue;
-		}
-		/*
-		 * Get the testfile information using
-		 * stat(2).
-		 */
-		if (stat(TESTFILE, &stat_buf) == -1)
-			tst_brkm(TFAIL | TTERRNO, cleanup,
-				 "stat failed");
+	/*
+	 * Call chmod(2) with specified mode argument
+	 * (sticky-bit set) on testfile.
+	 */
+	TEST(chmod(TESTFILE, PERMS));
+	if (TST_RET == -1) {
+		tst_brk(TFAIL | TTERRNO, "chmod(%s, %#o) failed",
+				TESTFILE, PERMS);
+	}
 
-		/* Check for expected mode permissions */
-		if ((stat_buf.st_mode & PERMS) == PERMS)
-			tst_resm(TPASS, "Functionality of "
-				 "chmod(%s, %#o) successful",
-				 TESTFILE, PERMS);
-		else
-			tst_resm(TFAIL, "%s: Incorrect modes 0%03o; "
-				 "expected 0%03o", TESTFILE,
-				 stat_buf.st_mode, PERMS);
+	/*
+	 * Get the testfile information using
+	 * stat(2).
+	 */
+	if (stat(TESTFILE, &stat_buf) == -1) {
+		tst_brk(TFAIL | TTERRNO, "stat failed");
 	}
 
-	cleanup();
-	tst_exit();
+	/* Check for expected mode permissions */
+	if ((stat_buf.st_mode & PERMS) == PERMS) {
+		tst_res(TPASS, "Functionality of chmod(%s, %#o) successful",
+				TESTFILE, PERMS);
+	} else {
+		tst_res(TFAIL, "%s: Incorrect modes 0%03o; expected 0%03o",
+				TESTFILE, stat_buf.st_mode, PERMS);
+	}
 }
 
 /*
- * void
- * setup() - performs all ONE TIME setup for this test.
- *  Create a temporary directory and change directory to it.
+ *  setup() - performs all ONE TIME setup for this test.
  *  Create a test file under temporary directory and close it
  *  Change the ownership of test file to that of "ltpuser1" user.
  */
@@ -160,32 +80,22 @@ void setup(void)
 	gid_t group1_gid;	/* user and process group id's */
 	uid_t user1_uid;
 
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
-
-	tst_require_root();
-
-	tst_tmpdir();
-
-	/* Get the uid of guest user - ltpuser1 */
-	if ((ltpuser = getpwnam(LTPUSER)) == NULL)
-		tst_brkm(TBROK, cleanup, "getpwnam failed");
+	/* Get the uid of guest user - nobody */
+	ltpuser = SAFE_GETPWNAM("nobody");
 	user1_uid = ltpuser->pw_uid;
 
-	/* Get the group id of guest user - ltpuser1 */
-	if ((ltpgroup = getgrnam(LTPGRP)) == NULL)
-		tst_brkm(TBROK, cleanup, "getgrnam failed");
+	ltpgroup = SAFE_GETGRNAM_FALLBACK("users", "daemon");
 	group1_gid = ltpgroup->gr_gid;
 
-	fd = SAFE_OPEN(cleanup, TESTFILE, O_RDWR | O_CREAT, FILE_MODE);
-	SAFE_CLOSE(cleanup, fd);
-	SAFE_CHOWN(cleanup, TESTFILE, user1_uid, group1_gid);
-
-	SAFE_SETGID(cleanup, group1_gid);
+	fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, FILE_MODE);
+	SAFE_CLOSE(fd);
+	SAFE_CHOWN(TESTFILE, user1_uid, group1_gid);
+	SAFE_SETGID(group1_gid);
 }
 
-void cleanup(void)
-{
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test_all = test_chmod,
+};
-- 
2.19.1.930.g4563a0d9d0-goog

[LTP] [PATCH 2/5] syscalls/chmod07: Use new ltp library

[Cyril Hrubis]

Hi!
> ---
>  testcases/kernel/syscalls/chmod/chmod07.c | 174 ++++++----------------
>  1 file changed, 42 insertions(+), 132 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/chmod/chmod07.c b/testcases/kernel/syscalls/chmod/chmod07.c
> index 6a3938840..4e43f2e32 100644
> --- a/testcases/kernel/syscalls/chmod/chmod07.c
> +++ b/testcases/kernel/syscalls/chmod/chmod07.c
> @@ -1,20 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0

Should have been GPL-2.0-or-later

I fixed that and also removed a few useless comments in the source and
pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 3/5] syscalls/fchmod02: fall back to use "daemon" group if "users" is absent.

[Sandeep Patil]

This makes the test work successfully on Android systems where
"users" group doesn't exist.

Signed-off-by: Sandeep Patil <sspatil@google.com>
---
 testcases/kernel/syscalls/fchmod/fchmod02.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/testcases/kernel/syscalls/fchmod/fchmod02.c b/testcases/kernel/syscalls/fchmod/fchmod02.c
index d0b35db12..e60cb33a6 100644
--- a/testcases/kernel/syscalls/fchmod/fchmod02.c
+++ b/testcases/kernel/syscalls/fchmod/fchmod02.c
@@ -22,9 +22,6 @@
 #include "tst_test.h"
 #include "fchmod.h"
 
-#define LTPUSER		"nobody"
-#define LTPGRP		"users"
-
 static int fd;
 
 static void verify_fchmod(void)
@@ -53,8 +50,8 @@ static void setup(void)
 	struct passwd *ltpuser;
 	struct group *ltpgroup;
 
-	ltpuser = SAFE_GETPWNAM(LTPUSER);
-	ltpgroup = SAFE_GETGRNAM(LTPGRP);
+	ltpuser = SAFE_GETPWNAM("nobody");
+	ltpgroup = SAFE_GETGRNAM_FALLBACK("users", "daemon");
 
 	fd = SAFE_OPEN(TESTFILE, O_RDWR | O_CREAT, FILE_MODE);
 	SAFE_CHOWN(TESTFILE, ltpuser->pw_uid, ltpgroup->gr_gid);
-- 
2.19.1.930.g4563a0d9d0-goog

[LTP] [PATCH 3/5] syscalls/fchmod02: fall back to use "daemon" group if "users" is absent.

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 4/5] syscalls/chmod05: Use new ltp library

[Sandeep Patil]

Use SPDX-Licence-Identifier and delete dead comments / code while at it.

Signed-off-by: Sandeep Patil <sspatil@google.com>
---
 testcases/kernel/syscalls/chmod/chmod05.c | 187 ++++------------------
 1 file changed, 33 insertions(+), 154 deletions(-)

diff --git a/testcases/kernel/syscalls/chmod/chmod05.c b/testcases/kernel/syscalls/chmod/chmod05.c
index 3cf4db5d0..c18a4778a 100644
--- a/testcases/kernel/syscalls/chmod/chmod05.c
+++ b/testcases/kernel/syscalls/chmod/chmod05.c
@@ -1,30 +1,12 @@
+/// SPDX-License-Identifier: GPL-2.0
 /*
- *
  *   Copyright (c) International Business Machines  Corp., 2001
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 /*
  * Test Name: chmod05
  *
  * Test Description:
-//wjh
-//This test seems to be invalid see comments below
-//The man page for chmod doesn't seem to indicate that the setgid bit can
-//only be set by root
  *  Verify that, chmod(2) will succeed to change the mode of a directory
  *  but fails to set the setgid bit on it if invoked by non-root (uid != 0)
  *  process with the following constraints,
@@ -36,48 +18,6 @@
  *  chmod() should return value 0 on success and though succeeds to change
  *  the mode of a directory but fails to set setgid bit on it.
  *
- * Algorithm:
- *  Setup:
- *   Setup signal handling.
- *   Create temporary directory.
- *   Pause for SIGUSR1 if option specified.
- *
- *  Test:
- *   Loop if the proper options are given.
- *   Execute system call
- *   Check return code, if system call failed (return=-1)
- *	Log the errno and Issue a FAIL message.
- *   Otherwise,
- *	Verify the Functionality of system call
- *      if successful,
- *		Issue Functionality-Pass message.
- *      Otherwise,
- *		Issue Functionality-Fail message.
- *  Cleanup:
- *   Print errno log and/or timing stats if options given
- *   Delete the temporary directory created.
- *
- * Usage:  <for command-line>
- *  chmod05 [-c n] [-e] [-f] [-i n] [-I x] [-P x] [-t]
- *     where,  -c n : Run n copies concurrently.
- *             -e   : Turn on errno logging.
- *             -f   : Turn off functionality Testing.
- *	       -i n : Execute test n times.
- *	       -I x : Execute test for x seconds.
- *	       -P x : Pause for x seconds between iterations.
- *	       -t   : Turn on syscall timing.
- *
- * HISTORY
- *	07/2001 Ported by Wayne Boyer
- *	05/2002 Fixes by wjhuie and Ferhan Shareef
- *                  changed conditional to check for valid tests only and
- *                  in a more logically understandable way
- *	07/2002 Additional fixes to #defines to allow comparisions to work.
- *		-Robbie Williamson
- *
- * RESTRICTIONS:
- *  This test should be run by root.
- *
  */
 
 #ifndef _GNU_SOURCE
@@ -96,126 +36,65 @@
 #include <grp.h>
 #include <pwd.h>
 
-#include "test.h"
-#include "safe_macros.h"
-
-#define DEBUG 0
+#include "tst_test.h"
 
 #define MODE_RWX	(mode_t)(S_IRWXU | S_IRWXG | S_IRWXO)
 #define DIR_MODE	(mode_t)(S_ISVTX | S_ISGID | S_IFDIR)
 #define PERMS		(mode_t)(MODE_RWX | DIR_MODE)
 #define TESTDIR		"testdir"
 
-char *TCID = "chmod05";
-int TST_TOTAL = 1;
-
-void setup();			/* Main setup function for test */
-void cleanup();			/* Main cleanup function for test */
-
-int main(int ac, char **av)
+void test_chmod(void)
 {
 	struct stat stat_buf;	/* stat struct */
-	int lc;
 	mode_t dir_mode;	/* mode permissions set on test directory */
 
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		TEST(chmod(TESTDIR, PERMS));
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL, "chmod(%s, %#o) failed",
-				 TESTDIR, PERMS);
-			continue;
-		}
-		/*
-		 * Get the directory information using
-		 * stat(2).
-		 */
-		if (stat(TESTDIR, &stat_buf) < 0) {
-			tst_brkm(TFAIL | TERRNO, cleanup,
-				 "stat(%s) failed", TESTDIR);
-		}
-		dir_mode = stat_buf.st_mode;
-#if DEBUG
-		printf("DIR_MODE = 0%03o\n", DIR_MODE);
-		printf("MODE_RWX = 0%03o\n", MODE_RWX);
-		printf("PERMS = 0%03o\n", PERMS);
-		printf("dir_mode = 0%03o\n", dir_mode);
-#endif
-		if ((PERMS & ~S_ISGID) != dir_mode)
-			tst_resm(TFAIL, "%s: Incorrect modes 0%03o, "
-				 "Expected 0%03o", TESTDIR, dir_mode,
-				 PERMS & ~S_ISGID);
-		else
-			tst_resm(TPASS,
-				 "Functionality of chmod(%s, %#o) successful",
-					 TESTDIR, PERMS);
+	TEST(chmod(TESTDIR, PERMS));
+	if (TST_RET == -1) {
+		tst_res(TFAIL, "chmod(%s, %#o) failed", TESTDIR, PERMS);
 	}
 
-	cleanup();
-	tst_exit();
+	/* Get the directory information using stat(2) and compare with
+	 * expected results.
+	 */
+	SAFE_STAT(TESTDIR, &stat_buf);
+	dir_mode = stat_buf.st_mode;
+	if ((PERMS & ~S_ISGID) != dir_mode) {
+		tst_res(TFAIL, "%s: Incorrect modes 0%03o, "
+				"Expected 0%03o", TESTDIR, dir_mode,
+				PERMS & ~S_ISGID);
+	} else {
+		tst_res(TPASS, "Functionality of chmod(%s, %#o) successful",
+				TESTDIR, PERMS);
+	}
 }
 
-/*
- * void
- * setup() - performs all ONE TIME setup for this test.
- *  Create a temporary directory and cd to it.
- *  Create a test directory under temporary directory.
-//wjh we are root so do we really need this kluged helper program?
- *  Invoke setuid to root program to modify group ownership
- *  on test directory.
- */
 void setup(void)
 {
 	struct passwd *nobody_u;
-	struct group *bin_group;
-
-	tst_require_root();
+	struct group *bin_gr;
 
-	TEST_PAUSE;
-
-	tst_tmpdir();
-
-	nobody_u = getpwnam("nobody");
-	if (nobody_u == NULL)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "getpwnam(\"nobody\") failed");
-
-	bin_group = getgrnam("bin");
-	if (bin_group == NULL)
-		tst_brkm(TBROK | TERRNO, cleanup, "getgrnam(\"bin\") failed");
+	nobody_u = SAFE_GETPWNAM("nobody");
+	bin_gr = SAFE_GETGRNAM("bin");
 
 	/*
 	 * Create a test directory under temporary directory with specified
 	 * mode permissions and change the gid of test directory to nobody's
 	 * gid.
 	 */
-	SAFE_MKDIR(cleanup, TESTDIR, MODE_RWX);
-
+	SAFE_MKDIR(TESTDIR, MODE_RWX);
 	if (setgroups(1, &nobody_u->pw_gid) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "setgroups to nobody's gid failed");
+		tst_brk(TBROK | TERRNO, "setgroups to nobody's gid failed");
 
-	SAFE_CHOWN(cleanup, TESTDIR, nobody_u->pw_uid, bin_group->gr_gid);
+	SAFE_CHOWN(TESTDIR, nobody_u->pw_uid, bin_group->gr_gid);
 
 	/* change to nobody:nobody */
-	if (setegid(nobody_u->pw_gid) == -1 || seteuid(nobody_u->pw_uid) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup,
-			 "Couldn't switch to nobody:nobody");
+	SAGE_SETEGID(nobody_u->pw_gid);
+	SAFE_SETEUID(nobody_u->pw_uid);
 }
 
-void cleanup(void)
-{
-	if (setegid(0) == -1)
-		tst_resm(TWARN | TERRNO, "setegid(0) failed");
-	if (seteuid(0) == -1)
-		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
-
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test_all = test_chmod,
+};
-- 
2.19.1.930.g4563a0d9d0-goog

[LTP] [PATCH 4/5] syscalls/chmod05: Use new ltp library

[Sandeep Patil]

On Mon, Nov 05, 2018 at 03:50:18PM -0800, Sandeep Patil wrote:
> Use SPDX-Licence-Identifier and delete dead comments / code while at it.
> 
> Signed-off-by: Sandeep Patil <sspatil@google.com>
> ---
>  testcases/kernel/syscalls/chmod/chmod05.c | 187 ++++------------------
>  1 file changed, 33 insertions(+), 154 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/chmod/chmod05.c b/testcases/kernel/syscalls/chmod/chmod05.c
> index 3cf4db5d0..c18a4778a 100644
> --- a/testcases/kernel/syscalls/chmod/chmod05.c
> +++ b/testcases/kernel/syscalls/chmod/chmod05.c
> @@ -1,30 +1,12 @@
> +/// SPDX-License-Identifier: GPL-2.0
>  /*
> - *
>   *   Copyright (c) International Business Machines  Corp., 2001
> - *
> - *   This program is free software;  you can redistribute it and/or modify
> - *   it under the terms of the GNU General Public License as published by
> - *   the Free Software Foundation; either version 2 of the License, or
> - *   (at your option) any later version.
> - *
> - *   This program is distributed in the hope that it will be useful,
> - *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
> - *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> - *   the GNU General Public License for more details.
> - *
> - *   You should have received a copy of the GNU General Public License
> - *   along with this program;  if not, write to the Free Software
> - *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
>  /*
>   * Test Name: chmod05
>   *
>   * Test Description:
> -//wjh
> -//This test seems to be invalid see comments below
> -//The man page for chmod doesn't seem to indicate that the setgid bit can
> -//only be set by root
>   *  Verify that, chmod(2) will succeed to change the mode of a directory
>   *  but fails to set the setgid bit on it if invoked by non-root (uid != 0)
>   *  process with the following constraints,
> @@ -36,48 +18,6 @@
>   *  chmod() should return value 0 on success and though succeeds to change
>   *  the mode of a directory but fails to set setgid bit on it.
>   *
> - * Algorithm:
> - *  Setup:
> - *   Setup signal handling.
> - *   Create temporary directory.
> - *   Pause for SIGUSR1 if option specified.
> - *
> - *  Test:
> - *   Loop if the proper options are given.
> - *   Execute system call
> - *   Check return code, if system call failed (return=-1)
> - *	Log the errno and Issue a FAIL message.
> - *   Otherwise,
> - *	Verify the Functionality of system call
> - *      if successful,
> - *		Issue Functionality-Pass message.
> - *      Otherwise,
> - *		Issue Functionality-Fail message.
> - *  Cleanup:
> - *   Print errno log and/or timing stats if options given
> - *   Delete the temporary directory created.
> - *
> - * Usage:  <for command-line>
> - *  chmod05 [-c n] [-e] [-f] [-i n] [-I x] [-P x] [-t]
> - *     where,  -c n : Run n copies concurrently.
> - *             -e   : Turn on errno logging.
> - *             -f   : Turn off functionality Testing.
> - *	       -i n : Execute test n times.
> - *	       -I x : Execute test for x seconds.
> - *	       -P x : Pause for x seconds between iterations.
> - *	       -t   : Turn on syscall timing.
> - *
> - * HISTORY
> - *	07/2001 Ported by Wayne Boyer
> - *	05/2002 Fixes by wjhuie and Ferhan Shareef
> - *                  changed conditional to check for valid tests only and
> - *                  in a more logically understandable way
> - *	07/2002 Additional fixes to #defines to allow comparisions to work.
> - *		-Robbie Williamson
> - *
> - * RESTRICTIONS:
> - *  This test should be run by root.
> - *
>   */
>  
>  #ifndef _GNU_SOURCE
> @@ -96,126 +36,65 @@
>  #include <grp.h>
>  #include <pwd.h>
>  
> -#include "test.h"
> -#include "safe_macros.h"
> -
> -#define DEBUG 0
> +#include "tst_test.h"
>  
>  #define MODE_RWX	(mode_t)(S_IRWXU | S_IRWXG | S_IRWXO)
>  #define DIR_MODE	(mode_t)(S_ISVTX | S_ISGID | S_IFDIR)
>  #define PERMS		(mode_t)(MODE_RWX | DIR_MODE)
>  #define TESTDIR		"testdir"
>  
> -char *TCID = "chmod05";
> -int TST_TOTAL = 1;
> -
> -void setup();			/* Main setup function for test */
> -void cleanup();			/* Main cleanup function for test */
> -
> -int main(int ac, char **av)
> +void test_chmod(void)
>  {
>  	struct stat stat_buf;	/* stat struct */
> -	int lc;
>  	mode_t dir_mode;	/* mode permissions set on test directory */
>  
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -	setup();
> -
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -		tst_count = 0;
> -
> -		TEST(chmod(TESTDIR, PERMS));
> -
> -		if (TEST_RETURN == -1) {
> -			tst_resm(TFAIL, "chmod(%s, %#o) failed",
> -				 TESTDIR, PERMS);
> -			continue;
> -		}
> -		/*
> -		 * Get the directory information using
> -		 * stat(2).
> -		 */
> -		if (stat(TESTDIR, &stat_buf) < 0) {
> -			tst_brkm(TFAIL | TERRNO, cleanup,
> -				 "stat(%s) failed", TESTDIR);
> -		}
> -		dir_mode = stat_buf.st_mode;
> -#if DEBUG
> -		printf("DIR_MODE = 0%03o\n", DIR_MODE);
> -		printf("MODE_RWX = 0%03o\n", MODE_RWX);
> -		printf("PERMS = 0%03o\n", PERMS);
> -		printf("dir_mode = 0%03o\n", dir_mode);
> -#endif
> -		if ((PERMS & ~S_ISGID) != dir_mode)
> -			tst_resm(TFAIL, "%s: Incorrect modes 0%03o, "
> -				 "Expected 0%03o", TESTDIR, dir_mode,
> -				 PERMS & ~S_ISGID);
> -		else
> -			tst_resm(TPASS,
> -				 "Functionality of chmod(%s, %#o) successful",
> -					 TESTDIR, PERMS);
> +	TEST(chmod(TESTDIR, PERMS));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL, "chmod(%s, %#o) failed", TESTDIR, PERMS);
>  	}
>  
> -	cleanup();
> -	tst_exit();
> +	/* Get the directory information using stat(2) and compare with
> +	 * expected results.
> +	 */
> +	SAFE_STAT(TESTDIR, &stat_buf);
> +	dir_mode = stat_buf.st_mode;
> +	if ((PERMS & ~S_ISGID) != dir_mode) {
> +		tst_res(TFAIL, "%s: Incorrect modes 0%03o, "
> +				"Expected 0%03o", TESTDIR, dir_mode,
> +				PERMS & ~S_ISGID);
> +	} else {
> +		tst_res(TPASS, "Functionality of chmod(%s, %#o) successful",
> +				TESTDIR, PERMS);
> +	}
>  }
>  
> -/*
> - * void
> - * setup() - performs all ONE TIME setup for this test.
> - *  Create a temporary directory and cd to it.
> - *  Create a test directory under temporary directory.
> -//wjh we are root so do we really need this kluged helper program?
> - *  Invoke setuid to root program to modify group ownership
> - *  on test directory.
> - */
>  void setup(void)
>  {
>  	struct passwd *nobody_u;
> -	struct group *bin_group;
> -
> -	tst_require_root();
> +	struct group *bin_gr;
>  
> -	TEST_PAUSE;
> -
> -	tst_tmpdir();
> -
> -	nobody_u = getpwnam("nobody");
> -	if (nobody_u == NULL)
> -		tst_brkm(TBROK | TERRNO, cleanup,
> -			 "getpwnam(\"nobody\") failed");
> -
> -	bin_group = getgrnam("bin");
> -	if (bin_group == NULL)
> -		tst_brkm(TBROK | TERRNO, cleanup, "getgrnam(\"bin\") failed");
> +	nobody_u = SAFE_GETPWNAM("nobody");
> +	bin_gr = SAFE_GETGRNAM("bin");
>  
>  	/*
>  	 * Create a test directory under temporary directory with specified
>  	 * mode permissions and change the gid of test directory to nobody's
>  	 * gid.
>  	 */
> -	SAFE_MKDIR(cleanup, TESTDIR, MODE_RWX);
> -
> +	SAFE_MKDIR(TESTDIR, MODE_RWX);
>  	if (setgroups(1, &nobody_u->pw_gid) == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup,
> -			 "setgroups to nobody's gid failed");
> +		tst_brk(TBROK | TERRNO, "setgroups to nobody's gid failed");
>  
> -	SAFE_CHOWN(cleanup, TESTDIR, nobody_u->pw_uid, bin_group->gr_gid);
> +	SAFE_CHOWN(TESTDIR, nobody_u->pw_uid, bin_group->gr_gid);
>  
>  	/* change to nobody:nobody */
> -	if (setegid(nobody_u->pw_gid) == -1 || seteuid(nobody_u->pw_uid) == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup,
> -			 "Couldn't switch to nobody:nobody");
> +	SAGE_SETEGID(nobody_u->pw_gid);
> +	SAFE_SETEUID(nobody_u->pw_uid);
>  }
>  
> -void cleanup(void)
> -{
> -	if (setegid(0) == -1)
> -		tst_resm(TWARN | TERRNO, "setegid(0) failed");
> -	if (seteuid(0) == -1)
> -		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
> -

It looks like I need to do this in some cleanup function for the new test.
Otherwise the library cleanup won't be able to delete the tmpdir. Let me
rework this..

- ssp

> -	tst_rmdir();
> -}
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.needs_tmpdir = 1,
> +	.setup = setup,
> +	.test_all = test_chmod,
> +};
> -- 
> 2.19.1.930.g4563a0d9d0-goog
> 

[LTP] [PATCH 4/5] syscalls/chmod05: Use new ltp library

[Cyril Hrubis]

Hi!
> > -void cleanup(void)
> > -{
> > -	if (setegid(0) == -1)
> > -		tst_resm(TWARN | TERRNO, "setegid(0) failed");
> > -	if (seteuid(0) == -1)
> > -		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
> > -
> 
> It looks like I need to do this in some cleanup function for the new test.
> Otherwise the library cleanup won't be able to delete the tmpdir. Let me
> rework this..

Actually the temporary directory cleanup in the new library is executed
from the parent process, the actuall test is executed from a forked
child so it shouldn't matter what euid and egid the child process ends
up with.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 4/5] syscalls/chmod05: Use new ltp library

[Sandeep Patil]

On Tue, Nov 06, 2018 at 05:01:19PM +0100, Cyril Hrubis wrote:
> Hi!
> > > -void cleanup(void)
> > > -{
> > > -	if (setegid(0) == -1)
> > > -		tst_resm(TWARN | TERRNO, "setegid(0) failed");
> > > -	if (seteuid(0) == -1)
> > > -		tst_resm(TWARN | TERRNO, "seteuid(0) failed");
> > > -
> > 
> > It looks like I need to do this in some cleanup function for the new test.
> > Otherwise the library cleanup won't be able to delete the tmpdir. Let me
> > rework this..
> 
> Actually the temporary directory cleanup in the new library is executed
> from the parent process, the actuall test is executed from a forked
> child so it shouldn't matter what euid and egid the child process ends
> up with.

Well too late, I added it again :). Anyway, this patch suffered from build
errors too. Turns out 'make install' doesn't build things again so I missed
them. I have fixed them up now and reset, feel free to delete the cleanup
function I added there (will respond there too)

 -ssp

> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz
> 
> -- 
> You received this message because you are subscribed to the Google Groups "kernel-team" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> 

[LTP] [PATCH 5/5] syscalls/posix_fadvise0[13]: Start using new library.

[Sandeep Patil]

Use SPDX-Licence-Identifier and delete dead comments / code while at it.
Make sure the tests can work on any system by creating its own file
and using it for testing.

Signed-off-by: Sandeep Patil <sspatil@google.com>
---
 .../kernel/syscalls/fadvise/posix_fadvise01.c | 135 ++++-----------
 .../kernel/syscalls/fadvise/posix_fadvise03.c | 163 ++++--------------
 2 files changed, 68 insertions(+), 230 deletions(-)

diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
index c12f0563c..ad139a66b 100644
--- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
+++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
@@ -1,20 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- *
  *   Copyright (c) Red Hat Inc., 2007
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 /*
@@ -34,15 +20,13 @@
  *	None
  */
 
-#define _XOPEN_SOURCE 600
 #include <fcntl.h>
 
 #include <unistd.h>
 #include <signal.h>
 #include <errno.h>
 
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 
 #include "lapi/syscalls.h"
 #ifndef _FILE_OFFSET_BITS
@@ -53,15 +37,8 @@
 #define __NR_fadvise64 0
 #endif
 
-void setup();
-void cleanup();
-
-TCID_DEFINE(posix_fadvise01);
-
-char fname[] = "/bin/cat";	/* test executable to open */
-int fd = -1;			/* initialized in open */
-
-int expected_return = 0;
+const char *fname = "testfile";
+int fd = -1;
 
 int defined_advise[] = {
 	POSIX_FADV_NORMAL,
@@ -72,91 +49,41 @@ int defined_advise[] = {
 	POSIX_FADV_DONTNEED,
 };
 
-#define defined_advise_total ARRAY_SIZE(defined_advise)
-
-int TST_TOTAL = defined_advise_total;
-
-int main(int ac, char **av)
+void test_posix_fadvise(unsigned int nr)
 {
-	int lc;
-	int i;
-
-	/* Check this system has fadvise64 system which is used
-	   in posix_fadvise. */
-	if ((_FILE_OFFSET_BITS != 64) && (__NR_fadvise64 == 0)) {
-		tst_resm(TWARN,
-			 "This test can only run on kernels that implements ");
-		tst_resm(TWARN, "fadvise64 which is used from posix_fadvise");
-		exit(0);
+	TEST(posix_fadvise(fd, 0, 0, defined_advise[nr]));
+	if (TST_RET == 0) {
+		tst_res(TPASS, "call succeeded expectedly");
+	} else {
+		tst_res(TFAIL, "unexpected return - %ld w/ advise %d",
+				TST_RET, defined_advise[nr]);
 	}
-
-	/*
-	 * parse standard options
-	 */
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	/*
-	 * perform global setup for test
-	 */
-	setup();
-
-	/*
-	 * check looping state if -i option given on the command line
-	 */
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		/* loop through the test cases */
-		for (i = 0; i < defined_advise_total; i++) {
-
-			TEST(posix_fadvise(fd, 0, 0, defined_advise[i]));
-
-			/* Man page says:
-			   "On error, an error number is returned." */
-			if (TEST_RETURN == expected_return) {
-				tst_resm(TPASS, "call succeeded expectedly");
-			} else {
-				tst_resm(TFAIL,
-					 "unexpected return value - %ld : %s, advise %d - "
-					 "expected %d",
-					 TEST_RETURN,
-					 strerror(TEST_RETURN),
-					 defined_advise[i], expected_return);
-			}
-		}
-	}
-
-	/*
-	 * cleanup and exit
-	 */
-	cleanup();
-
-	tst_exit();
 }
 
-/*
- * setup() - performs all ONE TIME setup for this test.
- */
 void setup(void)
 {
+	unsigned long pagesz = getpagesize();
+	char buf[10 * pagesz];
 
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
+	/* Check this system has fadvise64 system which is used
+	   in posix_fadvise. */
+	if ((_FILE_OFFSET_BITS != 64) && (__NR_fadvise64 == 0)) {
+		tst_brk(TCONF,
+			"This test can only run on kernels that implements "
+			"fadvise64 which is used from posix_fadvise");
+	}
 
-	TEST_PAUSE;
+	/* create 10 x pagesize file to be used for fadvise checks */
+	fd = SAFE_CREAT(fname, 0644);
+	SAFE_WRITE(1, fd, buf, 10 * pagesz);
+	SAFE_CLOSE(fd);
 
-	fd = SAFE_OPEN(cleanup, fname, O_RDONLY);
+	fd = SAFE_OPEN(fname, O_RDONLY);
 }
 
-/*
- * cleanup() - performs all ONE TIME cleanup for this test at
- *		completion or premature exit.
- */
-void cleanup(void)
-{
-
-	if (fd != -1) {
-		close(fd);
-	}
-
-}
+struct tst_test test = {
+	.tcnt = ARRAY_SIZE(defined_advise),
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test = test_posix_fadvise,
+};
diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise03.c b/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
index 4aa3a8cd1..ffb196743 100644
--- a/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
+++ b/testcases/kernel/syscalls/fadvise/posix_fadvise03.c
@@ -1,20 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
- *
  *   Copyright (c) Red Hat Inc., 2007
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY;  without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
- *   the GNU General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
 /*
@@ -34,14 +20,12 @@
  *	None
  */
 
-#define _XOPEN_SOURCE 600
 #include <fcntl.h>
 #include <unistd.h>
 #include <signal.h>
 #include <errno.h>
 #include <limits.h>
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 
 #include "lapi/syscalls.h"
 #ifndef _FILE_OFFSET_BITS
@@ -52,14 +36,10 @@
 #define __NR_fadvise64 0
 #endif
 
-void setup();
-void cleanup();
-
-TCID_DEFINE(posix_fadvise03);
-
-char fname[] = "/bin/cat";	/* test executable to open */
-int fd = -1;			/* initialized in open */
+#define ADVISE_LIMIT	(32)
 
+const char *fname = "testfile";
+int fd = -1;
 int expected_error = EINVAL;
 
 int defined_advise[] = {
@@ -87,24 +67,10 @@ int defined_advise[] = {
 #endif
 };
 
-#define defined_advise_total ARRAY_SIZE(defined_advise)
-
-#if 0
-/* Too many test cases. */
-int TST_TOTAL = (INT_MAX - defined_advise_total);
-int advise_limit = INT_MAX;
-#else
-int TST_TOTAL = (32 - defined_advise_total);
-int advise_limit = 32;
-#endif /* 0 */
-
-/* is_defined_advise:
-   Return 1 if advise is in defined_advise.
-   Return 0 if not. */
 static int is_defined_advise(int advise)
 {
 	int i;
-	for (i = 0; i < defined_advise_total; i++) {
+	for (i = 0; i < ARRAY_SIZE(defined_advise); i++) {
 		if (defined_advise[i] == advise)
 			return 1;
 	}
@@ -112,101 +78,46 @@ static int is_defined_advise(int advise)
 	return 0;
 }
 
-int main(int ac, char **av)
+int test_posix_fadvise(unsigned int nr)
 {
-	int lc;
-	int advise;
-
-	/* Check this system has fadvise64 system which is used
-	   in posix_fadvise. */
-	if ((_FILE_OFFSET_BITS != 64) && (__NR_fadvise64 == 0)) {
-		tst_resm(TWARN,
-			 "This test can only run on kernels that implements ");
-		tst_resm(TWARN, "fadvise64 which is used from posix_fadvise");
-		exit(0);
+	int advise = nr;
+	
+	/* Don't use defined advise as an argument. */
+	if (is_defined_advise(advise)) {
+		return;
 	}
 
-	/*
-	 * parse standard options
-	 */
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	/*
-	 * perform global setup for test
-	 */
-	setup();
-
-	/*
-	 * check looping state if -i option given on the command line
-	 */
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		/* loop through the test cases */
-		for (advise = 0; advise < advise_limit; advise++) {
-
-			/* Don't use defiend advise as an argument. */
-			if (is_defined_advise(advise)) {
-				continue;
-			}
-
-			TEST(posix_fadvise(fd, 0, 0, advise));
-
-			if (TEST_RETURN == 0) {
-				tst_resm(TFAIL, "call succeeded unexpectedly");
-				continue;
-			}
-
-			/* Man page says:
-			   "On error, an error number is returned." */
-			if (TEST_RETURN == expected_error) {
-				tst_resm(TPASS,
-					 "expected failure - "
-					 "returned value = %ld, advise = %d : %s",
-					 TEST_RETURN,
-					 advise, strerror(TEST_RETURN));
-			} else {
-				tst_resm(TFAIL,
-					 "unexpected return value - %ld : %s, advise %d - "
-					 "expected %d",
-					 TEST_RETURN,
-					 strerror(TEST_RETURN),
-					 advise, expected_error);
-			}
-		}
+	TEST(posix_fadvise(fd, 0, 0, advise));
+	if (TST_RET == 0) {
+		tst_res(TFAIL, "call succeeded unexpectedly");
+		return;
 	}
 
-	/*
-	 * cleanup and exit
-	 */
-	cleanup();
-
-	tst_exit();
+	if (TST_RET == expected_error) {
+		tst_res(TPASS, "expected failure - returned %ld for advise %d",
+				TST_RET, advise);
+	} else {
+		tst_res(TFAIL, "unexpected return - %ld for advise %d",
+				TST_RET, advise);
+	}
 }
 
-/*
- * setup() - performs all ONE TIME setup for this test.
- */
 void setup(void)
 {
-
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
-
-	fd = SAFE_OPEN(cleanup, fname, O_RDONLY);
-}
-
-/*
- * cleanup() - performs all ONE TIME cleanup for this test at
- *		completion or premature exit.
- */
-void cleanup(void)
-{
-
-	if (fd != -1) {
-		close(fd);
+	/* Check this system has fadvise64 system which is used
+	   in posix_fadvise. */
+	if ((_FILE_OFFSET_BITS != 64) && (__NR_fadvise64 == 0)) {
+		tst_brk(TCONF,
+			"This test can only run on kernels that implements "
+			"fadvise64 which is used from posix_fadvise");
 	}
 
+	fd = SAFE_OPEN(, fname, O_RDONLY);
 }
+
+struct tst_test test = {
+	.tcnt = ADVISE_LIMIT,
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test = test_posix_fadvise,
+};
-- 
2.19.1.930.g4563a0d9d0-goog

[LTP] [PATCH 5/5] syscalls/posix_fadvise0[13]: Start using new library.

[Amir Goldstein]

On Tue, Nov 6, 2018 at 1:51 AM Sandeep Patil <sspatil@google.com> wrote:
>
> Use SPDX-Licence-Identifier and delete dead comments / code while at it.
> Make sure the tests can work on any system by creating its own file
> and using it for testing.
>
> Signed-off-by: Sandeep Patil <sspatil@google.com>
> ---

Hi Sandeep,

Nice work.
Please compare with https://github.com/linux-test-project/ltp/pull/401
to see if you missed anything which I got.
I found some minor issue by diff (see below).

>  .../kernel/syscalls/fadvise/posix_fadvise01.c | 135 ++++-----------
>  .../kernel/syscalls/fadvise/posix_fadvise03.c | 163 ++++--------------
>  2 files changed, 68 insertions(+), 230 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> index c12f0563c..ad139a66b 100644
> --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> @@ -1,20 +1,6 @@
> +// SPDX-License-Identifier: GPL-2.0

// SPDX-License-Identifier: GPL-2.0-or-later

Here and in other test as well.

...

> -void cleanup(void)
> -{
> -
> -       if (fd != -1) {
> -               close(fd);
> +       /* Check this system has fadvise64 system which is used
> +          in posix_fadvise. */
> +       if ((_FILE_OFFSET_BITS != 64) && (__NR_fadvise64 == 0)) {
> +               tst_brk(TCONF,
> +                       "This test can only run on kernels that implements "
> +                       "fadvise64 which is used from posix_fadvise");

That is a good change compared to original test, but when
Jan Stancek asked me about this check in my readahead test:
https://marc.info/?l=linux-unionfs&m=153857388324517&w=2
I looked into it and couldn''t find a reason for the test at all.

posix_fadvise() is a documented API and the tests posix_fadvise0[14]
utilize this API. What does it matter which syscall variant is used to
implement the API?

The only thing that should matter in the context of this test is
whether or not posix_fadvise() is supported by the kernel.
For some reason that is not clear to me (are we tinyfying the kernel syscall
by syscall now?), the support for fadvise in the kernel can be configured out
with CONFIG_ADVISE_SYSCALLS=n. So IMO, this test really needs a
runtime check, not a build time check like above.

We can try POSIX_FADV_NORMAL in setup() and report TCONF if it fails.

>         }
>
> +       fd = SAFE_OPEN(, fname, O_RDONLY);

typo SAFE_OPEN(,

Thanks,
Amir.

[LTP] [PATCH 5/5] syscalls/posix_fadvise0[13]: Start using new library.

[Sandeep Patil]

On Tue, Nov 06, 2018 at 09:00:22AM +0200, Amir Goldstein wrote:
> On Tue, Nov 6, 2018 at 1:51 AM Sandeep Patil <sspatil@google.com> wrote:
> >
> > Use SPDX-Licence-Identifier and delete dead comments / code while at it.
> > Make sure the tests can work on any system by creating its own file
> > and using it for testing.
> >
> > Signed-off-by: Sandeep Patil <sspatil@google.com>
> > ---
> 
> Hi Sandeep,
> 
> Nice work.
> Please compare with https://github.com/linux-test-project/ltp/pull/401
> to see if you missed anything which I got.

I see Cyril merged that already now, so I'm going to drop these from the
patch series. Thanks for the heads up, I guess I need to watch the github
pull requests too now.

> I found some minor issue by diff (see below).
> 
> >  .../kernel/syscalls/fadvise/posix_fadvise01.c | 135 ++++-----------
> >  .../kernel/syscalls/fadvise/posix_fadvise03.c | 163 ++++--------------
> >  2 files changed, 68 insertions(+), 230 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > index c12f0563c..ad139a66b 100644
> > --- a/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > +++ b/testcases/kernel/syscalls/fadvise/posix_fadvise01.c
> > @@ -1,20 +1,6 @@
> > +// SPDX-License-Identifier: GPL-2.0
> 
> // SPDX-License-Identifier: GPL-2.0-or-later
> 
> Here and in other test as well.

Yeah, missed it
> 
> ...
> 
> > -void cleanup(void)
> > -{
> > -
> > -       if (fd != -1) {
> > -               close(fd);
> > +       /* Check this system has fadvise64 system which is used
> > +          in posix_fadvise. */
> > +       if ((_FILE_OFFSET_BITS != 64) && (__NR_fadvise64 == 0)) {
> > +               tst_brk(TCONF,
> > +                       "This test can only run on kernels that implements "
> > +                       "fadvise64 which is used from posix_fadvise");
> 
> That is a good change compared to original test, but when
> Jan Stancek asked me about this check in my readahead test:
> https://marc.info/?l=linux-unionfs&m=153857388324517&w=2
> I looked into it and couldn''t find a reason for the test at all.
> 
> posix_fadvise() is a documented API and the tests posix_fadvise0[14]
> utilize this API. What does it matter which syscall variant is used to
> implement the API?

Agree, I honestly didn't look into the API and wanted to make sure
I don't change the original behavior / code instead and then add changes
to it later, anyway, I guess it doesn't matter anymore. Dropping the
posix_fadvise() patches from this series now.

> 
> The only thing that should matter in the context of this test is
> whether or not posix_fadvise() is supported by the kernel.
> For some reason that is not clear to me (are we tinyfying the kernel syscall
> by syscall now?), the support for fadvise in the kernel can be configured out
> with CONFIG_ADVISE_SYSCALLS=n. So IMO, this test really needs a
> runtime check, not a build time check like above.
> 
> We can try POSIX_FADV_NORMAL in setup() and report TCONF if it fails.

, I guess I'm going to drop these
> 
> >         }
> >
> > +       fd = SAFE_OPEN(, fname, O_RDONLY);
> 
> typo SAFE_OPEN(,

ew ... its scary that this didn't produce any errors :(.

Thanks for looking into this and showing me the pull request.

- ssp


> 
> Thanks,
> Amir.

[LTP] [PATCH 5/5] syscalls/posix_fadvise0[13]: Start using new library.

[Cyril Hrubis]

Hi!
> The only thing that should matter in the context of this test is
> whether or not posix_fadvise() is supported by the kernel.
> For some reason that is not clear to me (are we tinyfying the kernel syscall
> by syscall now?), the support for fadvise in the kernel can be configured out
> with CONFIG_ADVISE_SYSCALLS=n. So IMO, this test really needs a
> runtime check, not a build time check like above.

Yes, I would expect that we would get ENOSYS in a case that kernel is
build without the support for the call. And we don't have to check in
the setup, we can do tst_brk(TCONF, ...) in the actuall test as well.

-- 
Cyril Hrubis
chrubis@suse.cz