From d78f43e947d6ab575033ea4faca208a3b68d68ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Tue, 5 Apr 2022 20:22:50 +0200 Subject: [PATCH] [thread/cleanup] Change GetThreadProvision to return OperationalDataset (#17054) * [thread] Change GetThreadProvision to return OperationalDataset OpenThread stack manager has OperationalDataset member only used as a temporary object for returning an operational dataset as ByteSpan in GetThreadProvision method. Change the interface to save >250 bytes of RAM. Additionally, add bound checks in OperationalDataset::GetExtendedPanId methods. * Restyled by clang-format Co-authored-by: Restyled.io --- src/include/platform/ThreadStackManager.h | 10 +++++--- src/lib/support/ThreadOperationalDataset.cpp | 24 ++++++++++++------- .../NetworkCommissioningThreadDriver.cpp | 10 +++----- src/platform/Linux/ThreadStackManagerImpl.cpp | 11 +++------ src/platform/Linux/ThreadStackManagerImpl.h | 2 +- ...enericNetworkCommissioningThreadDriver.cpp | 10 +++----- ...nericThreadStackManagerImpl_OpenThread.cpp | 16 ++++++------- ...GenericThreadStackManagerImpl_OpenThread.h | 5 ++-- 8 files changed, 41 insertions(+), 47 deletions(-) diff --git a/src/include/platform/ThreadStackManager.h b/src/include/platform/ThreadStackManager.h index 8a231ca4a743ac..a47da93ae59bf7 100644 --- a/src/include/platform/ThreadStackManager.h +++ b/src/include/platform/ThreadStackManager.h @@ -36,6 +36,10 @@ struct TextEntry; struct DnssdService; } // namespace Dnssd +namespace Thread { +class OperationalDataset; +} // namespace Thread + namespace DeviceLayer { class PlatformManagerImpl; @@ -93,7 +97,7 @@ class ThreadStackManager bool IsThreadEnabled(); bool IsThreadProvisioned(); bool IsThreadAttached(); - CHIP_ERROR GetThreadProvision(ByteSpan & netInfo); + CHIP_ERROR GetThreadProvision(Thread::OperationalDataset & dataset); CHIP_ERROR GetAndLogThreadStatsCounters(); CHIP_ERROR GetAndLogThreadTopologyMinimal(); CHIP_ERROR GetAndLogThreadTopologyFull(); @@ -342,9 +346,9 @@ inline bool ThreadStackManager::IsThreadAttached() return static_cast(this)->_IsThreadAttached(); } -inline CHIP_ERROR ThreadStackManager::GetThreadProvision(ByteSpan & netInfo) +inline CHIP_ERROR ThreadStackManager::GetThreadProvision(Thread::OperationalDataset & dataset) { - return static_cast(this)->_GetThreadProvision(netInfo); + return static_cast(this)->_GetThreadProvision(dataset); } inline CHIP_ERROR ThreadStackManager::SetThreadProvision(ByteSpan netInfo) diff --git a/src/lib/support/ThreadOperationalDataset.cpp b/src/lib/support/ThreadOperationalDataset.cpp index 59eaae1db0d4dd..bb3837df3555bb 100644 --- a/src/lib/support/ThreadOperationalDataset.cpp +++ b/src/lib/support/ThreadOperationalDataset.cpp @@ -277,28 +277,34 @@ CHIP_ERROR OperationalDataset::SetChannel(uint16_t aChannel) CHIP_ERROR OperationalDataset::GetExtendedPanId(uint8_t (&aExtendedPanId)[kSizeExtendedPanId]) const { - const ThreadTLV * tlv = Locate(ThreadTLV::kExtendedPanId); + ByteSpan extPanIdSpan; + CHIP_ERROR error = GetExtendedPanIdAsByteSpan(extPanIdSpan); - if (tlv != nullptr) + if (error != CHIP_NO_ERROR) { - memcpy(aExtendedPanId, tlv->GetValue(), sizeof(aExtendedPanId)); - return CHIP_NO_ERROR; + return error; } - return CHIP_ERROR_TLV_TAG_NOT_FOUND; + memcpy(aExtendedPanId, extPanIdSpan.data(), extPanIdSpan.size()); + return CHIP_NO_ERROR; } CHIP_ERROR OperationalDataset::GetExtendedPanIdAsByteSpan(ByteSpan & span) const { const ThreadTLV * tlv = Locate(ThreadTLV::kExtendedPanId); - if (tlv != nullptr) + if (tlv == nullptr) { - span = ByteSpan(reinterpret_cast(tlv->GetValue()), tlv->GetLength()); - return CHIP_NO_ERROR; + return CHIP_ERROR_TLV_TAG_NOT_FOUND; } - return CHIP_ERROR_TLV_TAG_NOT_FOUND; + if (tlv->GetLength() != kSizeExtendedPanId) + { + return CHIP_ERROR_INVALID_TLV_ELEMENT; + } + + span = ByteSpan(static_cast(tlv->GetValue()), tlv->GetLength()); + return CHIP_NO_ERROR; } CHIP_ERROR OperationalDataset::SetExtendedPanId(const uint8_t (&aExtendedPanId)[kSizeExtendedPanId]) diff --git a/src/platform/Linux/NetworkCommissioningThreadDriver.cpp b/src/platform/Linux/NetworkCommissioningThreadDriver.cpp index 74c071bc509e2d..29fd175e9fb65a 100644 --- a/src/platform/Linux/NetworkCommissioningThreadDriver.cpp +++ b/src/platform/Linux/NetworkCommissioningThreadDriver.cpp @@ -43,12 +43,10 @@ namespace NetworkCommissioning { CHIP_ERROR LinuxThreadDriver::Init(BaseDriver::NetworkStatusChangeCallback * networkStatusChangeCallback) { - ByteSpan currentProvision; VerifyOrReturnError(ConnectivityMgrImpl().IsThreadAttached(), CHIP_NO_ERROR); - VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, CHIP_NO_ERROR); + VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(mStagingNetwork) == CHIP_NO_ERROR, CHIP_NO_ERROR); - mSavedNetwork.Init(currentProvision); - mStagingNetwork.Init(currentProvision); + mSavedNetwork.Init(mStagingNetwork.AsByteSpan()); ThreadStackMgrImpl().SetNetworkStatusChangeCallback(networkStatusChangeCallback); @@ -185,14 +183,12 @@ bool LinuxThreadDriver::ThreadNetworkIterator::Next(Network & item) item.connected = false; exhausted = true; - ByteSpan currentProvision; Thread::OperationalDataset currentDataset; uint8_t enabledExtPanId[Thread::kSizeExtendedPanId]; // The Thread network is not actually enabled. VerifyOrReturnError(ConnectivityMgrImpl().IsThreadAttached(), true); - VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, true); - VerifyOrReturnError(currentDataset.Init(currentProvision) == CHIP_NO_ERROR, true); + VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentDataset) == CHIP_NO_ERROR, true); // The Thread network is not enabled, but has a different extended pan id. VerifyOrReturnError(currentDataset.GetExtendedPanId(enabledExtPanId) == CHIP_NO_ERROR, true); VerifyOrReturnError(memcmp(extpanid, enabledExtPanId, kSizeExtendedPanId) == 0, true); diff --git a/src/platform/Linux/ThreadStackManagerImpl.cpp b/src/platform/Linux/ThreadStackManagerImpl.cpp index 104f90d805cb7a..29c9a858680eb8 100644 --- a/src/platform/Linux/ThreadStackManagerImpl.cpp +++ b/src/platform/Linux/ThreadStackManagerImpl.cpp @@ -235,7 +235,7 @@ CHIP_ERROR ThreadStackManagerImpl::_SetThreadProvision(ByteSpan netInfo) return PlatformMgr().PostEvent(&event); } -CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo) +CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(Thread::OperationalDataset & dataset) { VerifyOrReturnError(mProxy, CHIP_ERROR_INCORRECT_STATE); @@ -257,7 +257,6 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo) if (response == nullptr) { - netInfo = ByteSpan(); return CHIP_ERROR_KEY_NOT_FOUND; } @@ -265,7 +264,6 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo) if (tupleContent == nullptr) { - netInfo = ByteSpan(); return CHIP_ERROR_KEY_NOT_FOUND; } @@ -273,7 +271,6 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo) if (value == nullptr) { - netInfo = ByteSpan(); return CHIP_ERROR_KEY_NOT_FOUND; } @@ -282,7 +279,7 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo) ReturnErrorOnFailure(mDataset.Init(ByteSpan(data, size))); } - netInfo = mDataset.AsByteSpan(); + dataset.Init(mDataset.AsByteSpan()); return CHIP_NO_ERROR; } @@ -716,20 +713,18 @@ void ThreadStackManagerImpl::_UpdateNetworkStatus() // Thread is not enabled, then we are not trying to connect to the network. VerifyOrReturn(IsThreadEnabled() && mpStatusChangeCallback != nullptr); - ByteSpan datasetTLV; Thread::OperationalDataset dataset; uint8_t extpanid[Thread::kSizeExtendedPanId]; // If we have not provisioned any Thread network, return the status from last network scan, // If we have provisioned a network, we assume the ot-br-posix is activitely connecting to that network. - CHIP_ERROR err = ThreadStackMgrImpl().GetThreadProvision(datasetTLV); + CHIP_ERROR err = ThreadStackMgrImpl().GetThreadProvision(dataset); if (err != CHIP_NO_ERROR) { ChipLogError(DeviceLayer, "Failed to get configured network when updating network status: %s", err.AsString()); return; } - VerifyOrReturn(dataset.Init(datasetTLV) == CHIP_NO_ERROR); // The Thread network is not enabled, but has a different extended pan id. VerifyOrReturn(dataset.GetExtendedPanId(extpanid) == CHIP_NO_ERROR); diff --git a/src/platform/Linux/ThreadStackManagerImpl.h b/src/platform/Linux/ThreadStackManagerImpl.h index 431812c8788238..829cbcd14f45a0 100644 --- a/src/platform/Linux/ThreadStackManagerImpl.h +++ b/src/platform/Linux/ThreadStackManagerImpl.h @@ -54,7 +54,7 @@ class ThreadStackManagerImpl : public ThreadStackManager void _OnPlatformEvent(const ChipDeviceEvent * event); - CHIP_ERROR _GetThreadProvision(ByteSpan & netInfo); + CHIP_ERROR _GetThreadProvision(Thread::OperationalDataset & dataset); CHIP_ERROR _SetThreadProvision(ByteSpan netInfo); diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp index ad25ce1194d8b2..0f98e42b1467c4 100644 --- a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp +++ b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp @@ -37,14 +37,12 @@ namespace NetworkCommissioning { CHIP_ERROR GenericThreadDriver::Init(Internal::BaseDriver::NetworkStatusChangeCallback * statusChangeCallback) { - ByteSpan currentProvision; ThreadStackMgrImpl().SetNetworkStatusChangeCallback(statusChangeCallback); VerifyOrReturnError(ThreadStackMgrImpl().IsThreadAttached(), CHIP_NO_ERROR); - VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, CHIP_NO_ERROR); + VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(mStagingNetwork) == CHIP_NO_ERROR, CHIP_NO_ERROR); - mSavedNetwork.Init(currentProvision); - mStagingNetwork.Init(currentProvision); + mSavedNetwork.Init(mStagingNetwork.AsByteSpan()); return CHIP_NO_ERROR; } @@ -182,14 +180,12 @@ bool GenericThreadDriver::ThreadNetworkIterator::Next(Network & item) item.connected = false; exhausted = true; - ByteSpan currentProvision; Thread::OperationalDataset currentDataset; uint8_t enabledExtPanId[Thread::kSizeExtendedPanId]; // The Thread network is not actually enabled. VerifyOrReturnError(ConnectivityMgrImpl().IsThreadAttached(), true); - VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentProvision) == CHIP_NO_ERROR, true); - VerifyOrReturnError(currentDataset.Init(currentProvision) == CHIP_NO_ERROR, true); + VerifyOrReturnError(ThreadStackMgrImpl().GetThreadProvision(currentDataset) == CHIP_NO_ERROR, true); // The Thread network is not enabled, but has a different extended pan id. VerifyOrReturnError(currentDataset.GetExtendedPanId(enabledExtPanId) == CHIP_NO_ERROR, true); VerifyOrReturnError(memcmp(extpanid, enabledExtPanId, kSizeExtendedPanId) == 0, true); diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 090075f1c6aa8b..f9850cff6a8c31 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -304,7 +304,7 @@ bool GenericThreadStackManagerImpl_OpenThread::_IsThreadProvisioned(v } template -CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetThreadProvision(ByteSpan & netInfo) +CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetThreadProvision(Thread::OperationalDataset & dataset) { VerifyOrReturnError(Impl()->IsThreadProvisioned(), CHIP_ERROR_INCORRECT_STATE); otOperationalDatasetTlvs datasetTlv; @@ -317,8 +317,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetThreadProvis return MapOpenThreadError(otErr); } - ReturnErrorOnFailure(mActiveDataset.Init(ByteSpan(datasetTlv.mTlvs, datasetTlv.mLength))); - netInfo = mActiveDataset.AsByteSpan(); + ReturnErrorOnFailure(dataset.Init(ByteSpan(datasetTlv.mTlvs, datasetTlv.mLength))); return CHIP_NO_ERROR; } @@ -1853,24 +1852,23 @@ void GenericThreadStackManagerImpl_OpenThread::_UpdateNetworkStatus() ByteSpan datasetTLV; Thread::OperationalDataset dataset; - uint8_t extpanid[chip::Thread::kSizeExtendedPanId]; + ByteSpan extpanid; // If we have not provisioned any Thread network, return the status from last network scan, // If we have provisioned a network, we assume the ot-br-posix is activitely connecting to that network. - ReturnOnFailure(ThreadStackMgrImpl().GetThreadProvision(datasetTLV)); - ReturnOnFailure(dataset.Init(datasetTLV)); + ReturnOnFailure(ThreadStackMgrImpl().GetThreadProvision(dataset)); // The Thread network is not enabled, but has a different extended pan id. - ReturnOnFailure(dataset.GetExtendedPanId(extpanid)); + ReturnOnFailure(dataset.GetExtendedPanIdAsByteSpan(extpanid)); // If we don't have a valid dataset, we are not attempting to connect the network. // We have already connected to the network, thus return success. if (ThreadStackMgrImpl().IsThreadAttached()) { - mpStatusChangeCallback->OnNetworkingStatusChange(Status::kSuccess, MakeOptional(ByteSpan(extpanid)), NullOptional); + mpStatusChangeCallback->OnNetworkingStatusChange(Status::kSuccess, MakeOptional(extpanid), NullOptional); } else { - mpStatusChangeCallback->OnNetworkingStatusChange(Status::kNetworkNotFound, MakeOptional(ByteSpan(extpanid)), + mpStatusChangeCallback->OnNetworkingStatusChange(Status::kNetworkNotFound, MakeOptional(extpanid), MakeOptional(static_cast(OT_ERROR_DETACHED))); } } diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h index 7cd8dac1a4fae9..af1b5c25a0352a 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h @@ -87,7 +87,7 @@ class GenericThreadStackManagerImpl_OpenThread bool _IsThreadProvisioned(void); bool _IsThreadAttached(void); - CHIP_ERROR _GetThreadProvision(ByteSpan & netInfo); + CHIP_ERROR _GetThreadProvision(Thread::OperationalDataset & dataset); CHIP_ERROR _SetThreadProvision(ByteSpan netInfo); CHIP_ERROR _AttachToThreadNetwork(ByteSpan netInfo, NetworkCommissioning::Internal::WirelessDriver::ConnectCallback * callback); void _OnThreadAttachFinished(void); @@ -149,8 +149,7 @@ class GenericThreadStackManagerImpl_OpenThread // ===== Private members for use by this class only. otInstance * mOTInst; - uint64_t mOverrunCount = 0; - Thread::OperationalDataset mActiveDataset = {}; + uint64_t mOverrunCount = 0; NetworkCommissioning::ThreadDriver::ScanCallback * mpScanCallback; NetworkCommissioning::Internal::WirelessDriver::ConnectCallback * mpConnectCallback;