[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Yang Xu]

This case tests whether sync() can return the correct value. But as man-page
said "sync() is always successful". So this case is meaningless
and remove it.

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                          |   1 -
 testcases/kernel/syscalls/sync/.gitignore |   1 -
 testcases/kernel/syscalls/sync/sync01.c   | 182 ----------------------
 3 files changed, 184 deletions(-)
 delete mode 100644 testcases/kernel/syscalls/sync/sync01.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 0443f9f3d..2e7108655 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1477,7 +1477,6 @@ symlink05 symlink05
 #symlinkat test cases
 symlinkat01 symlinkat01
 
-sync01 sync01
 sync02 sync02
 sync03 sync03
 
diff --git a/testcases/kernel/syscalls/sync/.gitignore b/testcases/kernel/syscalls/sync/.gitignore
index 04f4710dd..d006746c2 100644
--- a/testcases/kernel/syscalls/sync/.gitignore
+++ b/testcases/kernel/syscalls/sync/.gitignore
@@ -1,3 +1,2 @@
-/sync01
 /sync02
 /sync03
diff --git a/testcases/kernel/syscalls/sync/sync01.c b/testcases/kernel/syscalls/sync/sync01.c
deleted file mode 100644
index dd0a336c2..000000000
--- a/testcases/kernel/syscalls/sync/sync01.c
+++ /dev/null
@@ -1,182 +0,0 @@
-/*
- * Copyright (c) 2000 Silicon Graphics, Inc.  All Rights Reserved.
- *
- * 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.
- *
- * Contact information: Silicon Graphics, Inc., 1600 Amphitheatre Pkwy,
- * Mountain View, CA  94043, or:
- *
- * http://www.sgi.com
- *
- * For further information regarding this notice, see:
- *
- * http://oss.sgi.com/projects/GenInfo/NoticeExplan/
- *
- */
-/* $Id: sync01.c,v 1.6 2009/11/02 13:57:19 subrata_modak Exp $ */
-/**********************************************************
- *
- *    OS Test - Silicon Graphics, Inc.
- *
- *    TEST IDENTIFIER	: sync01
- *
- *    EXECUTED BY	: anyone
- *
- *    TEST TITLE	: Basic test for sync(2)
- *
- *    PARENT DOCUMENT	: usctpl01
- *
- *    TEST CASE TOTAL	: 1
- *
- *    WALL CLOCK TIME	: 1
- *
- *    CPU TYPES		: ALL
- *
- *    AUTHOR		: William Roske
- *
- *    CO-PILOT		: Dave Fenner
- *
- *    DATE STARTED	: 03/30/92
- *
- *    INITIAL RELEASE	: UNICOS 7.0
- *
- *    TEST CASES
- *
- *	1.) sync(2) returns...(See Description)
- *
- *    INPUT SPECIFICATIONS
- *	The standard options for system call tests are accepted.
- *	(See the parse_opts(3) man page).
- *
- *    OUTPUT SPECIFICATIONS
- *
- *    DURATION
- *	Terminates - with frequency and infinite modes.
- *
- *    SIGNALS
- *	Uses SIGUSR1 to pause before test if option set.
- *	(See the parse_opts(3) man page).
- *
- *    RESOURCES
- *	None
- *
- *    ENVIRONMENTAL NEEDS
- *      No run-time environmental needs.
- *
- *    SPECIAL PROCEDURAL REQUIREMENTS
- *	None
- *
- *    INTERCASE DEPENDENCIES
- *	None
- *
- *    DETAILED DESCRIPTION
- *	This is a Phase I test for the sync(2) system call.  It is intended
- *	to provide a limited exposure of the system call, for now.  It
- *	should/will be extended when full functional tests are written for
- *	sync(2).
- *
- *	Setup:
- *	  Setup signal handling.
- *	  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, Issue a PASS message.
- *
- *	Cleanup:
- *	  Print errno log and/or timing stats if options given
- *
- *
- *#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#**/
-
-#include <errno.h>
-#include <string.h>
-#include <signal.h>
-#include "test.h"
-
-void setup();
-void cleanup();
-
-char *TCID = "sync01";
-int TST_TOTAL = 1;
-
-int main(int ac, char **av)
-{
-	int lc;
-
-    /***************************************************************
-     * parse standard options
-     ***************************************************************/
-	tst_parse_opts(ac, av, NULL, NULL);
-
-    /***************************************************************
-     * perform global setup for test
-     ***************************************************************/
-	setup();
-
-    /***************************************************************
-     * check looping state if -c option given
-     ***************************************************************/
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		/*
-		 * Call sync(2)
-		 */
-		TEST_VOID(sync());
-
-		/* check return code */
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL, "sync() Failed, errno=%d : %s",
-				 TEST_ERRNO, strerror(TEST_ERRNO));
-		} else {
-			tst_resm(TPASS, "sync() returned %ld",
-				 TEST_RETURN);
-		}
-	}
-
-	cleanup();
-	tst_exit();
-}
-
-/***************************************************************
- * setup() - performs all ONE TIME setup for this test.
- ***************************************************************/
-void setup(void)
-{
-
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
-
-}
-
-/***************************************************************
- * cleanup() - performs all ONE TIME cleanup for this test at
- *		completion or premature exit.
- ***************************************************************/
-void cleanup(void)
-{
-
-}
-- 
2.23.0

