Details

Reviewers

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 26884
Build 25200: 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

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.

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

fixup: make test more robust by opening more files

Harbormaster completed remote builds in B26884: Diff 62931.Oct 5 2019, 1:18 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.

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

Fix problems in the kern_maxfiles__increase test
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 62931

sys/kern/vfs_bio.c

sys/kern/vfs_subr.c

sys/sys/sysctl.h

sys/ufs/ffs/ffs_softdep.c

sys/x86/x86/intr_machdep.c

tests/sys/kern/kern_descrip_test.c

Fix problems in the kern_maxfiles__increase testClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 62931

sys/kern/vfs_bio.c

sys/kern/vfs_subr.c

sys/sys/sysctl.h

sys/ufs/ffs/ffs_softdep.c

sys/x86/x86/intr_machdep.c

tests/sys/kern/kern_descrip_test.c

Fix problems in the kern_maxfiles__increase test
ClosedPublic
Actions

Revision Contents
Changeset List