Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Linux implementation and integration tests to support larger inner packets by allowing outer (encapsulated) fragmentation, and to preserve the TAP interface MTU semantics (so --mtu maps directly to the TAP MTU).
Changes:
- Set TAP MTU directly to the
--mtuvalue (no longer subtracting outer encapsulation overhead). - Disable IPv4 PMTU discovery on the raw socket to allow outer IPv4 fragmentation.
- Extend integration netns tests with IPv6 coverage and optional tcpdump capture + updated log handling.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
platform/linux/tap.c |
Changes TAP MTU calculation behavior to preserve the caller-provided MTU. |
platform/linux/socket.c |
Disables IPv4 PMTU discovery to allow outer fragmentation. |
tests/integration/setup_netns.sh |
Sets veth MTU and adds IPv6 addressing for outer/inner test coverage. |
tests/integration/run_netns_tests.sh |
Adds tcpdump capture, changes default MTU/log locations, and adds an IPv6 test pass. |
tests/integration/README.md |
Updates integration test documentation for the new behavior/logging/capture. |
.gitignore |
Ignores the repo-local tmp/ output directory and .DS_Store. |
Comments suppressed due to low confidence (1)
platform/linux/tap.c:40
- tap_open() now leaves the
domainparameter unused and still leaks the temporary AF_INET control socket used forSIOCSIFMTU(the fd returned bysocket()is never closed). This can cause fd leaks over time and adds new -Wall warnings after the domain-specific MTU logic was removed.
ifr.ifr_mtu = mtu;
if(ioctl(socket(AF_INET, SOCK_DGRAM, 0), SIOCSIFMTU, &ifr) == -1){
fprintf(stderr, "[ERROR]: Failed to SIOCSIFMTU: %s\n", strerror(errno));
close(*fd);
return -1;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+22
to
+24
| # capture outer EtherIP packets for the whole test run | ||
| sudo ip netns exec ns1 tcpdump -i veth1 -s 0 -U -w "$LOGDIR/ns1-etherip.pcap" "$TCPDUMP_FILTER" > "$LOGDIR/tcpdump-ns1.log" 2>&1 & echo $! > "$LOGDIR/tcpdump-ns1.pid" | ||
| sudo ip netns exec ns2 tcpdump -i veth2 -s 0 -U -w "$LOGDIR/ns2-etherip.pcap" "$TCPDUMP_FILTER" > "$LOGDIR/tcpdump-ns2.log" 2>&1 & echo $! > "$LOGDIR/tcpdump-ns2.pid" |
Comment on lines
+12
to
+15
| cleanup() { | ||
| if [ -f "$LOGDIR/tcpdump-ns1.pid" ]; then sudo kill "$(cat "$LOGDIR/tcpdump-ns1.pid")" || true; fi | ||
| if [ -f "$LOGDIR/tcpdump-ns2.pid" ]; then sudo kill "$(cat "$LOGDIR/tcpdump-ns2.pid")" || true; fi | ||
| } |
| Files | ||
| - `tests/integration/setup_netns.sh` — create/destroy namespaces, veth pairs, and TAPs | ||
| - `tests/integration/run_netns_tests.sh` — start two `etherip` instances, run `ping` with DF, optional `iperf3`, then cleanup | ||
| - `tests/integration/run_netns_tests.sh` — start two `etherip` instances, run `ping` with DF, optional `iperf3`, save logs under `~/tmp`, capture EtherIP packets with `tcpdump`, then cleanup |
Comment on lines
28
to
+30
| - The ping test uses `ping -M do -s 1472` to send an inner IP packet of 1500 bytes (20B IP + 8B ICMP + 1472 payload = 1500). DF is set on the inner packet; fragmentation of the outer (encapsulated) packet is allowed. | ||
| - The ping test uses `ping -M do -s 1472` to send an inner IP packet of 1500 bytes (20B IP + 8B ICMP + 1472 payload = 1500). DF is set on the inner packet; fragmentation of the outer (encapsulated) packet is allowed. | ||
| - IPv6 test: The script also runs an IPv6 mode test after the IPv4 test. It uses `ping -6 -M do -s 1452` to produce a 1500-byte inner IPv6 packet (40B IPv6 header + 8B ICMPv6 + 1452 payload = 1500). |
Comment on lines
36
to
+38
| - Logs are stored under `/tmp/etherip-test-<pid>` and the script prints the path on completion. | ||
| - Logs are stored under `./tmp/etherip-test-<pid>` by default. Override with `ETHERIP_LOGBASE=/some/path`. | ||
| - The script saves packet captures as `ns1-etherip.pcap` and `ns2-etherip.pcap` in the log directory. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#5 で頂いたパッチを変更し、IPv6 PMTU Discoveryは有効に戻した。