Page MenuHomeFreeBSD
Differential D21900

Fix problems in the kern_maxfiles__increase test
ClosedPublic
Actions

Authored by vangyzen on Oct 4 2019, 9:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Sep 6, 10:37 PM
Unknown Object (File)
Sun, Sep 6, 3:34 PM
Unknown Object (File)
Sun, Sep 6, 9:20 AM
Unknown Object (File)
Sat, Sep 5, 3:59 PM
Unknown Object (File)
Sat, Sep 5, 5:55 AM
Unknown Object (File)
Fri, Sep 4, 4:50 AM
Unknown Object (File)
Thu, Sep 3, 3:37 PM
Unknown Object (File)
Thu, Sep 3, 1:22 PM
View All Files
Subscribers
badger
dab
imp
koobs
uqs

Details

Reviewers
asomers
Commits
rS353305: Fix problems in the kern_maxfiles__increase test
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

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26885
Build 25201: arc lint + arc unit

Event Timeline

vangyzen created this revision.Oct 4 2019, 9:33 PM
Herald added subscribers: dab, badger. · View Herald TranscriptOct 4 2019, 9:33 PM
Harbormaster completed remote builds in B26877: Diff 62921.Oct 4 2019, 9:33 PM
vangyzen edited the test plan for this revision. (Show Details)Oct 4 2019, 9:50 PM
asomers added subscribers: uqs, koobs.Oct 4 2019, 9:57 PM

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

tests/sys/kern/kern_descrip_test.c
107

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–136

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

138

What's the point of this sleep?

177–182

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

199

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.Oct 4 2019, 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

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

133–136

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

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

177–182

I'll consider it.

199

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

vangyzen marked 4 inline comments as done.Oct 4 2019, 11:31 PM
vangyzen updated this revision to Diff 62931.Oct 5 2019, 1:18 PM
  • fixup: make test more robust by opening more files
Harbormaster completed remote builds in B26884: Diff 62931.Oct 5 2019, 1:18 PM
vangyzen updated this revision to Diff 62932.Oct 5 2019, 1:20 PM
  • rebase
Harbormaster completed remote builds in B26885: Diff 62932.Oct 5 2019, 1:20 PM
vangyzen marked an inline comment as done.Oct 5 2019, 1:27 PM
vangyzen added inline comments.
tests/sys/kern/kern_descrip_test.c
177–182

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.Oct 5 2019, 1:47 PM
asomers added inline comments.Oct 5 2019, 10:56 PM
tests/sys/kern/kern_descrip_test.c
133–136

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

138

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.Oct 6 2019, 12:28 AM
  • wait() for children instead of a sleep race
Harbormaster completed remote builds in B26891: Diff 62949.Oct 6 2019, 12:28 AM
vangyzen marked 2 inline comments as done.Oct 6 2019, 12:34 AM
vangyzen added inline comments.
tests/sys/kern/kern_descrip_test.c
133–136

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

138

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.Oct 8 2019, 1:43 PM
Closed by commit rS353305: Fix problems in the kern_maxfiles__increase test (authored by vangyzen). · Explain Why
This revision was automatically updated to reflect the committed changes.
vangyzen marked 2 inline comments as done.
vangyzen added a commit: rS353305: Fix problems in the kern_maxfiles__increase test.
Herald added a subscriber: imp. · View Herald TranscriptOct 8 2019, 1:43 PM

Revision Contents
Changeset List

PathSize
tests/
sys/
kern/
36 lines

Diff 62932

View Options

tests/sys/kern/kern_descrip_test.c

Loading...