protocols: complete the Mobility Header registry -- all 24 message types, 70 of 71 options, all 4 CGA extensions - #437
Conversation
|
Reviewed at head What I ran
Wire-format spot checks against the actual RFC textVerified field order, widths, and what the Length field counts by fetching the RFC text directly and comparing to the code, for:
Finding postedOne minor, non-blocking finding posted inline on What I did not coverThis is a ~13,000-line diff; I did not check every one of the 70 options and 24 message types individually against their RFCs — I covered a deliberately wide spread (listed above) plus everything the PR body specifically flagged as risky, and had multiple independent passes converge on the same "no wire-format defects" conclusion for the traps/contradictions/nested registries. I did not independently re-verify the mypy (128 errors, 41 files) or pylint (78 errors/fatals, score 8.76→8.55) deltas — out of scope per the review brief, and the numbers are disclosed rather than hidden. I did not check the two large doc-only files ( |
|
Reviewed at head Setup: PR branch checked out in a dedicated worktree; baseline was a real The two wire-format "traps" the body calls out — both verified correct against the RFC text and against direct probes:
The two RFCs claimed to contradict themselves — both contradictions independently confirmed real, and the PR's resolution checked against IANA:
Spread of RFC-vs-code checks, beyond the two required traps:
Round-trip / registry completeness (script run against Registry-lookup safety: grepped for bare Enum placement: independently fetched RFC 6705 §14 (IANA Considerations) — it registers only the two message-type codes (17, 18) and the MAG IPv6 Address option (51), not the LRA Status values from §10.2. Fetched RFC 5949 §8 — it registers three option types and four flag/status-code tables (marked with asterisks as new IANA tables), but not the LMAA option's Option-Code values from §6.2.2. Both The two disclosed behavior items, both verified directly rather than taken on faith:
Suite and metric claims: ran the full suite on the PR tree ( Not independently re-verified: the 15-capture tree/json byte-identity claim against Handlers/sections checked directly against RFC text: RFC 5142 §4, RFC 5213 §8.8, RFC 5535 §5, RFC 5648 §4.3, RFC 5844 §3.3.4, RFC 5949 §6.2.2 + §8, RFC 6089 §4.1, §4.2, §4.2.1.4, §4.2.2, §8, RFC 6705 §10.2 + §14, RFC 7222 §4.1, §4.2.1, §4.2.10, RFC 7411 §5.3-§5.5, RFC 7563 §3.2. (RFC 6275 §6.2.2/§6.2.3 pad-option logic was also read for the padding-rules category, but it is pre-existing code unchanged by this diff, so it is out of scope here.) |
|
Addendum at Re-ran the full suite with
Nothing from |
|
Reviewed at Wire-format traps (the two the body says it handled specially), verified against the RFC text itself, not just the docstrings:
The two "RFC contradicts itself" claims, verified against the RFC text and against
Spread of handlers checked against their RFCs directly (fixed-length, variable-length, nested registry, reserved field, padding):
Other nested sub-option registries (flow identification, RFC 6089 §4.2.1, 6 sub-options incl. Pad/PadN/BID-Reference/Traffic-Selector/Flow-Binding-Action/Target-CoA; quality-of-service, RFC 7222, 11 attributes; LMA-controlled MAG parameters, RFC 8127 §3.1, 2 sub-options) — constructed every sub-option/attribute type in each registry and confirmed byte-identical round-trips, except the QoS Vendor-Specific Attribute payload issue posted inline. Completeness and registry policy:
The two disclosed defects:
Verification run: Not independently re-verified (disclosed by the PR body, and outside what this review prioritized): the mypy (128 errors/41 files) and pylint (78 errors/fatals, score 8.76→8.55) counts, and the full 15-file × tree/json (30-output) capture-extraction byte-identity claim beyond the sample-generation check above. Also not exhaustively checked: the remaining ~55 options and ~20 message types beyond the ones named above (Home Agent Switch, Heartbeat, Binding Revocation, Localized Routing Init/Ack, Update Notification/Ack, Flow Binding, Subscription Query/Response, and most of the PMIPv6/NEMO/DMM option blocks) — spot-checking a representative spread rather than all ~94 handlers was the deliberate scope given the size of this diff. |
Correction: the review comments on this PR reported a clean run without checking CIThree review passes ran on This PR is not ready to merge. Six failures, all in the new nested sub-option tests: Probable root cause, and it is worth recordingOn Python 3.10,
It hits these four registries specifically because Ruled out with evidence: a double import (distinct Not established: the precise trigger. What is being doneRouted to the agent that owns this module, together with the two open findings on this PR, for a single push: the root cause fixed and confirmed on a real 3.10 interpreter, the Everything the reviews verified against RFC text stands, and is worth keeping: the RFC 7411 word-length and RFC 5213 §8.8 timestamp traps, the RFC 6089 §8 and RFC 7563 §3.2 self-contradictions, the four nested registries round-tripping byte-identically on 3.14, and registry-read safety. This is a dispatch-mechanism failure that 3.14 happens to mask, not a wire-format error. |
|
Reviewed at head CI status (checked first, per the standing note that earlier passes on this PR missed this)
The isinstance→code-based dispatch fix (
|
… 51 options, 3 CGA extensions `MH` implemented the RFC 5568 fast-handover messages and the options they need; everything else in the registry fell through to a generic handler and parsed as opaque. This fills it in, in both directions. * **All 24 registered message data types** are now dispatched. New: Home Agent Switch [RFC 5142], Heartbeat [RFC 5847], Binding Revocation [RFC 5846], Localized Routing Initiation and Acknowledgment [RFC 6705], Update Notification and its Acknowledgement [RFC 7077], Flow Binding [RFC 7109], Subscription Query and Response [RFC 7161]. * **70 of the 71 registered options** are now dispatched, spanning the PMIPv6, NEMO, flow-binding, QoS, multicast and DMM blocks, with nested sub-option registries for the flow identification, access network identifier, quality-of-service and LMA-controlled MAG parameter families. * **All 4 CGA extensions**: the three RFC 4581 experimental types join Multi-Prefix. RFC 4581 gives their extension data no structure, so it is opaque by specification rather than undecoded. * Two spec-only value sets go in `mh.py` rather than `pcapkit.const.mh`, since IANA registers neither: `LocalizedRoutingStatus` [RFC 6705] and `LMAAddressCode` [RFC 5949]. No new IANA enumeration or vendor crawler was needed -- every sub-registry was already generated. Fixes two Multi-Prefix CGA extension construction defects found on the way: `_make_ext_multiprefix` declared `1 + len(prefixes) * 16` data octets for a payload of `4 + len(prefixes) * 8`, so two prefixes claimed 33 octets where 20 were emitted and a re-parse overran the extension; and it forwarded the data model's `prefixes` tuple to a `ListField`, which rejects tuples, so a parsed extension could not be re-made at all. Every new message type, option and extension round-trips byte-for-byte, and the 14 sample captures produce identical tree and json output. The CGA Parameters option (type 12) is left on the generic handler and documented: it cannot be parsed on `main` either, and both halves of the fault are in shared field machinery rather than in the mobility header. Suite: 889 passed, 17 skipped, 1064 subtests (was 859/17 with 844 subtests). mypy unchanged at 128 errors in 41 files; pylint errors and fatals unchanged at 78.
Fixes six Python 3.10 failures in the new nested sub-option handlers. One root
cause, not six.
Every schema class descends from `collections.abc.Mapping`, and on Python 3.10
and older they do **not** each get their own `_abc_impl` -- they share
`Schema`'s. A single `abc.ABCMeta` cache therefore serves the whole family, and
it is keyed only on the class being *tested*, not on the class it is tested
*against*, so one answer poisons every later question about that class:
>>> issubclass(ANIGeoLocationSuboption, Schema) # True, cached
True
>>> issubclass(ANIGeoLocationSuboption, ANINetworkIdentifierSuboption)
True # wrong -- the cached True for Schema is handed back
That produced both shapes of failure. A false positive sent a sub-option down a
sibling's branch (`AttributeError: ... has no attribute 'flags'`), and the
mirror case -- a correct `False` against a sibling, cached and then returned for
`Schema` -- made `ListField.pack`'s `isinstance(item, Schema)` false, so packing
a valid option raised `FieldValueError` and reconstruction emitted zeros where
payload belonged. Python 3.11+ gives each class its own cache, which is why this
was invisible on 3.14.
The eight nested helpers now dispatch on the sub-option type code, which is on
the wire, is what the registry keys on, is what every other dispatch in this
module already uses, and cannot be poisoned. The shared `_abc_impl` is
pre-existing and untouched here; no MH code depended on those checks before.
Also fixes a version-independent defect the round-trip tests could not see: the
four `_make_*_suboption` helpers took the data model as a parameter named `data`,
which shadowed the field of that name on the vendor-specific quality-of-service
attribute [RFC 7222 section 4.2.11]. A caller's `data=` bound to the parameter
instead of reaching `**kwargs`, so the payload was dropped with no exception and
the length written as though empty, while `vendor` and `subtype` arrived intact.
The parameter is now `option`, matching `_make_opt_*`. Covered by a new test on
the raw-kwargs construction path, which the round-trip tests cannot reach
because rebuilding from a parsed model takes the other branch.
And two missing citations: the Civic-Location and ANI Update-Timer sub-options
are RFC 7563 sections 3.1 and 3.3, as their MAG-Group-Identifier sibling already
recorded for section 3.2.
Suite on 3.10: 34 passed, 262 subtests in test_mh_unit (was 6 failed).
All 70 options, 24 message types and 4 extensions still round-trip
byte-identically on both 3.10 and 3.14.
… the wish list The Help Wanted page tracks feature requests. Three of the things this PR added to it are defects in code that already exists, which belong in the issue tracker where they can be triaged, not in prose on a wish list where they cannot. - The two faults blocking the CGA Parameters option are now #445 (a nested schema cannot reach the enclosing packet's fields by name) and #446 (a ForwardMatchField's non-consuming bytes count towards the schema's length). Both are shared field machinery, and both must be fixed for the option to parse -- which is why the half-fix was reverted. The pinning test stays. - The MN-ID constructor mis-sizing is now #448. Measuring it turned out worse than reported: it sizes from the Python type of `identifier` rather than from `subtype`, so the method's own default arguments emit a 19-octet option declaring length 3. - `http.HTTP`'s explicit `version=` path is now #447. That paragraph is edited to match what has landed on `main`, so the two do not conflict on merge; the UDP/TCP port asymmetry above it stays, being a decision rather than a bug. The opaque-payload item stays, because it is a genuine request, and now records the approach rather than only the situation: carry those payloads as `Raw` rather than bare `bytes`, dispatched through a per-payload registry in the style of `MH.__option__`, keyed on the field that already names the format. `Raw` is what an unregistered dispatch falls back to everywhere else, so registering a real dissector later needs no change at the option site. It deliberately does not use `_decode_next_layer`: that is only called at a layer boundary and appends to the frame's protocol chain, and an MLD record inside a mobility option did not follow MH on the wire, so it would make `layer=` and `protocol=` limits behave wrongly. Docs only -- no code, no tests, no behaviour change.
7a4a066 to
d306dca
Compare
…d the CGA extension carrier PR #440's round-trip harness enumerates the registries, so completing `MH.__option__` and `MH.__extension__` added 64 cases to it -- and seven of them were red. Two different reasons, and only one of them is a defect. Four are the harness constructing an option with no arguments at all, which for these four is not a well-formed instance of the option: * Service Selection: `Length` of 0 "is not allowed" and the identifier is 1-255 octets [RFC 5149 section 3]. * Redirect: "Both the 'K' and 'N' flags cannot be set or unset simultaneously" [RFC 6463 section 4.2], so with neither address given the option's own length is undetermined. * Access Network Identifier: "MUST contain at least one ANI sub-option" [RFC 6757 section 3]; it is a pure container. * LMA-Controlled MAG Parameters: likewise at least one sub-option [RFC 8127 section 3]. The constructors refuse all four correctly, so the validations stay and `_mh_option_overrides` gains the arguments that make the codes reachable -- which is what that table is already for, alongside the eleven MH options whose no-argument default is likewise invalid. This raises coverage rather than avoiding anything: all four now round-trip carrying real content, e.g. the ANI option emits `340f 010d 0004 77696669 06 001122334455`. The other three are `mh-extension/Exp_FFF*`, and they are #445 rather than mine. Measured: all four extension codes -- including `Multi_Prefix`, which the table already records -- fail identically with `PARSE / KeyError: 'length'`, raised from `SchemaField.unpack` at `corekit/fields/misc.py:619` by way of `CGAParameter.extensions`, before any extension schema is unpacked at all. A CGA extension has no carrier but the CGA Parameters option, so the whole registry is unreachable until #445 and #446 land. Three `EXPECTED_FAILURES` entries record that, grouped with the existing one and naming the same site; that entry's `file:line` is also refreshed, since this branch moved the lambda from :516 to :873. `pep.rst` claimed all four CGA extensions round-trip byte-for-byte. Their handlers do, when driven directly, but nothing can reach them through the public API, so the claim is narrowed to what is actually true and points at the recorded gap instead. Suite on 3.14: 973 passed, 17 skipped, 1544 subtests. On 3.10: 899 passed, 91 skipped, 1446 subtests. Baseline e2d8ed6 on 3.14: 942 passed, 1256 subtests. The option harness is 7 passed / 363 subtests on both interpreters, and `make_samples.py` regenerates all 24 captures byte-identically.
|
Pushed Four were the harness building an option with no argumentsCompleting
So the four validations are correct and stay, and Three were #445, not this PR
A CGA extension has no carrier but the CGA Parameters option, so the whole One doc correction
VerificationEvery run with
Zero failures on either interpreter. The option harness is 7 passed / 363 subtests on both — identical counts, so nothing here is interpreter-conditional. Two notes on the baseline numbers, since they should not be taken at face value: the 3.14 baseline's extra 18 skips are
|
|
Standing in for Copilot (out of tokens). Reviewed Scope confirmation
CI
The four validation/override cases -- verified against RFC text, not just readFetched the actual RFC text for all four citations:
Checked the actual validating code on this head ( Confirmed the override mechanism is pre-existing (not invented here): The three
|
- main merged #437 (MH registry completion, including the four mh-extension codes and the _make_ext_multiprefix arithmetic fix) and #456/#446 (the ForwardMatchField double-count in Schema.__len__) since this branch's last merge. Combined with this PR's own fix, all four mh-extension/{Multi_Prefix,Exp_FFFD,Exp_FFFE,Exp_FFFF} cases now round-trip cleanly -- verified directly against the round-trip harness (all four return 'OK'), not assumed from the PR descriptions. Deleted their EXPECTED_FAILURES entries; a stale PARSE/KeyError expectation would otherwise have failed this module outright, per its own two-way assertion. - The issue's own 40-octet CGA Parameters reproduction now parses completely end to end (confirmed directly: MH(raw, len(raw), extension=True) returns a populated CGAParametersOption, no exception). Rewrote test_cga_parameters_option_reaches_the_446_boundary _not_a_keyerror, which asserted the (now stale) FieldValueError boundary, as test_cga_parameters_option_now_parses_end_to_end, asserting the parsed fields directly. - #437 had pinned the pre-fix KeyError as test_mh_cga_parameters_option_is_unparsable_upstream, explicitly so that "whoever fixes it finds out here" -- and it did: this run turned that test red once the merge above landed. Replaced it with test_mh_cga_parameters_option_now_parses, asserting the option parses and its fields are what the wire says, and fixed the now-stale cross-reference and claim in test_mh_pmipv6_options_round_trip_byte_for_byte's docstring (CGA_Parameters is still excluded from that test's cases, but no longer because it cannot be parsed -- that is now a separate, deliberate scope decision for whoever adds its full round-trip identity). - Merged origin/main (0283a6d) with one conflict, in this exact region of tests/protocols/test_option_roundtrip_unit.py, resolved by re-deriving the correct entries from the actual post-merge behaviour rather than picking either side. Verified: mypy pcapkit -> 123 errors/40 files (a fresh main, 0283a6d, is 124 -- unchanged from before this merge). Round-trip harness: 7 passed, 363 subtests passed, 0 failed (up from 299 subtests before #437 grew the mh-extension family to four codes). tests/protocols/ internet/test_mh_unit.py: 35 passed, 266 subtests passed, 0 failed. Full local suite result to follow in the PR description.
1 participant
Closes the Mobility Header item on the Help Wanted page, which
docs/source/pep.rstcalled "the section of this page with the most work left in it".What now dispatches
Verified against a
mainbaseline by reading the tables directly:MH.__message__14 → 24,MH.__option__20 → 71,MH.__extension__1 → 4.All 24 registered message data types. New: Home Agent Switch (:rfc:
5142§4), Heartbeat (:rfc:5847§3.3), Binding Revocation (:rfc:5846§5.1/§5.2), Localized Routing Initiation and Acknowledgment (:rfc:6705§10.1/§10.2), Update Notification and Acknowledgement (:rfc:7077§4.1/§4.2), Flow Binding (:rfc:7109§6.1.1/§6.1.2), Subscription Query and Response (:rfc:7161§4.3.1.2/§4.3.2.2).70 of 71 options — 17, 19, 20, 22–33, 35–70 — across :rfc:
5026, :rfc:5094, :rfc:5149, :rfc:5213, :rfc:5555, :rfc:5648, :rfc:5844, :rfc:5845, :rfc:5847, :rfc:5949, :rfc:6058, :rfc:6089, :rfc:6463, :rfc:6602, :rfc:6705, :rfc:6757, :rfc:6909, :rfc:7028, :rfc:7109, :rfc:7148, :rfc:7161, :rfc:7222, :rfc:7389, :rfc:7411, :rfc:8127, :rfc:8278and :rfc:8885. That includes nested sub-option registries for flow identification (6), access network identifier (6), quality-of-service (11 attributes) and LMA-controlled MAG parameters (2). Every entry cites its RFC and section in the docstring, with the RFC's own diagram.All 4 CGA extensions; the three :rfc:
4581§3 experimental types share one handler.Read and construction are symmetric throughout, so each of those is a
_read_and a_make_handler — the construction half is where this repository's defects cluster, so it was not left for later.One option deliberately left on the generic handler
CGA Parameters (type 12) cannot be parsed on
maineither, and this PR does not pretend otherwise: a well-formed 40-octet option raisesKeyError: 'length'on both trees.CGAParameter.extensionssizes itself frompkt['length'], butCGAParameterhas nolengthfield andSchemaFieldhands a nested schema a fresh packet dict. Making that lookup optional runs straight into a second fault in howForwardMatchFieldcounts toward a nested schema's length (FieldValueError) — shared field machinery, so the half-fix was reverted rather than shipped as a change of exception type. Pinned bytest_mh_cga_parameters_option_is_unparsable_upstreamso the day it starts working is visible.Registries
No new enum values and no vendor-crawler changes were needed — every sub-registry was already generated. Two spec-only value sets went into
mh.pyper the house policy, both confirmed absent from IANA:LocalizedRoutingStatus(:rfc:6705§10.2) andLMAAddressCode(:rfc:5949§6.2.2).Defects and specification traps found
Fixed, in the area being completed:
_make_ext_multiprefixdeclared1 + len(prefixes) * 16data octets for a4 + len(prefixes) * 8payload — 33 declared against 20 emitted for two prefixes — and forwarded the data model'sprefixestuple to aListField, which raisesProtocolUnbound. So a parsed Multi-Prefix extension could never be re-made.Reported, not fixed:
_make_opt_mn_idmeasureslen(identifier)even for theIPv6_Addresssubtype, so a string identifier declares 11 octets while the schema emits 16. Pre-existing; passing anIPv6Addressis correct.Two wire-format traps now documented at the handlers: :rfc:
7411's multicast options count length in 32-bit words excluding four octets, and :rfc:5213§8.8's timestamp is not an :rfc:1305NTP timestamp — 1970 epoch with a 48/16 split — so it gets its ownPMIPv6Timestamp.Two RFCs contradict themselves, noted in docstrings rather than silently resolved: :rfc:
6089§8 prints options 44 and 45 swapped relative to its own figures and to IANA, and :rfc:7563§3.2 disagrees with itself on the MAG-Group-Identifier width — diagram and Length field say two octets, prose says three.Verification
Suite 889 passed / 17 skipped / 1064 subtests (711 s), against a
3e696c7d1baseline of 859 / 35 / 844. The skip delta is fully accounted for: all 18 extras aretest_tier_guard.pyskipping because the baseline was agit archivewith no.git; nothing is newly skipped. +12 collected tests, all intest_mh_unit.py(22 → 34 methods).Round-trip byte-identical for all 70 options, all 24 message types and all 4 extensions — construct → parse → construct — with no parse falling through to the generic handler.
Captures: 15 files ×
treeandjsonwithip=True, tcp=True, reassembly=True, all 30 outputs byte-identical to baseline.make_samples.pyregenerates all 19 files byte-identically. mypy exactly unchanged at 128 errors in 41 files with identical per-file counts. pylint errors and fatals exactly unchanged at 78; the score moves 8.76 → 8.55 entirely in convention and warning classes the module already emits by design — long union annotations,**kwargsand TYPE_CHECKING-stub unused arguments, and thetypeparameter name that is this module's established_make_opt_*convention.docs/source/pep.rstis rewritten so its counts are accurate rather than stale, and it now spells out the CGA Parameters defect, the opaque MLD/IGMP and :rfc:6088payloads, and the MN-ID defect. The MH docs page and the class's three dispatch tables are generated from the registries themselves, so they cannot drift.