Re: [LTP] [PATCH v1] Refactor getdtablesize01 using new LTP API

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Andrea Cervesato <andrea.cervesato@suse.de> writes:

> From: Andrea Cervesato <andrea.cervesato@suse.com>
>
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  .../syscalls/getdtablesize/getdtablesize01.c  | 146 +++++++-----------
>  1 file changed, 52 insertions(+), 94 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c b/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> index d25cac261..80321e24f 100644
> --- a/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> +++ b/testcases/kernel/syscalls/getdtablesize/getdtablesize01.c
> @@ -1,119 +1,77 @@
> +// 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.
> - *
> + *     Prashant P Yendigeri <prashant.yendigeri@wipro.com>
> + *     Robbie Williamson <robbiew@us.ibm.com>
> + * Copyright (C) 2023 SUSE LLC Andrea Cervesato <andrea.cervesato@suse.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.

It seems this test has been broken for a long time unless I am
mistaken. glibc does not call the actual syscall, it calls
prlimit. Which has different behaviour (see below).

> - *
> - **********************************************************/
>  
> -#include <stdio.h>
> -#include <errno.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <fcntl.h>
> -#include <sys/time.h>
> -#include <sys/resource.h>
> -#include <unistd.h>
> -#include "test.h"
> +/*\
> + * [Description]
> + *
> + * This test verifies that getdtablesize() syscall returns the right max number
> + * of files which can be opened by a process. We test this in 2 ways: firstly by
> + * comparing its return value with RLIMIT_NOFILE, secondly by opening as many
> + * files as possible and then checking when open() raises EMFILE.
> + */
>  
> -void setup();
> -void cleanup();
> +#include <stdlib.h>
> +#include "tst_test.h"
>  
> -char *TCID = "getdtablesize01";
> -int TST_TOTAL = 1;
> +#define FILENAME "rofile"
>  
> -int main(void)
> +static void run(void)
>  {
> -	int table_size, fd = 0, count = 0;
> -	int max_val_opfiles;
> +	int *fds;
> +	int overfd;
> +	int open_files;
>  	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();
> -	}
> +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlp);
> +
> +	TST_EXP_EQ_LI(getdtablesize(), rlp.rlim_cur);

The actual syscall may not be equal to the prlimit AFAICT (this can be
tested using the prlimit command).

The table size can be set with a system wide sysctl /proc/sys/fs/nr_open
defined in fs/file_table.c. This is used to set the initial prlimit, but
can be reduced.

> +
> +	tst_res(TINFO, "Start opening as many files as possible");
> +
> +	overfd = rlp.rlim_cur + 5;
>  
> -	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);
> +	fds = (int *)SAFE_MALLOC(overfd * sizeof(int));
>  
> -		if (fd == -1)
> +	for (open_files = 0; open_files < overfd; open_files++) {
> +		TEST(open(FILENAME, O_RDONLY));
> +
> +		if (TST_RET == -1 && TST_ERR == EMFILE) {

This could also fail with ENOMEM at least. Especially in a memcg and
arch with large page sizes if there are any page sized allocations.

Perhaps on some FS there would be other limits as well, resulting in a
different error, but you can ignore that for now if you want.

> +			tst_res(TINFO, "Reached max amount of open files per process");
>  			break;
> -		count = fd;
> +		}
>  
> -#ifdef DEBUG
> -		printf("Opened file num %d\n", fd);
> -#endif
> +		fds[open_files] = TST_RET;
>  	}
>  
> -//Now the max files opened should be RLIMIT_NOFILE - 1 , why ? read getdtablesize man page
> +	--open_files;
>  
> -	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));
> +	tst_res(TINFO, "Opened %d files", open_files);
>  
> -	cleanup();
> -	tst_exit();
> -}
> +	for (int i = 0; i <= open_files; i++)
> +		SAFE_CLOSE(fds[i]);
>  
> -void setup(void)
> -{
> -	tst_sig(NOFORK, DEF_HANDLER, cleanup);
> +	free(fds);
>  
> -	TEST_PAUSE;
> +	TST_EXP_EXPR(getdtablesize() >= open_files,
> +		"max amount of open files per process hasn't been overflowed");
>  }
>  
> -void cleanup(void)
> +static void setup(void)
>  {
> +	int fd;
> +
> +	fd = SAFE_CREAT(FILENAME, 0644);
> +	SAFE_CLOSE(fd);
>  }
> +
> +static struct tst_test test = {
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_tmpdir = 1,

This tests the underlying file system to some extent, so we should test
all available file systems.

> +};
> -- 
> 2.35.3


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp