From d2d2f0092f95ef53f01a9bc23e38a420800b8724 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Tue, 26 Aug 2025 19:52:33 -0700 Subject: [PATCH] [ipv4] smaller improvements in `Address`, `Cidr`, and `Header` (#11863) This commit introduces several cleanups and smaller improvements to the IPv4 types. - Optimizes `Cidr::ToString(StringWriter&)` by writing the address directly to the provided writer, avoiding the allocation of a temporary `String` object. - Moves the IPv4 header field offset constants from the public `ip4_types.hpp` header into the unit test file, as this was their only place of use. This cleans up the `Ip4::Header` public API. - Replaces hardcoded values for address and string sizes with the corresponding `OT_IP4_*` definitions for consistency. - Corrects the format specifier in `Address::ToString()` from `%d` to `%u` to properly print unsigned octet values. --- src/core/net/ip4_types.cpp | 5 +++-- src/core/net/ip4_types.hpp | 23 +++++++---------------- tests/unit/test_ip4_header.cpp | 31 +++++++++++++++++++++---------- 3 files changed, 31 insertions(+), 28 deletions(-) diff --git a/src/core/net/ip4_types.cpp b/src/core/net/ip4_types.cpp index 63811d289..4755e790a 100644 --- a/src/core/net/ip4_types.cpp +++ b/src/core/net/ip4_types.cpp @@ -125,7 +125,7 @@ void Address::SynthesizeFromCidrAndHost(const Cidr &aCidr, const uint32_t aHost) void Address::ToString(StringWriter &aWriter) const { - aWriter.Append("%d.%d.%d.%d", mFields.m8[0], mFields.m8[1], mFields.m8[2], mFields.m8[3]); + aWriter.Append("%u.%u.%u.%u", mFields.m8[0], mFields.m8[1], mFields.m8[2], mFields.m8[3]); } void Address::ToString(char *aBuffer, uint16_t aSize) const @@ -169,7 +169,8 @@ exit: void Cidr::ToString(StringWriter &aWriter) const { - aWriter.Append("%s/%d", AsCoreType(&mAddress).ToString().AsCString(), mLength); + AsCoreType(&mAddress).ToString(aWriter); + aWriter.Append("/%u", mLength); } void Cidr::ToString(char *aBuffer, uint16_t aSize) const diff --git a/src/core/net/ip4_types.hpp b/src/core/net/ip4_types.hpp index fd1288adf..1b8f79aa7 100644 --- a/src/core/net/ip4_types.hpp +++ b/src/core/net/ip4_types.hpp @@ -91,14 +91,16 @@ class Cidr; OT_TOOL_PACKED_BEGIN class Address : public otIp4Address, public Equatable
, public Clearable
{ + friend class Cidr; + public: - static constexpr uint16_t kSize = 4; ///< Size of an IPv4 Address (in bytes). - static constexpr uint16_t kAddressStringSize = 17; ///< String size used by `ToString()`. + static constexpr uint16_t kSize = OT_IP4_ADDRESS_SIZE; ///< Size of an IPv4 Address (in bytes). + static constexpr uint16_t kInfoStringSize = OT_IP4_ADDRESS_STRING_SIZE; ///< String size used by `ToString()`. /** * Defines the fixed-length `String` object returned from `ToString()`. */ - typedef String InfoString; + typedef String InfoString; /** * Gets the IPv4 address as a pointer to a byte array. @@ -192,12 +194,12 @@ class Cidr : public otIp4Cidr, public Unequatable, public Clearable InfoString; + typedef String InfoString; /** * Converts the IPv4 CIDR string to binary. @@ -281,17 +283,6 @@ OT_TOOL_PACKED_BEGIN class Header : public Clearable
{ public: - static constexpr uint8_t kVersionIhlOffset = 0; - static constexpr uint8_t kTrafficClassOffset = 1; - static constexpr uint8_t kTotalLengthOffset = 2; - static constexpr uint8_t kIdentificationOffset = 4; - static constexpr uint8_t kFlagsFragmentOffset = 6; - static constexpr uint8_t kTtlOffset = 8; - static constexpr uint8_t kProtocolOffset = 9; - static constexpr uint8_t kHeaderChecksumOffset = 10; - static constexpr uint8_t kSourceAddressOffset = 12; - static constexpr uint8_t kDestinationAddressOffset = 16; - /** * Indicates whether or not the header appears to be well-formed. * diff --git a/tests/unit/test_ip4_header.cpp b/tests/unit/test_ip4_header.cpp index 7d9eec6de..b7fcaae72 100644 --- a/tests/unit/test_ip4_header.cpp +++ b/tests/unit/test_ip4_header.cpp @@ -47,6 +47,17 @@ void VerifyEcnDscp(const Header &aHeader, uint8_t aDscp, Ecn aEcn) void TestIp4Header(void) { + static constexpr uint8_t kHeaderVersionIhlOffset = 0; + static constexpr uint8_t kHeaderTrafficClassOffset = 1; + static constexpr uint8_t kHeaderTotalLengthOffset = 2; + static constexpr uint8_t kHeaderIdentificationOffset = 4; + static constexpr uint8_t kHeaderFlagsFragmentOffset = 6; + static constexpr uint8_t kHeaderTtlOffset = 8; + static constexpr uint8_t kHeaderProtocolOffset = 9; + static constexpr uint8_t kHeaderHeaderChecksumOffset = 10; + static constexpr uint8_t kHeaderSourceAddressOffset = 12; + static constexpr uint8_t kHeaderDestinationAddressOffset = 16; + static constexpr uint16_t kTotalLength = 84; static constexpr uint8_t kTtl = 64; @@ -89,13 +100,13 @@ void TestIp4Header(void) // Verify the offsets to different fields. - VerifyOrQuit(BigEndian::ReadUint16(headerBytes + Header::kTotalLengthOffset) == kTotalLength, + VerifyOrQuit(BigEndian::ReadUint16(headerBytes + kHeaderTotalLengthOffset) == kTotalLength, "kTotalLength is incorrect"); - VerifyOrQuit(headerBytes[Header::kProtocolOffset] == kProtoIcmp, "kProtocol is incorrect"); - VerifyOrQuit(headerBytes[Header::kTtlOffset] == kTtl, "kTtl is incorrect"); - VerifyOrQuit(memcmp(&headerBytes[Header::kSourceAddressOffset], &source, sizeof(source)) == 0, + VerifyOrQuit(headerBytes[kHeaderProtocolOffset] == kProtoIcmp, "kProtocol is incorrect"); + VerifyOrQuit(headerBytes[kHeaderTtlOffset] == kTtl, "kTtl is incorrect"); + VerifyOrQuit(memcmp(&headerBytes[kHeaderSourceAddressOffset], &source, sizeof(source)) == 0, "Source address is incorrect"); - VerifyOrQuit(memcmp(&headerBytes[Header::kDestinationAddressOffset], &destination, sizeof(destination)) == 0, + VerifyOrQuit(memcmp(&headerBytes[kHeaderDestinationAddressOffset], &destination, sizeof(destination)) == 0, "Destination address is incorrect"); for (uint8_t dscp : kDscps) @@ -111,14 +122,14 @@ void TestIp4Header(void) memcpy(&header, kExampleIp4Header, sizeof(header)); VerifyOrQuit(header.IsValid()); - VerifyOrQuit(memcmp(&headerBytes[Header::kSourceAddressOffset], &source, sizeof(source)) == 0, + VerifyOrQuit(memcmp(&headerBytes[kHeaderSourceAddressOffset], &source, sizeof(source)) == 0, "Source address is incorrect"); - VerifyOrQuit(memcmp(&headerBytes[Header::kDestinationAddressOffset], &destination, sizeof(destination)) == 0, + VerifyOrQuit(memcmp(&headerBytes[kHeaderDestinationAddressOffset], &destination, sizeof(destination)) == 0, "Destination address is incorrect"); - VerifyOrQuit(BigEndian::ReadUint16(headerBytes + Header::kTotalLengthOffset) == kTotalLength, + VerifyOrQuit(BigEndian::ReadUint16(headerBytes + kHeaderTotalLengthOffset) == kTotalLength, "kTotalLength is incorrect"); - VerifyOrQuit(headerBytes[Header::kProtocolOffset] == kProtoIcmp, "kProtocol is incorrect"); - VerifyOrQuit(headerBytes[Header::kTtlOffset] == kTtl, "kTtl is incorrect"); + VerifyOrQuit(headerBytes[kHeaderProtocolOffset] == kProtoIcmp, "kProtocol is incorrect"); + VerifyOrQuit(headerBytes[kHeaderTtlOffset] == kTtl, "kTtl is incorrect"); } } // namespace Ip4