Skip to content

refactor(protocols): every versioned family base dispatches on read and make (IGMP, IP, ICMP, L2TP, LinuxSLL) #2574

Closed

Description

@JarryShaw

Standing convention from the owner on #2544: on a versioned protocol family, the base class is a dispatcher on both the read and the make path. Parsing bytes through the base, or building through it (constructor or make), yields the matching member, never a base-class layer. #2544 applies it to HTTP (HTTPv1/HTTPv2) and writes it onto protocol-family-grouping.rst.

Families to bring into line, those protocol-family-grouping.rst lists with versioned members:

  • IGMP (IGMPv1–3). Its base is abstract today and dispatches only from Internet._import_next_layer.
  • IP (IPv4/IPv6).
  • ICMP (ICMPv4/ICMPv6).
  • L2TP (L2TPv2).
  • LinuxSLL (LinuxSLL1/2).

For each, check what the base does now on direct construction from bytes and from fields. Make both paths yield the member, mirroring #2544's mechanism, and keep byte-exact round trips. Then test each family, and show the tests fail before the change.

Blocked: this mirrors #2544's mechanism and edits files #2542 (IGMP), #2527 (LinuxSLL) and #2544 change. It unblocks when #2544 merges; IGMP and LinuxSLL also need #2542 and #2527 merged first.

Activity

  1. added
    refactorRestructuring for its own sake — neither a fix nor a new capability (refactor: prefix)
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Nov 1, 2026
  2. moved this to Blocked in PyPCAPKiton Nov 1, 2026
  3. JarryShaw commented on Nov 1, 2026

    @JarryShaw
    OwnerAuthor

    #2544 has merged. The family rollout still touches IGMP, whose names #2558 is changing, and LinuxSLL, which #2587 edits. Blocked: this unblocks when #2587 and #2558's PR merge.

  4. JarryShaw commented on Nov 1, 2026

    @JarryShaw
    OwnerAuthor

    Question for the rollout: does the SSL family (SSLv2, TLS, DTLS) count as a versioned family under the read-and-make dispatch rule?

    1. No, leave it out (recommended). One TLS class already reads every TLS version, with a fixed id() and the version in its alias, and SSLv3 is a thin subclass. SSLv2 is a separate class that TLS reaches through its fallback, and SSL defines no read(). A dispatching SSL base would add a third path without fixing anything observable.
    2. Yes: SSL(...) would dispatch to SSLv2, TLS or DTLS on read and make, as the HTTP base does.
      fix(link): honour a stop at LinuxSLL on its ARPHRD registry path #2587 has merged. This rollout stays blocked until fix(internet): give IGMPv2 and IGMPv3 versioned names #2590 (the IGMP names) merges, so the answer can arrive any time before then.
  5. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    #2587 and #2590 have merged. The final #2490 member-order reorder is now in progress, and it reorders IPv4 and IGMPv3, two of the family classes this rollout changes. Blocked: this unblocks when #2490's reorder PR merges, so that the dispatch change lands on the reordered classes rather than rebasing across them. The SSL-family question above is still open for the owner.

  6. added this to the PyPCAPKit v1.6 milestone on Nov 2, 2026
  7. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    #2613 (the final member-order reorder) has merged, so this is unblocked. A worker is rolling read-and-make dispatch out to IGMP, IP, ICMP, L2TP and LinuxSLL, mirroring #2544's HTTP mechanism, one commit per family. It leaves SSL out, as recommended above, unless you say otherwise.

  8. added
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Nov 2, 2026
  9. moved this from Blocked to WIP in PyPCAPKiton Nov 2, 2026
  10. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    The family dispatch is done locally, one commit per family, and the Sonnet review says GOOD TO GO. Through the base, IGMP, IP and L2TP now yield their member from octets, fields and from_data. ICMP and LinuxSLL do it from fields and from_data. A subclass is constructed as itself. The transport path's output and the registries match main exactly. It opens as a PR once the Actions queue drops below 20.

    Needs deciding:

    1. Octets with no version field. For ICMP and LinuxSLL, the IP protocol number or the link type is what names the member. (a) Refuse the octets through the base with ProtocolError, as HTTP refuses octets neither version takes. (b) Use the default member, which mislabels ICMPv6 and SLL2 octets. (c) Add a version= keyword. Recommend (a), and add (c) only if a caller needs it.
    2. Field dispatch reads names only. So IP(src='::1', dst='::2') picks IPv4 and then fails. (a) Keep it names-only. (b) Also read what a shared field's value names: the address family, or the enum class of type. Recommend (b), as a follow-up.
    3. Default members. The lowest version wins: IPv4, ICMPv4, L2TPv2, LinuxSLL1, and IGMPv1 for IGMP's defaults, mirroring HTTPv1. Recommend keeping them.

    The PR goes up implementing 1(a) and 3. If you choose otherwise, the PR changes before it merges.

  11. added
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    on Nov 2, 2026
  12. 4 remaining items

  13. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    and I think version= should be the source of truth and the only check to do. no need to check for version-only parameters.

  14. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    The owner refined answer 2, paraphrased: version= is the source of truth and the only check made. Construction does not select a member from fields that only one member takes.

    • Fields: version= alone picks the member, and fields without version= raise.
    • Octets: version= decides when it is given. Otherwise IGMP, IP and L2TP dispatch by the version their octets carry, and ICMP and LinuxSLL octets without version= raise.
    • from_data: unchanged.

    The rework in progress has been updated to match.

  15. moved this from Needs decision to WIP in PyPCAPKiton Nov 2, 2026
  16. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    The rework is done locally (wf/2574-r3, one commit per family, on fa9af520f), and a Sonnet review is running.

    • Picking the member: version= alone picks it. Fields without version= raise, even when a field only one member takes is passed. There is no default member.
    • Raw octets: IP, IGMP and L2TP still parse by the version their octets carry. ICMP and LinuxSLL raise without version=.
    • Choices you may want to veto:
      • Building a member with another member's version, e.g. IPv4(version=6), raises, and so does a bool version.
      • LinuxSLL's from_data now goes by data-model class, as ICMP's does.
      • IGMP's from_data still reads values, because IGMPv1 and IGMPv2 share one data model with no version field.
      • A version= that contradicts the octets fails in that member's own parser. For IP(v6 octets, version=4) the message is a schema padding error rather than a version error.
    • HTTP still has the HTTPv1 default and field-based picking. Bringing it in line is filed as refactor(application): the HTTP base follows #2574's version= rule, with no default member #2628, blocked until this lands.
  17. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    for IGMP, let's add a version attribute to their data model so to distinguish.

  18. JarryShaw commented on Nov 2, 2026

    @JarryShaw
    OwnerAuthor

    Owner ruling, paraphrased: IGMP's data model gains a version attribute that names its member. IGMP.from_data then dispatches on that attribute, as IP and L2TP read their data's version, rather than reading type, max_resp_time or the length. This resolves the one exception noted above. The IGMP commit is being reworked; wire bytes and protochains stay unchanged, and info gains a version key.

  19. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

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

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions