Page MenuHomeFreeBSD

IPv6 Fragmentation Regression Tests from OpenBSD
Needs ReviewPublic

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
Unit Tests Skipped

Event Timeline

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

deleted accidentally committed build artifact.

asomers requested changes to this revision.Oct 1 2018, 5:08 PM

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

  1. 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.
  2. Other concerns inline.
  3. I haven't fully reviewed the Python code. I'll do that after you fix the sh and make problems.
  4. 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
750

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

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.

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.

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?