Skip to content

Commit

Permalink
[thread/cleanup] Change GetThreadProvision to return OperationalDatas…
Browse files Browse the repository at this point in the history
…et (#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 <commits@restyled.io>
  • Loading branch information
2 people authored and pull[bot] committed Oct 27, 2023
1 parent 0eea1e0 commit d78f43e
Show file tree
Hide file tree
Showing 8 changed files with 41 additions and 47 deletions.
10 changes: 7 additions & 3 deletions src/include/platform/ThreadStackManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ struct TextEntry;
struct DnssdService;
} // namespace Dnssd

namespace Thread {
class OperationalDataset;
} // namespace Thread

namespace DeviceLayer {

class PlatformManagerImpl;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -342,9 +346,9 @@ inline bool ThreadStackManager::IsThreadAttached()
return static_cast<ImplClass *>(this)->_IsThreadAttached();
}

inline CHIP_ERROR ThreadStackManager::GetThreadProvision(ByteSpan & netInfo)
inline CHIP_ERROR ThreadStackManager::GetThreadProvision(Thread::OperationalDataset & dataset)
{
return static_cast<ImplClass *>(this)->_GetThreadProvision(netInfo);
return static_cast<ImplClass *>(this)->_GetThreadProvision(dataset);
}

inline CHIP_ERROR ThreadStackManager::SetThreadProvision(ByteSpan netInfo)
Expand Down
24 changes: 15 additions & 9 deletions src/lib/support/ThreadOperationalDataset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<const uint8_t *>(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<const uint8_t *>(tlv->GetValue()), tlv->GetLength());
return CHIP_NO_ERROR;
}

CHIP_ERROR OperationalDataset::SetExtendedPanId(const uint8_t (&aExtendedPanId)[kSizeExtendedPanId])
Expand Down
10 changes: 3 additions & 7 deletions src/platform/Linux/NetworkCommissioningThreadDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down
11 changes: 3 additions & 8 deletions src/platform/Linux/ThreadStackManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -257,23 +257,20 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(ByteSpan & netInfo)

if (response == nullptr)
{
netInfo = ByteSpan();
return CHIP_ERROR_KEY_NOT_FOUND;
}

std::unique_ptr<GVariant, GVariantDeleter> tupleContent(g_variant_get_child_value(response.get(), 0));

if (tupleContent == nullptr)
{
netInfo = ByteSpan();
return CHIP_ERROR_KEY_NOT_FOUND;
}

std::unique_ptr<GVariant, GVariantDeleter> value(g_variant_get_variant(tupleContent.get()));

if (value == nullptr)
{
netInfo = ByteSpan();
return CHIP_ERROR_KEY_NOT_FOUND;
}

Expand All @@ -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;
}
Expand Down Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion src/platform/Linux/ThreadStackManagerImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ bool GenericThreadStackManagerImpl_OpenThread<ImplClass>::_IsThreadProvisioned(v
}

template <class ImplClass>
CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_GetThreadProvision(ByteSpan & netInfo)
CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_GetThreadProvision(Thread::OperationalDataset & dataset)
{
VerifyOrReturnError(Impl()->IsThreadProvisioned(), CHIP_ERROR_INCORRECT_STATE);
otOperationalDatasetTlvs datasetTlv;
Expand All @@ -317,8 +317,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_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;
}
Expand Down Expand Up @@ -1853,24 +1852,23 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::_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<int32_t>(OT_ERROR_DETACHED)));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit d78f43e

Please sign in to comment.