Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ICD Storage #30808

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Improve ICD Storage #30808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db92cb1
Add ICD getEntry API to retrive the ICDClientInfo
yunhanw-google Dec 5, 2023
659438b
move iterator out from ICDStorage
yunhanw-google Dec 5, 2023
418b124
Revert "move iterator out from ICDStorage"
yunhanw-google Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/app/icd/client/DefaultICDClientStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,11 @@ CHIP_ERROR DefaultICDClientStorage::SetKey(ICDClientInfo & clientInfo, const Byt
return mpKeyStore->CreateKey(keyMaterial, clientInfo.shared_key);
}

void DefaultICDClientStorage::RemoveKey(ICDClientInfo & clientInfo)
{
mpKeyStore->DestroyKey(clientInfo.shared_key);
}

CHIP_ERROR DefaultICDClientStorage::SerializeToTlv(TLV::TLVWriter & writer, const std::vector<ICDClientInfo> & clientInfoVector)
{
TLV::TLVType arrayType;
Expand Down Expand Up @@ -385,6 +390,23 @@ CHIP_ERROR DefaultICDClientStorage::UpdateEntryCountForFabric(FabricIndex fabric
backingBuffer.Get(), static_cast<uint16_t>(len));
}

CHIP_ERROR DefaultICDClientStorage::GetEntry(const ScopedNodeId & peerNode, ICDClientInfo & clientInfo)
{
size_t clientInfoSize = 0;
std::vector<ICDClientInfo> clientInfoVector;
ReturnErrorOnFailure(Load(peerNode.GetFabricIndex(), clientInfoVector, clientInfoSize));
IgnoreUnusedVariable(clientInfoSize);
for (auto & info : clientInfoVector)
{
if (peerNode.GetNodeId() == info.peer_node.GetNodeId())
{
clientInfo = info;
return CHIP_NO_ERROR;
}
}
return CHIP_ERROR_NOT_FOUND;
}