[LTP] [PATCH 2/4] syscalls/sync02: Remove it

[Yang Xu]

As man-pages said
"According to the standard specification (e.g., POSIX.1-2001), sync() schedules the writes,
but may return before the actual writing is done. "

So checking the data whether has been written into disk make no sense. Also, we check the data
from read buffer in this test and we can't ensure the data from disk instead of buffer/cache.

So remove the confusing case.

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                          |   1 -
 testcases/kernel/syscalls/sync/.gitignore |   1 -
 testcases/kernel/syscalls/sync/sync02.c   | 204 ----------------------
 3 files changed, 206 deletions(-)
 delete mode 100644 testcases/kernel/syscalls/sync/sync02.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 2e7108655..afc27c142 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1477,7 +1477,6 @@ symlink05 symlink05
 #symlinkat test cases
 symlinkat01 symlinkat01
 
-sync02 sync02
 sync03 sync03
 
 syncfs01 syncfs01
diff --git a/testcases/kernel/syscalls/sync/.gitignore b/testcases/kernel/syscalls/sync/.gitignore
index d006746c2..8008fa6e2 100644
--- a/testcases/kernel/syscalls/sync/.gitignore
+++ b/testcases/kernel/syscalls/sync/.gitignore
@@ -1,2 +1 @@
-/sync02
 /sync03
