[LTP] [PATCH] syscalls/getdtablesize: Update to the new api

[LTP] [PATCH] syscalls/getdtablesize: Update to the new api

[Zhao Gongyi]

1)use some safe macros
2)open a temporary file instead of /etc/hosts since it might be not exist
3)cleanup all of the opened fd

Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
---
 .../syscalls/getdtablesize/getdtablesize01.c  | 161 ++++++++----------
 1 file changed, 73 insertions(+), 88 deletions(-)

diff --git a/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c b/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
index d25cac261..f16c54a68 100644
--- a/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
+++ b/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
@@ -1,119 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) International Business Machines  Corp., 2005
  * Copyright (c) Wipro Technologies Ltd, 2005.  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.
- *
- * 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.
- *
+ * AUTHOR: Prashant P Yendigeri <prashant.yendigeri@wipro.com>
+ *	   Robbie Williamson <robbiew@us.ibm.com>
  */
-/**********************************************************
- *
- *    TEST IDENTIFIER   : getdtablesize01
- *
- *    EXECUTED BY       : root / superuser
- *
- *    TEST TITLE        : Basic tests for getdtablesize01(2)
- *
- *    TEST CASE TOTAL   : 1
- *
- *    AUTHOR            : Prashant P Yendigeri
- *                        <prashant.yendigeri@wipro.com>
- *                        Robbie Williamson
- *                        <robbiew@us.ibm.com>
- *
- *    DESCRIPTION
- *      This is a Phase I test for the getdtablesize01(2) system call.
- *      It is intended to provide a limited exposure of the system call.
+
+/*\
+ * [Description]
+ * Test getdtablesize() returns the maximum number of files a process can
+ * have open, one more than the largest possible value for a file descriptor.
  *
- **********************************************************/
+ * [Algorithm]
+ * 1. Check the return value of getdtablesize() is equal to _SC_OPEN_MAX or
+ * the current limit on the number of open files per process.
+ * 2. Open the maximum allowed number of file descriptors and then check if
+ * it is equal to the return value of getdtablesize() - 1.
+ */

-#include <stdio.h>
-#include <errno.h>
-#include <sys/types.h>
+#include <stdlib.h>
 #include <sys/stat.h>
 #include <fcntl.h>
-#include <sys/time.h>
 #include <sys/resource.h>
 #include <unistd.h>
-#include "test.h"
+#include "tst_test.h"

-void setup();
-void cleanup();
+#define TESTFILE "getdtablesize01_testfile"
+#define FILE_OPEN_MAX SAFE_SYSCONF(_SC_OPEN_MAX)

-char *TCID = "getdtablesize01";
-int TST_TOTAL = 1;
+static int *fd, count;

