From 9e26a5deab8d8af348a2db032d78b14f5541067c Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Tue, 12 Oct 2021 12:07:48 -0700 Subject: [PATCH] [meshcop] add common `Timestamp::Compare()` method (#7059) This commit adds a new common `Timestamp::Compare()` methods which compares two given `Timestamp` pointers (determines whether they are equal or one is ahead or behind the other one). The new method allows either input (or even both) to be `nullptr`. A null timestamp is used to indicate when there is no timestamp. In terms of comparison result, a non-null timestamp is considered to be ahead of (or larger then) a null one and two null timestamps are considered equal. The new method is used in all different places where the `Timestamp`s are compared. This commit also adds a unit test to verify the `Timestamp` class (the existing `test_steering_data` is renamed to `test_meshcop` which is now used to include unit tests for all MeshCoP related definitions). --- src/core/meshcop/dataset_local.cpp | 6 -- src/core/meshcop/dataset_local.hpp | 20 +++--- src/core/meshcop/dataset_manager.cpp | 20 ++---- src/core/meshcop/dataset_manager.hpp | 12 ---- src/core/meshcop/dataset_manager_ftd.cpp | 6 +- src/core/meshcop/timestamp.cpp | 62 ++++++++++++------- src/core/meshcop/timestamp.hpp | 28 +++++---- src/core/thread/mle.cpp | 8 +-- src/core/thread/mle_router.cpp | 10 +-- tests/unit/CMakeLists.txt | 12 ++-- tests/unit/Makefile.am | 6 +- ...est_steering_data.cpp => test_meshcop.cpp} | 45 +++++++++++++- 12 files changed, 133 insertions(+), 102 deletions(-) rename tests/unit/{test_steering_data.cpp => test_meshcop.cpp} (81%) diff --git a/src/core/meshcop/dataset_local.cpp b/src/core/meshcop/dataset_local.cpp index 20138e540..6ed501855 100644 --- a/src/core/meshcop/dataset_local.cpp +++ b/src/core/meshcop/dataset_local.cpp @@ -193,11 +193,5 @@ exit: return error; } -int DatasetLocal::Compare(const Timestamp *aCompare) -{ - return (aCompare == nullptr) ? (!mTimestampPresent ? 0 : -1) - : (!mTimestampPresent ? 1 : mTimestamp.Compare(*aCompare)); -} - } // namespace MeshCoP } // namespace ot diff --git a/src/core/meshcop/dataset_local.hpp b/src/core/meshcop/dataset_local.hpp index 66c4d3fba..6af9f297f 100644 --- a/src/core/meshcop/dataset_local.hpp +++ b/src/core/meshcop/dataset_local.hpp @@ -88,6 +88,14 @@ public: */ bool IsTimestampPresent(void) const { return mTimestampPresent; } + /** + * This method returns a pointer to the Timestamp or `nullptr` when it is not present in the Dataset. + * + * @returns A pointer to the Timestamp or `nullptr` if timestamp is not present in the Dataset. + * + */ + const Timestamp *GetTimestamp(void) const { return mTimestampPresent ? &mTimestamp : nullptr; } + /** * This method restores and retrieves the dataset from non-volatile memory. * @@ -175,18 +183,6 @@ public: */ Error Save(const Dataset &aDataset); - /** - * This method compares this dataset to another based on the timestamp. - * - * @param[in] aCompare A reference to the timestamp to compare. - * - * @retval -1 if @p aCompare is older than this dataset. - * @retval 0 if @p aCompare is equal to this dataset. - * @retval 1 if @p aCompare is newer than this dataset. - * - */ - int Compare(const Timestamp *aCompare); - private: bool IsActive(void) const { return (mType == Dataset::kActive); } void SetTimestamp(const Dataset &aDataset); diff --git a/src/core/meshcop/dataset_manager.cpp b/src/core/meshcop/dataset_manager.cpp index 5eed7190e..f194a9e4c 100644 --- a/src/core/meshcop/dataset_manager.cpp +++ b/src/core/meshcop/dataset_manager.cpp @@ -67,19 +67,6 @@ const Timestamp *DatasetManager::GetTimestamp(void) const return mTimestampValid ? &mTimestamp : nullptr; } -int DatasetManager::Compare(const Timestamp &aTimestamp) const -{ - const Timestamp *timestamp = GetTimestamp(); - int rval = 1; - - if (timestamp) - { - rval = timestamp->Compare(aTimestamp); - } - - return rval; -} - Error DatasetManager::Restore(void) { Error error; @@ -146,7 +133,7 @@ Error DatasetManager::Save(const Dataset &aDataset) } } - compare = mLocal.Compare(mTimestampValid ? &mTimestamp : nullptr); + compare = Timestamp::Compare(mTimestampValid ? &mTimestamp : nullptr, mLocal.GetTimestamp()); if (isNetworkkeyUpdated || compare > 0) { @@ -270,7 +257,8 @@ void DatasetManager::SendSet(void) VerifyOrExit(!mMgmtPending, error = kErrorBusy); VerifyOrExit(Get().IsChild() || Get().IsRouter(), error = kErrorInvalidState); - VerifyOrExit(mLocal.Compare(GetTimestamp()) < 0, error = kErrorInvalidState); + + VerifyOrExit(Timestamp::Compare(GetTimestamp(), mLocal.GetTimestamp()) < 0, error = kErrorInvalidState); if (IsActiveDataset()) { @@ -280,7 +268,7 @@ void DatasetManager::SendSet(void) IgnoreError(Get().Read(pendingDataset)); if ((pendingDataset.GetTimestamp(Dataset::kActive, timestamp) == kErrorNone) && - (mLocal.Compare(×tamp) == 0)) + (Timestamp::Compare(×tamp, mLocal.GetTimestamp()) == 0)) { // stop registration attempts during dataset transition ExitNow(error = kErrorInvalidState); diff --git a/src/core/meshcop/dataset_manager.hpp b/src/core/meshcop/dataset_manager.hpp index 975012fdb..75ee9f7bc 100644 --- a/src/core/meshcop/dataset_manager.hpp +++ b/src/core/meshcop/dataset_manager.hpp @@ -70,18 +70,6 @@ public: */ Error Restore(void); - /** - * This method compares @p aTimestamp to the dataset's timestamp value. - * - * @param[in] aCompare A reference to the timestamp to compare. - * - * @retval -1 if @p aCompare is older than this dataset. - * @retval 0 if @p aCompare is equal to this dataset. - * @retval 1 if @p aCompare is newer than this dataset. - * - */ - int Compare(const Timestamp &aTimestamp) const; - /** * This method retrieves the dataset from non-volatile memory. * diff --git a/src/core/meshcop/dataset_manager_ftd.cpp b/src/core/meshcop/dataset_manager_ftd.cpp index 9f836c1fc..e7bbdc8f5 100644 --- a/src/core/meshcop/dataset_manager_ftd.cpp +++ b/src/core/meshcop/dataset_manager_ftd.cpp @@ -106,11 +106,11 @@ Error DatasetManager::HandleSet(Coap::Message &aMessage, const Ip6::MessageInfo Timestamp pendingTimestamp; SuccessOrExit(Tlv::Find(aMessage, pendingTimestamp)); - VerifyOrExit(mLocal.Compare(&pendingTimestamp) > 0); + VerifyOrExit(Timestamp::Compare(&pendingTimestamp, mLocal.GetTimestamp()) > 0); } else { - VerifyOrExit(mLocal.Compare(&activeTimestamp) > 0); + VerifyOrExit(Timestamp::Compare(&activeTimestamp, mLocal.GetTimestamp()) > 0); } // check channel @@ -158,7 +158,7 @@ Error DatasetManager::HandleSet(Coap::Message &aMessage, const Ip6::MessageInfo // no change to network key, active timestamp must be ahead const Timestamp *localActiveTimestamp = Get().GetTimestamp(); - VerifyOrExit(localActiveTimestamp == nullptr || localActiveTimestamp->Compare(activeTimestamp) > 0); + VerifyOrExit(Timestamp::Compare(&activeTimestamp, localActiveTimestamp) > 0); } // check commissioner session id diff --git a/src/core/meshcop/timestamp.cpp b/src/core/meshcop/timestamp.cpp index 9a523d1c5..5d8f59bf6 100644 --- a/src/core/meshcop/timestamp.cpp +++ b/src/core/meshcop/timestamp.cpp @@ -33,38 +33,54 @@ #include "timestamp.hpp" +#include "common/code_utils.hpp" + namespace ot { namespace MeshCoP { -int Timestamp::Compare(const Timestamp &aCompare) const +int Timestamp::Compare(const Timestamp *aFirst, const Timestamp *aSecond) { - uint64_t thisSeconds = GetSeconds(); - uint64_t compareSeconds = aCompare.GetSeconds(); - uint16_t thisTicks = GetTicks(); - uint16_t compareTicks = aCompare.GetTicks(); int rval; + uint64_t firstSeconds; + uint64_t secondSeconds; + uint16_t firstTicks; + uint16_t secondTicks; - if (compareSeconds > thisSeconds) + if (aFirst == nullptr) { - rval = 1; - } - else if (compareSeconds < thisSeconds) - { - rval = -1; - } - else if (compareTicks > thisTicks) - { - rval = 1; - } - else if (compareTicks < thisTicks) - { - rval = -1; - } - else - { - rval = 0; + // When `aFirst` is null but `aSecond is not, we return -1, + // (indicate `aFirst (null) < aSecond (non-null)`). + ExitNow(rval = (aSecond == nullptr) ? 0 : -1); } + if (aSecond == nullptr) + { + // When `aFirst` is not null, but `aSecond` is, we return +1, + // (indicate `aFirst (non-null) > aSecond (null)`). + ExitNow(rval = 1); + } + + // Both are non-null. + + firstSeconds = aFirst->GetSeconds(); + secondSeconds = aSecond->GetSeconds(); + + if (firstSeconds != secondSeconds) + { + ExitNow(rval = (firstSeconds > secondSeconds) ? 1 : -1); + } + + firstTicks = aFirst->GetTicks(); + secondTicks = aSecond->GetTicks(); + + if (firstTicks != secondTicks) + { + ExitNow(rval = (firstTicks > secondTicks) ? 1 : -1); + } + + rval = 0; + +exit: return rval; } diff --git a/src/core/meshcop/timestamp.hpp b/src/core/meshcop/timestamp.hpp index 061aafcc1..ea3b11583 100644 --- a/src/core/meshcop/timestamp.hpp +++ b/src/core/meshcop/timestamp.hpp @@ -59,18 +59,6 @@ OT_TOOL_PACKED_BEGIN class Timestamp : public Clearable { public: - /** - * This method compares this timestamp to another. - * - * @param[in] aCompare A reference to the timestamp to compare. - * - * @retval -1 if @p aCompare is less than this timestamp. - * @retval 0 if @p aCompare is equal to this timestamp. - * @retval 1 if @p aCompare is greater than this timestamp. - * - */ - int Compare(const Timestamp &aCompare) const; - /** * This method returns the Seconds value. * @@ -139,6 +127,22 @@ public: */ void AdvanceRandomTicks(void); + /** + * This static method compares two timestamps. + * + * Either one or both @p aFirst or @p aSecond can be `nullptr`. A non-null timestamp is considered greater than + * a null one. If both are null, they are considered as equal. + * + * @param[in] aFirst A pointer to the first timestamp to compare (can be nullptr). + * @param[in] aSecond A pointer to the second timestamp to compare (can be nullptr). + * + * @retval -1 if @p aFirst is less than @p aSecond (`aFirst < aSecond`). + * @retval 0 if @p aFirst is equal to @p aSecond (`aFirst == aSecond`). + * @retval 1 if @p aFirst is greater than @p aSecond (`aFirst > aSecond`). + * + */ + static int Compare(const Timestamp *aFirst, const Timestamp *aSecond); + private: static constexpr uint8_t kTicksOffset = 1; static constexpr uint16_t kTicksMask = 0x7fff << kTicksOffset; diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index e2d9f66a8..261249fa1 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -3165,7 +3165,7 @@ Error Mle::HandleLeaderData(const Message &aMessage, const Ip6::MessageInfo &aMe // if received timestamp does not match the local value and message does not contain the dataset, // send MLE Data Request - if (!IsLeader() && ((timestamp == nullptr) || (timestamp->Compare(activeTimestamp) != 0)) && + if (!IsLeader() && (MeshCoP::Timestamp::Compare(&activeTimestamp, timestamp) != 0) && (Tlv::FindTlvOffset(aMessage, Tlv::kActiveDataset, activeDatasetOffset) != kErrorNone)) { ExitNow(dataRequest = true); @@ -3190,7 +3190,7 @@ Error Mle::HandleLeaderData(const Message &aMessage, const Ip6::MessageInfo &aMe // if received timestamp does not match the local value and message does not contain the dataset, // send MLE Data Request - if (!IsLeader() && ((timestamp == nullptr) || (timestamp->Compare(pendingTimestamp) != 0)) && + if (!IsLeader() && (MeshCoP::Timestamp::Compare(&pendingTimestamp, timestamp) != 0) && (Tlv::FindTlvOffset(aMessage, Tlv::kPendingDataset, pendingDatasetOffset) != kErrorNone)) { ExitNow(dataRequest = true); @@ -3958,7 +3958,7 @@ void Mle::HandleAnnounce(const Message &aMessage, const Ip6::MessageInfo &aMessa localTimestamp = Get().GetTimestamp(); - if (localTimestamp == nullptr || localTimestamp->Compare(timestamp) > 0) + if (MeshCoP::Timestamp::Compare(×tamp, localTimestamp) > 0) { // No action is required if device is detached, and current // channel and pan-id match the values from the received MLE @@ -3981,7 +3981,7 @@ void Mle::HandleAnnounce(const Message &aMessage, const Ip6::MessageInfo &aMessa otLogNoteMle("Delay processing Announce - channel %d, panid 0x%02x", channel, panId); } - else if (localTimestamp->Compare(timestamp) < 0) + else if (MeshCoP::Timestamp::Compare(×tamp, localTimestamp) < 0) { SendAnnounce(channel); diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index 22fa09523..4de6d70d0 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -2255,7 +2255,8 @@ void MleRouter::HandleChildIdRequest(const Message & aMessage, switch (Tlv::Find(aMessage, timestamp)) { case kErrorNone: - needsActiveDatasetTlv = (Get().Compare(timestamp) != 0); + needsActiveDatasetTlv = + (MeshCoP::Timestamp::Compare(×tamp, Get().GetTimestamp()) != 0); break; case kErrorNotFound: break; @@ -2268,7 +2269,8 @@ void MleRouter::HandleChildIdRequest(const Message & aMessage, switch (Tlv::Find(aMessage, timestamp)) { case kErrorNone: - needsPendingDatasetTlv = (Get().Compare(timestamp) != 0); + needsPendingDatasetTlv = + (MeshCoP::Timestamp::Compare(×tamp, Get().GetTimestamp()) != 0); break; case kErrorNotFound: break; @@ -2733,7 +2735,7 @@ void MleRouter::HandleDataRequest(const Message & aMessage, switch (Tlv::Find(aMessage, timestamp)) { case kErrorNone: - if (Get().Compare(timestamp) == 0) + if (MeshCoP::Timestamp::Compare(×tamp, Get().GetTimestamp()) == 0) { break; } @@ -2752,7 +2754,7 @@ void MleRouter::HandleDataRequest(const Message & aMessage, switch (Tlv::Find(aMessage, timestamp)) { case kErrorNone: - if (Get().Compare(timestamp) == 0) + if (MeshCoP::Timestamp::Compare(×tamp, Get().GetTimestamp()) == 0) { break; } diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index b733fdb2c..4daacfdb7 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -705,26 +705,26 @@ target_link_libraries(ot-test-pskc add_test(NAME ot-test-pskc COMMAND ot-test-pskc) -add_executable(ot-test-steering-data - test_steering_data.cpp +add_executable(ot-test-meshcop + test_meshcop.cpp ) -target_include_directories(ot-test-steering-data +target_include_directories(ot-test-meshcop PRIVATE ${COMMON_INCLUDES} ) -target_compile_options(ot-test-steering-data +target_compile_options(ot-test-meshcop PRIVATE ${COMMON_COMPILE_OPTIONS} ) -target_link_libraries(ot-test-steering-data +target_link_libraries(ot-test-meshcop PRIVATE ${COMMON_LIBS} ) -add_test(NAME ot-test-steering-data COMMAND ot-test-steering-data) +add_test(NAME ot-test-meshcop COMMAND ot-test-meshcop) add_executable(ot-test-string test_string.cpp diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 239e00c28..a91ca5fea 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -129,6 +129,7 @@ check_PROGRAMS += \ ot-test-lowpan \ ot-test-mac-frame \ ot-test-macros \ + ot-test-meshcop \ ot-test-message \ ot-test-message-queue \ ot-test-multicast-listeners-table \ @@ -138,7 +139,6 @@ check_PROGRAMS += \ ot-test-pool \ ot-test-priority-queue \ ot-test-pskc \ - ot-test-steering-data \ ot-test-string \ ot-test-timer \ $(NULL) @@ -277,8 +277,8 @@ ot_test_priority_queue_SOURCES = $(COMMON_SOURCES) test_priority_queue.cpp ot_test_pskc_LDADD = $(COMMON_LDADD) ot_test_pskc_SOURCES = $(COMMON_SOURCES) test_pskc.cpp -ot_test_steering_data_LDADD = $(COMMON_LDADD) -ot_test_steering_data_SOURCES = $(COMMON_SOURCES) test_steering_data.cpp +ot_test_meshcop_LDADD = $(COMMON_LDADD) +ot_test_meshcop_SOURCES = $(COMMON_SOURCES) test_meshcop.cpp ot_test_string_LDADD = $(COMMON_LDADD) ot_test_string_SOURCES = $(COMMON_SOURCES) test_string.cpp diff --git a/tests/unit/test_steering_data.cpp b/tests/unit/test_meshcop.cpp similarity index 81% rename from tests/unit/test_steering_data.cpp rename to tests/unit/test_meshcop.cpp index a24ad40e1..bcf823ba4 100644 --- a/tests/unit/test_steering_data.cpp +++ b/tests/unit/test_meshcop.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, The OpenThread Authors. + * Copyright (c) 2020-21, The OpenThread Authors. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -32,6 +32,7 @@ #include "test_util.hpp" #include "meshcop/meshcop.hpp" +#include "meshcop/timestamp.hpp" namespace ot { @@ -108,6 +109,47 @@ void TestSteeringData(void) VerifyOrQuit(!steeringData.Contains(joinerId1), "after Init()"); VerifyOrQuit(!steeringData.Contains(joinerId2), "after Init()"); VerifyOrQuit(!steeringData.Contains(indexes), "after Init()"); + + printf("TestSteeringData() passed\n"); +} + +void TestTimestamp(void) +{ + MeshCoP::Timestamp t1; + MeshCoP::Timestamp t2; + + t1.Clear(); + t2.Clear(); + + VerifyOrQuit(t1.GetSeconds() == 0); + VerifyOrQuit(t1.GetTicks() == 0); + VerifyOrQuit(!t1.GetAuthoritative()); + + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, nullptr) > 0); + VerifyOrQuit(MeshCoP::Timestamp::Compare(nullptr, &t2) < 0); + VerifyOrQuit(MeshCoP::Timestamp::Compare(nullptr, nullptr) == 0); + + t1.SetTicks(10); + VerifyOrQuit(t1.GetTicks() == 10); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) > 0); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t2, &t1) < 0); + + t2.SetTicks(10); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); + + t1.SetAuthoritative(true); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); + + t1.SetSeconds(1); + VerifyOrQuit(t1.GetSeconds() == 1); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) > 0); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t2, &t1) < 0); + + t2.SetSeconds(1); + VerifyOrQuit(MeshCoP::Timestamp::Compare(&t1, &t2) == 0); + + printf("TestTimestamp() passed\n"); } } // namespace ot @@ -115,6 +157,7 @@ void TestSteeringData(void) int main(void) { ot::TestSteeringData(); + ot::TestTimestamp(); printf("\nAll tests passed.\n"); return 0; }