diff --git a/pcapkit/__init__.py b/pcapkit/__init__.py index ff43e796b..c7b7d373e 100644 --- a/pcapkit/__init__.py +++ b/pcapkit/__init__.py @@ -32,7 +32,7 @@ Synthesises file I/O and protocol analysis, coordinates information exchange in all network layers, as well as - provides the foundamental functions for :mod:`pcapkit`. + provides the fundamental functions for :mod:`pcapkit`. - Protocols (:mod:`pcapkit.protocols`) diff --git a/pcapkit/__main__.py b/pcapkit/__main__.py index 429ac62d5..30b9f18d4 100644 --- a/pcapkit/__main__.py +++ b/pcapkit/__main__.py @@ -6,7 +6,9 @@ .. important:: - This module requires ``emoji`` package to be installed. + The ``emoji`` package is optional. Without it, importing this module + warns :class:`~pcapkit.utilities.warnings.EmojiWarning` and the verbose + messages print as plain text. :mod:`pcapkit.__main__` was originally the module file of |jspcapy|_, which is deprecated and merged with :mod:`pcapkit`. @@ -84,8 +86,8 @@ def get_parser() -> 'ArgumentParser': # outside it is not rejected anywhere downstream -- it simply never matches a # protocol's ``__layer__`` and the parse silently runs to the top of the # stack, which is the failure mode GH-356 was about. ``type=str.lower`` keeps - # ``-L Internet`` working, as it did before the choices were declared, since - # ``Extractor.__init__`` lowercases the value anyway. + # ``-L Internet`` accepted, matching ``Extractor.__init__``, which lowercases + # the value anyway. # # The defaults are :data:`None` and not the ``'None'``/``'null'`` strings: # ``Extractor.__init__`` substitutes its own sentinels for an diff --git a/pcapkit/all.py b/pcapkit/all.py index ea38b61bf..98adfc5a2 100644 --- a/pcapkit/all.py +++ b/pcapkit/all.py @@ -111,7 +111,7 @@ 'NoPayload', # No Payload 'Raw', # Raw Packet 'ARP', 'C_Tag', 'DRARP', 'Ethernet', 'InARP', 'L2TP', 'L2TPv2', 'OSPF', - 'RARP', 'S_Tag', 'VLAN', # Link Layer + 'RARP', 'S_Tag', 'VLAN', # Link Layer (DRARP, OSPF, RARP: Application Layer) 'AH', 'IP', 'IPsec', 'IPv4', 'IPv6', 'IPX', # Internet Layer 'HIP', 'HOPOPT', 'IPv6_Frag', 'IPv6_Opts', 'IPv6_Route', 'MH', # IPv6 Extension Header diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index d4cd6429f..4a88cb56e 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -41,17 +41,6 @@ :class:`EnumRegistry` subclasses outside :mod:`pcapkit.const`. The mutating half therefore had no users to serve among the classes being re-parented. -.. note:: - - Re-parenting every non-registry enumeration onto :class:`EnumLookup` was - **phase 2** of GitHub issue :issue:`877`, and it is now **complete**: - introducing the base above was deliberately behaviour-preserving on its - own, so that it could land while other work was still in flight on the - files the re-parent touches, and the phase itself landed in two pull - requests for exactly that reason -- the first for the 17 enumerations that - were free to move at once, and the second, :issue:`930`, for the remaining - seven once the files holding them freed up. - The registry tier's own shape is the earlier design settled in GitHub issue :issue:`842`: ``get``, ``get_all``, ``register`` and ``register_alias`` are to exist on every const registry, so the abstraction is finished by moving them to @@ -70,9 +59,9 @@ overrides below and a few hand-written ``get`` overrides. 2. ``AppType``'s sub-base -- overrides all four, routing the two lookups through its ``_dispatch``, because a port lookup needs a transport protocol - to be answerable at all. Landed as of GitHub issue :issue:`860`: not in this - module, but in :class:`pcapkit.const.reg.apptype.apptype.AppType` itself, - which mixes in :class:`EnumRegistry` directly. ``register`` and + to be answerable at all. Defined not in this module but in + :class:`pcapkit.const.reg.apptype.apptype.AppType` itself (GitHub issue + :issue:`860`), which mixes in :class:`EnumRegistry` directly. ``register`` and ``register_alias`` are overridden without that dispatch, staying on the registry they are called on, so that a write never leaks onto a transport IANA never assigned the service to -- a minted member for the first, an @@ -80,17 +69,15 @@ attributes (``svc``, ``port``, ``proto``) that the generic one below does not know to set. 3. The ``AppType`` transport subclasses -- ``TCP``, ``UDP``, ``SCTP``, ``DCCP`` - -- turned out to need no override of their own at all: ``_dispatch`` + -- need no override of their own at all: ``_dispatch`` already returns ``cls`` unchanged the moment ``cls.__registry__`` is not :obj:`None`, which is true for exactly these four, so tier 2's lookups already answer correctly on each of them without a further layer. -Before this, the four methods lived as generated *text*: written out longhand in -:data:`pcapkit.vendor.default.LINE` and copied verbatim into each of the eleven -crawlers that replace that template wholesale, none of which carried -``register``, ``register_alias`` or ``get_all`` at all. Adding one method meant -editing every bespoke template by hand, which is the cost :issue:`775` asks to -remove. +Defining the four methods here, rather than as generated *text* written out +longhand in :data:`pcapkit.vendor.default.LINE` and copied into each crawler +that replaces that template wholesale, is what :issue:`775` asks for: as text, +adding one method means editing every bespoke template by hand. The contracts are those set out on GitHub issue :issue:`842`: ``get`` is a shortcut for the ``[]`` operation and returns the canonical enumeration member; @@ -177,8 +164,7 @@ def _validate_value(cls, value: 'Any') -> 'None': The base implementation accepts everything, because a base cannot know any subclass's range. Overriding it is how a subclass states one -- the - shape ``_missing_`` spells by hand across the generated registries - today:: + shape ``_missing_`` spells by hand across the generated registries:: @classmethod def _validate_value(cls, value: 'Any') -> 'None': @@ -258,10 +244,7 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': It never mints while resolving ``default``; ``key`` may still mint through a ``_missing_`` that GitHub issue :issue:`775`'s ruling - deliberately kept minting, on one registry (``CGAType``) -- the - ruling's final round converted the other two it originally held out, - ``EtherType`` and ``Socket``, so they no longer mint on any path - either. Registering a member any other way is :meth:`register`'s job + deliberately kept minting, on one registry (``CGAType``). Registering a member any other way is :meth:`register`'s job and nobody else's, which is the ruling :issue:`775` exists to carry out: an unrecognised or unregistered value does not become a registered member unless a user or caller explicitly creates one. A value inside a @@ -279,17 +262,13 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': For a ``str`` key, a name match wins over a value match -- the two are checked in that order, so a string that happens to be both a member's - name and a *different* member's value resolves to the name's member, - matching what already happened for a name that resolves today. The - value side of that check is a plain ``_value2member_map_`` lookup, not - ``cls(key)``: on a registry whose own ``_missing_`` mints for an + name and a *different* member's value resolves to the name's member. + The value side of that check is a plain ``_value2member_map_`` lookup, + not ``cls(key)``: on a registry whose own ``_missing_`` mints for an unrecognised value, routing a failed *name* lookup through the constructor would let a mere ``get()`` call mint a permanent member - where it previously just raised. Defensive rather than observed: of - the 127 classes that reach this method -- 125 until GitHub issue - :issue:`880`'s own PR added ``pcapkit/const/ngap/procedure_code.py`` - and ``pcapkit/const/ngap/protocol_ie.py``, remeasured while auditing - GitHub issue :issue:`903` -- the ``str``-valued ones + where it should just raise. Defensive rather than observed: none of + the ``str``-valued registries that reach this method (:class:`~pcapkit.const.ftp.command.Command`, :class:`~pcapkit.const. ftp.command.FEATCode`, :class:`~pcapkit.const.http.method.Method`, :class:`~pcapkit.const.pcapng.option_type.OptionType`, @@ -297,31 +276,23 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': transport subclasses :class:`~pcapkit.const.reg.apptype.tcp.TCP`, :class:`~pcapkit.const.reg.apptype.udp.UDP`, :class:`~pcapkit.const.reg.apptype.sctp.SCTP` and - :class:`~pcapkit.const.reg.apptype.dccp.DCCP` -- completing that set as - of GitHub issue :issue:`860`'s own PR 2 -- and, newest of them, - :class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel`, which GitHub - issue :issue:`877`'s own thread reclassified from a hand-written helper - to a generated registry once RFC 9850 ยง4.2 turned its member list into - a live IANA registry) no longer mint on any path, so no live witness - exists in this tree today. The one registry that still mints directly - via :func:`~aenum.extend_enum`, + :class:`~pcapkit.const.reg.apptype.dccp.DCCP`, and + :class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel`) mints on + any path, so no live witness exists in this tree. The one registry + that still mints directly via :func:`~aenum.extend_enum`, :class:`~pcapkit.const.mh.cga_type.CGAType`, is :class:`int`-valued, so a ``str`` name could not reach its mint branch even if this restriction did not exist; it is not an exception to it, just not reachable by it. - GitHub issue :issue:`775`'s final round converted the other two that - used to share this footnote, :class:`~pcapkit.const.ipx.socket.Socket` - and :class:`~pcapkit.const.reg.ethertype.EtherType`, so ``CGAType`` is - now the only one left. This is about a future ``str``-valued registry - (or a present one whose ``_missing_`` someday changes) reaching this - base with a minting ``_missing_`` of its own, which the restriction - below is written to stay correct for regardless. Restricting the value - side of ``key`` to an already-registered value keeps *that side* - non-minting on every ``str``-valued registry, not only the ones without - a minting ``_missing_``. Since :issue:`864`, that is no longer merely a - claim about the value side alone: ``default`` resolves through the same - kind of ``_value2member_map_`` lookup rather than ``cls(default)``, so - for a ``str`` key every path through this method -- name, value and - ``default`` alike -- is non-minting. + This is about a future ``str``-valued registry (or a present one whose + ``_missing_`` someday changes) reaching this base with a minting + ``_missing_`` of its own, which the restriction below is written to + stay correct for regardless. Restricting the value side of ``key`` to + an already-registered value keeps *that side* non-minting on every + ``str``-valued registry, not only the ones without a minting + ``_missing_``; and ``default`` resolves through the same kind of + ``_value2member_map_`` lookup rather than ``cls(default)`` + (:issue:`864`), so for a ``str`` key every path through this method -- + name, value and ``default`` alike -- is non-minting. That restriction has a cost the paragraph above glosses over: a *declared-but-unassigned* value -- the case resolved there through @@ -351,17 +322,14 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': rather than a builtin, per the ruling recorded on GitHub issue :issue:`923`: in-library code raises from ``pcapkit.utilities.exceptions`` rather than a builtin, and whether ``ValueError`` or ``KeyError`` applies - follows what stdlib's ``Enum`` raises in the same circumstance. The *shape* - is unchanged by that ruling and deliberately so -- a name miss stays - :exc:`KeyError`-derived and a value miss :exc:`ValueError`-derived, + follows what stdlib's ``Enum`` raises in the same circumstance: a name + miss is :exc:`KeyError`-derived and a value miss :exc:`ValueError`-derived, matching ``E['nosuch']`` and ``E(999)`` on a stdlib :class:`~enum.Enum`, - and matching the 119 of this tree's 127 concrete subclasses that already - answered a name miss that way. Only the provenance changed, so every - ``except KeyError`` and ``except ValueError`` around a call to this method - keeps catching. + so every ``except KeyError`` and ``except ValueError`` around a call to + this method catches. - Two details of that conversion are worth stating, since neither is - visible from the exception type alone: + Two details are worth stating, since neither is visible from the + exception type alone: * **The name miss is raised quietly** -- :exc:`~pcapkit.utilities.exceptions.EnumKeyError` with ``quiet=True``, @@ -369,7 +337,8 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': That is not a cosmetic choice: this method's name miss is in-library control flow at six call sites, and at :meth:`~pcapkit.const.http.method.Method.get` it is part of a - *successful* call -- that override catches it in order to mint. A loud + *successful* call -- that override catches it in order to build an + unregistered member. A loud error there would put a :data:`logging.CRITICAL` record on every such call and set :data:`sys.tracebacklimit` to ``0`` process-wide, which is exactly the GitHub issue :issue:`362` defect @@ -551,7 +520,7 @@ def _extend(cls, value: 'Any', name: 'str') -> 'Self': """The raw :func:`~aenum.extend_enum` call, shared by :meth:`register` and :meth:`register_alias`. - Neither public method calls the other: :meth:`register` now refuses an + Neither public method calls the other: :meth:`register` refuses an already-registered ``value`` before it would ever reach here, and :meth:`register_alias` depends on the opposite of that -- it verifies ``value`` *is* already registered and then relies on exactly the @@ -603,9 +572,9 @@ def register_alias(cls, value: 'Any', name: 'str') -> 'Self': On this base, an alias adds a *name*, not a member: ``__members__`` grows by one while ``_member_names_``, iteration and ``_value2member_map_`` are untouched. Calls :meth:`_extend` directly rather than :meth:`register`, which - would now refuse this call outright -- :meth:`register` and + would refuse this call outright -- :meth:`register` and :meth:`register_alias` test ``value``'s membership for opposite - outcomes, so neither can be the other's implementation any more. + outcomes, so neither can be the other's implementation. ``AppType``'s override differs: it mints a real member through :func:`~aenum.extend_enum`, so its iteration grows too. @@ -651,9 +620,9 @@ def _unregistered_member(cls, value: 'Any', name: 'str') -> 'Self': """Build a member absent from this registry's own lookup tables. Used by a registry's ``_missing_`` for a declared-but-unassigned value - it resolves without anyone asking for a name, so that such a lookup no - longer grows the registry -- contrast :meth:`register`, the explicit - path that still does. + it resolves without anyone asking for a name, so that such a lookup does + not grow the registry -- contrast :meth:`register`, the explicit path + that does. The member is constructed through ``cls._member_type_``, which :mod:`aenum` sets from the enumeration base, so this serves ``int``- and diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index 42546b56f..6fd6a798c 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -140,24 +140,23 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte if self._item_type is None: return file.read(length) - # NOTE: ``SchemaField`` used to be imported here, inside the method body, - # rather than at module level -- there is no cyclic import to dodge by - # doing so; :mod:`pcapkit.corekit.fields.misc` does not import this - # module. A local ``from ... import`` re-resolves against - # :data:`sys.modules` on *every* call, and normally that is a cheap - # dict lookup returning the same cached module -- except when something - # has popped :mod:`pcapkit.corekit.fields.misc` out of - # :data:`sys.modules` since this ``ListField`` was built (the ``#439`` - # regression tests do exactly that, to get a clean ABC-cache state - # between cases). The next call re-executes the module from scratch and - # mints a *second*, distinct ``SchemaField`` class, while - # ``self._item_type`` -- built long before, from the first one -- is - # still an instance of the original. ``isinstance`` against the new - # class then reports :data:`False` for an item that plainly is a - # ``SchemaField``, ``is_schema`` goes the wrong way, and this method's - # other branch bills each item by its *declared* length unconditionally - # instead of by what it actually consumed -- which is what let a - # malformed TCP SACK option's length check raise + # NOTE: ``SchemaField`` is imported at module level rather than inside + # this method body -- there is no cyclic import to dodge by doing so; + # :mod:`pcapkit.corekit.fields.misc` does not import this module. A local + # ``from ... import`` re-resolves against :data:`sys.modules` on *every* + # call, and normally that is a cheap dict lookup returning the same + # cached module -- except when something has popped + # :mod:`pcapkit.corekit.fields.misc` out of :data:`sys.modules` since + # this ``ListField`` was built (the ``#439`` regression tests do exactly + # that, to get a clean ABC-cache state between cases). The next call + # would then re-execute the module from scratch and mint a *second*, + # distinct ``SchemaField`` class, while ``self._item_type`` -- built long + # before, from the first one -- is still an instance of the original. + # ``isinstance`` against the new class would report :data:`False` for an + # item that plainly is a ``SchemaField``, ``is_schema`` would go the + # wrong way, and this method's other branch would bill each item by its + # *declared* length instead of by what it actually consumed -- so a + # malformed TCP SACK option's length check could raise # :exc:`~pcapkit.utilities.exceptions.FieldValueError` from here instead # of :exc:`~pcapkit.utilities.exceptions.ProtocolError` from # :meth:`TCP._read_mode_sack @@ -262,9 +261,8 @@ class OptionField(ListField, Generic[_TS]): All of that is true of every ``OptionField`` declared in this package. A base schema registered from outside it -- c.f. :mod:`pcapkit.foundation.registry` -- need not satisfy it, and is **not** - rejected: such a base schema is unpacked in full, exactly as it was before - the shortcut existed. It parses correctly and pays the cost of the second - parse. Only the shortcut is withheld, so a base schema whose type field + rejected: such a base schema is unpacked in full. It parses correctly and + pays the cost of the second parse. Only the shortcut is withheld, so a base schema whose type field comes second cannot be silently misread. """ @@ -331,8 +329,8 @@ def __init__(self, length: 'int | Callable[[dict[str, Any]], int]' = lambda _: - # established first. # # Every test is written so that an unexpected base schema selects the full - # unpack instead of raising, so this cannot turn a registration that works - # today into an import-time failure. + # unpack instead of raising, so this cannot turn a working registration + # into an import-time failure. fields = getattr(self._base_schema, '__fields__', {}) type_field = fields.get(type_name) @@ -404,7 +402,7 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list # NOTE: Where it can, this reads the base schema's type field alone rather # than the whole base schema. The option schema below re-reads the same # octets from the rewound stream, and the base schema's result is used for - # nothing but the type code, so unpacking it in full parsed every option + # nothing but the type code, so unpacking it in full parses every option # twice -- 2274 schema unpacks for 1137 options of # ``examples/captures/profile.pcapng``. # @@ -460,13 +458,12 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list # octets behind it -- an over-long ``ihl``, or a capture cut short by # the snapshot length -- exhausts ``file`` early, and the exhausted # read then decodes the type field as 0. For the IPv4, TCP and PCAP-NG - # registries 0 *is* the end-of-option-list code, so the break above has - # always absorbed that case and reported the rest of the area as - # padding. Checking progress first turned all of those into errors: - # measured on ``IPv4(bytes.fromhex('4a00001800010000400600000a0000010a000002'))``, + # registries 0 *is* the end-of-option-list code, so the break above + # absorbs that case and reports the rest of the area as padding. + # Checking progress first would turn all of those into errors: measured + # on ``IPv4(bytes.fromhex('4a00001800010000400600000a0000010a000002'))``, # 20 octets of options declared with none present, and on a TCP segment - # with a data offset of 10 and four option octets, both of which parse - # on ``main``. + # with a data offset of 10 and four option octets, both of which parse. # # The registries that spin are the ones where 0 is *not* the # end-of-option-list code, so they never reach the break: HOPOPT, diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index f3895e535..cf0ae0911 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -54,7 +54,7 @@ #: else a parse has already padded. #: #: :data:`_MAX_ZERO_PAD_LENGTH` bounds each field on its own, and a packet holds -#: many fields, so the *sum* was unbounded: a declared length just under the +#: many fields, so it leaves the *sum* unbounded: a declared length just under the #: ceiling is honoured however often it is declared. Measured on this tree, 200 #: minimal PCAP-NG Decryption Secrets Blocks -- 4,800 wire octets, each block #: declaring ``secrets_length`` of 262,142 against two supplied octets through @@ -76,7 +76,7 @@ #: field -- which is how an IP header, an IPv6 payload, a TCP or IPv4 option and #: a PCAP-NG option all declare their size -- so no shortfall that one of those #: can produce is subject to the budget at all, and every one of them is padded -#: unconditionally, exactly as before. Measured: the largest legitimate single +#: unconditionally. Measured: the largest legitimate single #: shortfall found anywhere was 65,495 octets, from a snapshot-truncated #: offload-sized frame (100 frames at ``incl_len`` 54 declaring an IPv4 total #: length of 65,535, padding 6,549,500 octets from 7,024 read), and 64,750 from a @@ -170,7 +170,7 @@ def _zero_pad_budget() -> 'Iterator[list[int]]': ``Extractor`` run, per file, per frame -- for a caller that wants it that way, and what makes it reproducible for a test. - Note that nothing in this package calls it yet: the layer that knows where + Note that nothing in this package calls it: the layer that knows where one parse ends is the layer that should, and that is not this one. Yields: @@ -247,8 +247,8 @@ class FieldBase(Generic[_T], metaclass=FieldMeta): # replace :meth:`__init__` without chaining to this one -- as # :class:`~pcapkit.corekit.fields.collections.ListField` does, since a list of # fields takes no default value of its own -- and reading :attr:`default` off - # one of those raised :exc:`AttributeError` for a private attribute rather - # than reporting that the field declares no default. See #422. + # one of those would otherwise raise :exc:`AttributeError` for a private + # attribute rather than report that the field declares no default. See #422. _default: '_T | NoValueType' = NO_VALUE @property @@ -356,10 +356,10 @@ def __copy__(self) -> 'Self': """Return a shallow copy of the field. Every field of every protocol is copied once per packet by - :meth:`__call__`, which made the generic :func:`copy.copy` path -- via + :meth:`__call__`, which makes the generic :func:`copy.copy` path -- via :meth:`object.__reduce_ex__` and :func:`copy._reconstruct` -- one of the - costlier things an extraction did. This does what that path would have - done, and only that: a new instance of the same class, its + costlier things an extraction would do. This does what that path would + do, and only that: a new instance of the same class, its :attr:`~object.__dict__` shallow-updated from this one. Note: @@ -470,9 +470,8 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': raises. """ - # NOTE: ``length`` recomputes struct.calcsize() on every read, so the - # three reads this method used to make were three calcsize() calls for - # one value. + # NOTE: ``length`` recomputes struct.calcsize() on every read, so it is + # read once here. length = self.length if not isinstance(buffer, bytes): @@ -498,9 +497,9 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': # NOTE: The ceiling above bounds one field; this bounds their sum. A # declared length just under :data:`_MAX_ZERO_PAD_LENGTH` passes it every - # time it is declared, so 200 of them amplified 4,800 wire octets into + # time it is declared, so 200 of them amplify 4,800 wire octets into # 50.0 MiB of retained zeros with the ceiling doing exactly what it was - # written to do -- the sum was never bounded at all. C.f. #573. + # written to do. C.f. #573. # # ``max(length, 0)`` is defensive rather than a case anything is known to # reach. :attr:`length` here resolves through :func:`struct.calcsize`, so @@ -567,20 +566,19 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_T': # NOTE: ``ljust()``, not ``rjust()``. A short read has lost the *trailing* # octets of the field -- the buffer ran out, so what is missing is # whatever came after what was read -- so the zeros belong at the end, - # where the unread octets were. ``rjust()`` instead put them at the + # where the unread octets were. ``rjust()`` would put them at the # front, which asserts that the octets never read were the *leading* # ones, and that is wrong for every byte order rather than only for - # little-endian ones. Measured on the tree that padded with ``rjust()``: - # one octet of a four-octet little-endian 120 (``0x78``) read as - # 2,013,265,920, and three octets of a four-octet big-endian - # ``0x01020304`` read as ``0x10203``. ``ljust()`` answers 120 and - # ``0x1020300``. The big-endian error is the more dangerous of the two -- - # it scales the value *down*, so it passes a sanity check far more easily - # than the inflated little-endian one, which is why the defect went - # unnoticed there. See #604. + # little-endian ones. Under ``rjust()``, one octet of a four-octet + # little-endian 120 (``0x78``) reads as 2,013,265,920, and three octets + # of a four-octet big-endian ``0x01020304`` read as ``0x10203``. + # ``ljust()`` answers 120 and ``0x1020300``. The big-endian error is the + # more dangerous of the two -- it scales the value *down*, so it passes a + # sanity check far more easily than the inflated little-endian one. See + # #604. # - # This changes what a truncated field *reports*, which is the point, and - # not *whether* a truncated capture parses, which must not change: the + # The choice decides what a truncated field *reports*, not *whether* a + # truncated capture parses, which must not change: the # accommodation itself is deliberate (#431) and the budget above is built # around preserving it. An entirely empty buffer pads to all zeros either # way, so the end-of-option-list and ``Pad1`` reads the option and list diff --git a/pcapkit/corekit/fields/ipaddress.py b/pcapkit/corekit/fields/ipaddress.py index 3324cf950..98703f99f 100644 --- a/pcapkit/corekit/fields/ipaddress.py +++ b/pcapkit/corekit/fields/ipaddress.py @@ -97,8 +97,7 @@ def _reject_bool(value: 'object', description: str) -> 'None': Every caller checks this *before* dispatching on the value's type, for the same placement reason :issue:`469` gives: a correct check - in the wrong position does not fire, and that placement mistake - has already been made twice in this repository's history. + in the wrong position does not fire. Guarding the field classes is necessary but not sufficient, because a ``_make_*`` that must know the address family before it can build the @@ -142,21 +141,17 @@ def parse_ip_address(value: 'IPv4Address | IPv6Address | bytes | int | str', Notes: This is the sanctioned way for a ``_make_*`` method to turn a caller-supplied address into an :mod:`ipaddress` object, and it exists - because doing it with :func:`ipaddress.ip_address` directly is what - :issue:`508` turned out to be: a ``_make_*`` that has to know the - address *family* before it can build the schema -- to size an option - whose length is the only thing on the wire that carries the family -- - must convert the argument itself, and that conversion happens - **before** the schema, so it launders a :obj:`bool` into an + because doing it with :func:`ipaddress.ip_address` directly is the + defect :issue:`508` found: a ``_make_*`` that has to know the address + *family* before it can build the schema -- to size an option whose + length is the only thing on the wire that carries the family -- must + convert the argument itself, and that conversion happens **before** + the schema, so it launders a :obj:`bool` into an :class:`~ipaddress.IPv4Address` that the guard added for :issue:`491` in :meth:`_IPAddressField.pre_process` can then only see as a - legitimate address. Seven such call sites took ``True`` / ``False`` - without complaint as ``0.0.0.1`` / ``0.0.0.0`` -- or ``::1`` / ``::`` - where the wire format fixes the family as IPv6 -- and six of them went - on to pack those octets. The seventh, :meth:`TCP._make_mptcp_addaddr - `, built an - equally corrupt schema and is only stopped from packing it by an - unrelated defect of its own. + legitimate address: ``True`` / ``False`` become ``0.0.0.1`` / + ``0.0.0.0`` -- or ``::1`` / ``::`` where the wire format fixes the + family as IPv6 -- without complaint. Routing every one of them through here rather than giving each its own :func:`isinstance` check is the whole point: :issue:`469` added diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index d53977d10..07c1f35c4 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -276,13 +276,13 @@ def protocol(self, protocol: 'Type[_TP] | str') -> 'None': from pcapkit.protocols import __proto__ # pylint: disable=import-outside-top-level # NOTE: The registry is keyed on the upper-cased class name, both - # when it is seeded (``pcapkit/protocols/__init__.py:75``) and when + # when it is seeded (``pcapkit/protocols/__init__.py:76``) and when # ``pcapkit.foundation.registry.protocols.register_protocol`` adds to - # it, so a name given in any other case missed every time -- and a - # miss leaves ``_protocol`` as :obj:`None`, which the property above - # resolves to :class:`~pcapkit.protocols.misc.raw.Raw`. So - # ``PayloadField(protocol='http')`` yielded a raw payload instead of - # HTTP, with nothing to say so (#787). + # it, so a name looked up in any other case would miss every time -- + # and a miss leaves ``_protocol`` as :obj:`None`, which the property + # above resolves to :class:`~pcapkit.protocols.misc.raw.Raw`. So + # ``PayloadField(protocol='http')`` would yield a raw payload instead + # of HTTP, with nothing to say so (#787). resolved = __proto__.get(protocol.upper()) # NOTE: Warned rather than left silent, and warned rather than @@ -311,12 +311,12 @@ def __init__(self, length: 'int | Callable[[dict[str, Any]], int]' = lambda _: - # NOTE: Through the property rather than straight to ``_protocol``, so a # name given here is resolved exactly as one assigned later is. Writing - # the attribute directly stored the :obj:`str` verbatim and the getter - # handed that same string back, so ``PayloadField(protocol='http')`` - # yielded neither the protocol nor - # :class:`~pcapkit.protocols.misc.raw.Raw` but ``'http'`` itself -- and - # ``protocol='HTTP'`` was no better, since the case was never what this - # path went wrong on (#787). The lookup the setter performs stays inside + # the attribute directly would store the :obj:`str` verbatim and the + # getter would hand that same string back, so + # ``PayloadField(protocol='http')`` -- or ``protocol='HTTP'`` -- would + # yield neither the protocol nor + # :class:`~pcapkit.protocols.misc.raw.Raw` but the string itself (#787). + # The lookup the setter performs stays inside # its ``isinstance(protocol, str)`` branch, so a field declared in a # schema class body -- every in-library use, none of which names a # protocol -- still does not import :mod:`pcapkit.protocols` while that @@ -589,39 +589,29 @@ def nested_packet_context(packet: 'dict[str, Any]') -> 'dict[str, Any]': observed through it -- ``__packet__`` remains bound to the live enclosing mapping for any callback that needs the current value. - No dedicated class and no :class:`~collections.ChainMap`. Earlier - versions of this function returned each in turn: a - :class:`~collections.ChainMap` first, then a hand-written - :class:`dict` subclass adopted when the ``ChainMap`` was suspected of - corrupting the shared :class:`~abc.ABCMeta` cache every - :class:`Schema ` subclass used - to share on CPython <= 3.10 (issue :issue:`439`), and then a - :class:`~collections.ChainMap` again once that suspicion was doubted. - A plain :class:`dict` ends the question: it satisfies every - ``packet: 'dict[str, Any]'`` annotation on the rest of the field - classes natively, so no :func:`~typing.cast` is needed at the call - site, and it cannot interact with :class:`~abc.ABCMeta` at all because - :class:`dict` is not an :class:`~abc.ABCMeta`-based class. - - On the :issue:`439` suspicion itself, for the record, since it drove - two rewrites: it is *probably* wrong and no longer decidable. What is - directly measured is that the cache keys on the **exact type - queried**, so asking about a :class:`~collections.ChainMap` instance - caches lookups for :class:`~collections.ChainMap` and not for - :class:`dict`, and that the poisoning observed in :issue:`439` came - from ordinary code asking :func:`isinstance` about a plain - :class:`dict` -- :func:`~pcapkit.corekit.infoclass.Info.__update__` - does exactly that. Against that, swapping the ``ChainMap`` for a plain - literal was, at the time and on a real CPython 3.10 venv, enough to - move ``test_pcapng_remaining_constructor_branches_and_custom_dispatch`` - between passing and failing, toggled both ways. The likeliest - reconciliation -- that the ``ChainMap`` was never causal but changed - which concrete types flowed through unrelated :func:`isinstance` calls - in the same run, and so changed *when* the pre-existing corruption - fired -- is plausible rather than demonstrated, and cannot now be - tested: :issue:`439` has been fixed directly, every :class:`Schema` - subclass gets its own ``_abc_impl``, and the original conditions no - longer exist. It does not affect correctness either way. + No dedicated class and no :class:`~collections.ChainMap`. A plain + :class:`dict` satisfies every ``packet: 'dict[str, Any]'`` annotation + on the rest of the field classes natively, so no :func:`~typing.cast` + is needed at the call site, and it cannot interact with + :class:`~abc.ABCMeta` at all because :class:`dict` is not an + :class:`~abc.ABCMeta`-based class. That also settles a suspicion, + raised on :issue:`439`, that a :class:`~collections.ChainMap` here + corrupted the :class:`~abc.ABCMeta` cache :class:`Schema + ` subclasses shared on CPython + <= 3.10. The suspicion is *probably* wrong and can no longer be + tested: the cache keys on the **exact type queried**, so asking about a + :class:`~collections.ChainMap` instance caches lookups for + :class:`~collections.ChainMap` and not for :class:`dict`, the poisoning + observed in :issue:`439` came from ordinary code asking + :func:`isinstance` about a plain :class:`dict`, and :issue:`439` has + been fixed directly -- every :class:`Schema` subclass gets its own + ``_abc_impl``. Against that, swapping the ``ChainMap`` for a plain + literal on CPython 3.10 once moved + ``test_pcapng_remaining_constructor_branches_and_custom_dispatch`` + between passing and failing, both ways; the likeliest reading is that + it changed which types flowed through unrelated :func:`isinstance` + calls rather than causing the corruption. It does not affect + correctness either way. """ return {**packet, '__packet__': packet} diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 7f6ecc502..bd5fc9315 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -62,12 +62,12 @@ class NumberField(Field[int], Generic[_T]): Notes: A subclass such as :class:`UInt32Field` fixes the sign through - ``__signed__``, so ``signed`` there is at best redundant. It used to be - discarded outright, in both directions, which meant - ``UInt32Field(signed=True)`` handed back an unsigned field whose values - only looked wrong once the high bit was set -- see GitHub issue - :issue:`545`. A contradicting value is now rejected instead; omitting - it, or passing the sign the class already fixes, stays legal. + ``__signed__``, so ``signed`` there is at best redundant. A + contradicting value is rejected rather than discarded, since + discarding it would make ``UInt32Field(signed=True)`` hand back an + unsigned field whose values only look wrong once the high bit is set + -- see GitHub issue :issue:`545`. Omitting it, or passing the sign the + class already fixes, stays legal. """ @@ -103,10 +103,10 @@ def __init__(self, length: 'Optional[int | Callable[[dict[str, Any]], int]]' = N self._bit_length, self._bit_mask = -1, -1 # NOTE: ``__signed__`` fixes the sign for a subclass such as - # :class:`UInt32Field`, and used to *discard* the ``signed`` argument to - # do it -- in both directions, so ``UInt32Field(signed=True)`` returned - # an unsigned field and ``Int8Field(signed=False)`` a signed one, both - # without a word. ``None`` is what "not given" looks like, which is what + # :class:`UInt32Field`; discarding a contradicting ``signed`` would make + # ``UInt32Field(signed=True)`` return an unsigned field and + # ``Int8Field(signed=False)`` a signed one, both without a word. + # ``None`` is what "not given" looks like, which is what # lets a contradicting value be told apart from the default and rejected # while leaving an agreeing one alone. See #545. if self.__signed__ is None: @@ -173,7 +173,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': Rebuilding the template here is what applies a callable ``length``, and :meth:`build_template` recomputes ``self._need_process`` as it goes, so the flag and the template always describe the same width. - They did not always: see GitHub issue :issue:`591`. + See GitHub issue :issue:`591`. """ new_self = super().__call__(packet) @@ -207,15 +207,14 @@ def build_template(self, length: 'int', signed: 'bool') -> 'str': Notes: ``self._need_process`` is **assigned** here rather than only ever raised, so that it always describes the ``length`` this template - was built for. It used to be - set :data:`True` in the fall-through branch and never put back, - which made it a latch: a callable ``length`` is a placeholder of - ``-1`` at construction, ``-1`` takes the fall-through branch, and - the flag then survived the rebuild in :meth:`__call__` that - resolved the real width. :meth:`pre_process` consequently handed - :obj:`bytes` to a template that had become ``>Q`` -- or ``>I``, - ``>H``, ``>B`` -- and :func:`struct.pack` refused it. See GitHub - issue :issue:`591`. + was built for. Only ever setting it :data:`True` in the + fall-through branch would make it a latch: a callable ``length`` is + a placeholder of ``-1`` at construction, ``-1`` takes the + fall-through branch, and the flag would then survive the rebuild in + :meth:`__call__` that resolves the real width. :meth:`pre_process` + would consequently hand :obj:`bytes` to a template that had become + ``>Q`` -- or ``>I``, ``>H``, ``>B`` -- and :func:`struct.pack` would + refuse it. See GitHub issue :issue:`591`. Assigning it is what tells a placeholder apart from a width that genuinely needs byte packing, without having to remember that a @@ -264,17 +263,16 @@ def pre_process(self, value: 'int', packet: 'dict[str, Any]') -> 'int | bytes': that rebuild can land on a width :func:`struct` has a native integer code for. The flag is therefore consulted **after** the rebuild rather than before it, since deciding first and rebuilding - second is how the template and the value being returned came to - disagree in the first place. C.f. :issue:`591`. + second would let the template and the value being returned + disagree. C.f. :issue:`591`. That width is a **ceiling** of the bit length over eight, and it is - written as one. It used to read - ``math.ceil(value.bit_length() // 8)``, which is not a ceiling at - all: :func:`math.ceil` of an :obj:`int` is that :obj:`int`, so the - ``//`` had already floored the quotient and the outer call did - nothing. Every value whose bit length is not an exact multiple of - eight was therefore sized one octet short -- ``256`` at one octet, - ``65536`` at two, and ``1`` itself at *zero* -- which + written as one. ``math.ceil(value.bit_length() // 8)`` is not a + ceiling at all: :func:`math.ceil` of an :obj:`int` is that + :obj:`int`, so the ``//`` has already floored the quotient and the + outer call does nothing. Every value whose bit length is not an + exact multiple of eight would be sized one octet short -- ``256`` at + one octet, ``65536`` at two, and ``1`` itself at *zero* -- which :meth:`int.to_bytes` and :func:`struct.pack` both refuse. See GitHub issue :issue:`599`. @@ -561,29 +559,29 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Stdli Notes: The registry is consulted through its constructor, which raises for a value no member and no ``_missing_`` rule accounts for. That raise - used to propagate, and it is :mod:`aenum`'s own bare - :exc:`ValueError`: not one of :mod:`pcapkit.utilities.exceptions`, - so a caller cannot tell it from a bug of its own, and not an - :exc:`EOFError`, so :meth:`Extractor.record_frames + is :mod:`aenum`'s own bare :exc:`ValueError`: not one of + :mod:`pcapkit.utilities.exceptions`, so a caller cannot tell it from + a bug of its own, and not an :exc:`EOFError`, so + :meth:`Extractor.record_frames ` does not - catch it. One unassigned code therefore cost the whole extraction. + catch it. Left to propagate, one unassigned code would cost the + whole extraction. - It also made the "unknown" reader the formats require unreachable - for any genuinely unassigned code -- PCAP-NG's + It would also make the "unknown" reader the formats require + unreachable for any genuinely unassigned code -- PCAP-NG's :class:`~pcapkit.protocols.schema.misc.pcapng.UnknownBlock`, and the ``unassigned`` option readers of IPv4, TCP, HOPOPT, MH and HIP -- - because the lookup failed several frames before the dispatch that - would have selected it. PCAP-NG repeats a block's total length at - both ends precisely so that a reader can skip a block type it does - not recognise; that skip is what this fallback restores. See GitHub - issue :issue:`701`. + because the lookup would fail before the dispatch that selects it. + PCAP-NG repeats a block's total length at both ends precisely so + that a reader can skip a block type it does not recognise; that skip + is what this fallback keeps working. See GitHub issue :issue:`701`. The fallback is the same nameless pseudo-member this method already builds for a field carrying no registry at all, so it is a value shape the package already produces and the dump layer already renders -- as ``:: [28]``, through - :func:`~pcapkit.dumpkit.common.render_enum`, not through the - ``name is None`` branch :issue:`648` added, which a member named + :func:`~pcapkit.dumpkit.common.render_enum`, not through its + ``name is None`` branch (:issue:`648`), which a member named ```` never takes -- and one an :class:`int`-keyed dispatch registry looks up by value like any declared member. It is built per value rather than grafted onto @@ -659,7 +657,7 @@ def _unregistered_member(namespace: 'Type[StdlibEnum] | Type[AenumEnum]', names -- ``isinstance`` against it and every ancestor holds, and it renders and dispatches exactly like a declared one -- provided building it never grows the registry, which is the whole reason the - field stopped calling ``get()`` unconditionally in the first place. + field does not call ``get()`` unconditionally. This is what gets there: it calls ``namespace``'s own storage base's ``__new__`` directly -- :class:`str` or :class:`int`, whichever ``namespace`` derives from -- which skips ``namespace``'s *own* @@ -683,12 +681,12 @@ def _unregistered_member(namespace: 'Type[StdlibEnum] | Type[AenumEnum]', The member this returns is absent from ``_value2member_map_``, so a *value*-keyed lookup on it -- ``self._namespace(value)`` -- - still raises exactly as it did before this existed. Nothing on the + still raises. Nothing on the parse or reconstruction path does that to a value it just resolved this way, which is what keeps this safe to return from ``post_process``. A direct call to :meth:`~pcapkit.const.reg.apptype.AppType.get` for the same port - is a different matter and deliberately unchanged: asking the + is a different matter and deliberately so: asking the registry for a name is an explicit request for a named member, so it still mints one -- measured on this tree, ``AppType.get(54321, proto=tcp)`` returns ``PORT_54321_tcp`` and @@ -707,17 +705,15 @@ def _unregistered_member(namespace: 'Type[StdlibEnum] | Type[AenumEnum]', :mod:`pickle` and :func:`copy.copy`/:func:`copy.deepcopy` all reduce an :class:`~enum.Enum` member through ``Enum.__reduce_ex__``, which returns ``(cls, (value,))`` -- the - one lookup this member is deliberately absent from. Left alone - that is a genuine regression rather than a pre-existing - limitation, because the call sites used to *mint*, so the member - was registered and a round-trip worked. Measured on CPython - 3.14.7, resolving port 53406 through - :class:`~pcapkit.protocols.schema.transport.tcp.PortEnumField`: - on ``83b58ebda`` ``pickle.loads(pickle.dumps(member))`` returned - the member, and with the mint removed and nothing in its place it - raised ``ValueError: 'unknown [53406 - tcp]' is not a valid TCP`` - -- while ``pickle.dumps`` still succeeded, so the failure - surfaced only on read-back rather than where it was caused. + one lookup this member is deliberately absent from. A minted + member is registered and so round-trips; left alone, this one + would not. Measured on CPython 3.14.7, resolving port 53406 + through :class:`~pcapkit.protocols.schema.transport.tcp.PortEnumField` + with nothing in place of the override, + ``pickle.loads(pickle.dumps(member))`` raised + ``ValueError: 'unknown [53406 - tcp]' is not a valid TCP`` -- while + ``pickle.dumps`` still succeeded, so the failure surfaced only on + read-back rather than where it was caused. So ``__reduce_ex__`` is set on the member itself, reducing it to :func:`_rebuild_unregistered_member` instead of to a value lookup. diff --git a/pcapkit/corekit/fields/strings.py b/pcapkit/corekit/fields/strings.py index bfed576e1..16856eb14 100644 --- a/pcapkit/corekit/fields/strings.py +++ b/pcapkit/corekit/fields/strings.py @@ -103,13 +103,13 @@ class StringField(_TextField[str]): default: Field default value, if any. encoding: The encoding with which to decode the :obj:`bytes`. If not provided, :mod:`pcapkit` will first try detecting its encoding - using |chardet|_. The fallback encoding would is **UTF-8**. + using |chardet|_. The fallback encoding is **UTF-8**. errors: The error handling scheme to use for the handling of decoding errors. The default is ``'strict'`` meaning that decoding errors raise a :exc:`UnicodeDecodeError`. Other possible values are ``'ignore'`` and ``'replace'`` as well as any other name registered with :func:`codecs.register_error` that can handle :exc:`UnicodeDecodeError`. - unquote: Whether to unquote the decoded string as a URL. Should decoding failed , + unquote: Whether to unquote the decoded string as a URL. Should decoding fail, the method will try again replacing ``'%'`` with ``'\x'`` then decoding the ``url`` as ``'utf-8'`` with ``'replace'`` for error handling. callback: Callback function to be called upon @@ -198,9 +198,9 @@ def __init__(self, length: 'int', # NOTE: A subfield reaching past the end of the field is a mistake in the # schema that declares it, not something a packet can cause, so it is - # rejected here rather than at packing time -- where it used to grow the - # buffer instead, and surface much later as an opaque ``OverflowError`` - # from :meth:`int.to_bytes`. + # rejected here rather than at packing time -- where it would instead grow + # the buffer, and surface much later as an opaque ``OverflowError`` from + # :meth:`int.to_bytes`. width = self.length * 8 for name, (start, size) in self._namespace.items(): if start < 0 or size < 1 or start + size > width: diff --git a/pcapkit/corekit/infoclass.py b/pcapkit/corekit/infoclass.py index 8db3076e6..cfa5010af 100644 --- a/pcapkit/corekit/infoclass.py +++ b/pcapkit/corekit/infoclass.py @@ -35,10 +35,10 @@ class FinalisedState(EnumLookup, enum.IntEnum): """Finalised state. - Re-parented onto :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub issue + Built on :class:`~pcapkit.corekit.enum.EnumLookup` per GitHub issue :issue:`877`'s ruling that every non-registry enumeration shares that - lookup contract -- pure re-parenting, since this class defines neither - ``get`` nor ``_missing_`` of its own to reconcile with the base. + lookup contract; this class defines neither ``get`` nor ``_missing_`` of + its own. """ @@ -108,7 +108,7 @@ def info_final(cls: 'ST', *, _finalised: 'bool' = True) -> 'ST': # reaches here with ``__final__`` already set by a decorator that generated # nothing: a ``__final__`` test would read that as "already finalised", skip # the generation, and hand back precisely the ``__init__``-less class - # :meth:`Info.__new__` now refuses -- while the opposite order worked. Keying + # :meth:`Info.__new__` refuses -- while the opposite order works. Keying # on ``__finalised__`` is what makes the two orders agree. # # ``cls.__dict__`` rather than ``getattr``: ``__finalised__`` is an ordinary @@ -280,9 +280,10 @@ def __new__(cls, name: 'str', bases: 'tuple[type, ...]', attrs: 'dict[str, Any]' class Info(Mapping[str, VT], Generic[VT], metaclass=InfoMeta): """Turn dictionaries into :obj:`object` like instances. - * :class:`Info` objects inherit from :obj:`dict` type - * :class:`Info` objects are *iterable*, and support all functions as - :obj:`dict` type + * :class:`Info` objects are :class:`~collections.abc.Mapping` instances, + not :obj:`dict` subclasses + * :class:`Info` objects are *iterable*, and support the read-only + :obj:`dict` interface * :class:`Info` objects are **immutable**, thus cannot set or delete attributes after initialisation @@ -352,8 +353,8 @@ def __init_subclass__(cls, /, *args: 'Any', **kwargs: 'Any') -> 'None': 'which is final') # NOTE: ``*args`` and ``**kwargs`` are forwarded rather than swallowed, so - # that a class keyword nobody accepts still reaches ``object`` and still - # fails there, as it did before this hook existed. + # that a class keyword nobody accepts still reaches ``object`` and fails + # there. super().__init_subclass__(*args, **kwargs) def __new__(cls, *args: 'VT', **kwargs: 'VT') -> 'Self': # pylint: disable=unused-argument @@ -393,8 +394,8 @@ def __new__(cls, *args: 'VT', **kwargs: 'VT') -> 'Self': # pylint: disable=unus # bare ``@final`` lands -- it cannot see the mistake, and no other # class-creation hook fires later. First instantiation is the next event # in the class's life this library controls, and it is also where the - # damage surfaced: an unfinalised class has no generated ``__init__``, so - # construction fell through to :meth:`__update__` and failed with + # damage surfaces: an unfinalised class has no generated ``__init__``, so + # construction would fall through to :meth:`__update__` and fail with # ``TypeError: 'int' object is not iterable``, naming neither the class # nor the mistake. # @@ -446,7 +447,7 @@ def __update__(self, dict_: 'Optional[Mapping[str, VT] | Iterable[tuple[str, VT] # NOTE: Keys with the same names as the class's builtin methods will be # renamed with the class name prefixed as mangled class variables # implicitly and internally. Such mapping information will be stored - # within: attr: `__map__` attribute. + # within the :attr:`__map__` attribute. __name__ = type(self).__name__ # pylint: disable=redefined-builtin diff --git a/pcapkit/corekit/io.py b/pcapkit/corekit/io.py index eb597da63..2812b2e4d 100644 --- a/pcapkit/corekit/io.py +++ b/pcapkit/corekit/io.py @@ -156,11 +156,10 @@ def _seek_buffer(self) -> 'int': read path needs both of them: * The buffer's own cursor is a **derived** quantity, ``_tell - _buffer_set``, rather - than a fourth piece of state to be maintained. Nothing was maintaining it: a read + than a fourth piece of state to be maintained. Nothing keeps it in step: a read served from the stream writes through :attr:`_buffer_view` and leaves the cursor - untouched, :meth:`_write_buffer` rewinds it to the start of the appended octets - when the window slides, and :meth:`peek` used to advance it while leaving ``_tell`` - alone. Deriving it here, at each point of use, is what makes the drift + untouched, and :meth:`_write_buffer` rewinds it to the start of the appended + octets when the window slides. Deriving it here, at each point of use, is what makes the drift unrepresentable instead of merely repaired afterwards by the next :meth:`seek`. * What is available is the run from the position to the end of the content, ``_buffer_set + _buffer_cur - _tell`` -- not ``_buffer_cur``, which is measured @@ -256,7 +255,7 @@ def readline(self, size: 'int | None' = -1, /) -> 'bytes': # NOTE: a request for no octets is answered without consulting the window, # which has nothing to say about it. Asking anyway refuses a zero-length # read from a position :meth:`_truncate_buffer` has left behind the window -- a - # refusal over data that was never wanted, and not what this used to do. + # refusal over data that was never wanted. # # Truthiness rather than ``size == 0`` on purpose. It is the same test for # every value this method can be reached with -- ``size`` is an ``int`` by @@ -264,7 +263,7 @@ def readline(self, size: 'int | None' = -1, /) -> 'bytes': # not normalise a ``None`` its signature does not permit, and there the two # spellings diverge: ``size == 0`` would send ``None`` on to the window and # report a type error as a position error. Keeping it falsy keeps that call - # failing as the :exc:`TypeError` it always was. + # failing as a :exc:`TypeError`. buf = b'' else: buf_rem = self._seek_buffer() @@ -326,20 +325,21 @@ def seek(self, offset: 'int', whence: 'int' = io.SEEK_SET, /) -> 'int': Note: The target is computed, then validated, and only then written to ``_tell``. A - branch that assigned the position before deciding whether to accept it left a - refused seek having moved it anyway, with the resync below -- the one thing that - puts the buffer's cursor back in step -- skipped on the way out. A caller that - catches the error and reasonably takes the position to be unchanged then read - from the rejected offset instead, silently and without a second error. + branch that assigned the position before deciding whether to accept it would + leave a refused seek having moved it anyway, with the resync below -- the one + thing that puts the buffer's cursor back in step -- skipped on the way out. A + caller that catches the error and reasonably takes the position to be unchanged + would then read from the rejected offset instead, silently and without a second + error. The negative check applies to every ``whence``, rather than only :data:`~io.SEEK_SET` looking at its offset. An absolute position below zero cannot exist under any of them, and it is not the same failure as a position that has merely slid out of the window -- which is ordinary, and recoverable with ``buffer_save=True``. Reporting the first as ``negative seek value`` keeps - the two distinguishable, and being a property of the position rather than of the - window it holds with a saved buffer as well, where the old refusal did not - apply at all and the seek returned a negative position as if it had worked. + the two distinguishable, and, being a property of the position rather than of + the window, it holds with a saved buffer as well, which is exempt from the window + check and would otherwise return a negative position as if the seek had worked. """ # NOTE: we mark the end of buffer content to the end of buffer @@ -386,8 +386,7 @@ def seek(self, offset: 'int', whence: 'int' = io.SEEK_SET, /) -> 'int': else: # NOTE: only a saved buffer reaches here -- the refusal above has already # turned away a position before the window when there is no file to serve it - # from, which is what the refusal used to do at this point instead, after - # ``_tell`` had been moved. + # from. self._buffer.seek(0, io.SEEK_SET) return self._tell @@ -416,10 +415,9 @@ def truncate(self, size: 'int | None' = None, /) -> 'int': ``_pyio.BufferedReader`` raise :exc:`io.UnsupportedOperation`, the latter from ``_BufferedIOMixin.truncate``'s ``_checkWritable()`` (:issue:`645`). - The resizing this used to perform is still reachable internally, as - :meth:`_truncate_buffer`. It never touched the underlying stream in the first place - -- it resizes a private lookback window -- which is why it survives under a private - name rather than being removed with the public method's behaviour. + Resizing the lookback buffer is reachable internally, as :meth:`_truncate_buffer`. + It never touches the underlying stream -- it resizes a private lookback window -- + which is why it stays, under a private name. """ raise UnsupportedOperation('truncate') @@ -431,8 +429,7 @@ def _truncate_buffer(self, size: 'int | None' = None, /) -> 'int': zero-filled. The new buffer size is returned. Note: - This is the internal half of what :meth:`truncate` used to do, which is all of it: - nothing here writes to the underlying stream -- :meth:`write` raises -- so what + Nothing here writes to the underlying stream -- :meth:`write` raises -- so what this resizes is the buffer, not the stream behind it. That is why :meth:`truncate` refuses (:issue:`645`) while this remains: the operation is a private-window one, not an :class:`io.IOBase` write. The buffer is a sliding window over a stream that @@ -459,7 +456,7 @@ def _truncate_buffer(self, size: 'int | None' = None, /) -> 'int': """ if size is None: # NOTE: an unspecified size means the current position, following the - # :meth:`io.IOBase.truncate` convention this used to implement. The buffer + # :meth:`io.IOBase.truncate` convention. The buffer # is indexed relative to ``_buffer_set``, and the position may sit before it # once a saved buffer has been rewound, in which case nothing is kept. size = max(self._tell - self._buffer_set, 0) @@ -491,15 +488,7 @@ def _truncate_buffer(self, size: 'int | None' = None, /) -> 'int': def writable(self) -> 'bool': """Return :obj:`True` if the stream supports writing. If :obj:`False`, :meth:`write` and - :meth:`truncate` will raise :exc:`OSError`. - - Note: - This was spelled ``writeable`` until :issue:`645`, which is not how the :mod:`io` - protocol spells it, so it overrode nothing and :mod:`io` never consulted it -- the - inherited :meth:`io.IOBase.writable` answered instead. Both returned :data:`False`, - so there was no observable divergence to notice; the coincidence is what hid it. - - """ + :meth:`truncate` will raise :exc:`OSError`.""" return False def writelines(self, lines: 'Iterable[Buffer]', /) -> 'None': @@ -671,7 +660,7 @@ def peek(self, size: 'int' = 0) -> 'bytes': # NOTE: read through the view rather than through ``self._buffer``, whose # cursor an ordinary read would advance. A preview must leave the position # alone, and the buffer's cursor is part of the position: advancing it here - # while leaving ``_tell`` untouched is what made the *next* buffered read + # while leaving ``_tell`` untouched would make the *next* buffered read # start from the wrong octet, with ``tell()`` reporting the right one. buf_rem = self._seek_buffer() buf_off = self._buffer.tell() diff --git a/pcapkit/corekit/module.py b/pcapkit/corekit/module.py index 24e9ed71d..31615530a 100644 --- a/pcapkit/corekit/module.py +++ b/pcapkit/corekit/module.py @@ -69,8 +69,8 @@ def klass(self) -> 'Type[_T]': extraction of :file:`many_interfaces.pcapng` performs. :func:`~importlib.import_module` keeps real per-call work for an already-imported module (locks, :class:`~importlib.machinery.ModuleSpec` - checks, the ``fromlist`` walk), so each repeat cost ~436 ns where - this property now costs ~117 ns. + checks, the ``fromlist`` walk), so each repeat costs ~436 ns + through it against ~117 ns through this property. The class is still read off the module with :func:`getattr` on every access, and nothing is memoised here. That is the point: @@ -87,9 +87,8 @@ def klass(self) -> 'Type[_T]': # ``module`` is a ``str`` and the caller never named a class -- # GitHub issue #832. Reporting that plainly, before ``getattr`` # ever sees it, is more useful than letting the sentinel reach - # ``getattr`` and be reported as an absent attribute named - # ``'(null)'``, which is what happened before #833 gave the - # sentinel a type no caller-supplied string can collide with. + # ``getattr``, which rejects a non-``str`` name with a bare + # ``TypeError``. raise ProtocolError(f'missing class name for module {self.module!r}: pass an ' 'explicit class_ argument') @@ -112,10 +111,10 @@ def klass(self) -> 'Type[_T]': try: return getattr(importlib.import_module(self.module), name) except AttributeError as error: - # GitHub issue #832: every ``register_*`` helper that builds a - # descriptor from a bad class name used to fail with this bare - # stdlib :exc:`AttributeError` -- a caller cannot catch that as a - # :mod:`pcapkit` error. Re-raised as :exc:`ProtocolError` naming - # both the module and the class that turned out missing; the - # message text itself is unchanged; only the type is not. + # GitHub issue #832: a ``register_*`` helper that builds a + # descriptor from a bad class name fails here with a bare stdlib + # :exc:`AttributeError`, which a caller cannot catch as a + # :mod:`pcapkit` error. Re-raised as :exc:`ProtocolError`, whose + # message still names both the module and the missing class; only + # the type changes. raise ProtocolError(str(error)) from error diff --git a/pcapkit/corekit/protochain.py b/pcapkit/corekit/protochain.py index 47d567598..3a761f820 100644 --- a/pcapkit/corekit/protochain.py +++ b/pcapkit/corekit/protochain.py @@ -181,7 +181,7 @@ def __repr__(self) -> 'str': return f"ProtoChain({', '.join(map(lambda p: p[1].__name__, self.__data__))})" def __str__(self) -> 'str': - """Returns formatted hex representation of source data stream. + """Returns the protocol aliases of the chain, joined by colons. Example: >>> protochain diff --git a/pcapkit/corekit/sentinels.py b/pcapkit/corekit/sentinels.py index 8d8bd9c21..84fcaafba 100644 --- a/pcapkit/corekit/sentinels.py +++ b/pcapkit/corekit/sentinels.py @@ -12,20 +12,19 @@ :file:`docs/source/contributing/conventions/sentinel-convention.rst` for the house rule the four below follow. -Before this module existed, each of the four lived beside the one class that -used it: :class:`NullType` in :mod:`pcapkit.corekit.module`, -:class:`NoValueType` in :mod:`pcapkit.corekit.fields.field`, -:class:`NoDefaultType` in :mod:`pcapkit.corekit.enum` and -:class:`AbsentType` in :mod:`pcapkit.protocols.protocol`. The owner's ruling -on GitHub issue :issue:`911`, choosing one shared module over one module per -sentinel, moves the four *definitions* here; each original module keeps a three-line -re-export so that no existing ``from import `` breaks, -including the ``if TYPE_CHECKING:``-only imports of the *types* that +The owner's ruling on GitHub issue :issue:`911` chose one shared module over +one module per sentinel. Each sentinel is also re-exported, in a single +import line, from the module of the one class that uses it -- +:class:`NullType` from :mod:`pcapkit.corekit.module`, :class:`NoValueType` +from :mod:`pcapkit.corekit.fields.field`, :class:`NoDefaultType` from +:mod:`pcapkit.corekit.enum` and :class:`AbsentType` from +:mod:`pcapkit.protocols.protocol` -- so that no ``from import `` +breaks, including the ``if TYPE_CHECKING:``-only imports of the *types* that :mod:`pcapkit.foundation.registry.foundation`, :mod:`pcapkit.foundation.registry.protocols` and :mod:`pcapkit.corekit.fields.ipaddress`, :mod:`~pcapkit.corekit.fields.misc`, :mod:`~pcapkit.corekit.fields.numbers` and :mod:`~pcapkit.corekit.fields.strings` -already carry. +carry. ``NO_VALUE`` and ``ABSENT`` differ in how far the ruling reaches. ``NO_VALUE`` is documented as the value of @@ -35,12 +34,9 @@ package. ``ABSENT`` is private to :mod:`pcapkit.protocols.protocol` -- nothing outside that module ever imports it, from here or from there -- so its re-export exists only so that module's own code keeps reading -``ABSENT`` rather than a fully-qualified name; see :class:`AbsentType`'s -own docstring below for why it stays private after the move. GitHub issue -:issue:`937` later dropped the leading underscore both used to carry -(``_Absent``, ``_AbsentType``) in favour of SCREAMING_SNAKE/CamelCase like -their two siblings; the privacy this paragraph describes did not move with the -name -- see :class:`AbsentType`'s docstring for what carries it now. +``ABSENT`` rather than a fully-qualified name. It carries no leading +underscore; :class:`AbsentType`'s docstring says what marks it private +instead. """ from typing import TYPE_CHECKING @@ -59,13 +55,10 @@ class NullType: """Type of :data:`NULL`, the omitted-``class_``/``module`` sentinel. - A distinct class rather than a plain :class:`str` -- which is what the - registry helpers in :mod:`pcapkit.foundation.registry.protocols` and - :mod:`pcapkit.foundation.registry.foundation` used to define, - independently of each other -- so that ``is`` comparisons against it mean - what they say: no :class:`str` a caller passes, including one that - happens to spell ``'(null)'`` itself, can compare equal to this sentinel - by identity. See GitHub issue :issue:`833`. + A distinct class rather than a plain :class:`str` such as ``'(null)'``, + so that ``is`` comparisons against it mean what they say: no + :class:`str` a caller passes, including one that happens to spell + ``'(null)'`` itself, can be this sentinel. See GitHub issue :issue:`833`. Genuinely a singleton, not merely a class this module happens to instantiate once: :meth:`__new__` always hands back the one instance @@ -74,37 +67,25 @@ class NullType: the scenes -- can end up holding a second object that fails an ``is NULL`` check downstream. :meth:`ModuleDescriptor.klass ` makes exactly that - check, and a stricter guard that raises on a second call would be truer - to "singleton" in the abstract, but it would also mean the module's own - ``NULL = NullType()`` below is the only call that is ever allowed to + check; a second instance would slip past it to :func:`getattr` and + downgrade a clean :exc:`~pcapkit.utilities.exceptions.ProtocolError` into + a bare :exc:`TypeError` (``attribute name must be string, not + 'NullType'``). A stricter guard that raises on a second call would be + truer to "singleton" in the abstract, but it would also mean the module's + own ``NULL = NullType()`` below is the only call that is ever allowed to succeed -- fragile for no real benefit, since nothing here needs *rejecting* a second construction, only preventing it from producing a distinct object. - That still leaves :func:`copy.deepcopy`, :func:`copy.copy` and - :mod:`pickle` unhandled: none of them constructs a new instance by - calling ``NullType()`` themselves, so the guard above never runs for - them. Each is therefore given its own override below, rather than left - to fall back to the default behaviour for a plain object: - - * :func:`copy.copy` and :func:`copy.deepcopy` check for - :meth:`__copy__`/:meth:`__deepcopy__` before ever falling back to - reduction, so :meth:`__deepcopy__` in particular has to be defined -- - its absence is the actual defect this class used to have: deepcopying - a :class:`~pcapkit.corekit.module.ModuleDescriptor` recursed into this - sentinel, reduced it, and rebuilt a second, non-identical - :class:`NullType` that then read as an ordinary attribute name to - :func:`getattr`, downgrading a clean - :exc:`~pcapkit.utilities.exceptions.ProtocolError` into a bare - :exc:`TypeError` (``attribute name must be string, not 'NullType'``). - * :mod:`pickle` protocols 2 and up reconstruct through - ``cls.__new__(cls)``, which the guarded :meth:`__new__` already keeps - to one instance -- but protocols 0 and 1 reconstruct through - :func:`copyreg._reconstructor`, which calls :func:`object.__new__` - *directly*, bypassing :meth:`__new__` entirely. :meth:`__reduce__` is - defined so that every protocol, not only the ones that happen to go - through this class's own :meth:`__new__`, is routed through the same - module-level getter instead of through reconstruction at all. + :func:`copy.copy`, :func:`copy.deepcopy` and :mod:`pickle` protocols 2 + and up reconstruct through ``cls.__new__(cls)``, which the guard above + already keeps to one instance. Pickle protocols 0 and 1 do not: they + reconstruct through :func:`copyreg._reconstructor`, which calls + :func:`object.__new__` *directly*, bypassing :meth:`__new__` entirely. + :meth:`__reduce__` is defined so that every protocol is routed through + the same module-level getter instead of through reconstruction at all, + and :meth:`__copy__`/:meth:`__deepcopy__` return ``self`` without + reducing anything. A caveat rather than a defect: :func:`importlib.reload` on this module re-executes ``NULL = NullType()`` below, producing a *second* singleton @@ -116,21 +97,17 @@ class NullType: of problem for the *class* it resolves, which is why it re-reads :data:`sys.modules` on every call rather than memoising; nothing equivalent is possible here, because unlike a resolved class there is no - live registry this sentinel could be re-read from. The pre-:issue:`833` - ``str`` sentinel had the same fragility for the same reason -- it is a - property of sharing one module-level binding across a reload, not - something this class's singleton guarantees claim to solve -- and nothing - in this package reloads :mod:`pcapkit.corekit.sentinels` after import. - - A second caveat, specific to this class now living apart from its one - caller-visible re-export: reloading :mod:`pcapkit.corekit.module` - itself -- rather than this module -- no longer has any effect on the - singleton at all. That module now only re-imports :data:`NULL` and + live registry this sentinel could be re-read from. It is a property of + sharing one module-level binding across a reload, not something this + class's singleton guarantees claim to solve, and nothing in this package + reloads :mod:`pcapkit.corekit.sentinels` after import. + + Reloading :mod:`pcapkit.corekit.module` instead has no effect on the + singleton at all. That module only re-imports :data:`NULL` and :class:`NullType` from here, and re-running an already-satisfied ``from ... import`` reads the current binding in :mod:`sys.modules` rather than re-executing anything, so it neither mints a new instance - nor loses the old one. The reload hazard this docstring describes moved - with the class definition; it did not double. + nor loses the old one. """ @@ -188,11 +165,9 @@ def __reduce__(self) -> 'tuple[Callable[[], NullType], tuple[()]]': #: NullType: Sentinel for an omitted ``class_`` argument to the ``register_*`` #: helpers in :mod:`pcapkit.foundation.registry.protocols` and -#: :mod:`pcapkit.foundation.registry.foundation`. Housed here, alongside the -#: package's other sentinels, rather than in :mod:`pcapkit.corekit.module` -#: where it used to live -- per the owner's ruling on GitHub issue -#: :issue:`911`, see the module docstring above. :mod:`pcapkit.corekit.module` -#: keeps a re-export so every existing +#: :mod:`pcapkit.foundation.registry.foundation`. Housed here with the +#: package's other sentinels, per the module docstring above; +#: :mod:`pcapkit.corekit.module` keeps a re-export so every #: ``from pcapkit.corekit.module import NULL`` keeps working. NULL = NullType() @@ -212,10 +187,10 @@ def _get_null() -> 'NullType': class NoValueType: """Type of :data:`NO_VALUE`, the default value for :mod:`pcapkit.corekit.fields`. - Housed here per GitHub issue :issue:`911` rather than in - :mod:`pcapkit.corekit.fields.field`, where it used to be defined and where + Housed here per GitHub issue :issue:`911`, and re-exported from + :mod:`pcapkit.corekit.fields.field`, where :attr:`FieldBase.default ` - still documents it as the field-default sentinel. + documents it as the field-default sentinel. """ @@ -228,8 +203,6 @@ def __bool__(self) -> 'Literal[False]': #: :attr:`FieldBase.default `. #: :mod:`pcapkit.corekit.fields.field` keeps a re-export, since that #: attribute's own documentation is a published contract naming this object. -#: Renamed from ``NoValue`` to ``NO_VALUE`` by GitHub issue :issue:`937`, -#: which normalised all four sentinel *objects* to SCREAMING_SNAKE. NO_VALUE = NoValueType() @@ -238,35 +211,16 @@ class NoDefaultType: """Type of :data:`NO_DEFAULT`, the omitted-``default`` sentinel for :meth:`EnumLookup.get `. - A dedicated class rather than a bare :class:`object`, per a ruling given in - review of the work for :issue:`857`, which asked for a dedicated class that - follows the house convention. A bare :class:`object` compares under - ``is`` exactly as safely as a dedicated class with no ``__eq__`` of its - own does -- identity comparison was never the problem an earlier - revision's docstring here overstated it to be. What a bare - :class:`object` actually lacks is a readable representation: it prints - as ```` in a signature, in :func:`help`, and in - a traceback, where ``NoDefaultType()`` - -- via :meth:`__repr__` below -- prints as ````. - - Named ``NoDefaultType`` for the *class* because that half of the house - convention is settled: both :class:`NullType` and :class:`NoValueType` - use ``Type``. At the time, the *instance*'s own name was not - similarly settled -- a follow-up given in review of the work for - :issue:`857` was explicit that ``NULL`` (``SCREAMING_CASE``) and - ``NoValue`` (``CapWords``) disagreed, and that the choice should follow - what each sentinel is needed for rather than a settled rule. The need here - was continuity: ``NO_DEFAULT`` was already the name on ``main`` -- - referenced in - :meth:`EnumLookup.get `'s signature, - its docstring, and both comparison sites -- and that change was to *what - the sentinel is*, not to *what it is called*, so it kept that name rather - than being renamed to match either precedent's instance casing for its own - sake. ``NULL``'s ``SCREAMING_CASE`` was the closer match regardless, since - :data:`NO_DEFAULT` was already spelled that way -- and GitHub issue - :issue:`937` later settled the question this paragraph left open: - ``NoValue`` became :data:`NO_VALUE` and ``_Absent`` became :data:`ABSENT`, - so every instance name now agrees on SCREAMING_SNAKE. + A dedicated class rather than a bare :class:`object`, as review of the + work for :issue:`857` asked, following the house convention. A bare + :class:`object` compares under ``is`` exactly as safely as a dedicated + class with no ``__eq__`` of its own does; what it lacks is a readable + representation: it prints as ```` in a signature, + in :func:`help`, and in a traceback, where ``NoDefaultType()`` -- via + :meth:`__repr__` below -- prints as ````. The names follow + the convention every sentinel here shares, settled by GitHub issue + :issue:`937`: ``Type`` for the class and SCREAMING_SNAKE for the + instance. Genuinely a singleton, not merely a class this module happens to instantiate once: :meth:`__new__` always hands back the one instance that @@ -320,76 +274,53 @@ class NoDefaultType: deep-copying either still reduces the sentinel itself through the same, guarded protocol-2 path measured above. - A caveat that turns out to be dormant rather than live, on the current - tree -- worth stating precisely rather than either repeating the older, - inaccurate claim or dropping the topic. :func:`importlib.reload` on this - module re-executes both ``class NoDefaultType:`` and ``NO_DEFAULT = - NoDefaultType()`` below, producing a fresh, distinct object; any consumer - that had already captured the pre-reload one -- as a bound parameter - default, say -- goes on holding the stale one, and a bare + A reload caveat that is dormant rather than live. :func:`importlib.reload` + on this module re-executes both ``class NoDefaultType:`` and + ``NO_DEFAULT = NoDefaultType()`` below, producing a fresh, distinct + object; any consumer that had already captured the pre-reload one -- as a + bound parameter default, say -- goes on holding the stale one, and a bare ``held_default is NO_DEFAULT`` comparison against the post-reload global then reads :data:`False` where it once read :data:`True`. - :meth:`EnumLookup.get ` looked - exactly like such a consumer before GitHub issue :issue:`864`: an - unrecognised, non-``NO_DEFAULT`` value used to fall through to - ``cls(default)``, so a stale sentinel handed to that call could raise a - :exc:`ValueError` a caller had no reason to expect from an *omitted* - argument. :issue:`864` closed a different hole -- ``default`` could mint - a new member -- by replacing that call with a - ``default not in cls._value2member_map_`` guard, and the guard happens to - close this one too: a :class:`NoDefaultType` instance, stale or fresh, is - never a registered enum value, so the guard's ``not in`` half reads - :data:`True` for it either way and ``get`` re-raises the original lookup - error correctly regardless of which :data:`NO_DEFAULT` a caller's stale - default is stale *against*. Measured on the current tree, guard - included:: + :meth:`EnumLookup.get ` is such a + consumer but is not affected: its fallback is guarded by + ``default not in cls._value2member_map_`` (added by GitHub issue + :issue:`864` to stop ``default`` minting a new member), and a + :class:`NoDefaultType` instance, stale or fresh, is never a registered + enum value, so the guard reads :data:`True` for it either way and ``get`` + re-raises the original lookup error regardless of which + :data:`NO_DEFAULT` a caller's default is stale *against*. Measured:: >>> Hardware.get('Definitely-Not-A-Member') # before reload - KeyError: 'Definitely-Not-A-Member' + EnumKeyError: "'Definitely-Not-A-Member' is not a valid Hardware" >>> importlib.reload(pcapkit.corekit.sentinels) >>> Hardware.get('Definitely-Not-A-Member') # after reload - KeyError: 'Definitely-Not-A-Member' - - So the docstring this class carried before GitHub issue :issue:`911`'s - move -- which claimed the second call above raises :exc:`ValueError` -- - was already wrong on ``main`` at ``d31c0aaf6``, independently of the - move: it described the pre-:issue:`864` ``cls(default)`` call, and - nobody had re-verified it against the guard :issue:`864` added - afterwards. Fixed here as a drive-by correction, not a consequence of - the housing change itself. - - None of that makes the underlying hazard theoretical elsewhere in this - package: ``-1`` never had this failure mode at all, since ``-1 == -1`` - compares by value rather than identity, and reload staleness is a - *tracked* defect class here for other constructs -- see + EnumKeyError: "'Definitely-Not-A-Member' is not a valid Hardware" + + The hazard is not theoretical elsewhere: reload staleness is a *tracked* + defect class here for other constructs -- see :meth:`pcapkit.protocols.protocol.ProtocolBase._lookup_next_layer`'s own docstring note on why it declines to memoise a resolved class, and :mod:`tests.protocols.test_dispatch_default_resolution_unit`'s own ``test_no_stale_class_survives_a_module_reload``, which reloads a module - deliberately to pin the fix for exactly that class of bug elsewhere. A - *future* comparison site written the vulnerable way -- a bare ``is - NO_DEFAULT`` with no independent guard behind it, the way :issue:`864`'s - fix itself was not -- would still reproduce it. "Nothing in this package - reloads :mod:`pcapkit.corekit.sentinels` after import" remains true - today, but it is a caveat to keep honest rather than a guarantee this - class enforces. + deliberately to pin the fix for exactly that class of bug. A *future* + comparison site written the vulnerable way -- a bare ``is NO_DEFAULT`` + with no independent guard behind it -- would reproduce it. Nothing in + this package reloads :mod:`pcapkit.corekit.sentinels` after import, but + that is a caveat to keep honest rather than a guarantee this class + enforces. .. note:: - GitHub issue :issue:`911` also changes *which* reload is the one that - matters, independently of the :issue:`864` finding above. - :mod:`pcapkit.corekit.enum` no longer defines ``NoDefaultType`` - itself; it only reads :data:`NO_DEFAULT` off this module once, at its - own import time, into its own module global. Reloading - :mod:`pcapkit.corekit.enum` alone therefore now just re-runs that - read, which -- so long as this module has not *also* been reloaded -- - fetches back the identical object and changes nothing. Producing a - fresh :data:`NO_DEFAULT` at all now takes reloading *this* module, - where the class statement lives; reloading only - :mod:`pcapkit.corekit.enum` is not enough on its own, because ``from - ... import`` binds a copy rather than a live alias, and that module's - own global stays pointed at whatever it read until something re-runs - that import. + Reloading :mod:`pcapkit.corekit.enum` alone does not produce a fresh + :data:`NO_DEFAULT`. That module does not define ``NoDefaultType``; it + reads :data:`NO_DEFAULT` off this module once, at its own import time, + into its own module global, and a reload just re-runs that read, + which -- so long as this module has not *also* been reloaded -- + fetches back the identical object. Producing a fresh one takes + reloading *this* module, where the class statement lives; and since + ``from ... import`` binds a copy rather than a live alias, + :mod:`pcapkit.corekit.enum`'s own global stays pointed at whatever it + read until something re-runs that import. Also unlike both :class:`NullType` and :class:`NoValueType`, this deliberately does *not* define ``__bool__``. Both of those model an @@ -402,9 +333,9 @@ class enforces. exactly the conflation this sentinel exists to rule out: code that writes ``if not default:`` instead of ``if default is NO_DEFAULT:`` would then read :data:`NO_DEFAULT` the same way it reads a caller's genuine falsy - default -- ``0``, ``''``, ``None`` or ``False`` -- which is the exact - collision ``-1`` used to cause under ``==`` and the reason :issue:`857` - exists. Leaving ``__bool__`` undefined makes ``NoDefaultType()`` truthy + default -- ``0``, ``''``, ``None`` or ``False`` -- which is the + collision with a genuine default that :issue:`857` exists to rule out. + Leaving ``__bool__`` undefined makes ``NoDefaultType()`` truthy (the default for any object defining neither ``__bool__`` nor ``__len__``), which at least does not *look* like one of the falsy values it must never be mistaken for. @@ -454,21 +385,17 @@ class AbsentType: and means "no value was given", not "this key is not here". Defined here, alongside the package's other sentinels, per the owner's - ruling on GitHub issue :issue:`911`. Originally named - ``_AbsentType``/``_Absent``, with the leading underscore standing in for - "private" -- GitHub issue :issue:`937` normalised every sentinel *object* - to SCREAMING_SNAKE and dropped it, so this pair now reads as - CamelCase/SCREAMING_SNAKE like their two siblings and privacy is no longer - signalled by the name at all. The owner's ruling on GitHub issue - :issue:`719` accepted that rename, and held that documenting ``ABSENT`` as - a private type and class, not for public use, is enough to replace the - underscore. So this class and :data:`ABSENT` stay exactly as private as - they were: nothing outside :mod:`pcapkit.protocols.protocol` reads - :data:`ABSENT`, from here or from there, and neither this module's nor - that module's :attr:`__all__` names either one. This docstring, and the - "Naming a Sentinel" section of + ruling on GitHub issue :issue:`911`. **Private**, although the names + carry no leading underscore: GitHub issue :issue:`937` put every sentinel + on the CamelCase/SCREAMING_SNAKE convention, and the owner's ruling on + GitHub issue :issue:`719` held that documenting ``ABSENT`` as a private + type and class, not for public use, is enough to replace the underscore. + Nothing outside :mod:`pcapkit.protocols.protocol` reads :data:`ABSENT`, + from here or from there, and neither this module's nor that module's + :attr:`__all__` names either one. This docstring, and the "Naming a + Sentinel" section of :file:`docs/source/contributing/conventions/sentinel-convention.rst`, are - what now records that fact in place of the leading underscore. + what record that. """ @@ -489,7 +416,6 @@ def __repr__(self) -> 'str': #: :attr:`ProtocolBase.__keywords__ #: `. Never leaves #: :mod:`pcapkit.protocols.protocol`, which keeps a private re-export of it -#: for exactly that one read. Private by convention and documentation only, -#: not by a leading underscore, which GitHub issue :issue:`937` dropped -- -#: see :class:`AbsentType`'s own docstring for why. +#: for exactly that one read. Private by documentation rather than by a +#: leading underscore -- see :class:`AbsentType`'s own docstring. ABSENT = AbsentType() diff --git a/pcapkit/dumpkit/common.py b/pcapkit/dumpkit/common.py index 794e509d3..bae86ff19 100644 --- a/pcapkit/dumpkit/common.py +++ b/pcapkit/dumpkit/common.py @@ -118,7 +118,7 @@ def __init_subclass__(cls, /, fmt: 'Optional[str]' = None, rather than having to inherit :class:`DumperBase` to avoid it, and it matches :meth:`EnumSchema.__init_subclass__ `, which - has guarded on its own ``code`` keyword all along. + guards on its own ``code`` keyword the same way. Note: Inferring ``fmt`` from the subclass' @@ -185,8 +185,8 @@ def render_enum(o: 'enum.Enum | aenum.Enum') -> 'str': Note: A :class:`~enum.Flag` value composed **entirely of undeclared bits** has no name at all -- :attr:`~enum.Enum.name` is :data:`None`, not a string -- - so interpolating it unguarded put the literal four characters ``None`` - into the name half and rendered + so interpolating it unguarded would put the literal four characters + ``None`` into the name half and render :class:`~pcapkit.const.tcp.flags.Flags` ``(0)`` as ``'Flags::None [0]'`` (GitHub issue :issue:`648`). @@ -205,8 +205,7 @@ def render_enum(o: 'enum.Enum | aenum.Enum') -> 'str': rendering with no declared bit to precede it, so ``Flags(9)`` becomes ``'Flags::9 [9]'`` and ``Flags(0)`` becomes ``'Flags::0 [0]'``. It also cannot be mistaken for a member name, since a Python identifier may not - begin with a digit -- none of the 1867 identifiers declared under - :mod:`pcapkit.const` is a bare decimal, and none ever can be. + begin with a digit. This is *not* an :mod:`aenum` quirk. A stdlib :class:`enum.IntFlag` built from the same members answers ``name is None`` identically on CPython @@ -268,19 +267,18 @@ def escape_key(key: 'Any') -> 'Any': A non-:class:`str` key is rendered with :func:`format`, which is the very conversion ``'{item}'.format(item=key)`` already applies to it, - so the ```` text is what it always was apart from the escaping. + so the ```` text is unchanged apart from the escaping. Rendering such a key rather than passing it over is deliberate: :file:`examples/captures/test.pcapng` keys the TLS key log entries of its decryption secrets block by a raw :class:`bytes` client random (:meth:`TLSKeyLog.post_process `), and - the ``bytes`` repr of that one carries ``&``, ``<`` *and* ``>``. That - is what made the fixture's ``plist`` report unparseable: - :func:`xml.etree.ElementTree.parse` stopped at the key's ``&`` on - line 1517 of 1958. The same key also breaks the fixture's ``json`` - report, but on the quotes in that repr rather than on these three - characters, so that half is :mod:`dictdumper`'s to fix and is left - exactly as it is. + the ``bytes`` repr of that one carries ``&``, ``<`` *and* ``>``. + Unescaped, that makes the fixture's ``plist`` report unparseable: + :func:`xml.etree.ElementTree.parse` stops at the key's ``&``. The + same key also breaks the fixture's ``json`` report, but on the quotes + in that repr rather than on these three characters, so that half is + :mod:`dictdumper`'s to fix and is left alone. """ if not escape_strings: diff --git a/pcapkit/dumpkit/pcap.py b/pcapkit/dumpkit/pcap.py index 429d93474..1e833ea09 100644 --- a/pcapkit/dumpkit/pcap.py +++ b/pcapkit/dumpkit/pcap.py @@ -43,7 +43,7 @@ #: Truncation mask for those four fields. :class:`~pcapkit.corekit.fields.numbers.UInt32Field`, #: the field class that would otherwise pack them, masks to the field width in #: :meth:`~pcapkit.corekit.fields.numbers.NumberField.pre_process` rather than -#: rejecting an out-of-range value -- a ``ts_sec`` of ``2**32 + 5`` was written as +#: rejecting an out-of-range value -- a ``ts_sec`` of ``2**32 + 5`` is written as #: ``5``. :func:`struct.pack` raises instead, so the mask is applied here to keep #: the two spellings writing the same octets for every input. _UINT32_MASK = 0xFFFF_FFFF @@ -169,22 +169,22 @@ def _append_value(self, value: 'Data_Frame', file: 'IO[bytes]', name: 'str') -> this writes them directly, rather than handing them to :class:`~pcapkit.protocols.misc.pcap.frame.Frame`, whose constructor packs the record and then **dissects it again** through the whole - protocol stack to arrive at bytes it was given. That round trip was - about 82% of the cost of a flow-traced extraction -- ``http.pcap``, + protocol stack to arrive at bytes it was given. That round trip + measured about 82% of the cost of a flow-traced extraction -- ``http.pcap``, 1117 frames, best of 7: 2319 ms with the rebuild against 1263 ms without, over a 1030 ms untraced baseline. - Dropping it is not only cheaper. The re-dissection re-emitted every - parse warning the frame had already produced once, and warned about - payloads it had no business parsing at all -- writing a 3-octet - payload raised ``SchemaWarning: packet length < 0: -3`` from a dumper - that only had to copy it. + Skipping it is not only cheaper. The re-dissection would re-emit + every parse warning the frame had already produced once, and warn + about payloads it has no business parsing at all -- writing a + 3-octet payload would raise ``SchemaWarning: packet length < 0: -3`` + from a dumper that only has to copy it. """ # NOTE: The payload is read before the metadata so that a caller passing a # mapping rather than a dissected frame -- which the flow-tracing adapters - # of several engines do -- still fails naming ``packet``, as the ``Frame`` - # construction did. :mod:`pcapkit.foundation.extraction` substitutes a + # of several engines do -- still fails naming ``packet``. + # :mod:`pcapkit.foundation.extraction` substitutes a # dict-capable trace format on the strength of that error. packet = value.packet frame_info = value.frame_info diff --git a/pcapkit/interface/misc.py b/pcapkit/interface/misc.py index 896eb59d0..a7401c9d9 100644 --- a/pcapkit/interface/misc.py +++ b/pcapkit/interface/misc.py @@ -149,10 +149,10 @@ def follow_tcp_stream(fin: 'Optional[str]' = None, verbose: 'bool' = False, # NOTE: ``Extractor.engine`` returns the running engine *instance* (see # :meth:`Extractor.engine `), - # never its name -- so the historical ``extraction.engine == 'dpkt'`` compared an - # object against a string and was *always* :data:`False`. Every capture then fell - # through to the pcapkit adapter, which crashed on DPKT frames and silently - # returned no streams on Scapy frames (#399). Dispatch on the engine *type* + # never its name -- so ``extraction.engine == 'dpkt'`` would compare an object + # against a string and be *always* :data:`False`, and every capture would fall + # through to the pcapkit adapter, which crashes on DPKT frames and silently + # returns no streams on Scapy frames (#399). Dispatch on the engine *type* # instead, and via :func:`isinstance` so that a third-party engine subclassing a # built-in still reaches the adapter that matches its frames. # diff --git a/pcapkit/toolkit/dpkt.py b/pcapkit/toolkit/dpkt.py index 074c3b2ec..23f81c614 100644 --- a/pcapkit/toolkit/dpkt.py +++ b/pcapkit/toolkit/dpkt.py @@ -142,7 +142,7 @@ def packet2dict(packet: 'Packet', timestamp: 'float', *, """Convert DPKT packet into :obj:`dict`. Args: - packet: Scapy packet. + packet: DPKT packet. timestamp: Timestamp of packet. data_link: Data link type. @@ -175,7 +175,7 @@ def ipv4_reassembly(packet: 'Packet', timestamp: 'float', *, timestamp: Capture timestamp of the packet, which drives the reassembly timeout. `DPKT`_'s reader yields it beside the record's octets rather than on the packet, so it is passed in -- as :func:`tcp_traceflow` - already does. A caller holding only a frame can read it back with + does. A caller holding only a frame can read it back with :func:`packet2timestamp`, which is where :class:`~pcapkit.foundation.engines.dpkt.DPKT` leaves it. count: Packet index. If not provided, default to ``-1``. @@ -266,8 +266,8 @@ def ipv6_reassembly(packet: 'Packet', timestamp: 'float', *, # (:rfc:`8200#section-4.5`), not the IPv6 header's Flow Label. The # label is optional and routinely zero, so keying on it collapses # every datagram between one address pair into a single buffer and - # interleaves their fragments; it also disagreed with ``bufid[2]`` - # in every other engine, which feeds + # interleaves their fragments; it would also disagree with + # ``bufid[2]`` in every other engine, which feeds # :attr:`pcapkit.foundation.reassembly.data.ip.DatagramID.id`. ipv6_frag.id, # identification Enum_TransType.get(ipv6_frag.nxt), # next header field in IPv6 Fragment Header diff --git a/pcapkit/toolkit/pcapng.py b/pcapkit/toolkit/pcapng.py index 9b654a93b..f8f7e8441 100644 --- a/pcapkit/toolkit/pcapng.py +++ b/pcapkit/toolkit/pcapng.py @@ -270,11 +270,9 @@ def block2frame(block: 'Packet', *, nanosecond: 'bool' = False) -> 'Data_Frame': # NOTE: ``len`` is the on-wire length and ``cap_len`` the captured one, # which is the way round Wireshark's ``packet-frame.c`` registers the two # field names this pair borrows. Do not swap them to match some other - # reader: #618 made this the reference and brought the PCAP reader in - # :mod:`pcapkit.protocols.misc.pcap.frame` into line with it, on the - # strength of those registered names rather than of which came first -- - # this spelling is in fact the *later* of the two, and the note at the - # corresponding site over there records the dates. + # reader: this is the reference, and the PCAP reader in + # :mod:`pcapkit.protocols.misc.pcap.frame` follows it (#618), on the + # strength of those registered names. len=block.original_len, cap_len=block.captured_len, ) diff --git a/pcapkit/toolkit/pypcapfile.py b/pcapkit/toolkit/pypcapfile.py index cf39236ac..dd73f5360 100644 --- a/pcapkit/toolkit/pypcapfile.py +++ b/pcapkit/toolkit/pypcapfile.py @@ -110,13 +110,10 @@ def _parse_ipv4_address(value: 'Any') -> 'IPv4Address': (it raises :exc:`~ipaddress.AddressValueError` otherwise). This helper also accepts a packed 4-byte :obj:`bytes` value directly, so a differently-represented `PyPCAPFile`_ fork or release still works. - Anything that is not :obj:`bytes` is rejected outright: an earlier - revision also accepted a plain :obj:`int`, kept only because this - module's own unit test stand-ins modelled the field that way -- - which meant production had been widened specifically to keep a - fixture passing that was hiding this very defect, so the stand-ins - were changed to use dotted-decimal :obj:`bytes` instead and the - ``int`` case was dropped. + Anything that is not :obj:`bytes`, a plain :obj:`int` included, is + rejected outright. The unit-test stand-ins model the field as + dotted-decimal :obj:`bytes`, as the released library does, rather than + this helper being widened to fit a fixture. Raises: ProtocolError: If ``value`` cannot be parsed as an IPv4 address. diff --git a/pcapkit/toolkit/pyshark.py b/pcapkit/toolkit/pyshark.py index 27481c260..d9e2324d6 100644 --- a/pcapkit/toolkit/pyshark.py +++ b/pcapkit/toolkit/pyshark.py @@ -225,19 +225,14 @@ #: that vocabulary is not :class:`LinkType`'s: Ethernet's filter name is ``eth``, never #: ``ETHERNET``. #: -#: Both entries were checked two ways: against Wireshark's dissector registrations -#: (``packet-eth.c`` against ``WTAP_ENCAP_ETHERNET``, ``packet-tr.c`` against -#: ``WTAP_ENCAP_TOKEN_RING``), and live with ``editcap -T`` to confirm which PDML root each -#: encapsulation produces. -#: #: Every entry comes from the same sweep as ``ENCAP_TYPE_TO_LINKTYPE`` above: the root ```` of ``tshark -r out.pcap -c 1 -T pdml`` against the DLT in ``out.pcap``'s own file #: header. The trailing comment on each line names the ``editcap -T`` encapsulation that produced #: it, so an entry can be re-checked, or corrected when upstream moves, by rerunning that one #: rewrite. Names are listed even where they already spell their :class:`LinkType` member #: (``docsis``, ``fddi``, ``pflog``, ...): there is deliberately **no** fallback onto a like-named -#: member, because upper-casing the name is exactly what answered 101 for a ``rawip6`` capture and -#: 0 for a ``DLT_LOOP`` one -- valid DLTs, wrong ones, and silent (:issue:`843`). +#: member, because upper-casing the name is exactly what would answer 101 for a ``rawip6`` capture +#: and 0 for a ``DLT_LOOP`` one -- valid DLTs, wrong ones, and silent (:issue:`843`). #: #: Three classes of name are absent, all three measured rather than assumed: #: @@ -326,7 +321,7 @@ def packet2dict(packet: 'Packet') -> 'dict[str, Any]': """Convert PyShark packet into :obj:`dict`. Args: - packet: Scapy packet. + packet: PyShark packet. Returns: A :obj:`dict` mapping of packet data. @@ -351,10 +346,10 @@ def tcp_traceflow(packet: 'Packet') -> 'TF_TCP_Packet | None': """Trace packet flow for TCP. Args: - packet: Scapy packet. + packet: PyShark packet. Returns: - Tuple[bool, Dict[str, Any]]: A tuple of data for TCP reassembly. + Data for TCP flow tracing. * If the ``packet`` can be used for TCP flow tracing. A packet can be reassembled if it contains TCP layer. @@ -380,7 +375,7 @@ def tcp_traceflow(packet: 'Packet') -> 'TF_TCP_Packet | None': # ``ENCAP_TYPE_TO_LINKTYPE`` (module level, above), and only on the # PDML root layer's *filter* name when the frame layer has no such # field. That name cannot do the job on its own: one filter name - # serves several DLTs, so keying on it answered ``RAW`` (101) for a + # serves several DLTs, so keying on it would answer ``RAW`` (101) for a # ``rawip6`` capture and ``NULL`` (0) for a ``DLT_LOOP`` one -- valid # DLTs, wrong ones, and silent (#843). ``frame.encap_type`` # distinguishes them; see that table's comment for the measurements. @@ -391,8 +386,8 @@ def tcp_traceflow(packet: 'Packet') -> 'TF_TCP_Packet | None': # of ``pyshark.packet.layers.base.BaseLayer.__getattr__``, which is # what the ``getattr`` default absorbs here. # - # Neither path substitutes a DLT on a miss. Since #775 tier 1 a - # lookup raises on an unresolvable key instead of minting one, and + # Neither path substitutes a DLT on a miss. A lookup raises on an + # unresolvable key rather than minting one (#775), and # NULL and RAW are genuine DLTs -- BSD loopback and raw IP framing, # respectively -- each meant to go with its own handler protocol # class, so neither is an honest stand-in for "unknown link type". diff --git a/pcapkit/toolkit/scapy.py b/pcapkit/toolkit/scapy.py index 517d57742..d32c720aa 100644 --- a/pcapkit/toolkit/scapy.py +++ b/pcapkit/toolkit/scapy.py @@ -25,10 +25,11 @@ a side effect, because by the time any of these functions runs the engine has already called ``sniff`` and every frame has already been dissected -- or not. - That distinction is what made :issue:`406` hard to see. Reaching - :func:`ipv6_reassembly` repaired ``conf.l2types`` mid-run, one call too late to - affect the frames being reassembled, so whether a process dissected correctly - depended on what had imported `Scapy`_ earlier. Populating the registries before + That distinction is what makes :issue:`406` hard to see. Reaching + :func:`ipv6_reassembly` populates ``conf.l2types`` mid-run, one call too late to + affect the frames being reassembled, so relying on it would make whether a + process dissects correctly depend on what had imported `Scapy`_ earlier. + Populating the registries before ``sniff`` is :class:`~pcapkit.foundation.engines.scapy.Scapy`'s job, and it does it by importing :mod:`scapy.all` in its constructor. @@ -232,7 +233,7 @@ def ipv6_reassembly(packet: 'Packet', *, count: 'int' = -1) -> 'IP_Packet[IPv6Ad mf=bool(ipv6_frag.m), # more fragment flag # NOTE: ``len(ipv6)`` counts the Fragment header, so it overstates # this by 8 -- and the reassembly machinery writes the payload over - # the span ``tl - ihl``, so those 8 octets became 8 octets of stray + # the span ``tl - ihl``, so using it would leave 8 octets of stray # zeroes in every reassembled datagram. tl=hdr_len + len(payload), # total length, header includes header=bytes(ipv6)[:hdr_len], # raw bytes type header before IPv6-Frag @@ -320,9 +321,9 @@ def tcp_traceflow(packet: 'Packet', *, count: 'int' = -1) -> 'TF_TCP_Packet | No ip = cast('IP', packet['IP']) if 'IP' in packet else cast('IPv6', packet['IPv6']) tcp = cast('TCP', packet['TCP']) - # NOTE: no default here, deliberately. Since #775 tier 1, ``get()`` - # with no default raises on an unresolvable name instead of minting - # one. NULL and RAW are genuine DLTs -- BSD loopback and raw IP + # NOTE: no default here, deliberately. ``get()`` with no default + # raises on an unresolvable name rather than minting one (#775). + # NULL and RAW are genuine DLTs -- BSD loopback and raw IP # framing, respectively -- each meant to go with its own handler # protocol class, so neither is an honest stand-in for "unknown link # type" and this must not paper over the miss with either. An @@ -331,7 +332,7 @@ def tcp_traceflow(packet: 'Packet', *, count: 'int' = -1) -> 'TF_TCP_Packet | No # is not a choice made here: an IPv6-rooted packet's name uppercases to # ``'IPV6'``, which *is* a member (``LinkType.IPV6``, 229), so it # resolves silently -- to a DLT the caller never chose. Only the v4 name - # happens to miss. The bare ``KeyError`` + # happens to miss. The ``KeyError`` # from :meth:`LinkType.get` is caught and re-raised as # :exc:`~pcapkit.utilities.exceptions.MissingKeyError` -- this # package's own house exception for a lookup miss -- rather than @@ -353,10 +354,10 @@ def tcp_traceflow(packet: 'Packet', *, count: 'int' = -1) -> 'TF_TCP_Packet | No dst=ipaddress.ip_address(ip.dst), # destination IP srcport=tcp.sport, # TCP source port dstport=tcp.dport, # TCP destination port - # NOTE: the *capture's* clock, not the host's. This read - # ``time.time()``, which put the moment of parsing into every flow - # label -- so the same capture traced twice produced different label - # strings and different output filenames. Scapy carries the record's + # NOTE: the *capture's* clock, not the host's. ``time.time()`` would + # put the moment of parsing into every flow label -- so the same + # capture traced twice would produce different label strings and + # different output filenames. Scapy carries the record's # own timestamp on ``Packet.time``, which is what every other # engine's adapter reports. timestamp=float(packet.time), # capture timestamp diff --git a/pcapkit/utilities/__init__.py b/pcapkit/utilities/__init__.py index d67b5ce77..bd1169d2a 100644 --- a/pcapkit/utilities/__init__.py +++ b/pcapkit/utilities/__init__.py @@ -6,8 +6,8 @@ .. module:: pcapkit.utilities :mod:`pcapkit.utilities` contains several useful functions -and classes which are fundations of :mod:`pcapkit`, including -decorater function :func:`~pcapkit.utilities.decorators.seekset` +and classes which are foundations of :mod:`pcapkit`, including +decorator function :func:`~pcapkit.utilities.decorators.seekset` and :func:`~pcapkit.utilities.decorators.beholder`, etc., and several user-refined exceptions and warnings. diff --git a/pcapkit/utilities/decorators.py b/pcapkit/utilities/decorators.py index a94b73168..878868df6 100644 --- a/pcapkit/utilities/decorators.py +++ b/pcapkit/utilities/decorators.py @@ -90,11 +90,11 @@ def beholder( Important: This decorator function is designed for decorating *class methods*. - This decorator first keep the current offset of - :attr:`self._file `, then - try to call the decorated function. Should any exception raised, it will - re-parse the :attr:`self._file ` - as :class:`~pcapkit.protocols.misc.raw.Raw` protocol. + This decorator calls the decorated function. Should any exception be + raised, it logs the error and re-parses the current layer's payload as + :class:`~pcapkit.protocols.misc.raw.Raw` -- or as + :class:`~pcapkit.protocols.misc.null.NoPayload`, if the error is an + end-of-file :exc:`~pcapkit.utilities.exceptions.StructError`. Note: The decorated function should have following signature:: @@ -122,7 +122,6 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder': except IndexError: length = None - # record file pointer try: # call method return func(*args, **kwargs) @@ -148,7 +147,7 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder': # ``payload`` field at all. Going through the schema there raises # ProtocolUnbound('unknown field: payload') *from the recovery path*, # turning a next-layer parse failure that should have degraded to - # Raw into a crash. Unreachable until something was registered on an + # Raw into a crash. Reachable once something is registered on an # SCTP payload protocol identifier, which NGAP is. file_ = self._get_payload() @@ -162,7 +161,7 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder': # Measured, because the layers differ and it is easy to state this too # broadly: SCTP's unregistered path keeps its enumeration -- an unknown # PPID gives ``SCTP:Unassigned`` and ``protocol=4243`` -- so - # without this line, *registering* NGAP on PPID 60 would have made a + # without this line, *registering* NGAP on PPID 60 would make a # failed parse report a bare ``SCTP:Raw`` and ``protocol=None``, less # than the same bytes gave while unregistered. TCP's unregistered # path keeps its code too, but as a plain integer: an unknown port @@ -172,8 +171,8 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder': # whether or not it is registered. So both *unknown* paths preserve # the code that arrived; what differs is only that SCTP's is an # enumeration the protochain can name, while TCP's is an ``int`` and - # renders as ``Raw``. That was #418; this line is - # about the *failure* path, which it makes uniform. + # renders as ``Raw`` (#418). This line is about the *failure* path, + # which it makes uniform. next_ = protocol(file_, length, error=str(exc), alias=proto) return cast('R_beholder', next_) return behold @@ -183,7 +182,7 @@ def prepare(func: 'Callable[Concatenate[Type[R_prepare], bytes | IO[bytes], Opti """Prepare schema packet data before unpacking. Important: - This decorate function is designed for decorating the + This decorator function is designed for decorating the :meth:`Schema.unpack ` *class method*. @@ -204,12 +203,9 @@ def prepare(func: 'Callable[Concatenate[Type[R_prepare], bytes | IO[bytes], Opti forwarded to -- the decorated function. :func:`prepare` is applied to exactly one function in this tree, :meth:`Schema.unpack `, - whose real signature has never had more than these four parameters, - and nothing calls it with more; an earlier revision of this note - nonetheless promised implementors a trailing ``*args, **kwargs``, which - the wrapper below never populated. A caller relying on that promise - got extras silently discarded instead of forwarded -- see :issue:`454` -- so the - wrapper raises :exc:`TypeError` for a fifth positional argument or + whose real signature has exactly these four parameters, and nothing + calls it with more. Rather than discard extras silently (:issue:`454`), + the wrapper raises :exc:`TypeError` for a fifth positional argument or an unconsumed keyword, the same as an ordinary call with too many arguments would. @@ -295,10 +291,10 @@ def unpack(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_prepare': # ``post_process`` a *second* time, with a packet context rebuilt from # the schema's own fields and therefore holding none of the enclosing # layer's, so it overwrites exactly the values ``post_process`` derived - # from that context. It is what discarded the IPv6 source address that - # ``pcapkit.protocols.schema.internet.hopopt.MPLOption.post_process`` - # had just resolved: ``OptionField.unpack`` measures each parsed option - # with ``len(data)``, which triggered the re-pack one option later. + # from that context. It would discard, for one, the IPv6 source address + # that ``pcapkit.protocols.schema.internet.hopopt.MPLOption.post_process`` + # has just resolved: ``OptionField.unpack`` measures each parsed option + # with ``len(data)``, which triggers the re-pack one option later. # # ``Schema.pack`` already orders the two the other way round -- clear the # flag *after* ``post_process``, not before -- so match it here and the diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index ceee237da..0e920dc36 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -5,7 +5,7 @@ .. module:: pcapkit.utilities.exceptions :mod:`pcapkit.utilities.exceptions` refined built-in exceptions. -Make it possible to show only user error stack infomation [*]_, +Make it possible to show only user error stack information [*]_, when exception raised on user's operation. .. [*] See |tbtrim|_ project for Pythonic implementation. @@ -86,12 +86,11 @@ def stacklevel() -> 'int': level ``outermost`` the interpreter entry point The frame to name is the first one *past* the boundary, hence - ``boundary + 1``. What makes this a fix rather than a rewrite is that - ``boundary`` is measured from the *inside* out: it depends only on how deep the - :mod:`pcapkit` frames run, never on how deep the caller's own stack is. - Numbering from the outside in, as this function once did, grew with the outer - stack, so the frame it named drifted one further out for every extra frame - above the boundary -- under :program:`pytest`, dozens of them. + ``boundary + 1``. ``boundary`` is measured from the *inside* out: it depends + only on how deep the :mod:`pcapkit` frames run, never on how deep the caller's + own stack is. Numbering from the outside in would grow with the outer stack, + so the frame named would drift one further out for every extra frame above the + boundary -- under :program:`pytest`, dozens of them. Both bounds are enforced, as neither consumer copes with a level outside them: @@ -338,7 +337,7 @@ def _threading_excepthook(args: 'ExceptHookArgs') -> 'None': as closely as the default thread hook's own output allows: a loud :class:`BaseError` prints the ``"Exception in thread ...:"`` header the default hook always prints first -- the one piece of that output - :data:`sys.tracebacklimit` never touched, since it only ever bounded the + :data:`sys.tracebacklimit` does not touch, since it only bounds the traceback -- then its own message tersely, with ``limit=0`` against the real traceback so a chained exception still comes out right. Anything else is handed to ``_previous_threading_excepthook`` -- or @@ -707,11 +706,11 @@ class EnumKeyError(BaseError, KeyError): circumstances, and raise it from this module rather than as a builtin. Deriving from :exc:`KeyError` is what makes that ruling cheap to carry out: - :meth:`~pcapkit.corekit.enum.EnumLookup.get` raised a bare builtin - :exc:`KeyError` on a name miss until :issue:`923`, and six in-library call sites - catch it -- :meth:`~pcapkit.const.http.method.Method.get` catches it in - order to *mint*, so for that one a failed name lookup is part of a - successful call. Every one of them keeps catching, unchanged. + six in-library call sites catch :meth:`~pcapkit.corekit.enum.EnumLookup.get`'s + name miss as :exc:`KeyError` -- :meth:`~pcapkit.const.http.method.Method.get` + catches it in order to build an unregistered member, so for that one a + failed name lookup is part of a successful call -- and every one of them + keeps catching. Note: Distinct from :exc:`~pcapkit.utilities.exceptions.MissingKeyError`,