Details

tests/sys/kern/coredump_phnum_test.sh
1 ↗	(On Diff #18609)	Add licensing info to this file?
6 ↗	(On Diff #18609)	Sort by name?
16 ↗	(On Diff #18609)	This needs atf_set "allow_sysctl_side_effects" "1"
33 ↗	(On Diff #18609)	Where does 66169 come from?

tests/sys/kern/coredump_phnum_test.sh
1 ↗	(On Diff #18609)	Some other such tests don't have licensing. What's the suggested format for these?
6 ↗	(On Diff #18609)	Ok.
16 ↗	(On Diff #18609)	In `head()`? Ok.
33 ↗	(On Diff #18609)	It doesn't have any real significance. It's just the correct result of running readelf against the output of the helper program. The helper program asks for 66535 mmap segments and ends up with 66169 kernel mappings. Well, ok, the significance is it's larger than 65535 (`UINT16_MAX`). The wrong (and previous) answer here is 633 (66169 mod 65536).

cem marked 2 inline comments as done.Jul 20 2016, 9:21 PM

Sort atf_sets and add sysctl knob.

tests/sys/kern/coredump_phnum_helper.c
38 ↗	(On Diff #18611)	`main(void)` ?
tests/sys/kern/coredump_phnum_test.sh
11 ↗	(On Diff #18611)	This violates the ATF sandbox. Stuff needs to be copied from atf_get_srcdir to the sandbox.

tests/sys/kern/coredump_phnum_test.sh
33 ↗	(On Diff #18609)	Please add a comment to this effect.

cem marked 2 inline comments as done.Jul 22 2016, 6:15 PM

Try to avoid violating ATF sandbox. Does it look okay now? Does the _cleanup
routine run in the same sandbox directory as the _body?

Added a comment about the magic numbers.

ngie added a subscriber: tests.

Seems fine to me but I hope one of the kyua experts can accept

In D7264#161644, @emaste wrote:

Seems fine to me but I hope one of the kyua experts can accept

Yeah, I've been waiting for review/sign-off from someone familiar with kyua. :-)

tests/sys/kern/Makefile
7 ↗	(On Diff #18677)	I'd rather make this BINDIR.coredump_phnum_helper and move below with the test program definition. Otherwise it's not obvious why this is here.
tests/sys/kern/coredump_phnum_helper.c
37 ↗	(On Diff #18677)	Add comment to describe what this helper program is intended to do please. I find the fact that this never exits successfully surprising.
tests/sys/kern/coredump_phnum_test.sh
13 ↗	(On Diff #18677)	Remove indentation from generated script.
15 ↗	(On Diff #18677)	Quote the $() expansion.
22 ↗	(On Diff #18677)	Are all these system-wide changes guaranteed to succeed? I think you should make sure these calls all exit successfully, and error out otherwise. If there are legitimate cases for these calls to fail, then consider skipping the test case instead of failing it.
50 ↗	(On Diff #18677)	Check for the existence of the script with "test -x" before running it. The script might not have been created before the cleanup runs if the test crashes early, and you don't want to double-error on cleanup.

cem added inline comments.Sep 7 2016, 9:17 PM

tests/sys/kern/Makefile
7 ↗	(On Diff #18677)	I can move the line below. Does `BINDIR.coredump_phnum_helper` actually work?
tests/sys/kern/coredump_phnum_helper.c
37 ↗	(On Diff #18677)	I can add a comment. It does have "coredump" in the name… :-)
tests/sys/kern/coredump_phnum_test.sh
13 ↗	(On Diff #18677)	`<<-` does so.
15 ↗	(On Diff #18677)	`kern.coredump` is an integer.
22 ↗	(On Diff #18677)	They should all succeed (run by root). Is there a good way to `set e` in atf-sh tests? I suppose `atf_check -s exit:0` would work.
50 ↗	(On Diff #18677)	Ok.

Move BINDIR near coredump_phnum_helper.

Add comment describing goals of coredump_phnum_helper.

Only run cleanup helper script if it exists and is executable.

tests/sys/kern/coredump_phnum_test.sh
22 ↗	(On Diff #18677)	I don't think there's any reason for these to fail.

This revision was automatically updated to reflect the committed changes.

Add test case for >65535 segment coredumps
ClosedPublic
Actions