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

* [LTP] [PATCH] syscalls/getdtablesize: Update to the new api
@ 2021-04-25  2:38 zhaogongyi
  0 siblings, 0 replies; 3+ messages in thread
From: zhaogongyi @ 2021-04-25  2:38 UTC (permalink / raw)
  To: ltp

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

^ permalink raw reply	[flat|nested] 3+ messages in thread