diff --git a/testcases/kernel/syscalls/sync/sync02.c b/testcases/kernel/syscalls/sync/sync02.c
deleted file mode 100644
index d4fd94c0e..000000000
--- a/testcases/kernel/syscalls/sync/sync02.c
+++ /dev/null
@@ -1,204 +0,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: sync02
- *
- * Test Description:
- *  Open a file for write; modify the file, then do a sync().
- *  Verify that the data has been written to disk by re-opening the file.
- *
- * Expected Result:
- *  sync() alawys returns 0 in Linux. The data written to the file should
- *  be updated to the disk.
- *
- * 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>
- *  sync02 [-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 John George
- *		-Ported
- *
- * Restrictions:
- *  None.
- */
-
-#include <stdio.h>
-#include <unistd.h>
-#include <sys/types.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <string.h>
-#include <signal.h>
-#include <sys/stat.h>
-
-#include "test.h"
-
-#define TEMP_FILE	"temp_file"
-#define FILE_MODE       S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
-
-char *TCID = "sync02";
-int TST_TOTAL = 1;
-char write_buffer[BUFSIZ];	/* buffer used to write data to file */
-int fildes;			/* file descriptor for temporary file */
-
-void setup();			/* Main setup function of test */
-void cleanup();			/* cleanup function for the test */
-
-int main(int ac, char **av)
-{
-	int lc;
-	char read_buffer[BUFSIZ];	/* buffer used to read data from file */
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-
-		tst_count = 0;
-
-		/*
-		 * Call sync(2) to commit buffer data to disk.
-		 */
-		TEST_VOID(sync());
-
-		if (TEST_RETURN == -1) {
-			tst_resm(TFAIL, "%s, Failed, errno=%d : %s",
-				 TCID, TEST_ERRNO, strerror(TEST_ERRNO));
-		} else {
-			/* Set the file ptr to b'nning of file */
-			if (lseek(fildes, 0, SEEK_SET) < 0) {
-				tst_brkm(TFAIL, cleanup, "lseek() "
-					 "failed on %s, error=%d",
-					 TEMP_FILE, errno);
-			}
-
-			/* Read the contents of file */
-			if (read(fildes, read_buffer,
-				 sizeof(read_buffer)) > 0) {
-				if (strcmp(read_buffer, write_buffer)) {
-					tst_resm(TFAIL, "Data read "
-						 "from %s doesn't match "
-						 "with witten data",
-						 TEMP_FILE);
-				} else {
-					tst_resm(TPASS, "Functionality "
-						 "of sync() successful");
-				}
-			} else {
-				tst_brkm(TFAIL, cleanup,
-					 "read() Fails on %s, error=%d",
-					 TEMP_FILE, errno);
-			}
-		}
-		tst_count++;	/* incr. TEST_LOOP counter */
-	}
-
-	cleanup();
-	tst_exit();
-}
-
-/*
- * void
- * setup() - performs all ONE TIME setup for this test.
- *  Create a temporary directory and change directory to it.
- *  Create a test file under temporary directory and write some
- *  data into it.
- */
-void setup(void)
-{
-
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	/* Pause if that option was specified
-	 * TEST_PAUSE contains the code to fork the test with the -i option.
-	 * You want to make sure you do this before you create your temporary
-	 * directory.
-	 */
-	TEST_PAUSE;
-
-	tst_tmpdir();
-
-	/* Copy some data into data buffer */
-	strcpy(write_buffer, "abcdefghijklmnopqrstuvwxyz");
-
-	/* Creat a temporary file under above directory */
-	if ((fildes = open(TEMP_FILE, O_RDWR | O_CREAT, FILE_MODE)) == -1) {
-		tst_brkm(TBROK, cleanup,
-			 "open(%s, O_RDWR | O_CREAT, %#o) Failed, errno=%d :%s",
-			 TEMP_FILE, FILE_MODE, errno, strerror(errno));
-	}
-
-	/* Write the buffer data into file */
-	if (write(fildes, write_buffer, strlen(write_buffer) + 1) !=
-	    strlen(write_buffer) + 1) {
-		tst_brkm(TBROK, cleanup,
-			 "write() failed to write buffer data to %s",
-			 TEMP_FILE);
-	}
-
-}
-
-/*
- * void
- * cleanup() - performs all ONE TIME cleanup for this test at
- *             completion or premature exit.
- *  Remove the test directory and testfile created in the setup.
- */
-void cleanup(void)
-{
-
-	/* Close the temporary file */
-	if (close(fildes) == -1) {
-		tst_brkm(TFAIL, NULL,
-			 "close(%s) Failed, errno=%d : %s",
-			 TEMP_FILE, errno, strerror(errno));
-	}
-
-	tst_rmdir();
-
-}
-- 
2.23.0

[LTP] [PATCH 3/4] syscalls/sync03: Remove useless judgement

[Yang Xu]

The TEST_VOID was defined as below in include/tst_test.h
define TEST_VOID(SCALL) \
        do { \
                errno = 0; \
                SCALL; \
                TST_ERR = errno; \
        } while (0)

It doesn't assign the value fot TST_RET like TEST macro. So
remove the useless judgement and use sync instead because
sync() is always successful.

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
1.I plan to remove TEST_VOID macro because only sync cases use it.
Also, I don't see any syscall doesn't have return value and still has
error describe(If having, please let me know).
 testcases/kernel/syscalls/sync/sync03.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/sync/sync03.c b/testcases/kernel/syscalls/sync/sync03.c
