From e48a1ff9807f66d448a9e76ad64038dc6c67adce Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Fri, 16 Oct 2020 12:38:59 -0700 Subject: [PATCH] [dataset-manager] misc enhancements (#5655) This commit contains miscellaneous smaller enhancements in `DataseManager` module: - keep `public`, `protected` `private` definitions in this order - add helper `GetType()`, `IsActiveDataset()`, `IsPendingDataset()` - use `Read()` helper instead of `mLocal.Read()` - remove unused definitions of `HandleUdpReceive()` - remove `mUriGet`, `mUriSet`, and determine them based on the dataset type (active/pending). - re-order member variables to help with packing - update method documentations --- src/core/meshcop/dataset_local.hpp | 3 +- src/core/meshcop/dataset_manager.cpp | 71 ++++++-------- src/core/meshcop/dataset_manager.hpp | 118 +++++++++++------------ src/core/meshcop/dataset_manager_ftd.cpp | 14 +-- 4 files changed, 97 insertions(+), 109 deletions(-) diff --git a/src/core/meshcop/dataset_local.hpp b/src/core/meshcop/dataset_local.hpp index 84d170516..26349ac4c 100644 --- a/src/core/meshcop/dataset_local.hpp +++ b/src/core/meshcop/dataset_local.hpp @@ -59,8 +59,7 @@ public: /** * This method indicates whether this is an Active or Pending Dataset. * - * @retval Tlv::kActiveTimestamp when this is an Active Dataset. - * @retval Tlv::kPendingTimetamp when this is a Pending Dataset. + * @returns The Dataset type. * */ Dataset::Type GetType(void) const { return mType; } diff --git a/src/core/meshcop/dataset_manager.cpp b/src/core/meshcop/dataset_manager.cpp index c2a188134..304a272ff 100644 --- a/src/core/meshcop/dataset_manager.cpp +++ b/src/core/meshcop/dataset_manager.cpp @@ -49,18 +49,12 @@ namespace ot { namespace MeshCoP { -DatasetManager::DatasetManager(Instance & aInstance, - Dataset::Type aType, - const char * aUriGet, - const char * aUriSet, - Timer::Handler aTimerHandler) +DatasetManager::DatasetManager(Instance &aInstance, Dataset::Type aType, Timer::Handler aTimerHandler) : InstanceLocator(aInstance) , mLocal(aInstance, aType) , mTimestampValid(false) - , mTimer(aInstance, aTimerHandler, this) - , mUriGet(aUriGet) - , mUriSet(aUriSet) , mCoapPending(false) + , mTimer(aInstance, aTimerHandler, this) { mTimestamp.Init(); } @@ -86,7 +80,7 @@ int DatasetManager::Compare(const Timestamp &aTimestamp) const otError DatasetManager::Restore(void) { otError error; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); const Timestamp *timestamp; mTimer.Stop(); @@ -103,7 +97,7 @@ otError DatasetManager::Restore(void) mTimestampValid = true; } - if (mLocal.GetType() == Dataset::kActive) + if (IsActiveDataset()) { IgnoreError(dataset.ApplyConfiguration(GetInstance())); } @@ -115,9 +109,9 @@ exit: otError DatasetManager::ApplyConfiguration(void) const { otError error; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); - SuccessOrExit(error = mLocal.Read(dataset)); + SuccessOrExit(error = Read(dataset)); SuccessOrExit(error = dataset.ApplyConfiguration(GetInstance())); exit: @@ -151,7 +145,7 @@ otError DatasetManager::Save(const Dataset &aDataset) mTimestamp = *timestamp; mTimestampValid = true; - if (mLocal.GetType() == Dataset::kActive) + if (IsActiveDataset()) { SuccessOrExit(error = aDataset.ApplyConfiguration(GetInstance(), &isMasterkeyUpdated)); } @@ -231,9 +225,9 @@ otError DatasetManager::GetChannelMask(Mac::ChannelMask &aChannelMask) const otError error; const MeshCoP::ChannelMaskTlv *channelMaskTlv; uint32_t mask; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); - SuccessOrExit(error = mLocal.Read(dataset)); + SuccessOrExit(error = Read(dataset)); channelMaskTlv = dataset.GetTlv(); VerifyOrExit(channelMaskTlv != nullptr, error = OT_ERROR_NOT_FOUND); @@ -257,13 +251,13 @@ void DatasetManager::SendSet(void) otError error; Coap::Message * message = nullptr; Ip6::MessageInfo messageInfo; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); VerifyOrExit(!mCoapPending, error = OT_ERROR_BUSY); VerifyOrExit(Get().IsAttached(), error = OT_ERROR_INVALID_STATE); VerifyOrExit(mLocal.Compare(GetTimestamp()) < 0, error = OT_ERROR_INVALID_STATE); - if (mLocal.GetType() == Dataset::kActive) + if (IsActiveDataset()) { Dataset pendingDataset(Dataset::kPending); IgnoreError(Get().Read(pendingDataset)); @@ -280,10 +274,11 @@ void DatasetManager::SendSet(void) VerifyOrExit((message = NewMeshCoPMessage(Get())) != nullptr, error = OT_ERROR_NO_BUFS); - SuccessOrExit(error = message->InitAsConfirmablePost(mUriSet)); + SuccessOrExit(error = + message->InitAsConfirmablePost(IsActiveDataset() ? UriPath::kActiveSet : UriPath::kPendingSet)); SuccessOrExit(error = message->SetPayloadMarker()); - IgnoreError(mLocal.Read(dataset)); + IgnoreError(Read(dataset)); SuccessOrExit(error = message->AppendBytes(dataset.GetBytes(), dataset.GetSize())); messageInfo.SetSockAddr(Get().GetMeshLocal16()); @@ -292,7 +287,7 @@ void DatasetManager::SendSet(void) SuccessOrExit( error = Get().SendMessage(*message, messageInfo, &DatasetManager::HandleCoapResponse, this)); - otLogInfoMeshCoP("Sent %s to leader", mUriSet); + otLogInfoMeshCoP("Sent %s set to leader", Dataset::TypeToString(GetType())); exit: @@ -307,7 +302,7 @@ exit: // fall through default: - otLogWarnMeshCoP("Failed to send %s to leader: %s", mUriSet, otThreadErrorToString(error)); + LogError("send Dataset set to leader", error); FreeMessage(message); break; } @@ -360,7 +355,7 @@ void DatasetManager::HandleGet(const Coap::Message &aMessage, const Ip6::Message } // MGMT_PENDING_GET.rsp must include Delay Timer TLV (Thread 1.1.1 Section 8.7.5.4) - VerifyOrExit(length > 0 && strcmp(mUriGet, UriPath::kPendingGet) == 0); + VerifyOrExit(length > 0 && IsPendingDataset()); for (uint8_t i = 0; i < length; i++) { @@ -383,9 +378,9 @@ void DatasetManager::SendGetResponse(const Coap::Message & aRequest, { otError error = OT_ERROR_NONE; Coap::Message *message; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); - IgnoreError(mLocal.Read(dataset)); + IgnoreError(Read(dataset)); VerifyOrExit((message = NewMeshCoPMessage(Get())) != nullptr, error = OT_ERROR_NO_BUFS); @@ -442,7 +437,8 @@ otError DatasetManager::SendSetRequest(const otOperationalDataset &aDataset, con VerifyOrExit((message = NewMeshCoPMessage(Get())) != nullptr, error = OT_ERROR_NO_BUFS); - SuccessOrExit(error = message->InitAsConfirmablePost(mUriSet)); + SuccessOrExit(error = + message->InitAsConfirmablePost(IsActiveDataset() ? UriPath::kActiveSet : UriPath::kPendingSet)); SuccessOrExit(error = message->SetPayloadMarker()); #if OPENTHREAD_CONFIG_COMMISSIONER_ENABLE && OPENTHREAD_FTD @@ -655,7 +651,8 @@ otError DatasetManager::SendGetRequest(const otOperationalDatasetComponents &aDa VerifyOrExit((message = NewMeshCoPMessage(Get())) != nullptr, error = OT_ERROR_NO_BUFS); - SuccessOrExit(error = message->InitAsConfirmablePost(mUriGet)); + SuccessOrExit(error = + message->InitAsConfirmablePost(IsActiveDataset() ? UriPath::kActiveGet : UriPath::kPendingGet)); if (aLength + length > 0) { @@ -700,7 +697,7 @@ exit: } ActiveDataset::ActiveDataset(Instance &aInstance) - : DatasetManager(aInstance, Dataset::kActive, UriPath::kActiveGet, UriPath::kActiveSet, ActiveDataset::HandleTimer) + : DatasetManager(aInstance, Dataset::kActive, ActiveDataset::HandleTimer) , mResourceGet(UriPath::kActiveGet, &ActiveDataset::HandleGet, this) #if OPENTHREAD_FTD , mResourceSet(UriPath::kActiveSet, &ActiveDataset::HandleSet, this) @@ -717,7 +714,7 @@ bool ActiveDataset::IsPartiallyComplete(void) const otError ActiveDataset::Save(const Timestamp &aTimestamp, const Message &aMessage, uint16_t aOffset, uint8_t aLength) { otError error = OT_ERROR_NONE; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); SuccessOrExit(error = dataset.Set(aMessage, aOffset, aLength)); dataset.SetTimestamp(aTimestamp); @@ -744,11 +741,7 @@ void ActiveDataset::HandleTimer(Timer &aTimer) } PendingDataset::PendingDataset(Instance &aInstance) - : DatasetManager(aInstance, - Dataset::kPending, - UriPath::kPendingGet, - UriPath::kPendingSet, - PendingDataset::HandleTimer) + : DatasetManager(aInstance, Dataset::kPending, PendingDataset::HandleTimer) , mDelayTimer(aInstance, PendingDataset::HandleDelayTimer, this) , mResourceGet(UriPath::kPendingGet, &PendingDataset::HandleGet, this) #if OPENTHREAD_FTD @@ -766,7 +759,7 @@ void PendingDataset::Clear(void) void PendingDataset::ClearNetwork(void) { - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); mTimestamp.Init(); mTimestampValid = false; @@ -798,7 +791,7 @@ exit: otError PendingDataset::Save(const Timestamp &aTimestamp, const Message &aMessage, uint16_t aOffset, uint8_t aLength) { otError error = OT_ERROR_NONE; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); SuccessOrExit(error = dataset.Set(aMessage, aOffset, aLength)); dataset.SetTimestamp(aTimestamp); @@ -812,9 +805,9 @@ exit: void PendingDataset::StartDelayTimer(void) { DelayTimerTlv *delayTimer; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); - IgnoreError(mLocal.Read(dataset)); + IgnoreError(Read(dataset)); mDelayTimer.Stop(); @@ -841,9 +834,9 @@ void PendingDataset::HandleDelayTimer(Timer &aTimer) void PendingDataset::HandleDelayTimer(void) { DelayTimerTlv *delayTimer; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); - IgnoreError(mLocal.Read(dataset)); + IgnoreError(Read(dataset)); // if the Delay Timer value is larger than what our Timer implementation can handle, we have to compute // the remainder and wait some more. diff --git a/src/core/meshcop/dataset_manager.hpp b/src/core/meshcop/dataset_manager.hpp index 895d94191..b30f6cf43 100644 --- a/src/core/meshcop/dataset_manager.hpp +++ b/src/core/meshcop/dataset_manager.hpp @@ -54,7 +54,7 @@ class DatasetManager : public InstanceLocator { public: /** - * This method returns a reference to the Timestamp. + * This method returns a pointer to the Timestamp. * * @returns A pointer to the Timestamp. * @@ -172,6 +172,18 @@ public: const uint8_t * aTlvTypes, uint8_t aLength, const otIp6Address * aAddress) const; +#if OPENTHREAD_FTD + /** + * This method appends the MLE Dataset TLV but excluding MeshCoP Sub Timestamp TLV. + * + * @param[in] aMessage The message to append the TLV to. + * + * @retval OT_ERROR_NONE Successfully append MLE Dataset TLV without MeshCoP Sub Timestamp TLV. + * @retval OT_ERROR_NO_BUFS Insufficient available buffers to append the message with MLE Dataset TLV. + * + */ + otError AppendMleDatasetTlv(Message &aMessage) const; +#endif protected: /** @@ -207,17 +219,19 @@ protected: * This constructor initializes the object. * * @param[in] aInstance A reference to the OpenThread instance. - * @param[in] aType Identifies Active or Pending Operational Dataset. - * @param[in] aUriGet The URI-PATH for getting the Operational Dataset. - * @param[in] aUriSet The URI-PATH for setting the Operational Dataset. + * @param[in] aType Dataset type, Active or Pending. * @param[in] aTimerHandler The registration timer handler. * */ - DatasetManager(Instance & aInstance, - Dataset::Type aType, - const char * aUriGet, - const char * aUriSet, - TimerMilli::Handler aTimerHandler); + DatasetManager(Instance &aInstance, Dataset::Type aType, TimerMilli::Handler aTimerHandler); + + /** + * This method gets the Operational Dataset type (Active or Pending). + * + * @returns The Operational Dataset type. + * + */ + Dataset::Type GetType(void) const { return mLocal.GetType(); } /** * This method clears the Operational Dataset. @@ -274,7 +288,6 @@ protected: /** * This method handles a MGMT_GET request message. * - * @param[in] aHeader The CoAP header. * @param[in] aMessage The CoAP message buffer. * @param[in] aMessageInfo The message info. * @@ -296,56 +309,10 @@ protected: */ void HandleTimer(void); - DatasetLocal mLocal; - Timestamp mTimestamp; - bool mTimestampValid : 1; - -private: - static void HandleUdpReceive(void *aContext, otMessage *aMessage, const otMessageInfo *aMessageInfo); - void HandleUdpReceive(Coap::Message &aMessage, const Ip6::MessageInfo &aMessageInfo); - - static void HandleCoapResponse(void * aContext, - otMessage * aMessage, - const otMessageInfo *aMessageInfo, - otError aError); - void HandleCoapResponse(void); - - void HandleDatasetUpdated(void); - void SendSet(void); - void SendGetResponse(const Coap::Message & aRequest, - const Ip6::MessageInfo &aMessageInfo, - uint8_t * aTlvs, - uint8_t aLength) const; - - enum - { - kMaxDatasetTlvs = 16, // Maximum number of TLVs in an `otOperationalDataset`. - kDelayNoBufs = 1000, // Milliseconds - }; - - TimerMilli mTimer; - - const char *mUriGet; - const char *mUriSet; - - bool mCoapPending : 1; - #if OPENTHREAD_FTD -public: - /** - * This method appends the MLE Dataset TLV but excluding MeshCoP Sub Timestamp TLV. - * - * @retval OT_ERROR_NONE Successfully append MLE Dataset TLV without MeshCoP Sub Timestamp TLV. - * @retval OT_ERROR_NO_BUFS Insufficient available buffers to append the message with MLE Dataset TLV. - * - */ - otError AppendMleDatasetTlv(Message &aMessage) const; - -protected: /** * This method handles the MGMT_SET request message. * - * @param[in] aHeader The CoAP header. * @param[in] aMessage The CoAP message buffer. * @param[in] aMessageInfo The message info. * @@ -354,17 +321,47 @@ protected: * */ otError HandleSet(Coap::Message &aMessage, const Ip6::MessageInfo &aMessageInfo); +#endif + + DatasetLocal mLocal; + Timestamp mTimestamp; + bool mTimestampValid : 1; private: + static void HandleCoapResponse(void * aContext, + otMessage * aMessage, + const otMessageInfo *aMessageInfo, + otError aError); + void HandleCoapResponse(void); + + bool IsActiveDataset(void) const { return GetType() == Dataset::kActive; } + bool IsPendingDataset(void) const { return GetType() == Dataset::kPending; } + void HandleDatasetUpdated(void); + void SendSet(void); + void SendGetResponse(const Coap::Message & aRequest, + const Ip6::MessageInfo &aMessageInfo, + uint8_t * aTlvs, + uint8_t aLength) const; + +#if OPENTHREAD_FTD void SendSetResponse(const Coap::Message &aRequest, const Ip6::MessageInfo &aMessageInfo, StateTlv::State aState); -#endif // OPENTHREAD_FTD +#endif + + enum + { + kMaxDatasetTlvs = 16, // Maximum number of TLVs in an `otOperationalDataset`. + kDelayNoBufs = 1000, // Milliseconds + }; + + bool mCoapPending : 1; + TimerMilli mTimer; }; class ActiveDataset : public DatasetManager, private NonCopyable { public: /** - * Constructor. + * This constructor initializes the ActiveDataset object. * * @param[in] aInstance A reference to the OpenThread instance. * @@ -494,9 +491,9 @@ class PendingDataset : public DatasetManager, private NonCopyable { public: /** - * Constructor. + * This constructor initializes the PendingDataset object. * - * @param[in] The Thread network interface. + * @param[in] aInstance A reference to the OpenThread instance. * */ explicit PendingDataset(Instance &aInstance); @@ -559,7 +556,6 @@ public: otError Save(const Timestamp &aTimestamp, const Message &aMessage, uint16_t aOffset, uint8_t aLength); #if OPENTHREAD_FTD - /** * This method starts the Leader functions for maintaining the Active Operational Dataset. * diff --git a/src/core/meshcop/dataset_manager_ftd.cpp b/src/core/meshcop/dataset_manager_ftd.cpp index 685de061c..f6378e6c6 100644 --- a/src/core/meshcop/dataset_manager_ftd.cpp +++ b/src/core/meshcop/dataset_manager_ftd.cpp @@ -59,9 +59,9 @@ namespace MeshCoP { otError DatasetManager::AppendMleDatasetTlv(Message &aMessage) const { - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); - IgnoreError(mLocal.Read(dataset)); + IgnoreError(Read(dataset)); return dataset.AppendMleDatasetTlv(aMessage); } @@ -77,7 +77,7 @@ otError DatasetManager::HandleSet(Coap::Message &aMessage, const Ip6::MessageInf bool doesAffectMasterKey = false; bool hasMasterKey = false; StateTlv::State state = StateTlv::kReject; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); ActiveTimestampTlv activeTimestamp; PendingTimestampTlv pendingTimestamp; @@ -105,7 +105,7 @@ otError DatasetManager::HandleSet(Coap::Message &aMessage, const Ip6::MessageInf // verify that does not overflow dataset buffer VerifyOrExit((offset - aMessage.GetOffset()) <= Dataset::kMaxSize); - type = (strcmp(mUriSet, UriPath::kActiveSet) == 0 ? Tlv::kActiveTimestamp : Tlv::kPendingTimestamp); + type = (GetType() == Dataset::kActive) ? Tlv::kActiveTimestamp : Tlv::kPendingTimestamp; if (Tlv::FindTlv(aMessage, Tlv::kActiveTimestamp, sizeof(activeTimestamp), activeTimestamp) != OT_ERROR_NONE) { @@ -358,12 +358,12 @@ exit: otError ActiveDataset::GenerateLocal(void) { otError error = OT_ERROR_NONE; - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); VerifyOrExit(Get().IsAttached(), error = OT_ERROR_INVALID_STATE); VerifyOrExit(!mLocal.IsTimestampPresent(), error = OT_ERROR_ALREADY); - IgnoreError(mLocal.Read(dataset)); + IgnoreError(Read(dataset)); if (dataset.GetTlv() == nullptr) { @@ -508,7 +508,7 @@ exit: void PendingDataset::ApplyActiveDataset(const Timestamp &aTimestamp, Coap::Message &aMessage) { uint16_t offset = aMessage.GetOffset(); - Dataset dataset(mLocal.GetType()); + Dataset dataset(GetType()); VerifyOrExit(Get().IsAttached());