-int main(void)
+static void run(void)
 {
-	int table_size, fd = 0, count = 0;
+	int temp_fd;
 	int max_val_opfiles;
 	struct rlimit rlp;

-	setup();
-	table_size = getdtablesize();
-	getrlimit(RLIMIT_NOFILE, &rlp);
-	max_val_opfiles = (rlim_t) rlp.rlim_cur;
-
-	tst_resm(TINFO,
-		 "Maximum number of files a process can have opened is %d",
-		 table_size);
-	tst_resm(TINFO,
-		 "Checking with the value returned by getrlimit...RLIMIT_NOFILE");
-
-	if (table_size == max_val_opfiles)
-		tst_resm(TPASS, "got correct dtablesize, value is %d",
-			 max_val_opfiles);
-	else {
-		tst_resm(TFAIL, "got incorrect table size, value is %d",
-			 max_val_opfiles);
-		cleanup();
-	}
+	TEST(getdtablesize());
+	tst_res(TINFO,
+		"Maximum number of files a process can have opened is %d",
+		TST_RET);

-	tst_resm(TINFO,
-		 "Checking Max num of files that can be opened by a process.Should be: RLIMIT_NOFILE - 1");
-	for (;;) {
-		fd = open("/etc/hosts", O_RDONLY);
+	tst_res(TINFO, "Checking with the value returned by getrlimit");

-		if (fd == -1)
-			break;
-		count = fd;
+	if (getrlimit(RLIMIT_NOFILE, &rlp))
+		max_val_opfiles = FILE_OPEN_MAX;
+	else
+		max_val_opfiles = (rlim_t)rlp.rlim_cur;

-#ifdef DEBUG
-		printf("Opened file num %d\n", fd);
-#endif
-	}
+	if (TST_RET == max_val_opfiles)
+		tst_res(TPASS, "got correct dtablesize, value is %d "
+			"max_val_opfiles value is %d",
+			TST_RET, max_val_opfiles);
+	else
+		tst_res(TFAIL, "got incorrect dtablesize, value is %d"
+			 "max_val_opfiles value is %d",
+			 TST_RET, max_val_opfiles);

-//Now the max files opened should be RLIMIT_NOFILE - 1 , why ? read getdtablesize man page
+	tst_res(TINFO,
+		"Checking Max num of files that can be opened by a process."
+		"Should be: RLIMIT_NOFILE - 1");

-	if (count > 0)
-		close(count);
-	if (count == (max_val_opfiles - 1))
-		tst_resm(TPASS, "%d = %d", count, (max_val_opfiles - 1));
-	else if (fd < 0 && errno == ENFILE)
-		tst_brkm(TCONF, cleanup, "Reached maximum number of open files for the system");
-	else
-		tst_resm(TFAIL, "%d != %d", count, (max_val_opfiles - 1));
+	while (1) {
+		temp_fd = open(TESTFILE, O_CREAT | O_RDONLY, 0755);
+		if (temp_fd == -1)
+			break;
+		fd[count++] = temp_fd;
+	}

-	cleanup();
-	tst_exit();
+	if (fd[count - 1] == (max_val_opfiles - 1))
+		tst_res(TPASS,
+			"max open file fd is correct, %d = %d",
+			fd[count - 1], (max_val_opfiles - 1));
+	else if (temp_fd < 0 && errno == ENFILE)
+		tst_brk(TCONF,
+			"Reached maximum number of open files for the system");
+	else
+		tst_res(TFAIL | TERRNO, "%d != %d", fd[count - 1], (max_val_opfiles - 1));
 }

-void setup(void)
+static void setup(void)
 {
-	tst_sig(NOFORK, DEF_HANDLER, cleanup);
-
-	TEST_PAUSE;
+	fd = SAFE_MALLOC(FILE_OPEN_MAX * sizeof(int));
 }

-void cleanup(void)
+static void cleanup(void)
 {
+	int i;
+	for (i = 0; i < count; i++)
+		SAFE_CLOSE(fd[i]);
+
+	free(fd);
+	fd = NULL;
 }
+
+static struct tst_test test = {
+	.needs_tmpdir = 1,
+	.test_all = run,
+	.setup = setup,
+	.cleanup = cleanup,
+};
+
--
2.17.1

[LTP] [PATCH] syscalls/getdtablesize: Update to the new api

[Cyril Hrubis]

Hi!
> --- a/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> +++ b/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> @@ -1,119 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) International Business Machines  Corp., 2005
>   * Copyright (c) Wipro Technologies Ltd, 2005.  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 is GPL-2.0 so the SPDX has to be without the -or-later part here.

> +#define TESTFILE "getdtablesize01_testfile"

This can be just "testfile" the test teporary directory is already
unique enough and has getdtablesize in it's name.

