From dcebe97f82fc30474237ea92cff8345d20c75119 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Tue, 20 Oct 2020 11:17:08 -0700 Subject: [PATCH] [border-agent] misc enhancements (#5663) This commit contains smaller enhancements in `BorderAgent` module - define `ForwardContext` as a private sub-type of BorderAgent - change `ForwardContext` to be `InstanceLocatorInit` and avoid `new` (use `Init()`) - add `HandleCoapResponse()` method which is called from `static` method `HandleCoapResponse()`. - convert file `static` functions `SendErrorMessage()` to `BorderAgent` methods - Add `State` enum and inline/remove `SetState()`. - Smaller style/documentation fixes (e.g., remove unnecessary `{ }`). --- src/core/api/border_agent_api.cpp | 2 +- src/core/meshcop/border_agent.cpp | 251 +++++++------------ src/core/meshcop/border_agent.hpp | 59 +++-- tests/toranj/openthread-core-toranj-config.h | 8 + 4 files changed, 146 insertions(+), 174 deletions(-) diff --git a/src/core/api/border_agent_api.cpp b/src/core/api/border_agent_api.cpp index 7edce8416..8154d44b6 100644 --- a/src/core/api/border_agent_api.cpp +++ b/src/core/api/border_agent_api.cpp @@ -46,7 +46,7 @@ otBorderAgentState otBorderAgentGetState(otInstance *aInstance) { Instance &instance = *static_cast(aInstance); - return instance.Get().GetState(); + return static_cast(instance.Get().GetState()); } #endif // OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE diff --git a/src/core/meshcop/border_agent.cpp b/src/core/meshcop/border_agent.cpp index af632fa5e..d8cad40e9 100644 --- a/src/core/meshcop/border_agent.cpp +++ b/src/core/meshcop/border_agent.cpp @@ -37,7 +37,6 @@ #include "common/instance.hpp" #include "common/locator-getters.hpp" #include "common/logging.hpp" -#include "common/new.hpp" #include "meshcop/meshcop.hpp" #include "meshcop/meshcop_tlvs.hpp" #include "thread/thread_netif.hpp" @@ -49,99 +48,40 @@ namespace ot { namespace MeshCoP { -class ForwardContext +void BorderAgent::ForwardContext::Init(Instance & aInstance, + const Coap::Message &aMessage, + bool aPetition, + bool aSeparate) { -public: - /** - * This constructor initializes a forward context. - * - * @param[in] aBorderAgent A reference to the border agent. - * @param[in] aHeader A reference to the request header. - * @param[in] aPetition Whether this request is a petition. - * @param[in] aSeparate Whether this original request expects separate response. - * - */ - ForwardContext(BorderAgent &aBorderAgent, const Coap::Message &aMessage, bool aPetition, bool aSeparate) - : mBorderAgent(aBorderAgent) - , mMessageId(aMessage.GetMessageId()) - , mPetition(aPetition) - , mSeparate(aSeparate) - , mTokenLength(aMessage.GetTokenLength()) - , mType(aMessage.GetType()) + InstanceLocatorInit::Init(aInstance); + mMessageId = aMessage.GetMessageId(); + mPetition = aPetition; + mSeparate = aSeparate; + mType = aMessage.GetType(); + mTokenLength = aMessage.GetTokenLength(); + memcpy(mToken, aMessage.GetToken(), mTokenLength); +} + +otError BorderAgent::ForwardContext::ToHeader(Coap::Message &aMessage, uint8_t aCode) +{ + if ((mType == Coap::kTypeNonConfirmable) || mSeparate) { - memcpy(mToken, aMessage.GetToken(), mTokenLength); + aMessage.Init(Coap::kTypeNonConfirmable, static_cast(aCode)); + } + else + { + aMessage.Init(Coap::kTypeAck, static_cast(aCode)); } - /** - * This method returns whether the request is a petition. - * - * @retval true This is a petition request. - * @retval false This is not a petition request. - * - */ - bool IsPetition(void) const { return mPetition; } - - /** - * This method returns the border agent sending this request. - * - * @returns A reference to the border agent sending this request. - * - */ - BorderAgent &GetBorderAgent(void) { return mBorderAgent; } - - /** - * This method returns the message id of the original request. - * - * @returns A message id of the original request. - * - */ - uint16_t GetMessageId(void) const { return mMessageId; } - - /** - * This method generate the response header according to the saved metadata. - * - * @param[out] aHeader A reference to the response header. - * @param[in] aCode The response code to fill in the response header. - * - * @retval OT_ERROR_NONE Successfully generated the response header. - * @retval OT_ERROR_NO_BUFS Insufficient message buffers available to generate the response header. - * - */ - otError ToHeader(Coap::Message &aMessage, uint8_t aCode) + if (!mSeparate) { - if ((mType == Coap::kTypeNonConfirmable) || mSeparate) - { - aMessage.Init(Coap::kTypeNonConfirmable, static_cast(aCode)); - } - else - { - aMessage.Init(Coap::kTypeAck, static_cast(aCode)); - } - - if (!mSeparate) - { - aMessage.SetMessageId(mMessageId); - } - - return aMessage.SetToken(mToken, mTokenLength); + aMessage.SetMessageId(mMessageId); } -private: - enum - { - kMaxTokenLength = Coap::Message::kMaxTokenLength, ///< The max token size - }; + return aMessage.SetToken(mToken, mTokenLength); +} - BorderAgent &mBorderAgent; - uint16_t mMessageId; ///< The CoAP Message ID of the original request. - bool mPetition : 1; ///< Whether the forwarding request is leader petition. - bool mSeparate : 1; ///< Whether the original request expects separate response. - uint8_t mTokenLength : 4; ///< The CoAP Token Length of the original request. - uint8_t mType : 2; ///< The CoAP Type of the original request. - uint8_t mToken[kMaxTokenLength]; ///< The CoAP Token of the original request. -}; - -static Coap::Message::Code CoapCodeFromError(otError aError) +Coap::Message::Code BorderAgent::CoapCodeFromError(otError aError) { Coap::Message::Code code; @@ -153,7 +93,6 @@ static Coap::Message::Code CoapCodeFromError(otError aError) case OT_ERROR_PARSE: code = Coap::kCodeBadRequest; - ; break; default: @@ -164,29 +103,28 @@ static Coap::Message::Code CoapCodeFromError(otError aError) return code; } -static void SendErrorMessage(Coap::CoapSecure &aCoapSecure, ForwardContext &aForwardContext, otError aError) +void BorderAgent::SendErrorMessage(ForwardContext &aForwardContext, otError aError) { - otError error = OT_ERROR_NONE; - Coap::Message *message = nullptr; + otError error = OT_ERROR_NONE; + Coap::CoapSecure &coaps = Get(); + Coap::Message * message = nullptr; - VerifyOrExit((message = NewMeshCoPMessage(aCoapSecure)) != nullptr, error = OT_ERROR_NO_BUFS); + VerifyOrExit((message = NewMeshCoPMessage(coaps)) != nullptr, error = OT_ERROR_NO_BUFS); SuccessOrExit(error = aForwardContext.ToHeader(*message, CoapCodeFromError(aError))); - SuccessOrExit(error = aCoapSecure.SendMessage(*message, aCoapSecure.GetMessageInfo())); + SuccessOrExit(error = coaps.SendMessage(*message, coaps.GetMessageInfo())); exit: FreeMessageOnError(message, error); LogError("send error CoAP message", error); } -static void SendErrorMessage(Coap::CoapSecure & aCoapSecure, - const Coap::Message &aRequest, - bool aSeparate, - otError aError) +void BorderAgent::SendErrorMessage(const Coap::Message &aRequest, bool aSeparate, otError aError) { - otError error = OT_ERROR_NONE; - Coap::Message *message = nullptr; + otError error = OT_ERROR_NONE; + Coap::CoapSecure &coaps = Get(); + Coap::Message * message = nullptr; - VerifyOrExit((message = NewMeshCoPMessage(aCoapSecure)) != nullptr, error = OT_ERROR_NO_BUFS); + VerifyOrExit((message = NewMeshCoPMessage(coaps)) != nullptr, error = OT_ERROR_NO_BUFS); if (aRequest.IsNonConfirmable() || aSeparate) { @@ -204,7 +142,7 @@ static void SendErrorMessage(Coap::CoapSecure & aCoapSecure, SuccessOrExit(error = message->SetTokenFromMessage(aRequest)); - SuccessOrExit(error = aCoapSecure.SendMessage(*message, aCoapSecure.GetMessageInfo())); + SuccessOrExit(error = coaps.SendMessage(*message, coaps.GetMessageInfo())); exit: FreeMessageOnError(message, error); @@ -218,43 +156,46 @@ void BorderAgent::HandleCoapResponse(void * aContext, { OT_UNUSED_VARIABLE(aMessageInfo); - ForwardContext & forwardContext = *static_cast(aContext); - BorderAgent & borderAgent = forwardContext.GetBorderAgent(); - Instance & instance = borderAgent.GetInstance(); - const Coap::Message *response = static_cast(aMessage); - Coap::Message * message = nullptr; - otError error; + ForwardContext &forwardContext = *static_cast(aContext); + + forwardContext.Get().HandleCoapResponse(forwardContext, static_cast(aMessage), + aResult); +} + +void BorderAgent::HandleCoapResponse(ForwardContext &aForwardContext, const Coap::Message *aResponse, otError aResult) +{ + Coap::Message *message = nullptr; + otError error; SuccessOrExit(error = aResult); - VerifyOrExit((message = NewMeshCoPMessage(instance.Get())) != nullptr, error = OT_ERROR_NO_BUFS); + VerifyOrExit((message = NewMeshCoPMessage(Get())) != nullptr, error = OT_ERROR_NO_BUFS); - if (forwardContext.IsPetition() && response->GetCode() == Coap::kCodeChanged) + if (aForwardContext.IsPetition() && aResponse->GetCode() == Coap::kCodeChanged) { uint8_t state; - SuccessOrExit(error = Tlv::FindUint8Tlv(*response, Tlv::kState, state)); + SuccessOrExit(error = Tlv::FindUint8Tlv(*aResponse, Tlv::kState, state)); if (state == StateTlv::kAccept) { uint16_t sessionId; - SuccessOrExit(error = Tlv::FindUint16Tlv(*response, Tlv::kCommissionerSessionId, sessionId)); + SuccessOrExit(error = Tlv::FindUint16Tlv(*aResponse, Tlv::kCommissionerSessionId, sessionId)); - IgnoreError(instance.Get().GetCommissionerAloc(borderAgent.mCommissionerAloc.GetAddress(), - sessionId)); - instance.Get().AddUnicastAddress(borderAgent.mCommissionerAloc); - IgnoreError(instance.Get().AddReceiver(borderAgent.mUdpReceiver)); + IgnoreError(Get().GetCommissionerAloc(mCommissionerAloc.GetAddress(), sessionId)); + Get().AddUnicastAddress(mCommissionerAloc); + IgnoreError(Get().AddReceiver(mUdpReceiver)); } } - SuccessOrExit(error = forwardContext.ToHeader(*message, response->GetCode())); + SuccessOrExit(error = aForwardContext.ToHeader(*message, aResponse->GetCode())); - if (response->GetLength() > response->GetOffset()) + if (aResponse->GetLength() > aResponse->GetOffset()) { SuccessOrExit(error = message->SetPayloadMarker()); } - SuccessOrExit(error = borderAgent.ForwardToCommissioner(*message, *response)); + SuccessOrExit(error = ForwardToCommissioner(*message, *aResponse)); exit: @@ -262,13 +203,21 @@ exit: { FreeMessage(message); - otLogWarnMeshCoP("Commissioner request[%hu] failed: %s", forwardContext.GetMessageId(), + otLogWarnMeshCoP("Commissioner request[%hu] failed: %s", aForwardContext.GetMessageId(), otThreadErrorToString(error)); - SendErrorMessage(instance.Get(), forwardContext, error); + SendErrorMessage(aForwardContext, error); } - instance.HeapFree(&forwardContext); + GetInstance().HeapFree(&aForwardContext); +} + +template +void BorderAgent::HandleRequest(void *aContext, otMessage *aMessage, const otMessageInfo *aMessageInfo) +{ + IgnoreError(static_cast(aContext)->ForwardToLeader( + *static_cast(aMessage), *static_cast(aMessageInfo), + (static_cast(aContext)->*aResource).GetUriPath(), false, false)); } template <> @@ -336,7 +285,7 @@ BorderAgent::BorderAgent(Instance &aInstance) , mProxyTransmit(UriPath::kProxyTx, BorderAgent::HandleRequest<&BorderAgent::mProxyTransmit>, this) , mUdpReceiver(BorderAgent::HandleUdpReceive, this) , mTimer(aInstance, HandleTimeout, this) - , mState(OT_BORDER_AGENT_STATE_STOPPED) + , mState(kStateStopped) { mCommissionerAloc.InitAsThreadOriginRealmLocalScope(); } @@ -364,25 +313,22 @@ exit: void BorderAgent::HandleProxyTransmit(const Coap::Message &aMessage) { - Message * message = nullptr; - Ip6::MessageInfo messageInfo; - uint16_t offset; - otError error; + Message * message = nullptr; + Ip6::MessageInfo messageInfo; + uint16_t offset; + otError error; + UdpEncapsulationTlv tlv; - { - UdpEncapsulationTlv tlv; + SuccessOrExit(error = Tlv::FindTlvOffset(aMessage, Tlv::kUdpEncapsulation, offset)); + SuccessOrExit(error = aMessage.Read(offset, tlv)); - SuccessOrExit(error = Tlv::FindTlvOffset(aMessage, Tlv::kUdpEncapsulation, offset)); - SuccessOrExit(error = aMessage.Read(offset, tlv)); + VerifyOrExit((message = Get().NewMessage(0)) != nullptr, error = OT_ERROR_NO_BUFS); + SuccessOrExit(error = message->SetLength(tlv.GetUdpLength())); + aMessage.CopyTo(offset + sizeof(tlv), 0, tlv.GetUdpLength(), *message); - VerifyOrExit((message = Get().NewMessage(0)) != nullptr, error = OT_ERROR_NO_BUFS); - SuccessOrExit(error = message->SetLength(tlv.GetUdpLength())); - aMessage.CopyTo(offset + sizeof(tlv), 0, tlv.GetUdpLength(), *message); - - messageInfo.SetSockPort(tlv.GetSourcePort() != 0 ? tlv.GetSourcePort() : Get().GetEphemeralPort()); - messageInfo.SetSockAddr(mCommissionerAloc.GetAddress()); - messageInfo.SetPeerPort(tlv.GetDestinationPort()); - } + messageInfo.SetSockPort(tlv.GetSourcePort() != 0 ? tlv.GetSourcePort() : Get().GetEphemeralPort()); + messageInfo.SetSockAddr(mCommissionerAloc.GetAddress()); + messageInfo.SetPeerPort(tlv.GetDestinationPort()); SuccessOrExit( error = Tlv::FindTlv(aMessage, Tlv::kIPv6Address, messageInfo.GetPeerAddr().mFields.m8, sizeof(Ip6::Address))); @@ -553,7 +499,7 @@ otError BorderAgent::ForwardToLeader(const Coap::Message & aMessage, forwardContext = static_cast(GetInstance().HeapCAlloc(1, sizeof(ForwardContext))); VerifyOrExit(forwardContext != nullptr, error = OT_ERROR_NO_BUFS); - forwardContext = new (forwardContext) ForwardContext(*this, aMessage, aPetition, aSeparate); + forwardContext->Init(GetInstance(), aMessage, aPetition, aSeparate); SuccessOrExit(error = message->InitAsConfirmablePost(aPath)); @@ -590,25 +536,30 @@ exit: } FreeMessage(message); - SendErrorMessage(Get(), aMessage, aSeparate, error); + SendErrorMessage(aMessage, aSeparate, error); } return error; } +void BorderAgent::HandleConnected(bool aConnected, void *aContext) +{ + static_cast(aContext)->HandleConnected(aConnected); +} + void BorderAgent::HandleConnected(bool aConnected) { if (aConnected) { otLogInfoMeshCoP("Commissioner connected"); - SetState(OT_BORDER_AGENT_STATE_ACTIVE); + mState = kStateActive; mTimer.Start(kKeepAliveTimeout); } else { otLogInfoMeshCoP("Commissioner disconnected"); Get().RemoveUnicastAddress(mCommissionerAloc); - SetState(OT_BORDER_AGENT_STATE_STARTED); + mState = kStateStarted; } } @@ -617,7 +568,7 @@ otError BorderAgent::Start(void) otError error; Coap::CoapSecure &coaps = Get(); - VerifyOrExit(mState == OT_BORDER_AGENT_STATE_STOPPED, error = OT_ERROR_ALREADY); + VerifyOrExit(mState == kStateStopped, error = OT_ERROR_ALREADY); SuccessOrExit(error = coaps.Start(kBorderAgentUdpPort)); SuccessOrExit(error = coaps.SetPsk(Get().GetPskc().m8, OT_PSKC_MAX_SIZE)); @@ -636,7 +587,7 @@ otError BorderAgent::Start(void) Get().AddResource(mRelayReceive); - SetState(OT_BORDER_AGENT_STATE_STARTED); + mState = kStateStarted; exit: return error; @@ -661,7 +612,7 @@ otError BorderAgent::Stop(void) otError error = OT_ERROR_NONE; Coap::CoapSecure &coaps = Get(); - VerifyOrExit(mState != OT_BORDER_AGENT_STATE_STOPPED, error = OT_ERROR_ALREADY); + VerifyOrExit(mState != kStateStopped, error = OT_ERROR_ALREADY); mTimer.Stop(); @@ -680,23 +631,15 @@ otError BorderAgent::Stop(void) coaps.Stop(); - SetState(OT_BORDER_AGENT_STATE_STOPPED); + mState = kStateStopped; exit: return error; } -void BorderAgent::SetState(otBorderAgentState aState) -{ - if (mState != aState) - { - mState = aState; - } -} - void BorderAgent::ApplyMeshLocalPrefix(void) { - VerifyOrExit(mState == OT_BORDER_AGENT_STATE_ACTIVE); + VerifyOrExit(mState == kStateActive); if (Get().HasUnicastAddress(mCommissionerAloc)) { diff --git a/src/core/meshcop/border_agent.hpp b/src/core/meshcop/border_agent.hpp index 20c9e6f21..e785bd22b 100644 --- a/src/core/meshcop/border_agent.hpp +++ b/src/core/meshcop/border_agent.hpp @@ -46,8 +46,6 @@ namespace ot { -class ThreadNetif; - namespace MeshCoP { class BorderAgent : public InstanceLocator, private NonCopyable @@ -56,7 +54,18 @@ class BorderAgent : public InstanceLocator, private NonCopyable public: /** - * This constructor initializes the BorderAgent object. + * This enumeration defines the Border Agent state. + * + */ + enum State : uint8_t + { + kStateStopped = OT_BORDER_AGENT_STATE_STOPPED, ///< Border agent is stopped/disabled. + kStateStarted = OT_BORDER_AGENT_STATE_STARTED, ///< Border agent is started. + kStateActive = OT_BORDER_AGENT_STATE_ACTIVE, ///< Border agent is connected with external commissioner. + }; + + /** + * This constructor initializes the `BorderAgent` object. * * @param[in] aInstance A reference to the OpenThread instance. * @@ -87,7 +96,7 @@ public: * @returns The state of the Border Agent service. * */ - otBorderAgentState GetState(void) const { return mState; } + State GetState(void) const { return mState; } /** * This method applies the Mesh Local Prefix. @@ -96,21 +105,34 @@ public: void ApplyMeshLocalPrefix(void); private: + class ForwardContext : public InstanceLocatorInit + { + public: + void Init(Instance &aInstance, const Coap::Message &aMessage, bool aPetition, bool aSeparate); + bool IsPetition(void) const { return mPetition; } + uint16_t GetMessageId(void) const { return mMessageId; } + otError ToHeader(Coap::Message &aMessage, uint8_t aCode); + + private: + uint16_t mMessageId; // The CoAP Message ID of the original request. + bool mPetition : 1; // Whether the forwarding request is leader petition. + bool mSeparate : 1; // Whether the original request expects separate response. + uint8_t mTokenLength : 4; // The CoAP Token Length of the original request. + uint8_t mType : 2; // The CoAP Type of the original request. + uint8_t mToken[Coap::Message::kMaxTokenLength]; // The CoAP Token of the original request. + }; + void HandleNotifierEvents(Events aEvents); - static void HandleConnected(bool aConnected, void *aContext) - { - static_cast(aContext)->HandleConnected(aConnected); - } - void HandleConnected(bool aConnected); + Coap::Message::Code CoapCodeFromError(otError aError); + void SendErrorMessage(ForwardContext &aForwardContext, otError aError); + void SendErrorMessage(const Coap::Message &aRequest, bool aSeparate, otError aError); + + static void HandleConnected(bool aConnected, void *aContext); + void HandleConnected(bool aConnected); template - static void HandleRequest(void *aContext, otMessage *aMessage, const otMessageInfo *aMessageInfo) - { - IgnoreError(static_cast(aContext)->ForwardToLeader( - *static_cast(aMessage), *static_cast(aMessageInfo), - (static_cast(aContext)->*aResource).GetUriPath(), false, false)); - } + static void HandleRequest(void *aContext, otMessage *aMessage, const otMessageInfo *aMessageInfo); static void HandleTimeout(Timer &aTimer); void HandleTimeout(void); @@ -119,6 +141,7 @@ private: otMessage * aMessage, const otMessageInfo *aMessageInfo, otError aResult); + void HandleCoapResponse(ForwardContext &aForwardContext, const Coap::Message *aResponse, otError aResult); otError ForwardToLeader(const Coap::Message & aMessage, const Ip6::MessageInfo &aMessageInfo, @@ -137,8 +160,6 @@ private: } bool HandleUdpReceive(const Message &aMessage, const Ip6::MessageInfo &aMessageInfo); - void SetState(otBorderAgentState aState); - enum { kKeepAliveTimeout = 50 * 1000, ///< Timeout to reject a commissioner. @@ -162,8 +183,8 @@ private: Ip6::Udp::Receiver mUdpReceiver; ///< The UDP receiver to receive packets from external commissioner Ip6::NetifUnicastAddress mCommissionerAloc; - TimerMilli mTimer; - otBorderAgentState mState; + TimerMilli mTimer; + State mState; }; } // namespace MeshCoP diff --git a/tests/toranj/openthread-core-toranj-config.h b/tests/toranj/openthread-core-toranj-config.h index 989033c75..5b8d0ef31 100644 --- a/tests/toranj/openthread-core-toranj-config.h +++ b/tests/toranj/openthread-core-toranj-config.h @@ -63,6 +63,14 @@ */ #define OPENTHREAD_CONFIG_COMMISSIONER_MAX_JOINER_ENTRIES 4 +/** + * @def OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE + * + * Define to 1 to enable Border Agent support. + * + */ +#define OPENTHREAD_CONFIG_BORDER_AGENT_ENABLE 1 + /** * @def OPENTHREAD_CONFIG_DIAG_ENABLE *