All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] open04: add EMFILE check
@ 2022-09-15  3:10 Li Wang
  2022-09-15 12:52 ` Petr Vorel
  0 siblings, 1 reply; 9+ messages in thread
From: Li Wang @ 2022-09-15  3:10 UTC (permalink / raw)
  To: ltp

[pre-release testing fix]

Test in automation easily get EMFILE error before reaching the fds_limit,
but hard to reproduce it again manually. The possible reason is that some
shared fd being opened in the parent shell and occupying the fd numbers
which inherited by test then results in open failed with EMFILE early.

This patch adds back of the EMFILE check in the open() loops, to flexible
test fd limitation.

  open04.c:36: TBROK: open(open04.1020,66,0777) failed: EMFILE (24)
  open04.c:53: TWARN: close(0) failed: EBADF (9)

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/syscalls/open/open04.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index d452405d4..01a8b12d6 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -33,7 +33,12 @@ static void setup(void)
 
 	for (i = first + 1; i < fds_limit; i++) {
 		sprintf(fname, FNAME ".%d", i);
-		fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0777);
+		fd = open(fname, O_RDWR | O_CREAT, 0777);
+		if (fd == -1) {
+			if (errno != EMFILE)
+				tst_brk(TBROK, "Expected EMFILE but got %d", errno);
+			break;
+		}
 		fds[i - first] = fd;
 	}
 }
-- 
2.35.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-15  3:10 [LTP] [PATCH] open04: add EMFILE check Li Wang
@ 2022-09-15 12:52 ` Petr Vorel
  2022-09-15 14:21   ` Cyril Hrubis
  0 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2022-09-15 12:52 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

Hi LI,

> [pre-release testing fix]

> Test in automation easily get EMFILE error before reaching the fds_limit,
> but hard to reproduce it again manually. The possible reason is that some
> shared fd being opened in the parent shell and occupying the fd numbers
> which inherited by test then results in open failed with EMFILE early.

> This patch adds back of the EMFILE check in the open() loops, to flexible
> test fd limitation.

>   open04.c:36: TBROK: open(open04.1020,66,0777) failed: EMFILE (24)
>   open04.c:53: TWARN: close(0) failed: EBADF (9)

> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  testcases/kernel/syscalls/open/open04.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

> diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
> index d452405d4..01a8b12d6 100644
> --- a/testcases/kernel/syscalls/open/open04.c
> +++ b/testcases/kernel/syscalls/open/open04.c
> @@ -33,7 +33,12 @@ static void setup(void)

>  	for (i = first + 1; i < fds_limit; i++) {
>  		sprintf(fname, FNAME ".%d", i);
> -		fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0777);
> +		fd = open(fname, O_RDWR | O_CREAT, 0777);
> +		if (fd == -1) {
> +			if (errno != EMFILE)
> +				tst_brk(TBROK, "Expected EMFILE but got %d", errno);
> +			break;
> +		}
>  		fds[i - first] = fd;
>  	}
>  }

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

Kind regards,
Petr


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-15 12:52 ` Petr Vorel
@ 2022-09-15 14:21   ` Cyril Hrubis
  2022-09-16  1:36     ` Li Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2022-09-15 14:21 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

Hi!
> > diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
> > index d452405d4..01a8b12d6 100644
> > --- a/testcases/kernel/syscalls/open/open04.c
> > +++ b/testcases/kernel/syscalls/open/open04.c
> > @@ -33,7 +33,12 @@ static void setup(void)
> 
> >  	for (i = first + 1; i < fds_limit; i++) {
> >  		sprintf(fname, FNAME ".%d", i);
> > -		fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0777);
> > +		fd = open(fname, O_RDWR | O_CREAT, 0777);
> > +		if (fd == -1) {
> > +			if (errno != EMFILE)
> > +				tst_brk(TBROK, "Expected EMFILE but got %d", errno);
> > +			break;
> > +		}
> >  		fds[i - first] = fd;
> >  	}
> >  }
> 
> LGTM.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>