> +#define FILE_OPEN_MAX SAFE_SYSCONF(_SC_OPEN_MAX)
> 
> -char *TCID = "getdtablesize01";
> -int TST_TOTAL = 1;
> +static int *fd, count;
> 
> -int main(void)
> +static void run(void)
>  {
> -	int table_size, fd = 0, count = 0;
> +	int temp_fd;
>  	int max_val_opfiles;
>  	struct rlimit rlp;
> 
> -	setup();
> -	table_size = getdtablesize();
> -	getrlimit(RLIMIT_NOFILE, &rlp);
> -	max_val_opfiles = (rlim_t) rlp.rlim_cur;
> -
> -	tst_resm(TINFO,
> -		 "Maximum number of files a process can have opened is %d",
> -		 table_size);
> -	tst_resm(TINFO,
> -		 "Checking with the value returned by getrlimit...RLIMIT_NOFILE");
> -
> -	if (table_size == max_val_opfiles)
> -		tst_resm(TPASS, "got correct dtablesize, value is %d",
> -			 max_val_opfiles);
> -	else {
> -		tst_resm(TFAIL, "got incorrect table size, value is %d",
> -			 max_val_opfiles);
> -		cleanup();
> -	}
> +	TEST(getdtablesize());
> +	tst_res(TINFO,
> +		"Maximum number of files a process can have opened is %d",
> +		TST_RET);
> 
> -	tst_resm(TINFO,
> -		 "Checking Max num of files that can be opened by a process.Should be: RLIMIT_NOFILE - 1");
> -	for (;;) {
> -		fd = open("/etc/hosts", O_RDONLY);
> +	tst_res(TINFO, "Checking with the value returned by getrlimit");
> 
> -		if (fd == -1)
> -			break;
> -		count = fd;
> +	if (getrlimit(RLIMIT_NOFILE, &rlp))
> +		max_val_opfiles = FILE_OPEN_MAX;
> +	else
> +		max_val_opfiles = (rlim_t)rlp.rlim_cur;

Why do we fallback to sysconf() here? Does hte getrlmit() fail in some
cases? The original test just called getrlimit() and I do not remmeber
it failing.

> -#ifdef DEBUG
> -		printf("Opened file num %d\n", fd);
> -#endif
> -	}
> +	if (TST_RET == max_val_opfiles)
> +		tst_res(TPASS, "got correct dtablesize, value is %d "
> +			"max_val_opfiles value is %d",
> +			TST_RET, max_val_opfiles);
> +	else
> +		tst_res(TFAIL, "got incorrect dtablesize, value is %d"
> +			 "max_val_opfiles value is %d",
> +			 TST_RET, max_val_opfiles);

The LKML coding style says that we shouldn't break strings even if they
are over 80 characters.

> -//Now the max files opened should be RLIMIT_NOFILE - 1 , why ? read getdtablesize man page
> +	tst_res(TINFO,
> +		"Checking Max num of files that can be opened by a process."
> +		"Should be: RLIMIT_NOFILE - 1");
> 
> -	if (count > 0)
> -		close(count);
> -	if (count == (max_val_opfiles - 1))
> -		tst_resm(TPASS, "%d = %d", count, (max_val_opfiles - 1));
> -	else if (fd < 0 && errno == ENFILE)
> -		tst_brkm(TCONF, cleanup, "Reached maximum number of open files for the system");
> -	else
> -		tst_resm(TFAIL, "%d != %d", count, (max_val_opfiles - 1));
> +	while (1) {
> +		temp_fd = open(TESTFILE, O_CREAT | O_RDONLY, 0755);
> +		if (temp_fd == -1)
> +			break;
> +		fd[count++] = temp_fd;
> +	}

Here we blindly assume that the file descriptors will fit into the
table, which may not be true if the system is broken.

Also why do we need the array in the first place? The file descriptors
_are_ allocated continuously so the last fd we get from the open() call
is the maximal number of fds - 1, since the standard streams start at 0.
Technically we can even close the these descriptors if we store the
first fd we got from the open() call since the rest of fds will be in
the range, [first_fd, last_fd].

> -	cleanup();
> -	tst_exit();
> +	if (fd[count - 1] == (max_val_opfiles - 1))
> +		tst_res(TPASS,
> +			"max open file fd is correct, %d = %d",
> +			fd[count - 1], (max_val_opfiles - 1));
> +	else if (temp_fd < 0 && errno == ENFILE)
> +		tst_brk(TCONF,
> +			"Reached maximum number of open files for the system");
> +	else
> +		tst_res(TFAIL | TERRNO, "%d != %d", fd[count - 1], (max_val_opfiles - 1));
>  }
> 
> -void setup(void)
> +static void setup(void)
>  {
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> -
> -	TEST_PAUSE;
> +	fd = SAFE_MALLOC(FILE_OPEN_MAX * sizeof(int));
>  }
> 
> -void cleanup(void)
> +static void cleanup(void)
>  {
> +	int i;
> +	for (i = 0; i < count; i++)
> +		SAFE_CLOSE(fd[i]);
> +
> +	free(fd);
> +	fd = NULL;
>  }
> +
> +static struct tst_test test = {
> +	.needs_tmpdir = 1,
> +	.test_all = run,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +};
> +
> --
> 2.17.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/getdtablesize: Update to the new api

[zhaogongyi]

Hi Cyril,

Thanks for your review!

I have some different opinions:

1) 
>> +	if (getrlimit(RLIMIT_NOFILE, &rlp))
>> +		max_val_opfiles = FILE_OPEN_MAX;
>> +	else
>> +		max_val_opfiles = (rlim_t)rlp.rlim_cur;

>Why do we fallback to sysconf() here? Does hte getrlmit() fail in some cases? The original test just called getrlimit() and I do not remmeber it failing.

In man 3 getdtablesize, we can see that the glibc version of getdtablesize() calls getrlimit(2) and returns the current RLIMIT_NOFILE limit, or OPEN_MAX when that fails,
So max_val_opfiles may be equal to SAFE_SYSCONF(_SC_OPEN_MAX) or  (rlim_t)rlp.rlim_cur.

In this case, just replace the getrlimit with SAFE_GETRLIMIT will be ok?

2)
> > +	while (1) {
> > +		temp_fd = open(TESTFILE, O_CREAT | O_RDONLY, 0755);
> > +		if (temp_fd == -1)
> > +			break;
> > +		fd[count++] = temp_fd;
> > +	}
> 
> Here we blindly assume that the file descriptors will fit into the table,
> which may not be true if the system is broken.
> 
> Also why do we need the array in the first place? The file descriptors
> _are_ allocated continuously so the last fd we get from the open() call is
> the maximal number of fds - 1, since the standard streams start at 0.
> Technically we can even close the these descriptors if we store the first fd
> we got from the open() call since the rest of fds will be in the range,
> [first_fd, last_fd].

For closing all the opened fds before test exit, I used a array to save the opend fds. And the array size is OPEN_MAX.
In my opinion, the size seems be enough in this case.


Thanks so much!

Best Regards,
Gongyi