CHIP_ERROR DefaultICDClientStorage::DeleteEntry(const ScopedNodeId & peerNode)
{
size_t clientInfoSize = 0;
Expand All @@ -395,7 +417,7 @@ CHIP_ERROR DefaultICDClientStorage::DeleteEntry(const ScopedNodeId & peerNode)
{
if (peerNode.GetNodeId() == it->peer_node.GetNodeId())
{
mpKeyStore->DestroyKey(it->shared_key);
RemoveKey(*it);
it = clientInfoVector.erase(it);
break;
}
Expand Down Expand Up @@ -430,7 +452,7 @@ CHIP_ERROR DefaultICDClientStorage::DeleteAllEntries(FabricIndex fabricIndex)
IgnoreUnusedVariable(clientInfoSize);
for (auto & clientInfo : clientInfoVector)
{
mpKeyStore->DestroyKey(clientInfo.shared_key);
RemoveKey(clientInfo);
}
ReturnErrorOnFailure(
mpClientInfoStore->SyncDeleteKeyValue(DefaultStorageKeyAllocator::ICDClientInfoKey(fabricIndex).KeyName()));
Expand Down
6 changes: 5 additions & 1 deletion src/app/icd/client/DefaultICDClientStorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,13 @@ class DefaultICDClientStorage : public ICDClientStorage

CHIP_ERROR SetKey(ICDClientInfo & clientInfo, const ByteSpan keyData) override;

void RemoveKey(ICDClientInfo & clientInfo) override;

CHIP_ERROR StoreEntry(const ICDClientInfo & clientInfo) override;

CHIP_ERROR DeleteEntry(const ScopedNodeId & peerNodeId) override;
CHIP_ERROR GetEntry(const ScopedNodeId & peerNode, ICDClientInfo & clientInfo) override;

CHIP_ERROR DeleteEntry(const ScopedNodeId & peerNode) override;

CHIP_ERROR DeleteAllEntries(FabricIndex fabricIndex) override;

Expand Down
23 changes: 19 additions & 4 deletions src/app/icd/client/ICDClientStorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class ICDClientStorage
* Called during ICD device registration in commissioning, commissioner/controller
* provides raw key data, the shared key handle in clientInfo is updated based upon raw key data
*
* @param[inout] aICDClientInfo the ICD Client information to be updated with keyData and be saved
* @param[inout] clientInfo the ICD Client information to be updated with keyData and be saved
* @param[in] aKeyData raw key data provided by application
*/
virtual CHIP_ERROR SetKey(ICDClientInfo & clientInfo, const ByteSpan keyData) = 0;
Expand All @@ -61,16 +61,31 @@ class ICDClientStorage
* Store updated ICD ClientInfo to storage when ICD registration completes or check-in message
* comes.
*
* @param[in] aICDClientInfo the updated ICD Client Info.
* @param[in] clientInfo the updated ICD Client Info.
*/
virtual CHIP_ERROR StoreEntry(const ICDClientInfo & clientInfo) = 0;

/**
* Remove ICD key from clientInfo when ICD registration fails
*
* @param[inout] clientInfo the updated ICD Client Info.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in what way? The one implementation here is not updating anything about the clientinfo, as far as I can tell.

*/
virtual void RemoveKey(ICDClientInfo & clientInfo) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should this do if there is no key for that clientinfo to start with?


/**
* Get ICD ClientInfo from storage
* One user case is to retrieve UserActiveModeTriggerHint and inform how user how to wake up sleepy device.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/user/use/

And

s/inform how user how to wake up sleepy/inform the user how to wake up a sleepy/ (note: multiple grammar fixes here).

* @param[in] peerNode scoped node with peer node id and fabric index
* @param[out] clientInfo the ICD Client Info.
*/
virtual CHIP_ERROR GetEntry(const ScopedNodeId & peerNode, ICDClientInfo & clientInfo) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no info for this node id, what's the expected way for the callee to communicate that? Needs to be documented.


/**
* Delete ICD Client persistent information associated with the specified scoped node Id.
* when ICD device is unpaired/removed, the corresponding entry in ICD storage is removed.
* @param aPeerNodeId scoped node with peer node id and fabric index
* @param peerNode scoped node with peer node id and fabric index
*/
virtual CHIP_ERROR DeleteEntry(const ScopedNodeId & peerNodeId) = 0;
virtual CHIP_ERROR DeleteEntry(const ScopedNodeId & peerNode) = 0;

/**
* Remove all ICDClient persistent information associated with the specified
Expand Down
7 changes: 5 additions & 2 deletions src/app/tests/TestDefaultICDClientStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ void TestClientInfoCount(nlTestSuite * apSuite, void * apContext)
FabricIndex fabricId = 1;
NodeId nodeId1 = 6666;
NodeId nodeId2 = 6667;

NodeId unknownNodeId = 6668;
TestPersistentStorageDelegate clientInfoStorage;
TestSessionKeystoreImpl keystore;

Expand Down Expand Up @@ -89,12 +89,15 @@ void TestClientInfoCount(nlTestSuite * apSuite, void * apContext)
err = manager.StoreEntry(clientInfo3);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

ICDClientInfo clientInfo;
manager.GetEntry(clientInfo3.peer_node, clientInfo);
NL_TEST_ASSERT(apSuite, clientInfo.peer_node.GetNodeId() == nodeId1);
NL_TEST_ASSERT(apSuite, CHIP_ERROR_NOT_FOUND == manager.GetEntry(ScopedNodeId(unknownNodeId, fabricId), clientInfo));
// Make sure iterator counts correctly
auto * iterator = manager.IterateICDClientInfo();
// same nodeId for clientInfo2 and clientInfo3, so the new one replace old one
NL_TEST_ASSERT(apSuite, iterator->Count() == 2);

ICDClientInfo clientInfo;
NL_TEST_ASSERT(apSuite, iterator->Next(clientInfo));
NL_TEST_ASSERT(apSuite, clientInfo.peer_node.GetNodeId() == nodeId2);
NL_TEST_ASSERT(apSuite, iterator->Next(clientInfo));
Expand Down
Loading