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

[Avinesh Kumar <akumar@suse.de>]

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



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