From a32e1f752e9594f2157ccfe672b7d429f3e63f62 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 12 Jan 2026 14:43:20 -0800 Subject: [PATCH] [meshcop] simplify Discovery Request or Response TLV processing (#12279) This change simplifies `DiscoveryRequestTlv` and `DiscoveryResponseTlv` generation and processing. New types `DiscoveryRequestTlvValue` and `DiscoveryResponseTlvValue` are introduced to represent the value (payload) of these TLVs. This s simplifies the call sites by using the generic `Tlv::Append()` and `Tlv::Read()`. --- src/core/meshcop/meshcop_tlvs.hpp | 106 +++++++++------------------ src/core/thread/discover_scanner.cpp | 46 +++++++----- src/core/thread/mle_ftd.cpp | 51 ++++++------- 3 files changed, 83 insertions(+), 120 deletions(-) diff --git a/src/core/meshcop/meshcop_tlvs.hpp b/src/core/meshcop/meshcop_tlvs.hpp index edff0b87c..6ea2f845a 100644 --- a/src/core/meshcop/meshcop_tlvs.hpp +++ b/src/core/meshcop/meshcop_tlvs.hpp @@ -973,44 +973,25 @@ private: typedef StringTlvInfo ThreadDomainNameTlv; /** - * Implements Discovery Request TLV generation and parsing. + * Implements Discovery Request TLV value format. */ OT_TOOL_PACKED_BEGIN -class DiscoveryRequestTlv : public Tlv, public TlvInfo +class DiscoveryRequestTlvValue : public Clearable { public: - /** - * Initializes the TLV. - */ - void Init(void) - { - SetType(kDiscoveryRequest); - SetLength(sizeof(*this) - sizeof(Tlv)); - mFlags = 0; - mReserved = 0; - } - - /** - * Indicates whether or not the TLV appears to be well-formed. - * - * @retval TRUE If the TLV appears to be well-formed. - * @retval FALSE If the TLV does not appear to be well-formed. - */ - bool IsValid(void) const { return GetLength() >= sizeof(*this) - sizeof(Tlv); } - /** * Returns the Version value. * * @returns The Version value. */ - uint8_t GetVersion(void) const { return mFlags >> kVersionOffset; } + uint8_t GetVersion(void) const { return ReadBits(mFlags[0]); } /** * Sets the Version value. * * @param[in] aVersion The Version value. */ - void SetVersion(uint8_t aVersion) { WriteBits(mFlags, aVersion); } + void SetVersion(uint8_t aVersion) { WriteBits(mFlags[0], aVersion); } /** * Indicates whether or not the Joiner flag is set. @@ -1018,64 +999,46 @@ public: * @retval TRUE If the Joiner flag is set. * @retval FALSE If the Joiner flag is not set. */ - bool IsJoiner(void) const { return (mFlags & kJoinerMask) != 0; } + bool GetJoinerFlag(void) const { return GetBit(mFlags[0], kJoinerFlagOffset); } /** * Sets the Joiner flag. - * - * @param[in] aJoiner TRUE if set, FALSE otherwise. */ - void SetJoiner(bool aJoiner) { WriteBit(mFlags, kJoinerOffset, aJoiner); } + void SetJoinerFlag(void) { SetBit(mFlags[0], kJoinerFlagOffset); } private: - static constexpr uint8_t kVersionOffset = 4; - static constexpr uint8_t kVersionMask = 0xf << kVersionOffset; - static constexpr uint8_t kJoinerOffset = 3; - static constexpr uint8_t kJoinerMask = 1 << kJoinerOffset; + static constexpr uint8_t kVersionOffset = 4; + static constexpr uint8_t kVersionMask = 0xf << kVersionOffset; + static constexpr uint8_t kJoinerFlagOffset = 3; - uint8_t mFlags; - uint8_t mReserved; + uint8_t mFlags[2]; } OT_TOOL_PACKED_END; /** - * Implements Discovery Response TLV generation and parsing. + * Defines Discovery Request TLV constants and types. + */ +typedef SimpleTlvInfo DiscoveryRequestTlv; + +/** + * Implements Discovery Response TLV value format. */ OT_TOOL_PACKED_BEGIN -class DiscoveryResponseTlv : public Tlv, public TlvInfo +class DiscoveryResponseTlvValue : public Clearable { public: - /** - * Initializes the TLV. - */ - void Init(void) - { - SetType(kDiscoveryResponse); - SetLength(sizeof(*this) - sizeof(Tlv)); - mFlags = 0; - mReserved = 0; - } - - /** - * Indicates whether or not the TLV appears to be well-formed. - * - * @retval TRUE If the TLV appears to be well-formed. - * @retval FALSE If the TLV does not appear to be well-formed. - */ - bool IsValid(void) const { return GetLength() >= sizeof(*this) - sizeof(Tlv); } - /** * Returns the Version value. * * @returns The Version value. */ - uint8_t GetVersion(void) const { return ReadBits(mFlags); } + uint8_t GetVersion(void) const { return ReadBits(mFlags[0]); } /** * Sets the Version value. * * @param[in] aVersion The Version value. */ - void SetVersion(uint8_t aVersion) { WriteBits(mFlags, aVersion); } + void SetVersion(uint8_t aVersion) { WriteBits(mFlags[0], aVersion); } /** * Indicates whether or not the Native Commissioner flag is set. @@ -1083,17 +1046,12 @@ public: * @retval TRUE If the Native Commissioner flag is set. * @retval FALSE If the Native Commissioner flag is not set. */ - bool IsNativeCommissioner(void) const { return GetBit(mFlags, kNativeOffset); } + bool GetNativeCommissionerFlag(void) const { return GetBit(mFlags[0], kNativeFlagOffset); } /** * Sets the Native Commissioner flag. - * - * @param[in] aNativeCommissioner TRUE if set, FALSE otherwise. */ - void SetNativeCommissioner(bool aNativeCommissioner) - { - WriteBit(mFlags, kNativeOffset, aNativeCommissioner); - } + void SetNativeCommissionerFlag(void) { SetBit(mFlags[0], kNativeFlagOffset); } /** * Indicates whether or not the Commercial Commissioning Mode flag is set. @@ -1101,25 +1059,27 @@ public: * @retval TRUE If the Commercial Commissioning Mode flag is set. * @retval FALSE If the Commercial Commissioning Mode flag is not set. */ - bool IsCommercialCommissioningMode(void) const { return GetBit(mFlags, kCcmOffset); } + bool GetCcmFlag(void) const { return GetBit(mFlags[0], kCcmFlagOffset); } /** * Sets the Commercial Commissioning Mode flag. - * - * @param[in] aCcm TRUE if set, FALSE otherwise. */ - void SetCommercialCommissioningMode(bool aCcm) { WriteBit(mFlags, kCcmOffset, aCcm); } + void SetCcmFlag(void) { SetBit(mFlags[0], kCcmFlagOffset); } private: - static constexpr uint8_t kVersionOffset = 4; - static constexpr uint8_t kVersionMask = 0xf << kVersionOffset; - static constexpr uint8_t kNativeOffset = 3; - static constexpr uint8_t kCcmOffset = 2; + static constexpr uint8_t kVersionOffset = 4; + static constexpr uint8_t kVersionMask = 0xf << kVersionOffset; + static constexpr uint8_t kNativeFlagOffset = 3; + static constexpr uint8_t kCcmFlagOffset = 2; - uint8_t mFlags; - uint8_t mReserved; + uint8_t mFlags[2]; } OT_TOOL_PACKED_END; +/** + * Defines Discovery Response TLV constants and types. + */ +typedef SimpleTlvInfo DiscoveryResponseTlv; + #if OPENTHREAD_CONFIG_JOINER_ADV_EXPERIMENTAL_ENABLE /** diff --git a/src/core/thread/discover_scanner.cpp b/src/core/thread/discover_scanner.cpp index 1c21ec089..82c6731cd 100644 --- a/src/core/thread/discover_scanner.cpp +++ b/src/core/thread/discover_scanner.cpp @@ -61,11 +61,11 @@ Error DiscoverScanner::Discover(const Mac::ChannelMask &aScanChannels, Handler aCallback, void *aContext) { - Error error = kErrorNone; - Mle::TxMessage *message = nullptr; - Tlv::Bookmark tlvBookmark; - Ip6::Address destination; - MeshCoP::DiscoveryRequestTlv discoveryRequest; + Error error = kErrorNone; + Mle::TxMessage *message = nullptr; + Tlv::Bookmark tlvBookmark; + Ip6::Address destination; + MeshCoP::DiscoveryRequestTlvValue discoveryRequestTlvValue; VerifyOrExit(Get().IsUp(), error = kErrorInvalidState); @@ -105,10 +105,15 @@ Error DiscoverScanner::Discover(const Mac::ChannelMask &aScanChannels, SuccessOrExit(error = Tlv::StartTlv(*message, Tlv::kDiscovery, tlvBookmark)); - discoveryRequest.Init(); - discoveryRequest.SetVersion(kThreadVersion); - discoveryRequest.SetJoiner(aJoiner); - SuccessOrExit(error = discoveryRequest.AppendTo(*message)); + discoveryRequestTlvValue.Clear(); + discoveryRequestTlvValue.SetVersion(kThreadVersion); + + if (aJoiner) + { + discoveryRequestTlvValue.SetJoinerFlag(); + } + + SuccessOrExit(error = Tlv::Append(*message, discoveryRequestTlvValue)); #if OPENTHREAD_CONFIG_JOINER_ADV_EXPERIMENTAL_ENABLE if (mAdvDataLength != 0) @@ -312,12 +317,11 @@ exit: void DiscoverScanner::HandleDiscoveryResponse(Mle::RxInfo &aRxInfo) const { - Error error = kErrorNone; - MeshCoP::DiscoveryResponseTlv discoveryResponse; - ScanResult result; - OffsetRange offsetRange; - Tlv::ParsedInfo tlvInfo; - bool didCheckSteeringData = false; + Error error = kErrorNone; + ScanResult result; + OffsetRange offsetRange; + Tlv::ParsedInfo tlvInfo; + bool didCheckSteeringData = false; Mle::Log(Mle::kMessageReceive, Mle::kTypeDiscoveryResponse, aRxInfo.mMessageInfo.GetPeerAddr()); @@ -347,11 +351,15 @@ void DiscoverScanner::HandleDiscoveryResponse(Mle::RxInfo &aRxInfo) const switch (tlvInfo.mType) { case MeshCoP::Tlv::kDiscoveryResponse: - SuccessOrExit(error = aRxInfo.mMessage.Read(offsetRange, discoveryResponse)); - VerifyOrExit(discoveryResponse.IsValid(), error = kErrorParse); - result.mVersion = discoveryResponse.GetVersion(); - result.mIsNative = discoveryResponse.IsNativeCommissioner(); + { + MeshCoP::DiscoveryResponseTlvValue respTlvValue; + + SuccessOrExit(error = Tlv::Read(aRxInfo.mMessage, offsetRange.GetOffset(), + respTlvValue)); + result.mVersion = respTlvValue.GetVersion(); + result.mIsNative = respTlvValue.GetNativeCommissionerFlag(); break; + } case MeshCoP::Tlv::kExtendedPanId: SuccessOrExit(error = Tlv::Read(aRxInfo.mMessage, offsetRange.GetOffset(), diff --git a/src/core/thread/mle_ftd.cpp b/src/core/thread/mle_ftd.cpp index db6d6a09d..ee542f213 100644 --- a/src/core/thread/mle_ftd.cpp +++ b/src/core/thread/mle_ftd.cpp @@ -2705,17 +2705,16 @@ void Mle::SetSteeringData(const Mac::ExtAddress *aExtAddress) void Mle::HandleDiscoveryRequest(RxInfo &aRxInfo) { - Error error = kErrorNone; - Tlv::ParsedInfo tlvInfo; - MeshCoP::DiscoveryRequestTlv discoveryRequestTlv; - MeshCoP::ExtendedPanId extPanId; - OffsetRange offsetRange; - DiscoveryResponseInfo responseInfo; + Error error = kErrorNone; + bool parsedDiscoveryRequestTlv = false; + Tlv::ParsedInfo tlvInfo; + MeshCoP::DiscoveryRequestTlvValue discoveryRequestTlvValue; + MeshCoP::ExtendedPanId extPanId; + OffsetRange offsetRange; + DiscoveryResponseInfo responseInfo; Log(kMessageReceive, kTypeDiscoveryRequest, aRxInfo.mMessageInfo.GetPeerAddr()); - discoveryRequestTlv.SetLength(0); - VerifyOrExit(IsRouterEligible(), error = kErrorInvalidState); SuccessOrExit(error = Tlv::FindTlvValueOffsetRange(aRxInfo.mMessage, Tlv::kDiscovery, offsetRange)); @@ -2732,9 +2731,9 @@ void Mle::HandleDiscoveryRequest(RxInfo &aRxInfo) switch (tlvInfo.mType) { case MeshCoP::Tlv::kDiscoveryRequest: - SuccessOrExit(error = aRxInfo.mMessage.Read(offsetRange, discoveryRequestTlv)); - VerifyOrExit(discoveryRequestTlv.IsValid(), error = kErrorParse); - + SuccessOrExit(error = Tlv::Read(aRxInfo.mMessage, offsetRange.GetOffset(), + discoveryRequestTlvValue)); + parsedDiscoveryRequestTlv = true; break; case MeshCoP::Tlv::kExtendedPanId: @@ -2749,20 +2748,20 @@ void Mle::HandleDiscoveryRequest(RxInfo &aRxInfo) } } - if (discoveryRequestTlv.IsValid()) + if (parsedDiscoveryRequestTlv) { if (mDiscoveryRequestCallback.IsSet()) { otThreadDiscoveryRequestInfo info; AsCoreType(&info.mExtAddress).SetFromIid(aRxInfo.mMessageInfo.GetPeerAddr().GetIid()); - info.mVersion = discoveryRequestTlv.GetVersion(); - info.mIsJoiner = discoveryRequestTlv.IsJoiner(); + info.mVersion = discoveryRequestTlvValue.GetVersion(); + info.mIsJoiner = discoveryRequestTlvValue.GetJoinerFlag(); mDiscoveryRequestCallback.Invoke(&info); } - if (discoveryRequestTlv.IsJoiner()) + if (discoveryRequestTlvValue.GetJoinerFlag()) { #if OPENTHREAD_CONFIG_MLE_STEERING_DATA_SET_OOB_ENABLE if (!mSteeringData.IsEmpty()) @@ -2793,10 +2792,10 @@ exit: Error Mle::SendDiscoveryResponse(const Ip6::Address &aDestination, const DiscoveryResponseInfo &aInfo) { - Error error = kErrorNone; - TxMessage *message; - Tlv::Bookmark tlvBookmark; - MeshCoP::DiscoveryResponseTlv discoveryResponseTlv; + Error error = kErrorNone; + TxMessage *message; + Tlv::Bookmark tlvBookmark; + MeshCoP::DiscoveryResponseTlvValue discoveryResponseTlvValue; VerifyOrExit((message = NewMleMessage(kCommandDiscoveryResponse)) != nullptr, error = kErrorNoBufs); message->SetDirectTransmission(); @@ -2807,8 +2806,8 @@ Error Mle::SendDiscoveryResponse(const Ip6::Address &aDestination, const Discove SuccessOrExit(error = Tlv::StartTlv(*message, Tlv::kDiscovery, tlvBookmark)); - discoveryResponseTlv.Init(); - discoveryResponseTlv.SetVersion(kThreadVersion); + discoveryResponseTlvValue.Clear(); + discoveryResponseTlvValue.SetVersion(kThreadVersion); #if OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE if (Get().GetSecurityPolicy().mNativeCommissioningEnabled) @@ -2816,20 +2815,16 @@ Error Mle::SendDiscoveryResponse(const Ip6::Address &aDestination, const Discove SuccessOrExit(error = Tlv::Append( *message, Get().GetUdpPort())); - discoveryResponseTlv.SetNativeCommissioner(true); + discoveryResponseTlvValue.SetNativeCommissionerFlag(); } - else #endif - { - discoveryResponseTlv.SetNativeCommissioner(false); - } if (Get().GetSecurityPolicy().mCommercialCommissioningEnabled) { - discoveryResponseTlv.SetCommercialCommissioningMode(true); + discoveryResponseTlvValue.SetCcmFlag(); } - SuccessOrExit(error = discoveryResponseTlv.AppendTo(*message)); + SuccessOrExit(error = Tlv::Append(*message, discoveryResponseTlvValue)); SuccessOrExit( error = Tlv::Append(*message, Get().GetExtPanId()));