syscalls/pipe07: Rewrite the test using new LTP API

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 testcases/kernel/syscalls/pipe/pipe07.c | 201 +++++++-----------------
 1 file changed, 60 insertions(+), 141 deletions(-)

Hi Avinesh,

generally LGTM, thank you.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

BTW it's funny that when run with valgrind, it fails because valgrind opens some
file descriptors:

$ valgrind ./pipe07
...
pipe07.c:45: TINFO: getdtablesize() = 1024
pipe07.c:49: TINFO: open fds before pipe() calls: 10
pipe07.c:54: TINFO: expected max fds to be opened by pipe(): 1014
==1629480== Warning: invalid file descriptor 1030 in syscall pipe2()
pipe07.c:69: TPASS: errno == EMFILE (24)
pipe07.c:70: TFAIL: exp_num_pipe_fds (1014) != num_pipe_fds (1020)

> +static void record_open_fds(void)
nit: num_opened_fds is used only in setup(), I'd personally return int
and store variable in setup().

>  {
> +	DIR *dir;
> +	struct dirent *ent;
> +	int fd;

> -	min = getdtablesize() - rec_fds_max;
> +	dir = SAFE_OPENDIR("/proc/self/fd");
...

> +static void run(void)
>  {
...
> +	do {
> +		TEST(pipe(fds));
> +		if (TST_RET != -1) {
nit: wouldn't be safer to use: if (!TST_RET) (i.e. for TST_RET == 0)
(we check that return on error is exactly -1, not > 0)

Kind regards,
Petr

> +			pipe_fds[num_pipe_fds++] = fds[0];
> +			pipe_fds[num_pipe_fds++] = fds[1];
>  		}
> +	} while (TST_RET != -1);

...

Hi!
> BTW it's funny that when run with valgrind, it fails because valgrind opens some
> file descriptors:

For that to happen they have to be opened after the test setup() in the
context of the process, which does sound strange....

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

Hi Petr,
Thank you for the review.

On Tuesday, July 25, 2023 3:15:57 PM IST Petr Vorel wrote:
> Hi Avinesh,
> 
> generally LGTM, thank you.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> BTW it's funny that when run with valgrind, it fails because valgrind opens
> some file descriptors:
> 
> $ valgrind ./pipe07
> ...
> pipe07.c:45: TINFO: getdtablesize() = 1024
> pipe07.c:49: TINFO: open fds before pipe() calls: 10
> pipe07.c:54: TINFO: expected max fds to be opened by pipe(): 1014
> ==1629480== Warning: invalid file descriptor 1030 in syscall pipe2()
> pipe07.c:69: TPASS: errno == EMFILE (24)
> pipe07.c:70: TFAIL: exp_num_pipe_fds (1014) != num_pipe_fds (1020)
> 
Ah yes, I also tried this. So valgrind is opening some file descriptors in the 
process context, but I don't know if it's ok to ignore this behavior. 

I'm taking care of other nit suggestions in revised patch.

> > +static void record_open_fds(void)
> 
> nit: num_opened_fds is used only in setup(), I'd personally return int
> and store variable in setup().
> 
> >  {
> > 
> > +	DIR *dir;
> > +	struct dirent *ent;
> > +	int fd;
> > 
> > -	min = getdtablesize() - rec_fds_max;
> > +	dir = SAFE_OPENDIR("/proc/self/fd");
> 
> ...
> 
> > +static void run(void)
> > 
> >  {
> 
> ...
> 
> > +	do {
> > +		TEST(pipe(fds));
> > +		if (TST_RET != -1) {
> 
> nit: wouldn't be safer to use: if (!TST_RET) (i.e. for TST_RET == 0)
> (we check that return on error is exactly -1, not > 0)
> 
> Kind regards,
> Petr
> 
> > +			pipe_fds[num_pipe_fds++] = fds[0];
> > +			pipe_fds[num_pipe_fds++] = fds[1];
> > 
> >  		}
> > 
> > +	} while (TST_RET != -1);
> 
> ...

--
Regards,
Avinesh

Hi Cyril,

Thank you for reviewing the patch and the suggested changes. I'm incorporating 
the suggested changes in the v2.

--
Regards,
Avinesh