refactor: extract duplicated wire-unit arithmetic across mh/ipv6_route/hopopt/ipv6_opts - #509
Merged
Merged
Conversation
…popt/ipv6_opts (#495) Per-class helpers for read-side length arithmetic that had been copy-pasted across many call sites -- the same pattern that caused #487 and #398. - ipv6_route.py: add ipv6_route_header_length() beside the existing ipv6_route_data_length(), replacing the 4 `header.length * 8 + 8` sites in _read_data_type_*. Finishes the read-side half of #487/#489. - mh.py: add MH._mh_message_length(), replacing the 25 identical `(header.length + 1) * 8` sites in _read_msg_*. Mirrors the write side already unified at make() (`(len(data_val) + 6) // 8 - 1`). - mh.py, hopopt.py, ipv6_opts.py: add one per-class option-length helper each (_mh_option_length, _hopopt_option_length, _ipv6_opts_option_length), replacing the 92/17/17 = 126 `<schema>.length + 2` / `<schema>.len + 2` sites in _read_opt_*. This is the exact +2/-2 mismatch #398 fixed six times independently. Deliberately left alone: the hopopt.py/ipv6_opts.py shared base class (cross-cutting, its own issue) and the TCP/IP reassembly pair, which must NOT be merged since RFC 791 and RFC 9293 mandate opposite overlap resolution. Pure refactor, no behaviour change: full protocols test suite (466 passed, 1260 subtests) and test_option_roundtrip_unit.py (6 passed, 358 subtests) are identical before and after, and construct-then-parse byte comparisons for MH/IPv6-Route/HOPOPT/IPv6-Opts match exactly pre- and post-change.
JarryShaw
commented
Sep 19, 2026
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.
Closes #495 — the last open item from the post-wave-1 consistency sweep. Pure refactor: 168 duplicated arithmetic sites replaced by five helpers, with no behaviour change.
Why this is not tidying
Two shipped defects came from exactly this duplication. #487 computed
Hdr Ext Lenin four places and got four different wrong answers. #398 (29dfd351b) was one conceptual+2/-2unit mismatch that had to be diagnosed once and then patched independently in six files —hopopt.py130 lines,ipv6_opts.py131,mh.py130, plus all three schema files at 46/46/47.What changed
ipv6_route.pytotal header octetsipv6_route_header_length(), beside the existingipv6_route_data_length()mh.pymessage lengthMH._mh_message_length()mh.pyoption lengthMH._mh_option_length()hopopt.pyoption lengthHOPOPT._hopopt_option_length()ipv6_opts.pyoption lengthIPv6_Opts._ipv6_opts_option_length()The
mh.pymessage-length case is the clearest: the write side was already unified atmh.py:1438while the read side stayed duplicated 25 ways, and that value feeds_decode_next_layer(mh, schema.next, length - mh.length)— the same roleHdr Ext Lenplayed in #487.ipv6_route_header_length()is defined as4 + ipv6_route_data_length(hdr_ext_len), which keeps the two related-but-distinct quantities visibly related:8 + 8ktotal header octets versus4 + 8kdata octets. Confusing those two was #487.Deliberately NOT done
The
hopopt.py/ipv6_opts.pyshared base class. Those two files are one implementation twice (406 changed lines out of ~1950, overwhelmingly renaming), and collapsing them would subsume a large part of this issue — but it is a cross-cutting change that deserves its own issue and its own review, so the option-length helpers here are per-class rather than one shared helper across the two files. That boundary was drawn on purpose.Also untouched, and already retired with reasons in #495:
pcap.py/pcapng.pynot scaling fragment offsets (they consume already-scaled values, unlike the third-party adapters, so the asymmetry is correct);pypcap.py/pcap_ct.pyraisingUnsupportedCallthroughout; HIP's singlelength=schema.len * 8 + 8call site, which has nothing to share with.The trap this change had to avoid
reassembly/tcp.pyandreassembly/ip.pylook like near-duplicates and must not be merged: RFC 791 lines 1892-1894 mandate last-write-wins for IP, RFC 9293 §3.10 mandates first-write-wins for TCP, and #443 settled that deliberately. Neither file is in this diff — 0 reassembly files, verifiable fromgit diff --name-only origin/main...HEAD.Verification
Helper arithmetic checked independently of the tests — 240 comparisons over
k= 0..39 against the original expressions, 0 mismatches:Every old pattern is gone, and the counts match #495's measurements exactly:
Byte-identity against
main, which is the only proof that matters for a refactor. Construct-then-parse across IPv6-Route source-route with 0/1/2/3 addresses, six MH message types, and the HOPOPT/IPv6-Opts Router-Alert option, dumped to JSON from both trees:Critically, 0 error-ish lines in that output — so this is a comparison of real results, not of two identical crashes. (A driver that dies the same way on both trees reports "identical" while proving nothing; that happened earlier in this programme and is worth guarding against explicitly.)
Tests on the directly affected paths:
test_option_roundtrip_unit.py+test_mh_unit.py+test_ipv6_extension_unit.py→ 98 passed, 860 subtests passed.EXPECTED_FAILURESinspected by importing the module (it cannot be grepped —**unpacking): 59 entries, none needing an update.One deliberate omission
No dedicated unit tests for the five helpers. That follows the house precedent set by #487/#489:
IPv6_Route._make_hdr_ext_lenandipv6_route_data_lengthhave no dedicated tests either and are exercised through the existing round-trip suite. Adding a new convention here rather than following the existing one seemed the wrong call for a refactor, but it is a judgement worth disagreeing with — the arithmetic check above is a script, not a committed test.