diff --git a/docs/source/contributing/conventions/protocol-family-grouping.rst b/docs/source/contributing/conventions/protocol-family-grouping.rst index 0f82014671..d9efa64218 100644 --- a/docs/source/contributing/conventions/protocol-family-grouping.rst +++ b/docs/source/contributing/conventions/protocol-family-grouping.rst @@ -65,7 +65,11 @@ Member Shapes * - Narrow to self - the member's own name only - :class:`~pcapkit.protocols.internet.ah.AH` ``('AH',)``, - :class:`~pcapkit.protocols.internet.esp.ESP` ``('ESP',)`` + :class:`~pcapkit.protocols.internet.esp.ESP` ``('ESP',)``; + :class:`IGMPv2 ` ``('IGMPv2',)`` -- + :class:`IGMPv1 ` narrows the same way, + but to the RFC canonical name rather than a versioned one + (:ref:`protocol-family-canonical-names`) * - Family-inclusive - the member's label plus the family name - :class:`~pcapkit.protocols.link.c_tag.C_Tag` ``('C_Tag', 'VLAN')``, @@ -108,11 +112,22 @@ preference on :pr:`2444`, not a settled rule. ``name`` of their own, so they report ``Hypertext Transfer Protocol``, which carries no version. -**ICMPv4 is the case where the RFC's own name carries no version.** The owner's -reply on :pr:`2446` proposes that ``ICMPv4`` keeps its versioned class and module name, -that its ``id()`` should probably drop the version for the RFC canonical name, and -that its ``name`` carries no "version 4", following RFC 792. It is a proposal, not a -ruling. +**ICMPv4 is the case where the RFC's own name carries no version.** The ruling on +:pr:`2446`: ``ICMPv4`` keeps its versioned class and module name, but its ``id()`` +drops the version for the RFC canonical name (:rfc:`792` names it plain ``ICMP``), +and its ``name`` carries no "version 4". ``ICMPv6`` keeps its own version, since +:rfc:`4443` names it with one. + +**IGMPv1 follows the same shape, by the same reasoning** (:issue:`2439`, :pr:`2506`). +:rfc:`1112` names IGMPv1's protocol plain ``IGMP``, so +:meth:`IGMPv1.id() ` is ``('IGMP',)`` and +its ``alias`` is ``'IGMP'``, while +:meth:`IGMPv2.id() ` keeps its own version, +``('IGMPv2',)``, since :rfc:`2236` names it with one. The family base's ``id()`` +follows: :meth:`IGMP.id() ` is +``('IGMP', 'IGMPv2')``, the family-inclusive shape, because IGMPv1's canonical name +and the family's own name are the same string -- the same shape +:meth:`ICMP.id() ` takes, ``('ICMP', 'ICMPv6')``. .. _protocol-family-selectors: diff --git a/pcapkit/protocols/internet/igmp.py b/pcapkit/protocols/internet/igmp.py index ca3d4eb60a..3e48e11dc6 100644 --- a/pcapkit/protocols/internet/igmp.py +++ b/pcapkit/protocols/internet/igmp.py @@ -92,6 +92,11 @@ def length(self) -> 'Literal[8]': def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv1"], Literal["IGMPv2"], Literal["IGMPv3"]]': """Index ID of the protocol. + :rfc:`1112` names the protocol ``IGMP``, without a version, the same + shape :class:`~pcapkit.protocols.internet.icmp.ICMP` takes for + :rfc:`792` -- so the family base's own name doubles as IGMPv1's + canonical name. :rfc:`2236` names IGMPv2 with its own version. + Returns: Index ID of the protocol, the family name and all three versions; the concrete IGMPv1, IGMPv2 and IGMPv3 classes narrow @@ -120,7 +125,7 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> '_PT': schema = cast('Schema_IGMP', self.__header__) if self._classify(schema.type, schema.max_resp_time, length) != self.version: - raise ProtocolError(f'{self.alias}: {schema.type!r} is not an {self.alias} ' + raise ProtocolError(f'{self.alias}: {schema.type!r} is not an IGMPv{self.version} ' f'message ({length} octets)') igmp = Data_IGMP( diff --git a/pcapkit/protocols/internet/igmpv1.py b/pcapkit/protocols/internet/igmpv1.py index 0ed7630223..86c33391d2 100644 --- a/pcapkit/protocols/internet/igmpv1.py +++ b/pcapkit/protocols/internet/igmpv1.py @@ -38,16 +38,26 @@ def version(self) -> 'Literal[1]': """IGMP version this class reads: ``1``.""" return 1 + @property + def alias(self) -> 'Literal["IGMP"]': + """Acronym of corresponding protocol.""" + return 'IGMP' + ########################################################################## # Methods. ########################################################################## @classmethod - def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv1"]]': # type: ignore[override] + def id(cls) -> 'tuple[Literal["IGMP"]]': # type: ignore[override] """Index ID of the protocol. + :rfc:`1112` names the protocol ``IGMP``, without a version. + + String ``==`` compares against ``(cls, *id())``, so ``== 'IGMPv1'`` is + :data:`False`, while ``'IGMPv1' in`` matches by class. + Returns: - Index ID of the protocol, the family name and the version. + Index ID of the protocol. """ - return ('IGMP', 'IGMPv1') + return ('IGMP',) diff --git a/pcapkit/protocols/internet/igmpv2.py b/pcapkit/protocols/internet/igmpv2.py index 5a07e5cfdb..e8ec7658c3 100644 --- a/pcapkit/protocols/internet/igmpv2.py +++ b/pcapkit/protocols/internet/igmpv2.py @@ -38,16 +38,23 @@ def version(self) -> 'Literal[2]': """IGMP version this class reads: ``2``.""" return 2 + @property + def alias(self) -> 'Literal["IGMPv2"]': + """Acronym of corresponding protocol.""" + return 'IGMPv2' + ########################################################################## # Methods. ########################################################################## @classmethod - def id(cls) -> 'tuple[Literal["IGMP"], Literal["IGMPv2"]]': # type: ignore[override] + def id(cls) -> 'tuple[Literal["IGMPv2"]]': # type: ignore[override] """Index ID of the protocol. + :rfc:`2236` names the protocol ``IGMPv2``, carrying its own version. + Returns: - Index ID of the protocol, the family name and the version. + Index ID of the protocol. """ - return ('IGMP', 'IGMPv2') + return ('IGMPv2',) diff --git a/tests/protocols/internet/test_igmp_unit.py b/tests/protocols/internet/test_igmp_unit.py index 9bf74ae098..f9d411a383 100644 --- a/tests/protocols/internet/test_igmp_unit.py +++ b/tests/protocols/internet/test_igmp_unit.py @@ -112,6 +112,16 @@ def igmp_class(version: 'str'): return {'IGMPv1': igmpv1.IGMP, 'IGMPv2': igmpv2.IGMP, 'IGMPv3': igmpv3.IGMP}[version] +def expected_alias(version: 'str') -> 'str': + """The alias a parsed layer reports for ``version``. + + :rfc:`1112` names the protocol ``IGMP``, without a version, so IGMPv1's + alias is the bare family name; :rfc:`2236` names IGMPv2 with its own + version, so IGMPv2's alias carries it. + """ + return 'IGMP' if version == 'IGMPv1' else version + + #: One fixture per message type IGMPv1 and IGMPv2 define, each a correctly #: checksummed 8-octet message, with the version it is read as. Type octets #: per :rfc:`1112#appendix-I` (``0x11``/``0x12``, v1) and :rfc:`2236#section-2.1` @@ -323,7 +333,7 @@ def test_alias_names_the_version_read(self) -> None: with self.subTest(packet=name): parsed = igmp_class(version)(io.BytesIO(packet), len(packet)) self.assertIsInstance(parsed, IGMPBase) - self.assertEqual(parsed.alias, version) + self.assertEqual(parsed.alias, expected_alias(version)) def test_refusals_name_the_version_refused(self) -> None: from pcapkit.protocols.internet.igmpv1 import IGMP as IGMPv1 @@ -333,7 +343,7 @@ def test_refusals_name_the_version_refused(self) -> None: v1_report, v2_report = IGMP_PACKETS['v1-report'][0], IGMP_PACKETS['v2-report'][0] v2_query = IGMP_PACKETS['v2-query-general'][0] - with self.assertRaisesRegex(ProtocolError, r'^IGMPv1: .+ is not an IGMPv1 message \(8 octets\)'): + with self.assertRaisesRegex(ProtocolError, r'^IGMP: .+ is not an IGMPv1 message \(8 octets\)'): IGMPv1(io.BytesIO(v2_report), len(v2_report)) with self.assertRaisesRegex(ProtocolError, r'^IGMPv2: .+ is not an IGMPv2 message \(8 octets\)'): IGMPv2(io.BytesIO(v1_report), len(v1_report)) @@ -350,7 +360,7 @@ def test_membership_query_max_resp_time_zero_is_v1(self) -> None: packet = igmp_packet(0x11, 0, '0.0.0.0') parsed = IGMP(io.BytesIO(packet), len(packet)) - self.assertEqual(parsed.alias, 'IGMPv1') + self.assertEqual(parsed.alias, 'IGMP') def test_membership_query_max_resp_time_nonzero_is_v2(self) -> None: from pcapkit.protocols.internet.igmpv2 import IGMP @@ -412,8 +422,8 @@ def test_id_and_version(self) -> None: from pcapkit.protocols.internet.igmpv2 import IGMP as IGMPv2 from pcapkit.protocols.internet.igmpv3 import IGMP as IGMPv3 - self.assertEqual(IGMPv1.id(), ('IGMP', 'IGMPv1')) - self.assertEqual(IGMPv2.id(), ('IGMP', 'IGMPv2')) + self.assertEqual(IGMPv1.id(), ('IGMP',)) + self.assertEqual(IGMPv2.id(), ('IGMPv2',)) self.assertEqual(IGMPv3.id(), ('IGMP', 'IGMPv3')) self.assertEqual(object.__new__(IGMPv1).version, 1) self.assertEqual(object.__new__(IGMPv2).version, 2) @@ -426,8 +436,8 @@ def test_base_id_covers_all_three_versions(self) -> None: def test_each_version_narrows_the_base_id(self) -> None: v1, v2, v3 = igmp_class('IGMPv1'), igmp_class('IGMPv2'), igmp_class('IGMPv3') - self.assertEqual(v1.id(), ('IGMP', 'IGMPv1')) - self.assertEqual(v2.id(), ('IGMP', 'IGMPv2')) + self.assertEqual(v1.id(), ('IGMP',)) + self.assertEqual(v2.id(), ('IGMPv2',)) self.assertEqual(v3.id(), ('IGMP', 'IGMPv3')) self.assertNotIn('IGMPv2', v1.id()) self.assertNotIn('IGMPv3', v1.id()) @@ -443,7 +453,7 @@ def test_igmpv1_parses_its_own_messages_directly(self) -> None: packet, _version = IGMP_PACKETS[name] with self.subTest(packet=name): parsed = IGMPv1(io.BytesIO(packet), len(packet)) - self.assertEqual(parsed.alias, 'IGMPv1') + self.assertEqual(parsed.alias, 'IGMP') def test_igmpv1_rejects_a_v2_only_message(self) -> None: from pcapkit.protocols.internet.igmpv1 import IGMP as IGMPv1 @@ -552,7 +562,7 @@ def test_versions_take_the_length_from_the_data_when_none_is_given(self) -> None packet, version = IGMP_PACKETS[name] with self.subTest(version=version): parsed = cls(packet) - self.assertEqual(parsed.alias, version) + self.assertEqual(parsed.alias, expected_alias(version)) self.assertEqual(len(parsed), 8) with warnings.catch_warnings(): @@ -681,7 +691,7 @@ def test_max_resp_time_bounds_round_trip(self) -> None: parsed = cls(io.BytesIO(packet), len(packet)) self.assertEqual(packet[1], value) self.assertEqual(parsed.info.max_resp_time, value) - self.assertEqual(parsed.alias, version) + self.assertEqual(parsed.alias, expected_alias(version)) self.assertEqual(cls.from_data(parsed.info).data, packet) @@ -1126,7 +1136,7 @@ def test_ipv4_dispatches_igmp_to_each_version(self) -> None: warnings.simplefilter('ignore') parsed = IPv4(io.BytesIO(outer), len(outer)) self.assertIsInstance(parsed.payload, expected_classes[version]) - self.assertEqual(parsed.payload.alias, version) + self.assertEqual(parsed.payload.alias, expected_alias(version)) if __name__ == '__main__':