[LTP] [PATCH] pipe07: close /proc/self/fd after counting fds

[LTP] [PATCH] pipe07: close /proc/self/fd after counting fds

[Edward Liaw via ltp]

Leaving the directory fd open will count against the max number of fds
opened, so the final expected count will be off.

Also, removed the halving / doubling of exp_num_pipes since it is
redundant.

Signed-off-by: Edward Liaw <edliaw@google.com>
---
 testcases/kernel/syscalls/pipe/pipe07.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/pipe/pipe07.c b/testcases/kernel/syscalls/pipe/pipe07.c
index 196485684..01f6b78f8 100644
--- a/testcases/kernel/syscalls/pipe/pipe07.c
+++ b/testcases/kernel/syscalls/pipe/pipe07.c
@@ -45,6 +45,8 @@ static int record_open_fds(void)
 		opened_fds[num_opened_fds++] = fd;
 	}
 
+	SAFE_CLOSEDIR(dir);
+
 	return num_opened_fds;
 }
 
@@ -56,8 +58,8 @@ static void setup(void)
 	tst_res(TINFO, "getdtablesize() = %d", max_fds);
 	pipe_fds = SAFE_MALLOC(max_fds * sizeof(int));
 
-	exp_num_pipes = (max_fds - record_open_fds()) / 2;
-	tst_res(TINFO, "expected max fds to be opened by pipe(): %d", exp_num_pipes * 2);
+	exp_num_pipes = max_fds - record_open_fds();
+	tst_res(TINFO, "expected max fds to be opened by pipe(): %d", exp_num_pipes);
 }
 
 static void run(void)
@@ -73,7 +75,7 @@ static void run(void)
 	} while (!TST_RET);
 
 	TST_EXP_EQ_LI(errno, EMFILE);
-	TST_EXP_EQ_LI(exp_num_pipes * 2, num_pipe_fds);
+	TST_EXP_EQ_LI(exp_num_pipes, num_pipe_fds);
 
 	for (int i = 0; i < num_pipe_fds; i++)
 		SAFE_CLOSE(pipe_fds[i]);
-- 
2.42.0.582.g8ccd20d70d-goog


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

Re: [LTP] [PATCH] pipe07: close /proc/self/fd after counting fds

[Petr Vorel]

Hi Edward,

> Leaving the directory fd open will count against the max number of fds
> opened, so the final expected count will be off.

> Also, removed the halving / doubling of exp_num_pipes since it is
> redundant.

> Signed-off-by: Edward Liaw <edliaw@google.com>
> ---
>  testcases/kernel/syscalls/pipe/pipe07.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

> diff --git a/testcases/kernel/syscalls/pipe/pipe07.c b/testcases/kernel/syscalls/pipe/pipe07.c
> index 196485684..01f6b78f8 100644
> --- a/testcases/kernel/syscalls/pipe/pipe07.c
> +++ b/testcases/kernel/syscalls/pipe/pipe07.c
> @@ -45,6 +45,8 @@ static int record_open_fds(void)
>  		opened_fds[num_opened_fds++] = fd;
>  	}

> +	SAFE_CLOSEDIR(dir);
IMHO this changes counting from 1020:

pipe07.c:56: TINFO: getdtablesize() = 1024
pipe07.c:60: TINFO: expected max fds to be opened by pipe(): 1020
pipe07.c:75: TPASS: errno == EMFILE (24)
pipe07.c:76: TPASS: exp_num_pipes * 2 == num_pipe_fds (1020)

to 1021, which leads to failure:

pipe07.c:58: TINFO: getdtablesize() = 1024
pipe07.c:62: TINFO: expected max fds to be opened by pipe(): 1021
pipe07.c:77: TPASS: errno == EMFILE (24)
pipe07.c:78: TFAIL: exp_num_pipes (1021) != num_pipe_fds (1020)

> +
>  	return num_opened_fds;
This is not elegant, but get's the correct count. Will it fail on AOSP?

return num_opened_fds + 1;
>  }

> @@ -56,8 +58,8 @@ static void setup(void)
>  	tst_res(TINFO, "getdtablesize() = %d", max_fds);
>  	pipe_fds = SAFE_MALLOC(max_fds * sizeof(int));

> -	exp_num_pipes = (max_fds - record_open_fds()) / 2;
> -	tst_res(TINFO, "expected max fds to be opened by pipe(): %d", exp_num_pipes * 2);
> +	exp_num_pipes = max_fds - record_open_fds();
> +	tst_res(TINFO, "expected max fds to be opened by pipe(): %d", exp_num_pipes);