> 
> Hi!
> > --- a/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> > +++ b/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> > @@ -1,119 +1,104 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * Copyright (c) International Business Machines  Corp., 2005
> >   * Copyright (c) Wipro Technologies Ltd, 2005.  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 is GPL-2.0 so the SPDX has to be without the -or-later part here.
> 
> > +#define TESTFILE "getdtablesize01_testfile"
> 
> This can be just "testfile" the test teporary directory is already unique
> enough and has getdtablesize in it's name.
> 
> > +#define FILE_OPEN_MAX SAFE_SYSCONF(_SC_OPEN_MAX)
> >
> > -char *TCID = "getdtablesize01";
> > -int TST_TOTAL = 1;
> > +static int *fd, count;
> >
> > -int main(void)
> > +static void run(void)
> >  {
> > -	int table_size, fd = 0, count = 0;
> > +	int temp_fd;
> >  	int max_val_opfiles;
> >  	struct rlimit rlp;
> >
> > -	setup();
> > -	table_size = getdtablesize();
> > -	getrlimit(RLIMIT_NOFILE, &rlp);
> > -	max_val_opfiles = (rlim_t) rlp.rlim_cur;
> > -
> > -	tst_resm(TINFO,
> > -		 "Maximum number of files a process can have opened is %d",
> > -		 table_size);
> > -	tst_resm(TINFO,
> > -		 "Checking with the value returned by
> getrlimit...RLIMIT_NOFILE");
> > -
> > -	if (table_size == max_val_opfiles)
> > -		tst_resm(TPASS, "got correct dtablesize, value is %d",
> > -			 max_val_opfiles);
> > -	else {
> > -		tst_resm(TFAIL, "got incorrect table size, value is %d",
> > -			 max_val_opfiles);
> > -		cleanup();
> > -	}
> > +	TEST(getdtablesize());
> > +	tst_res(TINFO,
> > +		"Maximum number of files a process can have opened is %d",
> > +		TST_RET);
> >
> > -	tst_resm(TINFO,
> > -		 "Checking Max num of files that can be opened by a
> process.Should be: RLIMIT_NOFILE - 1");
> > -	for (;;) {
> > -		fd = open("/etc/hosts", O_RDONLY);
> > +	tst_res(TINFO, "Checking with the value returned by getrlimit");
> >
> > -		if (fd == -1)
> > -			break;
> > -		count = fd;
> > +	if (getrlimit(RLIMIT_NOFILE, &rlp))
> > +		max_val_opfiles = FILE_OPEN_MAX;
> > +	else
> > +		max_val_opfiles = (rlim_t)rlp.rlim_cur;
> 
> Why do we fallback to sysconf() here? Does hte getrlmit() fail in some
> cases? The original test just called getrlimit() and I do not remmeber it
> failing.
> 
> > -#ifdef DEBUG
> > -		printf("Opened file num %d\n", fd);
> > -#endif
> > -	}
> > +	if (TST_RET == max_val_opfiles)
> > +		tst_res(TPASS, "got correct dtablesize, value is %d "
> > +			"max_val_opfiles value is %d",
> > +			TST_RET, max_val_opfiles);
> > +	else
> > +		tst_res(TFAIL, "got incorrect dtablesize, value is %d"
> > +			 "max_val_opfiles value is %d",
> > +			 TST_RET, max_val_opfiles);
> 
> The LKML coding style says that we shouldn't break strings even if they are
> over 80 characters.
> 
> > -//Now the max files opened should be RLIMIT_NOFILE - 1 , why ? read
> > getdtablesize man page
> > +	tst_res(TINFO,
> > +		"Checking Max num of files that can be opened by a process."
> > +		"Should be: RLIMIT_NOFILE - 1");
> >
> > -	if (count > 0)
> > -		close(count);
> > -	if (count == (max_val_opfiles - 1))
> > -		tst_resm(TPASS, "%d = %d", count, (max_val_opfiles - 1));
> > -	else if (fd < 0 && errno == ENFILE)
> > -		tst_brkm(TCONF, cleanup, "Reached maximum number of open
> files for the system");
> > -	else
> > -		tst_resm(TFAIL, "%d != %d", count, (max_val_opfiles - 1));
> > +	while (1) {
> > +		temp_fd = open(TESTFILE, O_CREAT | O_RDONLY, 0755);
> > +		if (temp_fd == -1)
> > +			break;
> > +		fd[count++] = temp_fd;
> > +	}
> 
> Here we blindly assume that the file descriptors will fit into the table,
> which may not be true if the system is broken.
> 
> Also why do we need the array in the first place? The file descriptors
> _are_ allocated continuously so the last fd we get from the open() call is
> the maximal number of fds - 1, since the standard streams start at 0.
> Technically we can even close the these descriptors if we store the first fd
> we got from the open() call since the rest of fds will be in the range,
> [first_fd, last_fd].
> 
> > -	cleanup();
> > -	tst_exit();
> > +	if (fd[count - 1] == (max_val_opfiles - 1))
> > +		tst_res(TPASS,
> > +			"max open file fd is correct, %d = %d",
> > +			fd[count - 1], (max_val_opfiles - 1));
> > +	else if (temp_fd < 0 && errno == ENFILE)
> > +		tst_brk(TCONF,
> > +			"Reached maximum number of open files for the system");
> > +	else
> > +		tst_res(TFAIL | TERRNO, "%d != %d", fd[count - 1],
> (max_val_opfiles
> > +- 1));
> >  }
> >
> > -void setup(void)
> > +static void setup(void)
> >  {
> > -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> > -
> > -	TEST_PAUSE;
> > +	fd = SAFE_MALLOC(FILE_OPEN_MAX * sizeof(int));
> >  }
> >
> > -void cleanup(void)
> > +static void cleanup(void)
> >  {
> > +	int i;
> > +	for (i = 0; i < count; i++)
> > +		SAFE_CLOSE(fd[i]);
> > +
> > +	free(fd);
> > +	fd = NULL;
> >  }
> > +
> > +static struct tst_test test = {
> > +	.needs_tmpdir = 1,
> > +	.test_all = run,
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +};
> > +
> > --
> > 2.17.1
> >
> >
> > --
> > Mailing list info: https://lists.linux.it/listinfo/ltp
> 
> --
> Cyril Hrubis
> chrubis@suse.cz