Skip to content

protocols: migrate IPv4 option and HIP parameter dispatch to the registry pattern (#429) - #434

Merged
JarryShaw merged 3 commits into
mainfrom
refactor/ipv4-hip-option-registry
Sep 17, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
refactor/ipv4-hip-option-registry

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Closes #429.

IPv4 and HIP were the last two of sixteen dispatch families still building a handler name from code.name.lower() and reaching for it with getattr. Both now carry an explicit class-level defaultdict read through ProtocolBase._lookup_registry, following HOPOPT/TCP/SCTP exactly — same declaration comment style, same lambda: '<fallback>', same isinstance(name, str) branch, same warn on re-registration.

  • pcapkit/protocols/internet/ipv4.py: __option__ at :196 (14 entries, fallback 'unassigned'), register_option at :481, reads at :590, :1234, :1265.
  • pcapkit/protocols/internet/hip.py: __parameter__ at :377 (49 entries, fallback 'unassigned', named to match SCTP.__parameter__), register_parameter at :648, reads at :745, :2890, :2907.
  • Both _make_* branches on each side — the list-of-tuples one and the OrderedMultiDict one — not just the parse side. That asymmetry is how Option, chunk and block registries leak on lookup miss, the same way __proto__ did #425 came to under-count its own scope.
  • HIP_Parameter.R1_Counter (128, v1) and R1_COUNTER (129) are now keyed separately; code.name.lower() had collapsed them onto one handler.
  • Every read goes through _lookup_registry, so a miss cannot insert: grep finds exactly two subscriptions in the two files, both the register_* writes.
  • docs/source/ext.rst's extensibility table collapses IPv4's and HIP's two-row _read_opt_${name} / _make_opt_${name} form into the uniform __registry__ row, so all 18 rows are the same shape. pcapkit/foundation/registry/protocols.py:361 also referenced pcapkit.protocols.internet.internet.IPv4, a module that does not exist; it now points at the registry.

One behaviour change, and it is the reason to read this PR carefully

The string form is bit-for-bit unchanged, including the RegistryError on an unknown handler name. The tuple form changes calling convention, and the change is from broken to working.

On main, setattr installed the pair as class attributes, so dispatch bound them as methods and passed self as the first positional argument. A pair written to the declared OptionParser / OptionConstructor signature could therefore never be called at all. Measured, registering a parser written exactly to Callable[[Schema_Option, NamedArg(Option, 'options')], Data_Option]:

origin/main : TypeError: my_parser() takes 1 positional argument but 2 positional
              arguments (and 1 keyword-only argument) were given
this branch : OK, parser invoked once with the schema

So a handler pair written as documented works now and did not before. The inverse is the breaking case: a pair written with an explicit leading self — the only shape that could work on main — will now break. Narrower second change: naming a tuple-registered handler by string afterwards used to succeed, because setattr had created the attribute, and now raises RegistryError. Every other registry-form family already behaved that way.

No new public register function. IPv4.register_option, HIP.register_parameter, register_ipv4_option and register_hip_parameter keep their signatures. What is new is inspectable surface — IPv4.__option__ and HIP.__parameter__ as documented class attributes, which is the issue's inspectability ask; len(IPv4.__option__) is 14 and len(HIP.__parameter__) is 49.

Evidence

The captures do not exercise this code, and saying otherwise would be the easy mistake here. Instrumenting _read_ipv4_options and _read_hip_param across all 15 captures records 0 IPv4 options and 0 HIP parameters on the wire — every IPv4 header is ihl=5 and there is no HIP at all. So the byte-identical comparison proves no collateral regression and nothing about dispatch.

Dispatch was therefore traced directly: every handler replaced by a self-recording stub, over all 30 IPv4 option numbers and all 62 HIP parameter codes plus 13 synthesised off-enum codes, in both directions and both constructor branches — 315 dispatch decisions, byte-identical between trees, all reached without error. Separately, resolving each of the 92 enum members through the registry lands on the same function object the code.name.lower() path did, 0 mismatches, and _lookup_registry inserts nothing on a miss.

Alongside that: make_samples.py regenerates all captures identically; 15 captures × tree and json with ip=True, tcp=True, reassembly=True gives 30 files, 17 MB, diff -rq clean; mypy 0 errors on the three changed modules; sphinx-build exits 0 with 0 unresolved references and both new attributes rendering; isort --check-only clean.

Suite: 877 passed / 17 skipped / 852 subtests on origin/main against 883 / 17 / 919 here — the delta is exactly the 6 new tests and their 67 subtests. One measurement trap worth recording: a first baseline run read 859/35, because git archive produces a directory that is not a git checkout and tests/test_tier_guard.py skips all 18 of its tests there; git init in the baseline moves all 18 to passed.

Known defects carried across unchanged, and three found on the way

Carried across deliberately, so this stays a refactor: IPv4._make_opt_ts passes data= where the schema field is ts_data (now ipv4.py:1523, was :1488), still emitting UnknownFieldWarning. And HIP._make_param_encrypted passes cipher=cipher_id where EncryptedParameter has no cipher field (now hip.py:3519) — the brief had cited :3445.

Found while working, all left alone as schema and behaviour changes rather than dispatch:

  • The Quick-Start option is genuinely broken, and worse than previously suspected. quick_start_data_selector returns SchemaField(length=5, …) at pcapkit/protocols/schema/internet/ipv4.py:128 while QuickStartRequestOption declares 8 octets. Measured identically on both trees: _make_opt_qs emits a correct 8-octet option 19 08 00 40 44 88 cd 10, and reparsing consumes 5, resynchronises on the nonce's second byte 0x88 as if it were option 136, and dies ProtocolError: IPv4: [OptNo 136] invalid format, with SchemaWarning: packet length < 0: -3. QuickStartReportOption separately emits 7 octets while writing length=8. And _make_opt_qs computes rate_val = floor(log2(rate*1000/40000)), which goes negative below 40 kbps and dies with a bare ValueError: invalid literal for int() with base 2: b'0000-011' rather than an in-library error.
  • IPv4._make_opt_unassigned declares data: 'bytes' keyword-only with no default (ipv4.py:1291) where every sibling fallback constructor defaults it to b'', so constructing an option with no dedicated constructor raises TypeError: missing 1 required keyword-only argument: 'data' from both branches — including the OrderedMultiDict branch, where the payload is sitting in option.data. Pre-existing and identical on both trees; the leak test asserts the TypeError with a comment saying why.
  • This migration hides a pylint finding for the item above. origin/main reports ipv4.py:1238: E1125: Missing mandatory keyword argument 'data' in method call; here it disappears, because the registry lookup widens the inferred type past static resolution. The runtime defect is unchanged — proved above — but the static warning that flagged it is gone, which a reviewer should know. That is the only pylint message that differs; the other change is W0404 Reimport 'timedelta' shifting a line.

One thing deliberately not done: HIP's class docstring table still has only a "Parameter Parser" column where IPv4/HOPOPT/TCP have two. Adding 49 constructor rows is a docs change with no bearing on the migration.

#429)

IPv4 and HIP were the last two of sixteen dispatch families to build a
method name from the enum member and `getattr` it off the instance, with
`register_option` / `register_parameter` installing handlers as class
attributes via `setattr`. Both now carry an explicit class-level
`defaultdict` read through `ProtocolBase._lookup_registry`, as TCP, SCTP,
HOPOPT, IPv6-Opts, IPv6-Route, MH, HTTP/2 and PCAP-NG already do.

- `IPv4.__option__`, 14 entries, fallback `'unassigned'`; `HIP.__parameter__`,
  49 entries, fallback `'unassigned'`. `R1_Counter` (128) and `R1_COUNTER`
  (129) are keyed separately, since `code.name.lower()` collapsed them.
- Both parse and construct sides migrated, including both branches of
  `_make_ipv4_options` and `_make_hip_param`. All six read sites go through
  `_lookup_registry`, so an unrecognised code no longer needs a leak guard
  it never had.
- `register_option` / `register_parameter` warn on `code in cls.__option__`
  rather than on `hasattr(cls, f'_read_opt_{name}')`, which could not tell a
  user registration from a shipped handler.
- A registered `(parser, constructor)` pair is now called with the signature
  `OptionParser` / `OptionConstructor` declares. Under `setattr` the pair
  became a descriptor, so dispatch passed `self` as the first positional
  argument and a handler written to the declared signature could not be
  called at all.
- Docs: `__option__` / `__parameter__` documented on both pages, and the
  extensibility table in `ext.rst` collapses to the one uniform form.

Dispatch is unchanged for every code: traced over all 30 IPv4 option numbers
and 62 HIP parameter codes plus 13 synthesised off-enum codes, in both
directions and both constructor branches, the selected handler is identical
before and after. The 14 sample captures regenerate byte-identically and
produce byte-identical `tree` and `json` output. Full suite 883 passed,
17 skipped; mypy clean on the changed files; pylint no worse.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Reviewed at head 73cf06f2f (branch refactor/ipv4-hip-option-registry). Standing in for GitHub Copilot on this one.

No findings. Every claim in the PR body is independently reproducible, and I could not make any of the described equivalences fail. What I actually ran, all with PYTHONSAFEPATH=1 and pcapkit.__file__ printed to confirm which tree ran, comparing this branch against a fresh git clone of origin/main (not git archive, to avoid the test_tier_guard skip-count trap the body warns about):

  • Registry contents match exactly. IPv4.__option__ has the same 14 keys, HIP.__parameter__ the same 49 keys (including both R1_Counter and R1_COUNTER mapping to 'r1_counter'), as the set of codes for which main had a real _read_opt_*/_read_param_* method reachable via code.name.lower().
  • Dispatch equivalence, all 92 real enum members (30 IPv4 + 62 HIP) in both directions. Resolved every OptionNumber/Parameter member through main's getattr(cls, f'_read_..._{code.name.lower()}', ...) and through this branch's ProtocolBase._lookup_registry(...), for read and make. Diffed the two code→handler-name listings: identical apart from cosmetic header lines. len(IPv4.__option__)/len(HIP.__parameter__) unchanged after resolving all 92 members plus off-registry codes, confirming a miss never inserts.
  • The tuple-form behavior change is exactly as described. Registered a handler pair written to the documented signature (def my_parser(schema, *, options), no leading self): on the pristine origin/main clone, calling it the way setattr-installed dispatch would raises TypeError: my_parser() takes 1 positional argument but 2 positional arguments (and 1 keyword-only argument) were given — the exact string quoted in the PR body. On this branch, the same call returns cleanly. And naming a tuple-registered handler by string afterward: succeeds on main (register_ipv4_option(other_code, 'unassigned_30') — matching the earlier code's lowered name — succeeds because setattr had created that attribute), and raises RegistryError: method must be a valid IPv4 option parser function on this branch.
  • Re-registration warning still fires, matching HOPOPT's identical if code in cls.__option__: warn(...) pattern — checked directly with mock-free warnings.catch_warnings on both IPv4.register_option and HIP.register_parameter.
  • The "captures don't exercise this" claim holds. Regenerated all 15 sample captures (examples/generators/make_samples.py) on both trees and instrumented IPv4._read_ipv4_options / HIP._read_hip_param: 0 calls across all 15 captures on this branch, confirming every IPv4 header is ihl=5 and there's no HIP traffic, exactly as claimed.
  • Static analysis. mypy (project mypy.ini) reports 0 errors on all three changed modules on this branch. pylint --enable=E1125,W0404 on ipv4.py reproduces the claimed diff exactly: main reports E1125 at ipv4.py:1238 (missing-kwoa on _make_opt_unassigned's pre-existing data bug), and it disappears on this branch — confirming the registry lookup widens the inferred type as described, with the underlying runtime defect unchanged.
  • Full suite, both trees, from each tree's own root (pytest tests): this branch — 883 passed / 17 skipped / 919 subtests in 9:55. origin/main (fresh clone) — 877 passed / 17 skipped / 852 subtests in 9:26. Delta is exactly 6 tests / 67 subtests, matching the body's claim precisely.
  • No stray dispatch sites. grep confirms every read of __option__/__parameter__ in ipv4.py/hip.py goes through _lookup_registry (the only bare-subscript accesses are the non-mutating code in cls.__option__ membership checks in register_option/register_parameter), and no other file in the tree references the old _read_opt_${name}/_read_param_${name} setattr-created attributes for these two classes.
  • Spot-checked the disclosed pre-existing defects left alone by this migration (ipv4.py:1523 data= vs ts_data, ipv4.py:1291 keyword-only data with no default, hip.py:3519 cipher= on EncryptedParameter) — all present at exactly the cited lines, unrelated to dispatch, correctly left out of scope.

Nothing here rises to a blocking concern, and I did not find a claim in the body that the diff fails to support. Good to merge, from my read.

@JarryShaw
JarryShaw merged commit c28ffc2 into main Sep 17, 2026
24 checks passed
JarryShaw added a commit that referenced this pull request Sep 17, 2026
…erpreter, not the code

Three things, all consequences of #432, #434 and #439 landing under the branch.

- `INTERPRETER_GAPS`: seven PCAP-NG name-resolution cases round-trip from Python
  3.11 on and fail to construct on 3.10, so one recorded outcome per case no
  longer suffices. The root cause is #439 in the *schema* hierarchy:
  `NameResolutionBlock.post_process` asks
  `isinstance(record, (IPv4Record, IPv6Record))` at
  `pcapkit/protocols/schema/misc/pcapng.py:1248`, every `Schema` subclass shares
  one `_abc_impl` on <= 3.10 (measured: `EndRecord._abc_impl is
  IPv4Record._abc_impl` is True on 3.10.20, False on 3.14.7), and the block's
  terminating `EndRecord` therefore tests True and has `.names` read off it.
  Measured in the real path, not inferred. The `Info` data models are unaffected
  on both interpreters, and that boundary is asserted too.
  The table overrides rather than sits beside `EXPECTED_FAILURES`, because the
  three `ns_dns*` options fail on every interpreter but for different reasons.
- `test_schema_isinstance_is_interpreter_dependent` pins that mechanism, so the
  seven are excused by evidence about a named library bug rather than by a
  version comparison. On >= 3.11 they are still held to `OK`; fixing #439 turns
  3.10 red.
- The two SMF_DPD entries: `hopopt-option/SMF_DPD` now round-trips and its entry
  is deleted, since #429's over-read fix landed with #432's progress guard.
  `ipv6-opts-option/SMF_DPD` raises instead of hanging and is re-recorded as
  `PARSE`. The two schema modules are line-for-line duplicates, so that is one
  fix applied once where it was needed twice. The sweep keeps its deadline: it
  guards the next non-progress defect, not this one.

Also: #434 gave IPv4 and HIP real registries, so the generator now reads
`HIP.__parameter__` instead of falling back to enum-crossed-with-handler.

No library file is touched.

3.14.7: 258 cases, 173 round-trip. 3.10.20: 258 cases, 169 round-trip -- the
difference is exactly the four cases in `INTERPRETER_GAPS` that pass on 3.14.
@JarryShaw
JarryShaw deleted the refactor/ipv4-hip-option-registry branch September 18, 2026 20:22
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) labels Sep 22, 2026
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Migrate IPv4 and HIP option dispatch from setattr/getattr to the registry pattern

1 participant