I faintly remmeber a similar patch where we decided not to work around
for a test harness leaking filedescriptors into testcases.

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-15 14:21   ` Cyril Hrubis
@ 2022-09-16  1:36     ` Li Wang
  2022-09-16  9:39       ` Cyril Hrubis
  0 siblings, 1 reply; 9+ messages in thread
From: Li Wang @ 2022-09-16  1:36 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 1327 bytes --]

On Thu, Sep 15, 2022 at 10:19 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > > diff --git a/testcases/kernel/syscalls/open/open04.c
> b/testcases/kernel/syscalls/open/open04.c
> > > index d452405d4..01a8b12d6 100644
> > > --- a/testcases/kernel/syscalls/open/open04.c
> > > +++ b/testcases/kernel/syscalls/open/open04.c
> > > @@ -33,7 +33,12 @@ static void setup(void)
> >
> > >     for (i = first + 1; i < fds_limit; i++) {
> > >             sprintf(fname, FNAME ".%d", i);
> > > -           fd = SAFE_OPEN(fname, O_RDWR | O_CREAT, 0777);
> > > +           fd = open(fname, O_RDWR | O_CREAT, 0777);
> > > +           if (fd == -1) {
> > > +                   if (errno != EMFILE)
> > > +                           tst_brk(TBROK, "Expected EMFILE but got
> %d", errno);
> > > +                   break;
> > > +           }
> > >             fds[i - first] = fd;
> > >     }
> > >  }
> >
> > LGTM.
> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> I faintly remmeber a similar patch where we decided not to work around
> for a test harness leaking filedescriptors into testcases.
>

This also should be a solution, I searched the mailing list and got a
patch[1].
Do you mean adding that close-on-exec flag when opening fd in harness?

[1] https://lists.linux.it/pipermail/ltp/2020-November/019650.html

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2465 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-16  1:36     ` Li Wang
@ 2022-09-16  9:39       ` Cyril Hrubis
  2022-09-19 12:12         ` Li Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Cyril Hrubis @ 2022-09-16  9:39 UTC (permalink / raw)
  To: Li Wang; +Cc: LTP List

Hi!
> > I faintly remmeber a similar patch where we decided not to work around
> > for a test harness leaking filedescriptors into testcases.
> >
> 
> This also should be a solution, I searched the mailing list and got a
> patch[1].
> Do you mean adding that close-on-exec flag when opening fd in harness?

Yes, that way you can be sure that no file descriptors are leaked to the
tests.

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-16  9:39       ` Cyril Hrubis
@ 2022-09-19 12:12         ` Li Wang
  2022-09-20  5:53           ` Li Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Li Wang @ 2022-09-19 12:12 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 2378 bytes --]

Cyril Hrubis <chrubis@suse.cz> wrote:

Hi!
> > > I faintly remmeber a similar patch where we decided not to work around
> > > for a test harness leaking filedescriptors into testcases.
> > >
> >
> > This also should be a solution, I searched the mailing list and got a
> > patch[1].
> > Do you mean adding that close-on-exec flag when opening fd in harness?
>
> Yes, that way you can be sure that no file descriptors are leaked to the
> tests.
>

Ok, should I send patch v2 like this below?

Note: the automation test open04 got passed but I'm not sure
if this has a side effect on logs. But from my observation, some
tests (with old-API) log can't be collected anymore.

--- a/pan/ltp-pan.c
+++ b/pan/ltp-pan.c
@@ -443,7 +443,7 @@ int main(int argc, char **argv)
        }

        if (outputfilename) {
-               if (!freopen(outputfilename, "a+", stdout)) {
+               if (!freopen(outputfilename, "a+e", stdout)) {
                        fprintf(stderr,
                                "pan(%s): Error %s (%d) opening output file
'%s'\n",
                                panname, strerror(errno), errno,
@@ -565,7 +565,7 @@ int main(int argc, char **argv)
                } else if (starts == -1)        //wjh
                {
                        FILE *f = (FILE *) - 1;
-                       if ((f = fopen(PAN_STOP_FILE, "r")) != 0) {
+                       if ((f = fopen(PAN_STOP_FILE, "r+")) != 0) {
                                printf("Got %s Stopping!\n", PAN_STOP_FILE);
                                fclose(f);
                                unlink(PAN_STOP_FILE);
@@ -1277,7 +1277,7 @@ static char *slurp(char *file)
        int fd;
        struct stat sbuf;

-       if ((fd = open(file, O_RDONLY)) < 0) {
+       if ((fd = open(file, O_RDONLY | O_CLOEXEC)) < 0) {
                fprintf(stderr,
                        "pan(%s): open(%s,O_RDONLY) failed.  errno:%d
 %s\n",
                        panname, file, errno, strerror(errno));
@@ -1372,7 +1372,7 @@ static void write_kmsg(const char *fmt, ...)
        FILE *kmsg;
        va_list ap;

-       if ((kmsg = fopen("/dev/kmsg", "r+")) == NULL) {
+       if ((kmsg = fopen("/dev/kmsg", "r+e")) == NULL) {
                fprintf(stderr, "Error %s: (%d) opening /dev/kmsg\n",
                                strerror(errno), errno);
                exit(1);


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 3939 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-19 12:12         ` Li Wang
@ 2022-09-20  5:53           ` Li Wang
  2022-09-20 14:21             ` Cyril Hrubis
  2022-09-20 18:29             ` Petr Vorel
  0 siblings, 2 replies; 9+ messages in thread
From: Li Wang @ 2022-09-20  5:53 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: LTP List


[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

On Mon, Sep 19, 2022 at 8:12 PM Li Wang <liwang@redhat.com> wrote:

> Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
>> > > I faintly remmeber a similar patch where we decided not to work around
>> > > for a test harness leaking filedescriptors into testcases.
>> > >
>> >
>> > This also should be a solution, I searched the mailing list and got a
>> > patch[1].
>> > Do you mean adding that close-on-exec flag when opening fd in harness?
>>
>> Yes, that way you can be sure that no file descriptors are leaked to the
>> tests.
>>
>
> Ok, should I send patch v2 like this below?
>
> Note: the automation test open04 got passed but I'm not sure
> if this has a side effect on logs. But from my observation, some
> tests (with old-API) log can't be collected anymore.
>

Seems we shouldn't fix by adding 'close-on-exec' flag simply,
it brings more issues to some old-API tests, I'm still looking into
the problems which look like caused by ltp-pan designed.

So can we just merge the patch as the original?

-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 2401 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-20  5:53           ` Li Wang
@ 2022-09-20 14:21             ` Cyril Hrubis
  2022-09-20 18:29             ` Petr Vorel
  1 sibling, 0 replies; 9+ messages in thread
