Re: [LTP] [PATCH] syscalls/pipe07: Rewrite the test using new LTP API

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> +	while ((ent = SAFE_READDIR(dir))) {
> +		if (!strcmp(ent->d_name, ".") ||
> +			!strcmp(ent->d_name, ".."))
> +			continue;
> +		fd = atoi(ent->d_name);

What about the if (fd == dir_fd) continue; why did you drop that part
from here?

> +		opened_fds = SAFE_REALLOC(opened_fds, (num_opened_fds + 1) * sizeof(int));

It's more usuall to double array size if we go out of space since
incresing the size by one element is` slow in case of realloc(). I guess
that it does not matter us much here, but I would do something as:

	int arr_size = 0;
	int arr_used = 0;
	int *array = NULL;

	if (arr_used >= arr_size) {
		arr_size = MAX(1, arr_size * 2);
		array = realloc(array, arr_size * sizeof(int));
	}

	array[arr_used++] = foo;

> +		opened_fds[num_opened_fds++] = fd;
>  	}
> -
> -	cleanup();
> -	tst_exit();
>  }
>  
>  static void setup(void)
>  {
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	TEST_PAUSE;
> +	int max_fds;
> +
> +	max_fds = getdtablesize();
> +	tst_res(TINFO, "getdtablesize() = %d", max_fds);
> +	pipe_fds = SAFE_MALLOC(max_fds * sizeof(int));
>  
>  	record_open_fds();
> +	tst_res(TINFO, "open fds before pipe() calls: %d", num_opened_fds);
> +
> +	exp_num_pipe_fds = max_fds - num_opened_fds;
> +	exp_num_pipe_fds = (exp_num_pipe_fds % 2) ?
> +						(exp_num_pipe_fds - 1) : exp_num_pipe_fds;

The previous code that compared the number of pipes was IMHO easier to
read, i.e.

	exp_num_pipes = (max_fds - num_opened_fds)/2;

Then you can use:

	2 * exp_num_pipes as the number of expected fds

> +	tst_res(TINFO, "expected max fds to be opened by pipe(): %d", exp_num_pipe_fds);
>  }
>  
> -static void record_open_fds(void)
> +static void run(void)
>  {
> -	DIR *dir = opendir("/proc/self/fd");
> -	int dir_fd, fd;
> -	struct dirent *file;
> +	int fds[2];
>  
> -	if (dir == NULL)
> -		tst_brkm(TBROK | TERRNO, cleanup, "opendir()");
> -
> -	dir_fd = dirfd(dir);
> -
> -	if (dir_fd == -1)
> -		tst_brkm(TBROK | TERRNO, cleanup, "dirfd()");
> -
> -	errno = 0;
> -
> -	while ((file = readdir(dir))) {
> -		if (!strcmp(file->d_name, ".") || !strcmp(file->d_name, ".."))
> -			continue;
> -
> -		fd = atoi(file->d_name);
> -
> -		if (fd == dir_fd)
> -			continue;
> -
> -		if (rec_fds_max >= (int)ARRAY_SIZE(rec_fds)) {
> -			tst_brkm(TBROK, cleanup,
> -			         "Too much file descriptors open");
> +	do {
> +		TEST(pipe(fds));
> +		if (TST_RET != -1) {
> +			pipe_fds[num_pipe_fds++] = fds[0];
> +			pipe_fds[num_pipe_fds++] = fds[1];
>  		}
> +	} while (TST_RET != -1);
>  
> -		rec_fds[rec_fds_max++] = fd;
> -	}
> -
> -	if (errno)
> -		tst_brkm(TBROK | TERRNO, cleanup, "readdir()");
> +	TST_EXP_EQ_LI(errno, EMFILE);
> +	TST_EXP_EQ_LI(exp_num_pipe_fds, num_pipe_fds);
>  
> -	closedir(dir);
> +	for (int i = 0; i < num_pipe_fds; i++)

I suppose that this is fine since we now compile with -std=gnu99

> +		SAFE_CLOSE(pipe_fds[i]);
>  
> -	tst_resm(TINFO, "Found %u files open", rec_fds_max);
> +	num_pipe_fds = 0;
>  }
>  
> -static int not_recorded(int fd)
> +static void cleanup(void)
>  {
> -	int i;
> +	if (opened_fds)
> +		free(opened_fds);
>  
> -	for (i = 0; i < rec_fds_max; i++)
> -		if (fd == rec_fds[i])
> -			return 0;
> +	if (pipe_fds)
> +		free(pipe_fds);
>  
> -	return 1;
> -}
> -
> -static void close_test_fds(int max_fd)
> -{
> -	int i;
> -
> -	for (i = 0; i <= max_fd; i++) {
> -		if (not_recorded(i)) {
> -			if (close(i)) {
> -				if (errno == EBADF)
> -					continue;
> -				tst_resm(TWARN | TERRNO, "close(%i)", i);
> -			}
> -		}
> -	}
> +	for (int i = 0; i < num_pipe_fds; i++)
> +		if (pipe_fds[i])
                      ^
		      This should be pipe_fds[i] > 0

However this branch is never triggered as long as the loop that closes
all pipe_fds in run succeeds. I'm not sure that if we fail to close a
pipe it makes sense to contine closing the rest. Since pipes are only
backed by RAM the possible failures from close() would mean that
something horrible happend to the kernel, possibly RAM corruption or
something along the lines.

> +			SAFE_CLOSE(pipe_fds[i]);
>  }
>  
> -static void cleanup(void)
> -{
> -}
> +static struct tst_test test = {
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run
> +};
> -- 
> 2.41.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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