index c5c02f877..dc093d863 100644
--- a/testcases/kernel/syscalls/sync/sync03.c
+++ b/testcases/kernel/syscalls/sync/sync03.c
@@ -37,10 +37,7 @@ static void verify_sync(void)
 
 	tst_fill_fd(fd, 0, TST_MB, FILE_SIZE_MB);
 
-	TEST_VOID(sync());
-
-	if (TST_RET)
-		tst_brk(TFAIL | TTERRNO, "sync() failed");
+	sync();
 
 	written = tst_dev_bytes_written(tst_device->dev);
 
-- 
2.23.0

[LTP] [PATCH 4/4] syscalls/sync: Rename sync03.c to sync01.c

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                                      | 2 +-
 testcases/kernel/syscalls/sync/.gitignore             | 2 +-
 testcases/kernel/syscalls/sync/{sync03.c => sync01.c} | 0
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename testcases/kernel/syscalls/sync/{sync03.c => sync01.c} (100%)

diff --git a/runtest/syscalls b/runtest/syscalls
index afc27c142..0dcd3d66d 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -1477,7 +1477,7 @@ symlink05 symlink05
 #symlinkat test cases
 symlinkat01 symlinkat01
 
-sync03 sync03
+sync01 sync01
 
 syncfs01 syncfs01
 
diff --git a/testcases/kernel/syscalls/sync/.gitignore b/testcases/kernel/syscalls/sync/.gitignore
index 8008fa6e2..74d01da47 100644
--- a/testcases/kernel/syscalls/sync/.gitignore
+++ b/testcases/kernel/syscalls/sync/.gitignore
@@ -1 +1 @@
-/sync03
+/sync01
diff --git a/testcases/kernel/syscalls/sync/sync03.c b/testcases/kernel/syscalls/sync/sync01.c
similarity index 100%
rename from testcases/kernel/syscalls/sync/sync03.c
rename to testcases/kernel/syscalls/sync/sync01.c
-- 
2.23.0

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Yang Xu]

Hi!
Ping. I think this patchset is simple.

Best Regards
Yang Xu
> This case tests whether sync() can return the correct value. But as man-page
> said "sync() is always successful". So this case is meaningless
> and remove it.
> 
> Signed-off-by: Yang Xu<xuyang2018.jy@cn.fujitsu.com>
> ---
>   runtest/syscalls                          |   1 -
>   testcases/kernel/syscalls/sync/.gitignore |   1 -
>   testcases/kernel/syscalls/sync/sync01.c   | 182 ----------------------
>   3 files changed, 184 deletions(-)
>   delete mode 100644 testcases/kernel/syscalls/sync/sync01.c
> 
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 0443f9f3d..2e7108655 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1477,7 +1477,6 @@ symlink05 symlink05
>   #symlinkat test cases
>   symlinkat01 symlinkat01
> 
> -sync01 sync01
>   sync02 sync02
>   sync03 sync03
> 
> diff --git a/testcases/kernel/syscalls/sync/.gitignore b/testcases/kernel/syscalls/sync/.gitignore
> index 04f4710dd..d006746c2 100644
> --- a/testcases/kernel/syscalls/sync/.gitignore
> +++ b/testcases/kernel/syscalls/sync/.gitignore
> @@ -1,3 +1,2 @@
> -/sync01
>   /sync02
>   /sync03
> diff --git a/testcases/kernel/syscalls/sync/sync01.c b/testcases/kernel/syscalls/sync/sync01.c
> deleted file mode 100644
> index dd0a336c2..000000000
> --- a/testcases/kernel/syscalls/sync/sync01.c
> +++ /dev/null
> @@ -1,182 +0,0 @@
> -/*
> - * Copyright (c) 2000 Silicon Graphics, Inc.  All Rights Reserved.
> - *
> - * 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.
> - *
> - * Contact information: Silicon Graphics, Inc., 1600 Amphitheatre Pkwy,
> - * Mountain View, CA  94043, or:
> - *
> - * http://www.sgi.com
> - *
> - * For further information regarding this notice, see:
> - *
> - * http://oss.sgi.com/projects/GenInfo/NoticeExplan/
> - *
> - */
> -/* $Id: sync01.c,v 1.6 2009/11/02 13:57:19 subrata_modak Exp $ */
> -/**********************************************************
> - *
> - *    OS Test - Silicon Graphics, Inc.
> - *
> - *    TEST IDENTIFIER	: sync01
> - *
> - *    EXECUTED BY	: anyone
> - *
> - *    TEST TITLE	: Basic test for sync(2)
> - *
> - *    PARENT DOCUMENT	: usctpl01
> - *
> - *    TEST CASE TOTAL	: 1
> - *
> - *    WALL CLOCK TIME	: 1
> - *
> - *    CPU TYPES		: ALL
> - *
> - *    AUTHOR		: William Roske
> - *
> - *    CO-PILOT		: Dave Fenner
> - *
> - *    DATE STARTED	: 03/30/92
> - *
> - *    INITIAL RELEASE	: UNICOS 7.0
> - *
> - *    TEST CASES
> - *
> - *	1.) sync(2) returns...(See Description)
> - *
> - *    INPUT SPECIFICATIONS
> - *	The standard options for system call tests are accepted.
> - *	(See the parse_opts(3) man page).
> - *
> - *    OUTPUT SPECIFICATIONS
> - *
> - *    DURATION
> - *	Terminates - with frequency and infinite modes.
> - *
> - *    SIGNALS
> - *	Uses SIGUSR1 to pause before test if option set.
> - *	(See the parse_opts(3) man page).
> - *
> - *    RESOURCES
> - *	None
> - *
> - *    ENVIRONMENTAL NEEDS
> - *      No run-time environmental needs.
> - *
> - *    SPECIAL PROCEDURAL REQUIREMENTS
> - *	None
> - *
> - *    INTERCASE DEPENDENCIES
> - *	None
> - *
> - *    DETAILED DESCRIPTION
> - *	This is a Phase I test for the sync(2) system call.  It is intended
> - *	to provide a limited exposure of the system call, for now.  It
> - *	should/will be extended when full functional tests are written for
> - *	sync(2).
> - *
> - *	Setup:
> - *	  Setup signal handling.
> - *	  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, Issue a PASS message.
> - *
> - *	Cleanup:
> - *	  Print errno log and/or timing stats if options given
> - *
> - *
> - *#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#*#**/
> -
> -#include<errno.h>
> -#include<string.h>
> -#include<signal.h>
> -#include "test.h"
> -
> -void setup();
> -void cleanup();
> -
> -char *TCID = "sync01";
> -int TST_TOTAL = 1;
> -
> -int main(int ac, char **av)
> -{
> -	int lc;
> -
> -    /***************************************************************
> -     * parse standard options
> -     ***************************************************************/
> -	tst_parse_opts(ac, av, NULL, NULL);
> -
> -    /***************************************************************
> -     * perform global setup for test
> -     ***************************************************************/
> -	setup();
> -
> -    /***************************************************************
> -     * check looping state if -c option given
> -     ***************************************************************/
> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -
> -		tst_count = 0;
> -
> -		/*
> -		 * Call sync(2)
> -		 */
> -		TEST_VOID(sync());
> -
> -		/* check return code */
> -		if (TEST_RETURN == -1) {
> -			tst_resm(TFAIL, "sync() Failed, errno=%d : %s",
> -				 TEST_ERRNO, strerror(TEST_ERRNO));
> -		} else {
> -			tst_resm(TPASS, "sync() returned %ld",
> -				 TEST_RETURN);
> -		}
> -	}
> -
> -	cleanup();
> -	tst_exit();
> -}
> -
> -/***************************************************************
> - * setup() - performs all ONE TIME setup for this test.
> - ***************************************************************/
> -void setup(void)
> -{
> -
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> -
> -}
> -
> -/***************************************************************
> - * cleanup() - performs all ONE TIME cleanup for this test at
> - *		completion or premature exit.
> - ***************************************************************/
> -void cleanup(void)
> -{
> -
> -}

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Cyril Hrubis]

Hi!
> Ping. I think this patchset is simple.

Great cleanup, pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Xiao Yang]

On 11/6/20 8:36 PM, Cyril Hrubis wrote:
> Hi!
>> Ping. I think this patchset is simple.
> Great cleanup, pushed, thanks.

