protocols: bind FTP-DATA, HTTP-alt, OSPF, L2TP and the 802.1ad S-Tag, and fix four dispatch defects - #436
Merged
Conversation
…ables that reach them Split VLAN into an abstract base plus concrete tags, and fill in the dispatch entries whose dissectors already existed but were reachable from no registry. * ``VLAN`` becomes an abstract base holding the tag layout, with ``C_Tag`` (802.1Q, ``0x8100``) and ``S_Tag`` (802.1ad, ``0x88A8``) as concrete subclasses. The two layouts are identical -- the TPID that tells them apart belongs to the encapsulating header -- so the split is not about parsing but about Q-in-Q: both tags appear in one frame, and ``info_name`` is what keeps them distinct in the parsed output. A stacked frame previously collapsed into a single opaque ``Raw`` payload, losing the inner tag and everything under it. * ``VLAN.read`` reported the DEI flag as ``bool(tci['pcp'])`` instead of reading its own bit, so it was wrong whenever priority and drop-eligibility disagreed. The existing test passed because it pinned a case where they did not. * TCP 20 to ``FTP_DATA`` and 8080 to HTTP/1; UDP 8080 to HTTP and 1701 to ``L2TP``. Port numbers, service names and descriptions are IANA service-name-registry assignments. 8443 is deliberately left unbound: IANA registers it as ``pcsync-https``, and pcapkit implements no TLS. * OSPF is bound at ``TransType`` 89. Three defects had kept it from parsing anything at all: ``read`` consulted the schema *class* rather than the parsed header, ``alias`` reached for an ``_info`` that does not exist until ``read`` has returned, and the remaining payload length was dispatched as if it were a protocol code. L2TP shared the last of those; both now dispatch on the ``-1`` sentinel, as ARP does. * ``TransType`` 115 stays unbound: it references RFC 3931, i.e. L2TPv3 over IP, whose session header differs from the RFC 2661 v2 framing this dissector implements. Every registry read goes through ``ProtocolBase._lookup_registry``, so a miss does not grow the shared tables. Suite 891 passed / 18 skipped, against 876 / 18 on main (+15 tests, +17 subtests). All 15 sample captures produce byte-identical tree and json output; none of them contains a VLAN tag, an OSPF or L2TP packet, or traffic on any newly bound port, so the new paths are covered by synthetic frames instead. mypy unchanged at 128 errors in 41 files; pylint adds no message and drops one over-long line.
JarryShaw
commented
Sep 17, 2026
Owner
Author
|
Reviewed at What I checked and ran:
One finding posted inline (cosmetic, non-blocking): the Nothing else turned up. I could not find a case where the new bindings, the declined bindings, or the four fixes disagreed with what the PR body claims for them. Verdict: good to merge, once the one-line |
…80 and 8080 are bound twice
…he L2TP versions
Follows the project's existing convention, which the ARP family already encodes:
a protocol with its own ``__index__`` gets its own module, and siblings may share
one only when they share an index. ``InARP`` shares :mod:`~pcapkit.protocols.link.arp`
because it inherits ``ARP``'s index; ``RARP`` declares a different index and so
has :mod:`~pcapkit.protocols.link.rarp`, which ``DRARP`` then shares.
* ``C_Tag`` and ``S_Tag`` move to ``link/c_tag.py`` and ``link/s_tag.py``, and
each now **declares the EtherType it is reached by** -- ``0x8100`` and
``0x88A8``. That declaration was the missing piece: both were bound in
``Link.__proto__`` as distinct EtherTypes while inheriting a ``__index__``
that raised. The ``VLAN`` base keeps raising, which is correct for an abstract
protocol nothing dispatches to, and keeps the shared tag layout.
* ``L2TP`` becomes an abstract base and ``L2TPv2`` carries the RFC 2661
implementation, in ``link/l2tpv2.py``. What existed was v2 only, presented as
though it were L2TP in general. The base holds no header parsing at all, in
the way ``internet.ip.IP`` holds none: the versions genuinely do not share a
header, only the version nibble in the first 16-bit word. UDP 1701 now binds
the concrete class.
* ``OSPF.__index__`` returns ``TransType.OSPFIGP`` instead of raising -- the same
gap as the VLAN tags, since it is dispatched from ``Internet.__proto__`` at 89.
``TransType`` 115 stays unbound, and the reason is now structural rather than
incidental: it is L2TPv3 (RFC 3931), and there is no ``L2TPv3`` class for it to
point at. 115 is also the first index anything in the family would carry, so v3
gets its own module when written. Recorded in the module and in ``pep.rst``.
Neither ``L2TP`` nor ``L2TPv2`` declares an index: v2 is reached by a UDP *port*,
and a port is not an ``__index__`` value anywhere here -- every non-raising
``__index__`` returns a ``TransType``, ``EtherType`` or ``LinkType``, and
``Application.__index__`` raises for that reason.
``id()`` follows the HTTP family: canonical name first, then the version- or
variant-flavoured alias, since callers take element zero as canonical. ``L2TP``
and ``L2TPv2`` both return ``('L2TP', 'L2TPv2')``; the ``VLAN`` base claims
``('VLAN', 'C_Tag', 'S_Tag')`` while each tag keeps its own name canonical, as
the tags are distinct protocols rather than versions of one.
``info_name`` is declared on the ``L2TP`` base so a consumer finds the datagram
under ``l2tp`` whichever version was on the wire; the version is reported by
``alias`` instead.
The follow-up stream implementing L2TPv3 and L2F has what it needs written into
the base's docstring, including that ``Ver == 1`` selects **L2F** (RFC 2341), a
separate protocol, to be named ``L2F`` with ``L2TPv1`` only as an ``id()`` alias.
Suite 915 passed / 18 skipped, against 898 / 18 on main (+17 tests, +16
subtests). All 15 sample captures produce byte-identical tree, json and
reassembly output; ``make_samples.py`` regenerates byte-identically. mypy
unchanged at 128 errors in 41 files; pylint adds no message and still drops one
over-long line.
JarryShaw
commented
Sep 17, 2026
…-in-Q illustration Review feedback on #436, all cosmetic. * The module-docstring heading rule now runs three characters past the end of the title in ``vlan.py``, ``c_tag.py``, ``s_tag.py`` and ``l2tpv2.py``. Since the title starts at source column 3, after the ``\"\"\"``, that leaves three rule characters either side of it -- the title centred on the rule, which is what 172 of the package's 189 module headings already do, ``arp.py`` and ``rarp.py`` among them. ``l2tp.py`` already matched, having kept its original rule. * The Q-in-Q illustration in ``vlan.rst`` puts every ``=`` in one column, and the ``<-`` annotations in another. * Drops the note on ``OSPF.__index__`` recording that it used to raise. The behaviour stays -- it returns ``TransType.OSPFIGP``, since OSPF is dispatched from ``Internet.__proto__`` at 89 -- and the docstring now has the same shape as ``ARP.__index__``. The two ``read`` comments explaining the ``-1`` sentinel are left alone: those are source comments where the history *is* the explanation, not published API documentation where it is noise. No behaviour change. All 45 capture output files byte-identical to origin/main, ``make_samples.py`` regenerates byte-identically, suite 915 passed / 18 skipped unchanged, mypy 128 errors in 41 files unchanged, pylint unchanged.
Owner
Author
Review at head
|
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
1 participant
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 the port-binding, VLAN S-Tag and OSPF/L2TP items from the owner's list.
What is bound, and on whose authority
Port numbers and service names from IANA's Service Name and Transport Protocol Port Number Registry (CSV fetched today); protocol numbers from IANA's Protocol Numbers registry.
FTP_DATAftp-data, "File Transfer [Default Data]"httpv1.HTTPhttp-alt, "HTTP Alternate (see port 80)"http.HTTPL2TPl2tpTransType89 →OSPFOSPFIGP(:rfc:2328)0x8100→C_Tag,0x88A8→S_TagThree bindings declined, with reasons rather than omission. Port 8443 is registered as
pcsync-https, not an HTTP alternate; real traffic there is TLS, andapplication/NotImplemented/tls.pyis empty, so binding it would feed a TLS record to an HTTP parser.TransType115 references :rfc:3931(L2TPv3 over IP), a different session header from the :rfc:2661v2 framing this dissector implements —make()hardcodesversion=2. And FTP on UDP 20/21 is IANA-registered but is not where FTP actually runs.The implemented application set is only FTP, FTP_DATA, HTTPv1, HTTPv2 and NGAP; NGAP is already bound on SCTP PPID 60/66 and IANA registers it on sctp alone. Everything else under
NotImplemented/is a zero-byte file, so there was nothing further to bind.VLAN: an abstract base, and the reason is QinQ
VLANbecomes an abstract base carrying the whole tag — read, make,_make_data,length— withC_TagandS_Tagadding onlyname,alias,info_nameandid(). It is abstract for the same mechanical reasonIPis: it does not definename, whichProtocolBasedeclares abstract, so no newabstractmethodwas needed.The layouts really are identical — only the TPID differs — so the justification is QinQ, with one refinement worth recording: a single class bound at both EtherTypes would not collide, it would nest, giving
ethernet.c_tag.c_tagwith nothing to say which was the service tag. Distinctinfo_nameis what fixes that.Two gotchas found on the way.
__init_subclass__resolves an omittedschema=/data=by looking the subclass name up inpcapkit.protocols.schemaand assigns unconditionally, so both tags must restate them or silently receiveSchema_Raw. Andpcapkit/protocols/__init__.pyrebuilds__proto__from its own__all__, overwriting auto-registration, so both names had to be added there too.Four defects fixed, none of them in the brief — and OSPF had never worked
vlan.py:121readdei=bool(tci['pcp'])instead oftci['dei'], so drop-eligibility was wrong whenever it disagreed with priority. The existing test passed only because it pinnedpcp=EE, dei=True.ospf.py:133usedschema = self.__schema__— the schema class — where every sibling usesself.__header__. Any input raisedTypeError: unsupported operand type(s) for -: 'UInt16Field' and 'int', verified onmain. So OSPF has never parsed anything, which is why binding it needed fixes rather than a table entry.ospf.py'salias/namereadself._info, but_infois not assigned untilread()returns, while_decode_next_layerreadsaliasduringread(). Now held on_version, mirroringARP._acnm.ospf.pyandl2tp.pydispatched the remaining payload length as theprotopositional. That resolved toRawonly because lengths rarely collide with a registered EtherType — a 2048-byte body would have parsed as IPv4. Both now use the-1sentinel, asarp.py:205does.The existing OSPF tests had masked two of these by assigning an instance to
__schema__and mocking a two-argument_decode_next_layer; both are corrected.Verification
Suite 891 passed / 18 skipped / 869 subtests against a baseline of 876 / 18 / 852 — exactly +15 tests and +17 subtests, no regressions. The baseline was built with
git clone, notgit archive: an archive has no.git, sotest_tier_guard.pyskips ~19 there and inflates the count — which produced a wrong first measurement before it was caught.Captures byte-identical, 45 files (15 × tree/json/reassembly), by both
diff -rand an md5 manifest, with the inputs verified identical first.make_samples.pyregenerates byte-identically. mypy identical at 128 errors across 41 files. pylint: zero new messages and one pre-existing long line removed. Sphinx: 85 warnings, identical sets, exit 0 both trees.No capture output changed, and that was checked rather than assumed. A scan of all 1,339 frames found zero VLAN tags, zero OSPF or L2TP packets and no traffic on ports 20, 1701, 8080 or 8443 — every new code path is unreachable from this corpus. The deliberate changes are therefore demonstrable only on synthetic frames:
All registry reads go through
ProtocolBase._lookup_registry, and two tests assert no registry grows, including when parsing an unregistered port.Left alone deliberately
OSPF and L2TP living under
protocols/link/is a misclassification — both reportlayer == 'Link'while being carried inside IP and UDP respectively. Moving them changes public import paths, so they stay put; it is inert for layer-limited extraction because IPv4/IPv6 terminate aninternetextraction first. Documented in both modules and inpep.rst.http.HTTP's explicitversion=path is broken independently of this change: it passes an already-drained file object, soversion=1raisesValueErrorandversion=2raisesKeyError. Only the auto-guess path works, which is why UDP 80 and 8080 function. UDP therefore still points athttp.HTTPand TCP athttpv1.HTTP; repointing would change existing output and wants its own change. Recorded inpep.rst.Also noted, all pre-existing:
Extractornever closes its input handle, surfacing as aResourceWarningattributed to the wrong capture; andAppType.get(80, proto='tcp')returnswww_httprather thanhttpbecause__members_proto__is last-definition-wins.tcp.py's docstring claimed port 80 →http.HTTPwhile the code boundhttpv1.HTTP; that one is fixed here.