Summary
MH._make_opt_mn_id accepts int as a documented identifier type, but for every
Enum_MNIDSubtype except IPv6_Address an int identifier produces a
schema that cannot be packed at all — and the declared length it computes is
wrong regardless.
This is the other half of #448. That issue fixed the IPv6_Address subtype, where
the width is now taken from subtype_val; the elif isinstance(identifier, int)
branch still sizes from the identifier's Python type for all the remaining
subtypes, which is the same type-vs-subtype confusion.
Reproduction
Measured on 074ca2c09 (current main). Identical on PR #464's branch, so this
is pre-existing and not a regression from that PR.
from pcapkit.const.mh.option import Option
from pcapkit.const.mh.mn_id_subtype import MNIDSubtype
from pcapkit.protocols.internet.mh import MH
proto = object.__new__(MH)
for st in ('NAI', 'IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address', 'GUTI', 'DUID'):
s = proto._make_opt_mn_id(Option.MN_ID_OPTION_TYPE,
subtype=getattr(MNIDSubtype, st),
identifier=0x1234)
s.pack()
NAI: length=3 pack raises AttributeError: 'int' object has no attribute 'encode'
IMSI: length=3 pack raises struct.error: argument for 's' must be a bytes object
P_TMSI: length=3 pack raises struct.error: argument for 's' must be a bytes object
EUI_48_address: length=3 pack raises struct.error: argument for 's' must be a bytes object
EUI_64_address: length=3 pack raises struct.error: argument for 's' must be a bytes object
GUTI: length=3 pack raises struct.error: argument for 's' must be a bytes object
DUID: length=3 pack raises struct.error: argument for 's' must be a bytes object
Note NAI fails differently from the other six — AttributeError from an
attempted .encode() rather than struct.error — because that subtype's field is
a string rather than raw octets.
The length=3 is itself wrong in every case: it comes from
math.ceil(identifier.bit_length() / 8) + 1, i.e. it describes the integer's own
width rather than the octet count the subtype's field will pack.
Why it matters
int is a documented identifier type, not an accidental one:
pcapkit/protocols/internet/mh.py:7637 — identifier: 'bytes | str | IPv6Address | int' = '::'
pcapkit/protocols/schema/internet/mh.py:727 — the Schema_MNIDOption.__init__
stub carries the same union.
So the signature promises support that does not exist for 7 of the 8 subtypes,
and no test exercises it — which is why #448 was found only for the default
subtype.
Both exception types are also bare stdlib exceptions rather than in-library ones
from pcapkit.utilities.exceptions, the same family as #465, #458 and #438
(closed).
Suggested direction
Decide per subtype what an int identifier should mean, then either convert it to
the subtype's wire form with a width taken from subtype_val (as #448 now does
for IPv6_Address), or reject it with a ProtocolError/FieldValueError naming
the subtype — and narrow the documented type union to match whichever is chosen.
Silently accepting a value that cannot pack is the worst of the three.
Provenance
Surfaced by the review of #464 (the fix for #448), which flagged it as out of
that PR's scope — correctly. I verified it independently on both trees before
filing, and the per-subtype breakdown above is mine: the review reported
struct.error generally, which holds for six of the seven but not for NAI.
Summary
MH._make_opt_mn_idacceptsintas a documented identifier type, but for everyEnum_MNIDSubtypeexceptIPv6_Addressanintidentifier produces aschema that cannot be packed at all — and the declared
lengthit computes iswrong regardless.
This is the other half of #448. That issue fixed the
IPv6_Addresssubtype, wherethe width is now taken from
subtype_val; theelif isinstance(identifier, int)branch still sizes from the identifier's Python type for all the remaining
subtypes, which is the same type-vs-subtype confusion.
Reproduction
Measured on
074ca2c09(currentmain). Identical on PR #464's branch, so thisis pre-existing and not a regression from that PR.
Note
NAIfails differently from the other six —AttributeErrorfrom anattempted
.encode()rather thanstruct.error— because that subtype's field isa string rather than raw octets.
The
length=3is itself wrong in every case: it comes frommath.ceil(identifier.bit_length() / 8) + 1, i.e. it describes the integer's ownwidth rather than the octet count the subtype's field will pack.
Why it matters
intis a documented identifier type, not an accidental one:pcapkit/protocols/internet/mh.py:7637—identifier: 'bytes | str | IPv6Address | int' = '::'pcapkit/protocols/schema/internet/mh.py:727— theSchema_MNIDOption.__init__stub carries the same union.
So the signature promises support that does not exist for 7 of the 8 subtypes,
and no test exercises it — which is why #448 was found only for the default
subtype.
Both exception types are also bare stdlib exceptions rather than in-library ones
from
pcapkit.utilities.exceptions, the same family as #465, #458 and #438(closed).
Suggested direction
Decide per subtype what an
intidentifier should mean, then either convert it tothe subtype's wire form with a width taken from
subtype_val(as #448 now doesfor
IPv6_Address), or reject it with aProtocolError/FieldValueErrornamingthe subtype — and narrow the documented type union to match whichever is chosen.
Silently accepting a value that cannot pack is the worst of the three.
Provenance
Surfaced by the review of #464 (the fix for #448), which flagged it as out of
that PR's scope — correctly. I verified it independently on both trees before
filing, and the per-subtype breakdown above is mine: the review reported
struct.errorgenerally, which holds for six of the seven but not forNAI.