It'd be slightly more readable if this was in separate patch
(as it modifies the same variable), but it's up to you.

Kind regards,
Petr

>  }

>  static void run(void)
> @@ -73,7 +75,7 @@ static void run(void)
>  	} while (!TST_RET);

>  	TST_EXP_EQ_LI(errno, EMFILE);
> -	TST_EXP_EQ_LI(exp_num_pipes * 2, num_pipe_fds);
> +	TST_EXP_EQ_LI(exp_num_pipes, num_pipe_fds);

>  	for (int i = 0; i < num_pipe_fds; i++)
>  		SAFE_CLOSE(pipe_fds[i]);

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

Re: [LTP] [PATCH] pipe07: close /proc/self/fd after counting fds

[Edward Liaw via ltp]

On Fri, Sep 29, 2023, 11:34 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Edward,
>
> > Leaving the directory fd open will count against the max number of fds
> > opened, so the final expected count will be off.
>
> > Also, removed the halving / doubling of exp_num_pipes since it is
> > redundant.


> > Signed-off-by: Edward Liaw <edliaw@google.com>
> > ---
> >  testcases/kernel/syscalls/pipe/pipe07.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
>
> > diff --git a/testcases/kernel/syscalls/pipe/pipe07.c
> b/testcases/kernel/syscalls/pipe/pipe07.c
> > index 196485684..01f6b78f8 100644
> > --- a/testcases/kernel/syscalls/pipe/pipe07.c
> > +++ b/testcases/kernel/syscalls/pipe/pipe07.c
> > @@ -45,6 +45,8 @@ static int record_open_fds(void)
> >               opened_fds[num_opened_fds++] = fd;
> >       }
>
> > +     SAFE_CLOSEDIR(dir);
> IMHO this changes counting from 1020:
>
> pipe07.c:56: TINFO: getdtablesize() = 1024
> pipe07.c:60: TINFO: expected max fds to be opened by pipe(): 1020
> pipe07.c:75: TPASS: errno == EMFILE (24)
> pipe07.c:76: TPASS: exp_num_pipes * 2 == num_pipe_fds (1020)
>
> to 1021, which leads to failure:
>
> pipe07.c:58: TINFO: getdtablesize() = 1024
> pipe07.c:62: TINFO: expected max fds to be opened by pipe(): 1021
> pipe07.c:77: TPASS: errno == EMFILE (24)
> pipe07.c:78: TFAIL: exp_num_pipes (1021) != num_pipe_fds (1020)
>
> > +
> >       return num_opened_fds;
> This is not elegant, but get's the correct count. Will it fail on AOSP?
>
> return num_opened_fds + 1;
>

I'll get back to you on Monday, but I think this is because each pipe
creates two fds, the resulting exp number of pipes will have to be even,
and I messed that up by removing the halving/doubling.  When I tested it,
the exp_num_pipes happened to be even, so I didn't run into the odd case.

>  }
>
> > @@ -56,8 +58,8 @@ static void setup(void)
> >       tst_res(TINFO, "getdtablesize() = %d", max_fds);
> >       pipe_fds = SAFE_MALLOC(max_fds * sizeof(int));
>
> > -     exp_num_pipes = (max_fds - record_open_fds()) / 2;
> > -     tst_res(TINFO, "expected max fds to be opened by pipe(): %d",
> exp_num_pipes * 2);
> > +     exp_num_pipes = max_fds - record_open_fds();
> > +     tst_res(TINFO, "expected max fds to be opened by pipe(): %d",
> exp_num_pipes);
>
> It'd be slightly more readable if this was in separate patch
> (as it modifies the same variable), but it's up to you.
>
> Kind regards,
> Petr
>
> >  }
>
> >  static void run(void)
> > @@ -73,7 +75,7 @@ static void run(void)
> >       } while (!TST_RET);
>
> >       TST_EXP_EQ_LI(errno, EMFILE);
> > -     TST_EXP_EQ_LI(exp_num_pipes * 2, num_pipe_fds);
> > +     TST_EXP_EQ_LI(exp_num_pipes, num_pipe_fds);
>
> >       for (int i = 0; i < num_pipe_fds; i++)
> >               SAFE_CLOSE(pipe_fds[i]);
>

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