From 2841ca02d7fe588aada9c090980285cb10f2cc15 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Thu, 30 May 2024 11:17:00 -0700 Subject: [PATCH] [dataset-updater] enhance `DatasetUpdater` (#10293) This commit simplifies the `DatasetUpdater`: - Restricts `DatasetUpdater::RequestUpdate()` to be used only when the device is fully configured and has a valid Active Dataset; otherwise, it returns `kErrorInvalidState`. This simplification allows for the removal of the timer (previously used to delay the update if no Active Dataset was present). - Adds a check in `RequestUpdate()` to determine if the requested Dataset changes are already present in the current Active Dataset, returning `kErrorAlready` if so. - Enhances `RequestUpdate()` to prepare and set the Pending Dataset immediately. It stores the requested Dataset as a sequence of TLVs and retains the Active and Pending Timestamp values for later use in determining update success or failure. - Simplifies the code for checking Dataset changes and reporting update outcomes. A common `HandleDatasetChanged(Dataset::Type)` method is added, which can be used when the Active or Pending Dataset changes and performs the necessary checks, eliminating duplicate code. --- include/openthread/dataset_updater.h | 3 +- include/openthread/instance.h | 2 +- src/core/meshcop/dataset.cpp | 11 -- src/core/meshcop/dataset.hpp | 16 +- src/core/meshcop/dataset_updater.cpp | 217 ++++++++++++++------------- src/core/meshcop/dataset_updater.hpp | 30 ++-- 6 files changed, 127 insertions(+), 152 deletions(-) diff --git a/include/openthread/dataset_updater.h b/include/openthread/dataset_updater.h index f9b7f64ae..4943257d9 100644 --- a/include/openthread/dataset_updater.h +++ b/include/openthread/dataset_updater.h @@ -82,7 +82,8 @@ typedef void (*otDatasetUpdaterCallback)(otError aError, void *aContext); * @param[in] aContext An arbitrary context passed to callback. * * @retval OT_ERROR_NONE Dataset update started successfully (@p aCallback will be invoked on completion). - * @retval OT_ERROR_INVALID_STATE Device is disabled (MLE is disabled). + * @retval OT_ERROR_INVALID_STATE Device is disabled or not fully configured (missing or incomplete Active Dataset). + * @retval OT_ERROR_ALREADY The @p aDataset fields already match the existing Active Dataset. * @retval OT_ERROR_INVALID_ARGS The @p aDataset is not valid (contains Active or Pending Timestamp). * @retval OT_ERROR_BUSY Cannot start update, a previous one is ongoing. * @retval OT_ERROR_NO_BUFS Could not allocated buffer to save Dataset. diff --git a/include/openthread/instance.h b/include/openthread/instance.h index df87b06cb..690277cf9 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -53,7 +53,7 @@ extern "C" { * @note This number versions both OpenThread platform and user APIs. * */ -#define OPENTHREAD_API_VERSION (415) +#define OPENTHREAD_API_VERSION (416) /** * @addtogroup api-instance diff --git a/src/core/meshcop/dataset.cpp b/src/core/meshcop/dataset.cpp index ca28ef42c..09364a418 100644 --- a/src/core/meshcop/dataset.cpp +++ b/src/core/meshcop/dataset.cpp @@ -101,17 +101,6 @@ exit: return error; } -bool Dataset::Info::IsSubsetOf(const Info &aOther) const -{ - Dataset dataset; - Dataset other; - - dataset.SetFrom(*this); - other.SetFrom(aOther); - - return dataset.IsSubsetOf(other); -} - Dataset::Dataset(void) : mLength(0) , mUpdateTime(0) diff --git a/src/core/meshcop/dataset.hpp b/src/core/meshcop/dataset.hpp index 534fbab1b..7bfe02247 100644 --- a/src/core/meshcop/dataset.hpp +++ b/src/core/meshcop/dataset.hpp @@ -212,20 +212,6 @@ public: */ Error GenerateRandom(Instance &aInstance); - /** - * Checks whether the Dataset is a subset of another one, i.e., all the components in the current - * Dataset are also present in the @p aOther and the component values fully match. - * - * The matching of components in the two Datasets excludes Active/Pending Timestamp and Delay components. - * - * @param[in] aOther The other Dataset to check against. - * - * @retval TRUE The current Dataset is a subset of @p aOther. - * @retval FALSE The current Dataset is not a subset of @p aOther. - * - */ - bool IsSubsetOf(const Info &aOther) const; - private: Components &GetComponents(void) { return static_cast(mComponents); } const Components &GetComponents(void) const { return static_cast(mComponents); } @@ -680,7 +666,7 @@ public: * * @param[in] aOther The other Dataset to check against. * - * @retval TRUE The current dataset is a subset of @p aOther. + * @retval TRUE The current Dataset is a subset of @p aOther. * @retval FALSE The current Dataset is not a subset of @p aOther. * */ diff --git a/src/core/meshcop/dataset_updater.cpp b/src/core/meshcop/dataset_updater.cpp index 3f2f582b3..d6d03991f 100644 --- a/src/core/meshcop/dataset_updater.cpp +++ b/src/core/meshcop/dataset_updater.cpp @@ -48,32 +48,78 @@ namespace MeshCoP { DatasetUpdater::DatasetUpdater(Instance &aInstance) : InstanceLocator(aInstance) - , mTimer(aInstance) , mDataset(nullptr) { } Error DatasetUpdater::RequestUpdate(const Dataset::Info &aDataset, UpdaterCallback aCallback, void *aContext) { - Error error = kErrorNone; - Message *message = nullptr; + Dataset dataset; - VerifyOrExit(!Get().IsDisabled(), error = kErrorInvalidState); - VerifyOrExit(mDataset == nullptr, error = kErrorBusy); + dataset.SetFrom(aDataset); + return RequestUpdate(dataset, aCallback, aContext); +} - VerifyOrExit(!aDataset.IsPresent() && !aDataset.IsPresent(), - error = kErrorInvalidArgs); +Error DatasetUpdater::RequestUpdate(Dataset &aDataset, UpdaterCallback aCallback, void *aContext) +{ + Error error; + Message *message = nullptr; + Dataset activeDataset; + Timestamp activeTimestamp; + Timestamp pendingTimestamp; + + error = kErrorInvalidState; + VerifyOrExit(!Get().IsDisabled()); + SuccessOrExit(Get().Read(activeDataset)); + SuccessOrExit(activeDataset.Read(activeTimestamp)); + + error = kErrorInvalidArgs; + SuccessOrExit(aDataset.ValidateTlvs()); + VerifyOrExit(!aDataset.ContainsTlv(Tlv::kActiveTimestamp)); + VerifyOrExit(!aDataset.ContainsTlv(Tlv::kPendingTimestamp)); + + VerifyOrExit(!IsUpdateOngoing(), error = kErrorBusy); + + VerifyOrExit(!aDataset.IsSubsetOf(activeDataset), error = kErrorAlready); + + // Set the Active and Pending Timestamps for new requested Dataset + // by advancing ticks on the current timestamp values. + + activeTimestamp.AdvanceRandomTicks(); + SuccessOrExit(error = aDataset.Write(activeTimestamp)); + + if (Get().GetTimestamp() != nullptr) + { + pendingTimestamp = *Get().GetTimestamp(); + } + else + { + pendingTimestamp.Clear(); + } + + pendingTimestamp.AdvanceRandomTicks(); + SuccessOrExit(error = aDataset.Write(pendingTimestamp)); + + if (!aDataset.ContainsTlv(Tlv::kDelayTimer)) + { + SuccessOrExit(error = aDataset.Write(kDefaultDelay)); + } + + SuccessOrExit(error = activeDataset.WriteTlvsFrom(aDataset)); + + // Store the dataset in an allocated message to track update + // status and report the outcome via the callback. message = Get().Allocate(Message::kTypeOther); VerifyOrExit(message != nullptr, error = kErrorNoBufs); - SuccessOrExit(error = message->Append(aDataset)); + SuccessOrExit(error = message->AppendBytes(aDataset.GetBytes(), aDataset.GetLength())); + + Get().SaveLocal(activeDataset); mCallback.Set(aCallback, aContext); mDataset = message; - mTimer.Start(1); - exit: FreeMessageOnError(message, error); return error; @@ -81,130 +127,85 @@ exit: void DatasetUpdater::CancelUpdate(void) { - VerifyOrExit(mDataset != nullptr); + VerifyOrExit(IsUpdateOngoing()); FreeMessage(mDataset); mDataset = nullptr; - mTimer.Stop(); exit: return; } -void DatasetUpdater::HandleTimer(void) { PreparePendingDataset(); } - -void DatasetUpdater::PreparePendingDataset(void) -{ - Dataset dataset; - Dataset::Info requestedDataset; - Error error; - - VerifyOrExit(!Get().IsDisabled(), error = kErrorInvalidState); - - IgnoreError(mDataset->Read(0, requestedDataset)); - - error = Get().Read(dataset); - - if (error != kErrorNone) - { - // If there is no valid Active Dataset but MLE is not disabled, - // set the timer to try again after the retry interval. This - // handles the situation where a dataset update request comes - // right after the network is formed but before the active - // dataset is created. - - mTimer.Start(kRetryInterval); - ExitNow(error = kErrorNone); - } - - SuccessOrExit(dataset.WriteTlvsFrom(requestedDataset)); - - if (!requestedDataset.IsPresent()) - { - uint32_t delay = kDefaultDelay; - - SuccessOrExit(error = dataset.Write(delay)); - } - - { - Timestamp timestamp; - - if (Get().GetTimestamp() != nullptr) - { - timestamp = *Get().GetTimestamp(); - } - else - { - timestamp.Clear(); - } - - timestamp.AdvanceRandomTicks(); - IgnoreError(dataset.Write(timestamp)); - } - - { - Timestamp timestamp = dataset.FindTlv(Tlv::kActiveTimestamp)->ReadValueAs(); - - timestamp.AdvanceRandomTicks(); - IgnoreError(dataset.Write(timestamp)); - } - - Get().SaveLocal(dataset); - -exit: - if (error != kErrorNone) - { - Finish(error); - } -} - void DatasetUpdater::Finish(Error aError) { - OT_ASSERT(mDataset != nullptr); + VerifyOrExit(IsUpdateOngoing()); FreeMessage(mDataset); mDataset = nullptr; - mCallback.InvokeIfSet(aError); + +exit: + return; } void DatasetUpdater::HandleNotifierEvents(Events aEvents) { - Dataset::Info requestedDataset; - Dataset::Info dataset; - - VerifyOrExit(mDataset != nullptr); - - VerifyOrExit(aEvents.ContainsAny(kEventActiveDatasetChanged | kEventPendingDatasetChanged)); - - IgnoreError(mDataset->Read(0, requestedDataset)); - - if (aEvents.Contains(kEventActiveDatasetChanged) && Get().Read(dataset) == kErrorNone) + if (aEvents.Contains(kEventActiveDatasetChanged)) { - if (requestedDataset.IsSubsetOf(dataset)) + HandleDatasetChanged(Dataset::kActive); + } + + if (aEvents.Contains(kEventPendingDatasetChanged)) + { + HandleDatasetChanged(Dataset::kPending); + } +} + +void DatasetUpdater::HandleDatasetChanged(Dataset::Type aType) +{ + Dataset requestedDataset; + Dataset newDataset; + Timestamp newTimestamp; + Timestamp requestedTimestamp; + + VerifyOrExit(IsUpdateOngoing()); + + SuccessOrExit(requestedDataset.SetFrom(*mDataset, /* aOffset */ 0, mDataset->GetLength())); + + if (aType == Dataset::kActive) + { + SuccessOrExit(Get().Read(newDataset)); + } + else + { + SuccessOrExit(Get().Read(newDataset)); + } + + // Check if the new dataset includes the requested changes. If + // found in the Active Dataset, report success and finish. If + // found in the Pending Dataset, wait for it to be applied as + // Active. + + if (requestedDataset.IsSubsetOf(newDataset)) + { + if (aType == Dataset::kActive) { Finish(kErrorNone); } - else - { - Timestamp requestedDatasetTimestamp; - Timestamp activeDatasetTimestamp; - requestedDataset.Get(requestedDatasetTimestamp); - dataset.Get(activeDatasetTimestamp); - if (Timestamp::Compare(requestedDatasetTimestamp, activeDatasetTimestamp) <= 0) - { - Finish(kErrorAlready); - } - } + ExitNow(); } - if (aEvents.Contains(kEventPendingDatasetChanged) && Get().Read(dataset) == kErrorNone) + // If the new timestamp is ahead of the requested timestamp, it + // means there was a conflicting update (possibly from another + // device). In this case, report the update as a failure. + + SuccessOrExit(newDataset.ReadTimestamp(aType, newTimestamp)); + SuccessOrExit(requestedDataset.ReadTimestamp(aType, requestedTimestamp)); + + if (Timestamp::Compare(newTimestamp, requestedTimestamp) >= 0) { - if (!requestedDataset.IsSubsetOf(dataset)) - { - Finish(kErrorAlready); - } + Finish(kErrorAlready); } exit: diff --git a/src/core/meshcop/dataset_updater.hpp b/src/core/meshcop/dataset_updater.hpp index 7d0a93cee..09570fe5c 100644 --- a/src/core/meshcop/dataset_updater.hpp +++ b/src/core/meshcop/dataset_updater.hpp @@ -61,6 +61,12 @@ class DatasetUpdater : public InstanceLocator, private NonCopyable friend class ot::Notifier; public: + /** + * Default delay (in ms). + * + */ + static constexpr uint32_t kDefaultDelay = OPENTHREAD_CONFIG_DATASET_UPDATER_DEFAULT_DELAY; + /** * Initializes a `DatasetUpdater` object. * @@ -89,7 +95,8 @@ public: * @param[in] aContext An arbitrary context passed to callback. * * @retval kErrorNone Dataset update started successfully (@p aCallback will be invoked on completion). - * @retval kErrorInvalidState Device is disabled (MLE is disabled). + * @retval kErrorInvalidState Device is disabled or not fully configured (missing or incomplete Active Dataset). + * @retval kErrorAlready The @p aDataset fields already match the existing Active Dataset. * @retval kErrorInvalidArgs The @p aDataset is not valid (contains Active or Pending Timestamp). * @retval kErrorBusy Cannot start update, a previous one is ongoing. * @retval kErrorNoBufs Could not allocated buffer to save Dataset. @@ -110,25 +117,16 @@ public: * @retval FALSE There is no ongoing update. * */ - bool IsUpdateOngoing(void) const { return mDataset != nullptr; } + bool IsUpdateOngoing(void) const { return (mDataset != nullptr); } private: - // Default delay (in ms) in Pending Dataset. - static constexpr uint32_t kDefaultDelay = OPENTHREAD_CONFIG_DATASET_UPDATER_DEFAULT_DELAY; + Error RequestUpdate(Dataset &aDataset, UpdaterCallback aCallback, void *aContext); + void Finish(Error aError); + void HandleNotifierEvents(Events aEvents); + void HandleDatasetChanged(Dataset::Type aType); - // Retry interval (in ms) when preparing and/or sending Pending Dataset fails. - static constexpr uint32_t kRetryInterval = 1000; - - void HandleTimer(void); - void PreparePendingDataset(void); - void Finish(Error aError); - void HandleNotifierEvents(Events aEvents); - - using UpdaterTimer = TimerMilliIn; - - Callback mCallback; - UpdaterTimer mTimer; Message *mDataset; + Callback mCallback; }; } // namespace MeshCoP