Add test cases for ping with IP options in the response
MFC after: 1 week
Details
Details
ATF tests included
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Errors Severity Location Code Message Error sbin/ping/tests/injection.py:11 E402 PEP8 E402 Error sbin/ping/tests/injection.py:12 E402 PEP8 E402 Error sbin/ping/tests/injection.py:13 E402 PEP8 E402 Error sbin/ping/tests/injection.py:14 E402 PEP8 E402 - Unit
No Test Coverage - Build Status
Buildable 48572 Build 45458: arc lint + arc unit
Event Timeline
| sbin/ping/tests/Makefile | ||
|---|---|---|
| 16 | Missing file? | |
Comment Actions
Although fearing my suggestions could make the tests more brittle, I'm still interested to know what do you think?
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 163 | I wonder if the output should also be checked: | |
| 176 | Same here: | |
| 191 | Even here: | |
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 176 | Without this patch, the 40 01s from injection.py will show. It replies: PING 192.0.2.15 (192.0.2.15): 56 data bytes 132 bytes from 192.0.2.15: Destination Host Unreachable Vr HL TOS Len ID Flg off TTL Pro cks Src Dst 4 f 00 007c 0001 0 0000 40 01 d84e 192.0.2.15 192.0.2.14 01010101010101010101010101010101010101010101010101010101010101010101010101010101 --- 192.0.2.15 ping statistics --- 1 packets transmitted, 0 packets received, 100.0% packet loss After the patch,: PING 192.0.2.15 (192.0.2.15): 56 data bytes 132 bytes from 192.0.2.15: Destination Host Unreachable Vr HL TOS Len ID Flg off TTL Pro cks Src Dst 4 f 00 007c 0001 0 0000 40 01 d84e 192.0.2.15 192.0.2.14 16110000ee8b2005171100000301fcfe000000004f00007c000100004001d84ec000020fc000020e --- 192.0.2.15 ping statistics --- 1 packets transmitted, 0 packets received, 100.0% packet loss Destination Host Unreachable would match either way... But I guess what matters most is the exit code. | |
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 176 | This is where I think the test might get brittle. If one runs the test suite a number of times enough to find a collision with this pattern. But I do not understand fully the idea behind it. I would have guessed testing for inner packet too short or something more trivial. | |
Comment Actions
I may also have a slight suspicion you applied your diff to a different ping.c base. After all, the idea was just to add test cases.
Comment Actions
Yes I did. But I can't figure out how to make the ping.c changes go away from phabricator. Can we just pretend, for the sake of review, that they aren't there?
Comment Actions
😂 that changes everything!
But in all seriousness, we may have hit a bug? in the opts case.
Here is what I used for the pip case:
atf_check -s exit:2 \
-o match:"Destination Host Unreachable" \
-o match:"(01){40}" \
python3 $(atf_get_srcdir)/injection.py pipComment Actions
Yes, but the bug fix is being reviewed separately. I couldn't figure out how to add my test changes to that review, so I opened a second one. Let's ignore the bug fix here.
Comment Actions
Remembering the late Mike Muuss, I decided to take a stab at this problem while we wait for the cavalry to arrive.
The short version is revert d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52, and start from that.
Let me know if you are interested, and I can open a Draft Pull Request on GitHub.
Without knowing the full picture, here is the full version:
- A DROP commit that runs the tests, mandoc lint, etc.
- Add a few tests to avoid further regressions + a test that fails (skipped).
- Revert commit d9cacf605e2ac0f704e1ce76357cbfbe6cb63d52 + NO_WCAST_ALIGN= + remove skip on failed test.
- A few courtesy commits to fix style, old stuff, etc.
- A few minor bug fixes.
- A few suggestions/improvements (can all be dropped, some of them are really proposals).
- WIP Actual alignment fix (fix most of the alignment issues), I could use your help here! (eventually remove NO_WCAST_ALIGN=).
I plan on keep this going for ping6 (ping -6) as well, depending on your feedback.
It has been fun and while at it, I learned a few tricks I wasn't aware ping was able to do (in other OSs as well). Finally, it was a great excuse for me to learn Scapy, a fun tool to experiment with.
Thank you!
Comment Actions
One problem I noticed is that the tests can spuriously fail with some error messages from scapy:
markj@nuc> sudo kyua debug ping_test:inject_opts Executing command [ python3 /usr/tests/sbin/ping/injection.py opts ] Fail: stderr not empty --- /dev/null 2022-11-30 14:43:29.782039000 +0000 +++ /tmp/kyua.FOws6H/2/work/check.VgUqKM/stderr 2022-11-30 14:43:27.574257000 +0000 @@ -0,0 +1,5 @@ + +(injection.py:37468): dbind-WARNING **: 14:43:27.258: Couldn't connect to accessibility bus: Failed to connect to socket /tmp/kyua.y0IpE0/7/work/.cache/at-spi/bus_0: No such file or directory +WARNING: No IPv4 address found on bridge0 ! +WARNING: No IPv4 address found on epair0a ! +WARNING: more No IPv4 address found on epair0b ! Files left in work directory after failure: .cache, .config, tun.txt ping_test:inject_opts -> failed: atf-check failed; see the output of the test for details
Comment Actions
I see that. It worked when I ran it in a VM, but now that I run it on bare metal with unused ethernet ports I get these warnings. The warnings come from Scapy and are harmless, but Kyua isn't expecting them. I'll try to figure out how to suppress them.
Comment Actions
The sledgehammer approach would be to just add -e ignore to the atf_check parameters, but hopefully there's a better way than that.
Comment Actions
- Add missing file
- Match ping's stdout in the injection tests.
- Python style
- More specific output checking for inject_pip
- fixup:
| sbin/ping/tests/injection.py | ||
|---|---|---|
| 11 | I don't know how to suppress this PEP8 lint error. The imports _must_ be in this order, because Scapy prints those warning messages at import time. | |
| sbin/ping/tests/injection.py | ||
|---|---|---|
| 11 | I got rid of those, and pass pylint validation by just using: import scapy.all as sc Like the other Scapy tests from sys/netinet. | |
| sbin/ping/tests/injection.py | ||
|---|---|---|
| 11 | That doesn't work. I still get the warning. | |
Comment Actions
@markj @jlduran_gmail.com I fixed the network errors that markj found. A necessary side effect seems to be the pep-8 style warnings, and I don't see any way to suppress those. Are there any other problems here?
Comment Actions
The test inject_pip_body is currently passing, however, at the moment it should be failing.
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 176 | I believe the test should check for the 40 01s in the original IP header. At the moment is checking for the absence of 4 NOPs, I would guess this is wrong. | |
Comment Actions
The network errors are gone, but I still see
markj@nuc> sudo kyua debug ping_test:inject_pip Executing command [ python3 /usr/tests/sbin/ping/injection.py pip ] Fail: stderr not empty --- /dev/null 2022-12-19 15:56:11.362786000 +0000 +++ /tmp/kyua.Y6LlPJ/2/work/check.u8irfX/stderr 2022-12-19 15:56:07.798303000 +0000 @@ -0,0 +1,2 @@ + +(injection.py:7882): dbind-WARNING **: 15:56:07.798: Couldn't connect to accessibility bus: Failed to connect to socket /tmp/kyua.y0IpE0/7/work/.cache/at-spi/bus_0: No such file or directory Files left in work directory after failure: .cache, .config, tun.txt ping_test:inject_pip -> failed: atf-check failed; see the output of the test for details
Unsetting DISPLAY in the environment seems to fix the problem but that's a weird thing to have to do.
OTOH, there are other network tests that fail for me in the same way. I didn't notice since I usually run tests in a VM. So the problem shouldn't block this patch.
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 157 | Opening braces should be on their own line. | |
Comment Actions
I can't reproduce markj's problem about dbind, whether I run on a system with or without X . I'm going to guess that it's something about his particular environment. I'll fix the braces and commit.
| sbin/ping/tests/ping_test.sh | ||
|---|---|---|
| 176 | I think you were looking at an older version of the test? The latest one in the review does check for those 01 bytes. | |