Hi Cyril, Petr


I have a doubt after reading Xu's patch[1] and Martin's patch[2]:

1) Xu removed sync01 because sync() always return 0.

2) On error, normal syscall always return -1 but Martin added a check 
for undefined return value(i.e. negative value except -1).

Is it necessary to check undefined return value?


Patches:

[1]: [PATCH 1/4] syscalls/sync01: Remove it

[2]: [PATCH 03/19] Unify error handling in lib/tst_safe_timerfd.c


Best Regards,

Xiao Yang

>

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Cyril Hrubis]

Hi!
> I have a doubt after reading Xu's patch[1] and Martin's patch[2]:
> 
> 1) Xu removed sync01 because sync() always return 0.

Actually sync() is defined as void function, so the tests were bogusly
checking the TST_RET value which haven't been set at all.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Xiao Yang]

On 11/7/20 12:47 AM, Cyril Hrubis wrote:
> Hi!
>> I have a doubt after reading Xu's patch[1] and Martin's patch[2]:
>>
>> 1) Xu removed sync01 because sync() always return 0.
> Actually sync() is defined as void function, so the tests were bogusly
> checking the TST_RET value which haven't been set at all.

Hi Cyril,

Oops, I gave a wrong example. :-(

On error, I just wonder if we need to check all return value(i.e. 
negative value except -1).

IOW, Is it possible for syscall to get a error value which is not -1?

Best Regards,

Xiao Yang

>

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Yang Xu]

Hi Xiao, Cyril
> On 11/7/20 12:47 AM, Cyril Hrubis wrote:
>> Hi!
>>> I have a doubt after reading Xu's patch[1] and Martin's patch[2]:
>>>
>>> 1) Xu removed sync01 because sync() always return 0.
>> Actually sync() is defined as void function, so the tests were bogusly
>> checking the TST_RET value which haven't been set at all.
>
> Hi Cyril,
>
> Oops, I gave a wrong example. :-(
>
> On error, I just wonder if we need to check all return value(i.e.
> negative value except -1).
>
> IOW, Is it possible for syscall to get a error value which is not -1?
IMO, get a error and syscall return -1 that is a normal situation. 
Martin creates a standard model for it and doesn't match this rule is 
wrong, so we can check syscall whether return the right value when 
kernel changes these api in the future.

Best Regards,
Yang Xu
>
> Best Regards,
>
> Xiao Yang
>
>>
>
>

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Petr Vorel]

Hi,

> On 11/7/20 12:47 AM, Cyril Hrubis wrote:
> > Hi!
> > > I have a doubt after reading Xu's patch[1] and Martin's patch[2]:

> > > 1) Xu removed sync01 because sync() always return 0.
> > Actually sync() is defined as void function, so the tests were bogusly
> > checking the TST_RET value which haven't been set at all.

> Hi Cyril,

> Oops, I gave a wrong example. :-(

> On error, I just wonder if we need to check all return value(i.e. negative
> value except -1).

> IOW, Is it possible for syscall to get a error value which is not -1?
There are probably other examples, but I've found only these:

man malloc_get_state(3)

	If the implementation detects that state does not point to a correctly
	formed data structure, malloc_set_state() returns -1.
	If the implementation detects that the version of the data structure
	referred to by state is a more recent version than this implementation knows
	about, malloc_set_state() returns -2.

man mmap(2)
	On error, the value MAP_FAILED (that is, (void *) -1) is returned.

> Best Regards,

> Xiao Yang


Kind regards,
Petr

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Xiao Yang]

