IPv6 Fragmentation Regression Tests from OpenBSD
Needs ReviewPublic
Actions

Authored by julius.flohr_uni-due.de on Oct 1 2018, 4:10 PM.

Details

Reviewers

thj
asomers

Group Reviewers

transport

Summary

Hi,

I started porting the IPv6 Regression tests from the OpenBSD project [1] to FreeBSD. This change includes the fragmentation tests to Kyua and uses ATF test; i.e. they can be run by running make check. Each tests spins up a jail, configures ipv6 and sends ICMP packets in order to test various aspects of the fragmentation functionality.
Other tests can also be ported but require more work.

Requirements: VNET, python2, scapy.

[1]: http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/regress/sys/netinet6/frag6/

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

julius.flohr_uni-due.de updated this revision to Diff 48624.Oct 1 2018, 4:10 PM

julius.flohr_uni-due.de created this revision.

julius.flohr_uni-due.de created this object with visibility "julius.flohr_uni-due.de (Julius Flohr)".

julius.flohr_uni-due.de edited the summary of this revision. (Show Details)Oct 1 2018, 4:14 PM

julius.flohr_uni-due.de added reviewers: thj, transport.

julius.flohr_uni-due.de changed the visibility from "julius.flohr_uni-due.de (Julius Flohr)" to "Public (No Login Required)".

Herald added subscribers: ae, asomers. · View Herald TranscriptOct 1 2018, 4:14 PM

deleted accidentally committed build artifact.

Wow, nice contribution! But a few things need to be fixed:

When we import foreign code, if there's any possibility at all that we might sync it again in the future, then we import it to the vendor directory before merging it to the contrib directory. See contrib/netbsd-tests for an example.
Other concerns inline.
I haven't fully reviewed the Python code. I'll do that after you fix the sh and make problems.
Any files that come from OpenBSD should be committed verbatim to the vendor directory, then copied to the contrib directory. Then they can be modified in contrib.

etc/mtree/BSD.tests.dist
751	The `ipv6` directory is redundant with `netinet6`. And I think you should leave out the `regress` directory too.
tests/sys/netinet6/ipv6/Makefile
6 ↗	(On Diff #48624)	Add `TEST_METADATA.fragmentation+= required_programs="python2"`. Otherwise the tests will fail when python is not installed. You also need some way to detect whether scapy is installed. Perhaps a `required_files`. And if every single test case requires root, then you can add `TEST_METADATA.fragmentation+= required_user="root"` here instead of in each individual test case header.
tests/sys/netinet6/ipv6/fragmentation
1 ↗	(On Diff #48624)	Remove this whole file. It's a built file, not a source file.
tests/sys/netinet6/ipv6/fragmentation.sh
1 ↗	(On Diff #48624)	Remove the shebang. Make will add it to the built file.
10 ↗	(On Diff #48624)	The filename isn't a useful description. Either give a real description, or leave it out entirely.
tests/sys/netinet6/ipv6/regress/frag6/frag6_maxlen.py
40 ↗	(On Diff #48624)	Using random behavior in tests is a poor technique because it makes the tests irreproducible. Better to put the packets in a fixed order. Or if you're going to write a random test, use many repetitions, and in any failure message print enough info for a user to reconstruct the failing test.
48 ↗	(On Diff #48624)	No magic numbers, please.
tests/sys/netinet6/ipv6/utils.subr
21 ↗	(On Diff #48624)	misaligned whitespace
59 ↗	(On Diff #48624)	You can assume that Kyua will always start you out with a blank directory.
60 ↗	(On Diff #48624)	`touch` is unnecessary. The following line will create the file.
69 ↗	(On Diff #48624)	Why do you need to copy the test script to `$PWD`?
69 ↗	(On Diff #48624)	Up above you used `python2.7` shebangs, but here you use `python2`. Can you please make it consistent?
77 ↗	(On Diff #48624)	What's the meaning of these magic numbers?

This revision now requires changes to proceed.Oct 1 2018, 5:08 PM

lwhsu added a subscriber: lwhsu.Oct 4 2018, 9:06 PM

Thank you for your feedback,

I don't think the tests from OpenBSD are of a good quality and I see them more as a starting point to evolve from there. I don't think we will import from them again. So I don't know weather putting them into the vendordirectory is necessary.

Everything else I have addressed / commented on inline.

tests/sys/netinet6/ipv6/utils.subr
69 ↗	(On Diff #48624)	I somehow need to get the IP Adresses of my local IF and the remote jail into the python script. To do this I generate a file called addr.py which I later include in the python script. Unfortunately the python import system is rather unwieldy and the easiest is to have the two files lying next to each other. I've also copied this approach from tests/sys/netpfil
77 ↗	(On Diff #48624)	These are local IP addresses I assign to the local IF and my jail I test against. tests/sys/netpfil does the same.

julius.flohr_uni-due.de updated this revision to Diff 49985.Nov 4 2018, 4:43 AM

kp added a subscriber: kp.Dec 1 2018, 5:24 PM

kp added inline comments.

tests/sys/netinet6/ipv6/Makefile
6 ↗	(On Diff #48624)	The pf tests do 'atf_set require.progs scapy', which means you don't need to explicitly depend on python (as scapy depends on it). It's already installed in the test VM in Jenkins, so that's a nice bonus.

asomers added inline comments.Dec 5 2018, 4:23 PM

tests/sys/netinet6/frag6/frag6.py
28	sleeps in tests are nasty. Can you eliminate it?
tests/sys/netinet6/utils.subr
24	Every single test is trying to create a jail with the same name. That means that no two of these tests can run together. So you'll have to put `TEST_METADATA+= is_exclusive=true` in the Makefile. Or better yet, you can embed the test case's name into the jail's name. That would be good anyway, in case something outside of this test suite creates "jail1".
73	This `BASE_FOLDER` argument seems to be unused. Why not eliminate it?