Skip to content

refactor(link): dispatch LinuxSLL's non-EtherType ARPHRD values through its own registry (#2510) - #2527

Merged
JarryShaw merged 3 commits into
mainfrom
refactor/2510-linux-sll-arphrd-registry
Nov 1, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
refactor/2510-linux-sll-arphrd-registry

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Nov 1, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else
  • const — regenerates constant or registry tables, members keeping their numeric values

Description of your pull request and other information

Closes #2510.

  • LinuxSLL owns a __proto__ keyed by ARPHRD, listed in enum-value order: FRAD, IPGRE/IP6GRE → GRE, IEEE80211_RADIOTAP → Radiotap (feat(link): add a radiotap dissector for IEEE80211_RADIOTAP on LinuxSLL #2509), NETLINK. It declares __proto_key__ = (ARPHRD, False). The _ARPHRD_NOT_ETHERTYPE branch chain is gone; it never shipped.
  • __index__ now returns FRAD → ARPHRD.FRAD, GRE → ARPHRD.IPGRE (both keys registered) and Netlink → ARPHRD.NETLINK.
  • LinuxSLL, FRAD and Netlink each get their own register override, and register_arphrd, register_frad and register_netlink join the link-layer section of foundation.registry.protocols.
  • Dispatch is unchanged: a probe of 7 arphrd cases × 3 stop settings gives byte-identical output before and after. The new tests fail on main.

Open questions are below; the Sonnet review is in flight.

@JarryShaw JarryShaw added refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) needs: decision Waiting on the maintainer to decide — not blocked by other work review: running A cross-review is in flight against the current head - no verdict yet labels Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Decisions needed (the conventions do not settle these):

  1. Stop handling. On main, a parse told to stop at LinuxSLL still dispatches FRAD/GRE/Netlink (protocol='LinuxSLL2' on a FRAD frame gives LINUX_SLL2:FRAD:IPv4:UDP). This PR keeps that parity. Options: keep it, or honour the stop in its own issue. Recommendation: keep it here and file the stop fix separately.
  2. EtherType path. The ruling said to hand EtherTypes to super()._import_next_layer. Once LinuxSLL owns __proto__, though, super() would look EtherTypes up in the ARPHRD map, so this path names Link.__proto__ directly, as FRAD does for SNAP. One side effect: LinuxSLL.register now registers ARPHRD keys, where it used to register EtherTypes through Link. Options: accept, or restructure. Recommendation: accept, and label this breaking if that register change counts as public contract.
  3. GRE.__index__ returns ARPHRD.IPGRE, the lower of its two keys. Recommendation: keep.
  4. Wrapper naming. register_arphrd takes its name from the key enum, as register_ethertype does, while register_frad/register_netlink take the owner's name, as register_tcp does, because their keys have no enum. Recommendation: write that rule into refactor(registry): every class-level registry map gets its foundation.registry wrapper #2511's convention section.
  5. The bare-int key check (_check_number, which Loopback in feat(registry): foundation.registry wrappers for IPX and SCTP sub-maps; record the convention (#2511) #2521 needs as well). Options: (a) keep it local; (b) fold it into _check_code as kind=None once feat(registry): foundation.registry wrappers for IPX and SCTP sub-maps; record the convention (#2511) #2521 lands. Recommendation: (b).
  6. ARPHRD.__proto_dest__, which would let code=ARPHRD.X infer LinuxSLL. Recommendation: a follow-up issue, since it regenerates a const module.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review (Sonnet) at 30aabb248: NEEDS CHANGES, for wording only. Code, dispatch and registry behaviour all held up. 13,824 combinations matched origin/main byte for byte: LinuxSLL1/2, every ARPHRD value plus extras, 8 protocol_type values, 8 payload shapes and 9 stop settings. 540 round trips also match, and Link.__proto__, FRAD.__proto__ and the name registry are unchanged. Without the fix, 13 of the new module's 17 tests fail.

Required:

  • L2TPv2.__index__'s docstring claims every non-raising __index__ returns a TransType, EtherType or LinkType, and this PR makes that false.
  • LinuxSLL.register's docstring narrates the change ("no longer has its protocol_type read as an EtherType").

Recommended: drop the count in GRE.__index__'s docstring. The fix round is running; it is pushed once queued runs drop below 20.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

drop the count.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Owner: drop the count in GRE.__index__'s docstring. That is already in this PR's fix round, alongside the two required wording fixes. Q1 to Q6 in the decision comment above (stop handling, the EtherType path, GRE.__index__, wrapper naming, the bare-int check, ARPHRD.__proto_dest__) are still open, so needs: decision stays.

@JarryShaw

Copy link
Copy Markdown
Owner Author

take all your recommendations on Q1-6

@JarryShaw

Copy link
Copy Markdown
Owner Author

Owner took all six recommendations, paraphrased and reconciled:

  1. Stop handling keeps main's behaviour here; the fix is fix(link): a parse told to stop at LinuxSLL still dispatches FRAD, GRE and Netlink #2532.
  2. The EtherType path naming Link.__proto__ is accepted. LinuxSLL.register now takes ARPHRD keys and refuses EtherTypes, and LinuxSLL shipped in 1.5.1, so this PR is now labelled breaking.
  3. GRE.__index__ stays ARPHRD.IPGRE.
  4. The wrapper naming rule (named after the key enum, or after the owning protocol for bare-int keys) goes into registry-protocol.rst via feat(registry): foundation.registry wrappers for IPX and SCTP sub-maps; record the convention (#2511) #2521's fix round.
  5. _check_number folds into _check_code(kind=None) once feat(registry): foundation.registry wrappers for IPX and SCTP sub-maps; record the convention (#2511) #2521 lands with that helper. That becomes a follow-up commit here.
  6. ARPHRD.__proto_dest__ is feat(const): ARPHRD declares __proto_dest__ so code=ARPHRD.X infers LinuxSLL #2533.

The wording fix round requested by the review is still running; it is pushed once queued runs drop below 20.

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) and removed needs: decision Waiting on the maintainer to decide — not blocked by other work labels Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Fix round at e2b30f2e3, cross-reviewed (Sonnet): GOOD TO GO. It rewrites the docstrings the review flagged:

  • L2TPv2.__index__ lists ARPHRD among its return types;
  • LinuxSLL.register states current behaviour, not a version comparison;
  • GRE.__index__ names IP6GRE without counting keys.

Q5 (folding _check_number into _check_code(kind=None)) follows here once #2521 lands.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Nov 1, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Nov 1, 2026
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Coverage: 93.95% (unit tier, Python 3.15, e2b30f2e3, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 20087 144 2710 125 98.78%
pcapkit/corekit 2605 74 818 37 96.52%
pcapkit/dumpkit 281 3 100 3 98.43%
pcapkit/foundation 3232 27 1128 31 98.67%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 19935 213 5244 207 98.32%
pcapkit/toolkit 924 36 334 10 95.55%
pcapkit/utilities 502 7 144 5 98.14%
pcapkit/vendor 5009 2222 1148 182 52.31%

Per-file detail: the coverage-html artifact of this run.

…gh its own registry (#2510)

- LinuxSLL owns __proto__ keyed by ARPHRD (FRAD, IPGRE, IEEE80211_RADIOTAP
  as Raw, IP6GRE, NETLINK) with __proto_key__ = (ARPHRD, False); it replaces
  _ARPHRD_NOT_ETHERTYPE and the branch chain, and every other arphrd_type
  still dispatches protocol_type through Link.__proto__.
- LinuxSLL, FRAD and Netlink override register for their own key.
- FRAD, GRE and Netlink __index__ return ARPHRD.FRAD, ARPHRD.IPGRE and
  ARPHRD.NETLINK.
- register_arphrd, register_frad and register_netlink wrappers, exported and
  documented beside register_ethertype; FRAD's and Netlink's take a bare int.

Tests: new test_linux_sll_arphrd_registry_unit; registrar sweeps, id/index
tests and the __proto_key__ sweep extended. Dispatch is unchanged for every
arphrd_type, including under a stop at this layer.
- L2TPv2.__index__: list ARPHRD among the types a non-raising __index__
  returns, as FRAD, GRE and Netlink return one.
- LinuxSLL.register: state the dispatch of a registered arphrd_type as
  current behaviour rather than relative to a previous version.
- GRE.__index__ and its unit-test docstring: name IP6GRE alongside IPGRE
  without counting the arphrd_type keys.
…de (#2510)

- register_frad and register_netlink call _check_code(code, None, owner),
  the bare-int check #2521 added, in place of _check_number.
- _check_number is deleted; it raised the same messages _check_code does
  with kind=None.

Tests: test_int_only_registrars_reject_anything_but_an_int checks FRAD
through _check_code, with the same expected messages.
@JarryShaw
JarryShaw force-pushed the refactor/2510-linux-sll-arphrd-registry branch from e2b30f2 to c271988 Compare November 1, 2026 16:09
@JarryShaw

Copy link
Copy Markdown
Owner Author

Rebased onto main after #2509 and #2521 merged: e2b30f2e3 → c271988f3. There are three commits; the first two are the old two, re-applied.

@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Re-review (Sonnet) at c271988f3: GOOD TO GO.

  • Dispatch: a 73,600-row harness over LinuxSLL1/2, 23 ARPHRD values, 10 protocol types, 16 payload shapes and 10 stop settings is byte-identical on the head and on main. Radiotap frames parse as LINUX_SLLn:Radiotap:Raw with exact round trips, and FRAD, GRE and Netlink and the truncated-FRAD Raw are unchanged.
  • Mutations: pointing radiotap's entry back at Raw, or breaking Radiotap.__index__, fails tests.
  • Q5: _check_code(.., None, ..) matches _check_number on all 52 code/target pairs, and no reference remains.
  • Nothing lost: none from feat(link): add a radiotap dissector for IEEE80211_RADIOTAP on LinuxSLL #2509 or feat(registry): foundation.registry wrappers for IPX and SCTP sub-maps; record the convention (#2511) #2521. The PR adds no member-order finding.
  • Known red: the radiotap wrapper-coverage failure is identical on main, which has a test-only fix in progress.
  • Non-blocking: registry-protocol.rst's Registration Wrappers names only IPX and Loopback as overriding register; LinuxSLL, FRAD and Netlink do too.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Nov 1, 2026
@JarryShaw
JarryShaw merged commit be82c57 into main Nov 1, 2026
55 of 57 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 1, 2026
@JarryShaw
JarryShaw deleted the refactor/2510-linux-sll-arphrd-registry branch November 1, 2026 16:29
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 1, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Correction to my reconciliation above: LinuxSLL did not ship in 1.5.1. The v1.5.1 tag has no linux_sll module, and the commit that added it is not an ancestor of the tag. So LinuxSLL.register changing to ARPHRD keys alters no released contract, and I have removed the breaking label. The central changelog will list this change as non-breaking.

@JarryShaw JarryShaw removed the breaking Breaks public-facing behaviour or API (apply alongside the type label) label Nov 1, 2026
JarryShaw added a commit that referenced this pull request Nov 1, 2026
- Added: one's-complement checksum verification and computation (#2543),
  and the IPX and SCTP registration wrappers (#2521).
- Folded into unreleased entries: LinuxSLL/FRAD/Netlink registries (#2527),
  the IGMP class names (#2542), TLS record chaining and the SSLv2 trial's
  warnings (#2537), DTLS reassembly via Extractor and toolkit (#2528), and
  IP_Pseudo's walk at any depth (#2543). ICMP drops its stale checksum line.
- Breaking: make() checksums, DRARP's module (#2540), IPv6 overlapping
  fragments (#2569), TCP_GAP_LIMIT replacing TCP.__window__ (#2579), and
  Extractor/extract's positional dtls (#2528).
- Changed: member order (#2490), conventions (#2542, #2521, #2570), CI -n
  logical (#2572). Fixed: AppType.get (#2568), trial warnings (#2519, #2577).
- CHANGELOG.md regenerated; util/changelog_md.py --check passes.
@github-actions github-actions Bot added this to the PyPCAPKit v1.6 milestone Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

refactor(link): dispatch LinuxSLL's non-EtherType ARPHRD values through its own registry map

1 participant