From: Cyril Hrubis @ 2022-09-20 14:21 UTC (permalink / raw)
  To: Li Wang; +Cc: LTP List

Hi!
> >> > > I faintly remmeber a similar patch where we decided not to work around
> >> > > for a test harness leaking filedescriptors into testcases.
> >> > >
> >> >
> >> > This also should be a solution, I searched the mailing list and got a
> >> > patch[1].
> >> > Do you mean adding that close-on-exec flag when opening fd in harness?
> >>
> >> Yes, that way you can be sure that no file descriptors are leaked to the
> >> tests.
> >>
> >
> > Ok, should I send patch v2 like this below?
> >
> > Note: the automation test open04 got passed but I'm not sure
> > if this has a side effect on logs. But from my observation, some
> > tests (with old-API) log can't be collected anymore.
> >
> 
> Seems we shouldn't fix by adding 'close-on-exec' flag simply,
> it brings more issues to some old-API tests, I'm still looking into
> the problems which look like caused by ltp-pan designed.

Sound strange, what is the problem here.

> So can we just merge the patch as the original?

Sounds sensible for the release, feel free to add:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [LTP] [PATCH] open04: add EMFILE check
  2022-09-20  5:53           ` Li Wang
  2022-09-20 14:21             ` Cyril Hrubis
@ 2022-09-20 18:29             ` Petr Vorel
  1 sibling, 0 replies; 9+ messages in thread
From: Petr Vorel @ 2022-09-20 18:29 UTC (permalink / raw)
  To: Li Wang; +Cc: LTP List

> On Mon, Sep 19, 2022 at 8:12 PM Li Wang <liwang@redhat.com> wrote:

> > Cyril Hrubis <chrubis@suse.cz> wrote:

> > Hi!
> >> > > I faintly remmeber a similar patch where we decided not to work around
> >> > > for a test harness leaking filedescriptors into testcases.


> >> > This also should be a solution, I searched the mailing list and got a
> >> > patch[1].
> >> > Do you mean adding that close-on-exec flag when opening fd in harness?

> >> Yes, that way you can be sure that no file descriptors are leaked to the
> >> tests.


> > Ok, should I send patch v2 like this below?

> > Note: the automation test open04 got passed but I'm not sure
> > if this has a side effect on logs. But from my observation, some
> > tests (with old-API) log can't be collected anymore.


> Seems we shouldn't fix by adding 'close-on-exec' flag simply,
> it brings more issues to some old-API tests, I'm still looking into
> the problems which look like caused by ltp-pan designed.

> So can we just merge the patch as the original?

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

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-09-20 18:29 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-15  3:10 [LTP] [PATCH] open04: add EMFILE check Li Wang
2022-09-15 12:52 ` Petr Vorel
2022-09-15 14:21   ` Cyril Hrubis
2022-09-16  1:36     ` Li Wang
2022-09-16  9:39       ` Cyril Hrubis
2022-09-19 12:12         ` Li Wang
2022-09-20  5:53           ` Li Wang
2022-09-20 14:21             ` Cyril Hrubis
2022-09-20 18:29             ` Petr Vorel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.