On  020/11/8 0:55, Petr Vorel wrote:
> Hi,
>
>> On 11/7/20 12:47 AM, Cyril Hrubis wrote:
>>> Hi!
>>>> I have a doubt after reading Xu's patch[1] and Martin's patch[2]:
>>>> 1) Xu removed sync01 because sync() always return 0.
>>> Actually sync() is defined as void function, so the tests were bogusly
>>> checking the TST_RET value which haven't been set at all.
>> Hi Cyril,
>> Oops, I gave a wrong example. :-(
>> On error, I just wonder if we need to check all return value(i.e. negative
>> value except -1).
>> IOW, Is it possible for syscall to get a error value which is not -1?
> There are probably other examples, but I've found only these:
>
> man malloc_get_state(3)
>
> 	If the implementation detects that state does not point to a correctly
> 	formed data structure, malloc_set_state() returns -1.
> 	If the implementation detects that the version of the data structure
> 	referred to by state is a more recent version than this implementation knows
> 	about, malloc_set_state() returns -2.
>
> man mmap(2)
> 	On error, the value MAP_FAILED (that is, (void *) -1) is returned.
Hi,

Sorry, I didn't describe the doubt clearly.
For example:
1) open(2) will return -1 if an error occur.
     Is it necessary to check invalid return value(except -1) if an 
error occur?
2) mmap(2) will return MAP_FAILED if an error occurs.
     Is it necessary to check invalid value(except MAP_FAILED) if an 
error occur?
Martin's patches have added a check for invalid return value in many 
safe macros but a lot of syscall tests(e.g. after doingTEST()) don't add 
the check for now.
I am not sure if we need to add the check for all syscall tests. :-)
BTW: In my opinion, it is hardly to get invalid return value so the 
check seems unnecessary and redundance.

Best Regards,
Xiao Yang
>> Best Regards,
>> Xiao Yang
>
> Kind regards,
> Petr
>



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20201109/f564f88c/attachment-0001.htm>

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Petr Vorel]

Hi,

> Sorry, I didn't describe the doubt clearly.
> For example:
> 1) open(2) will return -1 if an error occur.
>     Is it necessary to check invalid return value(except -1) if an error
> occur?
> 2) mmap(2) will return MAP_FAILED if an error occurs.
>     Is it necessary to check invalid value(except MAP_FAILED) if an error
> occur?
> Martin's patches have added a check for invalid return value in many safe
> macros but a lot of syscall tests(e.g. after doingTEST()) don't add the
> check for now.
> I am not sure if we need to add the check for all syscall tests. :-)
> BTW: In my opinion, it is hardly to get invalid return value so the check
> seems unnecessary and redundance.
Agree it's hard to get these errors. That's why I would bother just in the
library (in these safe_*() functions).

Kind regards,
Petr Vorel

> Best Regards,
> Xiao Yang

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Cyril Hrubis]

Hi!
> 1) open(2) will return -1 if an error occur.
>      Is it necessary to check invalid return value(except -1) if an 
> error occur?

Well if there are values that are never supposed to be returned it makes
sense to catch these and return a TBROK or TFAIL.

If we are expecially testing a syscall() I would say that we should
check for all kinds of errors including the values that shall not be
returned e.g.:

	TEST(open(...));

	if (TST_RET == -1) {
		tst_ret(TFAIL | TTERRNO, "open() failed");
		return;
	}

	if (TST_RET < 0) {
		tst_ret(TFAIL | TTERRNO, "Invalid open() retval %ld", TST_RET);
		return;
	}

	...


If the syscall is part of the test preparation and there is no safe
macro I would say that it's enough to cover all invalid values in one
condition e.g.:

	fd = open(...);
	if (fd < 0)
		tst_brk(TBROK | TERRNO, "open() failed");


> 2) mmap(2) will return MAP_FAILED if an error occurs.
>      Is it necessary to check invalid value(except MAP_FAILED) if an 
> error occur?

Actually return value from mmap() is pointer, right? And the only value
that is not supposed to be returned is MAP_FAILED or do I miss
something?

> Martin's patches have added a check for invalid return value in many 
> safe macros but a lot of syscall tests(e.g. after doingTEST()) don't add 
> the check for now.
> I am not sure if we need to add the check for all syscall tests. :-)

I would say that at least for newly added test we should make sure that
there is no unexpected value returned.

> BTW: In my opinion, it is hardly to get invalid return value so the 
> check seems unnecessary and redundance.

Well it's not a common case but I've seen this to happen a few times,
once it was because a backported patch applied cleanly but the code was
incorrect and as a result syscall started to return really unexpected
values.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Radoslav Kolev]

