Page MenuHomeFreeBSD

Fix problems in the kern_maxfiles__increase test
ClosedPublic

Authored by vangyzen on Fri, Oct 4, 9:33 PM.

Details

Summary

ATF functions such as ATF_REQUIRE do not work correctly in child processes.
Use plain C functions to report errors instead.

In the parent, check for the untimely demise of children. Without this,
the test hung until the framework's timeout.

Raise the resource limit on the number of open files. If this was too low,
the test hit the two problems above.

Restore the kern.maxfiles sysctl OID in the cleanup function.
The body prematurely removed the symlink in which the old value was saved.

Sort includes.

Clean up a temporary file created by the test ("afile").

Test Plan
$ ulimit -n
234792
$ kyua test kern_descrip_test:kern_maxfiles__increase
kern_descrip_test:kern_maxfiles__increase  ->  passed  [0.369s]

Results file id is usr_tests_sys_kern.20191004-214922-978634
Results saved to /root/.kyua/store/results.usr_tests_sys_kern.20191004-214922-978634.db

1/1 passed (0 failed)
$ ulimit -n 1000
$ ulimit -n
1000
$ kyua test kern_descrip_test:kern_maxfiles__increase
kern_descrip_test:kern_maxfiles__increase  ->  passed  [0.420s]

Results file id is usr_tests_sys_kern.20191004-214932-330605
Results saved to /root/.kyua/store/results.usr_tests_sys_kern.20191004-214932-330605.db

1/1 passed (0 failed)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vangyzen created this revision.Fri, Oct 4, 9:33 PM
vangyzen edited the test plan for this revision. (Show Details)Fri, Oct 4, 9:50 PM
asomers added subscribers: uqs, koobs.Fri, Oct 4, 9:57 PM

That's not much of a test plan. Perhaps try sudo?

tests/sys/kern/kern_descrip_test.c
107 ↗(On Diff #62921)

FWIW Coverity is going to complain about a file descriptor leak here. I've been working with @uqs and @koobs to find a way to annotate these leaks, but so far nothing will make Coverity shut up.

133 ↗(On Diff #62921)

If you remove WNOHANG, then, the 1000us sleep would not be necessary.

138 ↗(On Diff #62921)

What's the point of this sleep?

182 ↗(On Diff #62921)

I think the test would be more robust if you changed 1 to EXPAND / 2.

199 ↗(On Diff #62921)

This should be unnecessary. The temporary files are contained within Kyua's temporary directory, which will be automatically cleaned up.

vangyzen marked 4 inline comments as done.Fri, Oct 4, 11:31 PM

That's not much of a test plan. Perhaps try sudo?

No kidding. I ran it in the wrong terminal window and didn't notice until right _after_ I created the review. (The testing during coding was run as root.)

tests/sys/kern/kern_descrip_test.c
107 ↗(On Diff #62921)

Thank you. Coverity is really helpful, but training it to avoid FPs seems to take a lot of work.

133 ↗(On Diff #62921)

The test would hang if I did that. The children don't exit until the parent unlinks the RENDEZVOUS file. The children need to hit their max file descriptor count simultaneously in order for the test to be valid.

138 ↗(On Diff #62921)

Presumably to let all the children exit before the parent does. It wasn't broken before, so I didn't fix it.

182 ↗(On Diff #62921)

I'll consider it.

199 ↗(On Diff #62921)

It's unnecessary, but I consider it good form.

vangyzen marked 4 inline comments as done.Fri, Oct 4, 11:31 PM
vangyzen updated this revision to Diff 62931.Sat, Oct 5, 1:18 PM
  • fixup: make test more robust by opening more files
vangyzen updated this revision to Diff 62932.Sat, Oct 5, 1:20 PM
  • rebase
vangyzen marked an inline comment as done.Sat, Oct 5, 1:27 PM
vangyzen added inline comments.
tests/sys/kern/kern_descrip_test.c
182 ↗(On Diff #62921)

As it turns out, your suggestion is essential for the validity of the test, due to the integer division on line 131. Thanks!

vangyzen removed a reviewer: pho.Sat, Oct 5, 1:47 PM
asomers added inline comments.Sat, Oct 5, 10:56 PM
tests/sys/kern/kern_descrip_test.c
133 ↗(On Diff #62921)

Oh, I get it. RENDEZVOUS is a hokey barrier. You could do this more elegantly with pthread_barrier_wait and friends.

138 ↗(On Diff #62921)

But it doesn't actually do that. It just creates a race. I think you should either delete it, or fix it by using proper synchronization primitives.

vangyzen updated this revision to Diff 62949.Sun, Oct 6, 12:28 AM
  • wait() for children instead of a sleep race
vangyzen marked 2 inline comments as done.Sun, Oct 6, 12:34 AM
vangyzen added inline comments.
tests/sys/kern/kern_descrip_test.c
133 ↗(On Diff #62921)

I could, but I should move on to other work. Feel free, if you're interested.

138 ↗(On Diff #62921)

You mean it doesn't guarantee that. In practice, it probably did let the children exit. Even if it didn't, the race was harmless.

I didn't want to put any more effort into this test, but since you found a nice bug, I'll return the favor. :)

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Oct 8, 1:43 PM
This revision was automatically updated to reflect the committed changes.
vangyzen marked 2 inline comments as done.