On Mon, 2020-11-09 at 13:42 +0100, Cyril Hrubis wrote:
> Hi!
> > 1) open(2) will return -1 if an error occur.
> >      Is it necessary to check invalid return value(except -1) if
> > an 
> > error occur?
> 
> Well if there are values that are never supposed to be returned it
> makes
> sense to catch these and return a TBROK or TFAIL.
> 
> If we are expecially testing a syscall() I would say that we should
> check for all kinds of errors including the values that shall not be
> returned e.g.:
> 
> 	TEST(open(...));
> 
> 	if (TST_RET == -1) {
> 		tst_ret(TFAIL | TTERRNO, "open() failed");
> 		return;
> 	}
> 
> 	if (TST_RET < 0) {
> 		tst_ret(TFAIL | TTERRNO, "Invalid open() retval %ld",
> TST_RET);
> 		return;
> 	}
> 
> 	...

I see no downside in checking for this unexpected negative value,
except copy/pasting this test condition in every syscall testcase.

I don't know the LTP codebase well enough yet, but what would you say
is a good way to have this somewhere in the library. A TEST_SYSCALL
macro, or something else, which fails if the return value is < -1?

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Petr Vorel]

Hi,

> On Mon, 2020-11-09 at 13:42 +0100, Cyril Hrubis wrote:
> > Hi!
> > > 1) open(2) will return -1 if an error occur.
> > >      Is it necessary to check invalid return value(except -1) if
> > > an 
> > > error occur?

> > Well if there are values that are never supposed to be returned it
> > makes
> > sense to catch these and return a TBROK or TFAIL.

> > If we are expecially testing a syscall() I would say that we should
> > check for all kinds of errors including the values that shall not be
> > returned e.g.:

> > 	TEST(open(...));

> > 	if (TST_RET == -1) {
> > 		tst_ret(TFAIL | TTERRNO, "open() failed");
> > 		return;
> > 	}

> > 	if (TST_RET < 0) {
> > 		tst_ret(TFAIL | TTERRNO, "Invalid open() retval %ld",
> > TST_RET);
> > 		return;
> > 	}

> > 	...

> I see no downside in checking for this unexpected negative value,
> except copy/pasting this test condition in every syscall testcase.

> I don't know the LTP codebase well enough yet, but what would you say
> is a good way to have this somewhere in the library. A TEST_SYSCALL
> macro, or something else, which fails if the return value is < -1?
LGTM. I was thinking about adding it directly into TEST() and define _TEST()
which would not do that and be used in that few cases which ret < -1 is valid,
but that would be ugly.

Another candidate is macro for new API tst_syscall() defined in
include/lapi/syscalls.h (generated in include/lapi/syscalls/regen.sh).

Kind regards,
Petr

[LTP] [PATCH 1/4] syscalls/sync01: Remove it

[Cyril Hrubis]

Hi!
> > I see no downside in checking for this unexpected negative value,
> > except copy/pasting this test condition in every syscall testcase.
> 
> > I don't know the LTP codebase well enough yet, but what would you say
> > is a good way to have this somewhere in the library. A TEST_SYSCALL
> > macro, or something else, which fails if the return value is < -1?
> LGTM. I was thinking about adding it directly into TEST() and define _TEST()
> which would not do that and be used in that few cases which ret < -1 is valid,
> but that would be ugly.

Well it would have to be a set of macros at least since:

* There are different classes of functions by return values

* We have possitive and negative testcases

For example we would have to have two macros for functions that return
file descriptors, one for a cases where we expect the function to return
a valid file descriptor and one when we expect the function to fail.

So it would look like:

	TEST_FD(open("/foo/bar", O_RDONLY));

or:
	TEST_FAIL(open((void*)-1, O_RDONLY));


The TEST_FD() macro would fail the test if the return value is < 0

And the TEST_FAIL() will fail the test unless we the return value is set
to -1. Maybe we can even have a version with errno as well something as:

	TEST_FAIL_ERR(open((void*)-1, O_RDONLY), EFAULT);

-- 
Cyril Hrubis
chrubis@suse.cz