diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index 1d50ecaa820c6b..6af089c9559615 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -63,9 +63,9 @@ void CASESessionManager::ReleaseSession(PeerId peerId) ReleaseSession(FindExistingSession(peerId)); } -void CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId compressedFabricId) +void CASESessionManager::ReleaseSessionsForFabric(FabricIndex fabricIndex) { - mConfig.devicePool->ReleaseDevicesForFabric(compressedFabricId); + mConfig.devicePool->ReleaseDevicesForFabric(fabricIndex); } void CASESessionManager::ReleaseAllSessions() diff --git a/src/app/CASESessionManager.h b/src/app/CASESessionManager.h index 3d454b5fe2517f..32e50047153e9b 100644 --- a/src/app/CASESessionManager.h +++ b/src/app/CASESessionManager.h @@ -67,7 +67,7 @@ class CASESessionManager void ReleaseSession(PeerId peerId); - void ReleaseSessionsForFabric(CompressedFabricId compressedFabricId); + void ReleaseSessionsForFabric(FabricIndex fabricIndex); void ReleaseAllSessions(); diff --git a/src/app/OperationalDeviceProxyPool.h b/src/app/OperationalDeviceProxyPool.h index 2b504c07b4b04b..e8092e1bab5340 100644 --- a/src/app/OperationalDeviceProxyPool.h +++ b/src/app/OperationalDeviceProxyPool.h @@ -35,7 +35,7 @@ class OperationalDeviceProxyPoolDelegate virtual OperationalDeviceProxy * FindDevice(PeerId peerId) = 0; - virtual void ReleaseDevicesForFabric(CompressedFabricId compressedFabricId) = 0; + virtual void ReleaseDevicesForFabric(FabricIndex fabricIndex) = 0; virtual void ReleaseAllDevices() = 0; @@ -76,10 +76,10 @@ class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate return foundDevice; } - void ReleaseDevicesForFabric(CompressedFabricId compressedFabricId) override + void ReleaseDevicesForFabric(FabricIndex fabricIndex) override { mDevicePool.ForEachActiveObject([&](auto * activeDevice) { - if (activeDevice->GetPeerId().GetCompressedFabricId() == compressedFabricId) + if (activeDevice->GetFabricIndex() == fabricIndex) { Release(activeDevice); } diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 6f8b301b90f18f..b95ff3cf51ae5f 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -23,9 +23,9 @@ namespace { -class BindingFabricTableDelegate : public chip::FabricTableDelegate +class BindingFabricTableDelegate : public chip::FabricTable::Delegate { - void OnFabricDeletedFromStorage(chip::CompressedFabricId compressedFabricId, chip::FabricIndex fabricIndex) + void OnFabricDeletedFromStorage(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override { chip::BindingTable & bindingTable = chip::BindingTable::GetInstance(); auto iter = bindingTable.begin(); @@ -40,14 +40,14 @@ class BindingFabricTableDelegate : public chip::FabricTableDelegate ++iter; } } - chip::BindingManager::GetInstance().FabricRemoved(compressedFabricId, fabricIndex); + chip::BindingManager::GetInstance().FabricRemoved(fabricIndex); } // Intentionally left blank - void OnFabricRetrievedFromStorage(chip::FabricInfo * fabricInfo) {} + void OnFabricRetrievedFromStorage(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override {} // Intentionally left blank - void OnFabricPersistedToStorage(chip::FabricInfo * fabricInfo) {} + void OnFabricPersistedToStorage(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override {} }; BindingFabricTableDelegate gFabricTableDelegate; @@ -180,10 +180,13 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err mInitParams.mCASESessionManager->ReleaseSession(peerId); } -void BindingManager::FabricRemoved(CompressedFabricId compressedFabricId, FabricIndex fabricIndex) +void BindingManager::FabricRemoved(FabricIndex fabricIndex) { mPendingNotificationMap.RemoveAllEntriesForFabric(fabricIndex); - mInitParams.mCASESessionManager->ReleaseSessionsForFabric(compressedFabricId); + + // TODO(#18436): NOC cluster should handle fabric removal without needing binding manager + // to execute such a release. Currently not done because paths were not tested. + mInitParams.mCASESessionManager->ReleaseSessionsForFabric(fabricIndex); } CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, ClusterId cluster, void * context) diff --git a/src/app/clusters/bindings/BindingManager.h b/src/app/clusters/bindings/BindingManager.h index 28fc11959e62ec..91455a94c5e977 100644 --- a/src/app/clusters/bindings/BindingManager.h +++ b/src/app/clusters/bindings/BindingManager.h @@ -104,7 +104,7 @@ class BindingManager * Notifies the BindingManager that a fabric is removed from the device * */ - void FabricRemoved(CompressedFabricId compressedId, FabricIndex fabricIndex); + void FabricRemoved(FabricIndex fabricIndex); /* * Notify a cluster change to **all** bound devices associated with the (endpoint, cluster) tuple. diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 7c02d23d3730af..e66f42fcf032d2 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -300,7 +300,7 @@ void FailSafeCleanup(const chip::DeviceLayer::ChipDeviceEvent * event) FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex); VerifyOrReturn(fabricInfo != nullptr); - caseSessionManager->ReleaseSessionsForFabric(fabricInfo->GetCompressedId()); + caseSessionManager->ReleaseSessionsForFabric(fabricInfo->GetFabricIndex()); } SessionManager & sessionMgr = Server::GetInstance().GetSecureSessionManager(); @@ -354,11 +354,11 @@ void fabricListChanged() // only that should be modifed to perosst/read/write fabrics. // TODO: Once attributes are persisted, implement reading/writing/manipulation fabrics around that and remove fabricTable // logic. -class OpCredsFabricTableDelegate : public FabricTableDelegate +class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate { // Gets called when a fabric is deleted from KVS store - void OnFabricDeletedFromStorage(CompressedFabricId compressedFabricId, FabricIndex fabricIndex) override + void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override { emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Fabric index 0x%x was deleted from fabric storage.", static_cast(fabricIndex)); @@ -380,8 +380,12 @@ class OpCredsFabricTableDelegate : public FabricTableDelegate } // Gets called when a fabric is loaded into the FabricTable from storage - void OnFabricRetrievedFromStorage(FabricInfo * fabric) override + void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override { + FabricInfo * fabric = fabricTable.FindFabricWithIndex(fabricIndex); + // Safety check, but should not happen by the code paths involved + VerifyOrReturn(fabric != nullptr); + emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Fabric index 0x%x was retrieved from storage. FabricId 0x" ChipLogFormatX64 ", NodeId 0x" ChipLogFormatX64 ", VendorId 0x%04X", @@ -391,8 +395,12 @@ class OpCredsFabricTableDelegate : public FabricTableDelegate } // Gets called when a fabric in FabricTable is persisted to storage - void OnFabricPersistedToStorage(FabricInfo * fabric) override + void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override { + FabricInfo * fabric = fabricTable.FindFabricWithIndex(fabricIndex); + // Safety check, but should not happen by the code paths involved + VerifyOrReturn(fabric != nullptr); + emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Fabric index 0x%x was persisted to storage. FabricId " ChipLogFormatX64 ", NodeId " ChipLogFormatX64 ", VendorId 0x%04X", diff --git a/src/app/server/Server.h b/src/app/server/Server.h index 86d5114e1815e3..b31e8896b3ec4f 100644 --- a/src/app/server/Server.h +++ b/src/app/server/Server.h @@ -290,7 +290,7 @@ class Server ServerTransportMgr * mTransports; }; - class ServerFabricDelegate final : public FabricTableDelegate + class ServerFabricDelegate final : public chip::FabricTable::Delegate { public: ServerFabricDelegate() {} @@ -303,16 +303,35 @@ class Server return CHIP_NO_ERROR; }; - void OnFabricDeletedFromStorage(CompressedFabricId compressedId, FabricIndex fabricIndex) override + void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override { - (void) compressedId; + (void) fabricTable; auto & sessionManager = mServer->GetSecureSessionManager(); sessionManager.FabricRemoved(fabricIndex); + // Remove all CASE session resumption state + auto * sessionResumptionStorage = mServer->GetSessionResumptionStorage(); + if (sessionResumptionStorage != nullptr) + { + CHIP_ERROR err = sessionResumptionStorage->DeleteAll(fabricIndex); + if (err != CHIP_NO_ERROR) + { + ChipLogError(AppServer, + "Warning, failed to delete session resumption state for fabric index 0x%x: %" CHIP_ERROR_FORMAT, + static_cast(fabricIndex), err.Format()); + } + } + Credentials::GroupDataProvider * groupDataProvider = mServer->GetGroupDataProvider(); if (groupDataProvider != nullptr) { - groupDataProvider->RemoveFabric(fabricIndex); + CHIP_ERROR err = groupDataProvider->RemoveFabric(fabricIndex); + if (err != CHIP_NO_ERROR) + { + ChipLogError(AppServer, + "Warning, failed to delete GroupDataProvider state for fabric index 0x%x: %" CHIP_ERROR_FORMAT, + static_cast(fabricIndex), err.Format()); + } } { @@ -324,15 +343,24 @@ class Server { while (count) { - Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, --count); + (void) Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, --count); } } } app::EventManagement::GetInstance().FabricRemoved(fabricIndex); }; - void OnFabricRetrievedFromStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } - void OnFabricPersistedToStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override + { + (void) fabricTable; + (void) fabricIndex; + } + + void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override + { + (void) fabricTable; + (void) fabricIndex; + } private: Server * mServer = nullptr; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index f910f87fe3f3a1..06d33134130ba9 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -225,7 +225,7 @@ CHIP_ERROR DeviceController::Shutdown() { // Shut down any ongoing CASE session activity we have. We're going to // assume that all sessions for our fabric belong to us here. - mSystemState->CASESessionMgr()->ReleaseSessionsForFabric(GetCompressedFabricId()); + mSystemState->CASESessionMgr()->ReleaseSessionsForFabric(mFabricInfo->GetFabricIndex()); // TODO: The CASE session manager does not shut down existing CASE // sessions. It just shuts down any ongoing CASE session establishment diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 9b45c0fe25d6b6..d85a878104e216 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -167,7 +167,8 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) auto delegate = chip::Platform::MakeUnique(); ReturnErrorOnFailure(delegate->Init(stateParams.sessionMgr, stateParams.groupDataProvider)); - ReturnErrorOnFailure(stateParams.fabricTable->AddFabricDelegate(delegate.get())); + stateParams.fabricTableDelegate = delegate.get(); + ReturnErrorOnFailure(stateParams.fabricTable->AddFabricDelegate(stateParams.fabricTableDelegate)); delegate.release(); ReturnErrorOnFailure(stateParams.sessionMgr->Init(stateParams.systemLayer, stateParams.transportMgr, @@ -322,6 +323,17 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown() ChipLogDetail(Controller, "Shutting down the System State, this will teardown the CHIP Stack"); + if (mFabricTableDelegate != nullptr) + { + if (mFabrics != nullptr) + { + mFabrics->RemoveFabricDelegate(mFabricTableDelegate); + } + + chip::Platform::Delete(mFabricTableDelegate); + mFabricTableDelegate = nullptr; + } + if (mCASEServer != nullptr) { chip::Platform::Delete(mCASEServer); diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index e8b8fa62c665fb..0862e48a8dfff1 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -151,11 +151,9 @@ class DeviceControllerFactory // const DeviceControllerSystemState * GetSystemState() const { return mSystemState; } - class ControllerFabricDelegate final : public FabricTableDelegate + class ControllerFabricDelegate final : public chip::FabricTable::Delegate { public: - ControllerFabricDelegate() : FabricTableDelegate(true) {} - CHIP_ERROR Init(SessionManager * sessionManager, Credentials::GroupDataProvider * groupDataProvider) { VerifyOrReturnError(sessionManager != nullptr, CHIP_ERROR_INVALID_ARGUMENT); @@ -166,8 +164,10 @@ class DeviceControllerFactory return CHIP_NO_ERROR; }; - void OnFabricDeletedFromStorage(CompressedFabricId compressedId, FabricIndex fabricIndex) override + void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override { + (void) fabricTable; + if (mSessionManager != nullptr) { mSessionManager->FabricRemoved(fabricIndex); @@ -178,9 +178,17 @@ class DeviceControllerFactory } }; - void OnFabricRetrievedFromStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override + { + (void) fabricTable; + (void) fabricIndex; + } - void OnFabricPersistedToStorage(FabricInfo * fabricInfo) override { (void) fabricInfo; } + void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override + { + (void) fabricTable; + (void) fabricIndex; + } private: SessionManager * mSessionManager = nullptr; diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index f3c9efc6e90f70..5b1cb9b6ff2ecb 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -91,6 +91,7 @@ struct DeviceControllerSystemStateParams CASESessionManager * caseSessionManager = nullptr; OperationalDevicePool * operationalDevicePool = nullptr; CASEClientPool * caseClientPool = nullptr; + FabricTable::Delegate * fabricTableDelegate = nullptr; }; // A representation of the internal state maintained by the DeviceControllerFactory @@ -109,7 +110,7 @@ class DeviceControllerSystemState mExchangeMgr(params.exchangeMgr), mMessageCounterManager(params.messageCounterManager), mFabrics(params.fabricTable), mCASEServer(params.caseServer), mCASESessionManager(params.caseSessionManager), mOperationalDevicePool(params.operationalDevicePool), mCASEClientPool(params.caseClientPool), - mGroupDataProvider(params.groupDataProvider) + mGroupDataProvider(params.groupDataProvider), mFabricTableDelegate(params.fabricTableDelegate) { #if CONFIG_NETWORK_LAYER_BLE mBleLayer = params.bleLayer; @@ -179,6 +180,7 @@ class DeviceControllerSystemState OperationalDevicePool * mOperationalDevicePool = nullptr; CASEClientPool * mCASEClientPool = nullptr; Credentials::GroupDataProvider * mGroupDataProvider = nullptr; + FabricTable::Delegate * mFabricTableDelegate = nullptr; // If mTempFabricTable is not null, it was created during // DeviceControllerFactory::InitSystemState and needs to be diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp index 94c1cc3c13ac53..cd32d304727443 100644 --- a/src/credentials/FabricTable.cpp +++ b/src/credentials/FabricTable.cpp @@ -457,15 +457,13 @@ CHIP_ERROR FabricInfo::VerifyCredentials(const ByteSpan & noc, const ByteSpan & FabricTable::~FabricTable() { - FabricTableDelegate * delegate = mDelegate; + // Remove all links to every delegate + FabricTable::Delegate * delegate = mDelegateListRoot; while (delegate) { - FabricTableDelegate * temp = delegate->mNext; - if (delegate->mOwnedByFabricTable) - { - chip::Platform::Delete(delegate); - } - delegate = temp; + FabricTable::Delegate * temp = delegate->next; + delegate->next = nullptr; + delegate = temp; } } @@ -525,26 +523,27 @@ FabricInfo * FabricTable::FindFabricWithCompressedId(CompressedFabricId fabricId return nullptr; } -CHIP_ERROR FabricTable::Store(FabricIndex index) +CHIP_ERROR FabricTable::Store(FabricIndex fabricIndex) { CHIP_ERROR err = CHIP_NO_ERROR; FabricInfo * fabric = nullptr; VerifyOrExit(mStorage != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - fabric = FindFabricWithIndex(index); + fabric = FindFabricWithIndex(fabricIndex); VerifyOrExit(fabric != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); err = fabric->CommitToStorage(mStorage); exit: - if (err == CHIP_NO_ERROR && mDelegate != nullptr) + if (err == CHIP_NO_ERROR && mDelegateListRoot != nullptr) { - ChipLogProgress(Discovery, "Fabric (%d) persisted to storage. Calling OnFabricPersistedToStorage", index); - FabricTableDelegate * delegate = mDelegate; + ChipLogProgress(Discovery, "Fabric (0x%x) persisted to storage. Calling OnFabricPersistedToStorage", + static_cast(fabricIndex)); + FabricTable::Delegate * delegate = mDelegateListRoot; while (delegate) { - delegate->OnFabricPersistedToStorage(fabric); - delegate = delegate->mNext; + delegate->OnFabricPersistedToStorage(*this, fabricIndex); + delegate = delegate->next; } } return err; @@ -558,12 +557,12 @@ CHIP_ERROR FabricTable::LoadFromStorage(FabricInfo * fabric) { ReturnErrorOnFailure(fabric->LoadFromStorage(mStorage)); - FabricTableDelegate * delegate = mDelegate; + FabricTable::Delegate * delegate = mDelegateListRoot; while (delegate) { - ChipLogProgress(Discovery, "Fabric (%d) loaded from storage", fabric->GetFabricIndex()); - delegate->OnFabricRetrievedFromStorage(fabric); - delegate = delegate->mNext; + ChipLogProgress(Discovery, "Fabric (0x%x) loaded from storage", static_cast(fabric->GetFabricIndex())); + delegate->OnFabricRetrievedFromStorage(*this, fabric->GetFabricIndex()); + delegate = delegate->next; } } return CHIP_NO_ERROR; @@ -696,15 +695,13 @@ CHIP_ERROR FabricTable::AddNewFabricInner(FabricInfo & newFabric, FabricIndex * return CHIP_ERROR_NO_MEMORY; } -CHIP_ERROR FabricTable::Delete(FabricIndex index) +CHIP_ERROR FabricTable::Delete(FabricIndex fabricIndex) { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - FabricInfo * fabric = FindFabricWithIndex(index); + FabricInfo * fabric = FindFabricWithIndex(fabricIndex); bool fabricIsInitialized = fabric != nullptr && fabric->IsInitialized(); - CompressedFabricId compressedFabricId = - fabricIsInitialized ? fabric->GetPeerId().GetCompressedFabricId() : kUndefinedCompressedFabricId; - CHIP_ERROR err = FabricInfo::DeleteFromStorage(mStorage, index); // Delete from storage regardless + CHIP_ERROR err = FabricInfo::DeleteFromStorage(mStorage, fabricIndex); // Delete from storage regardless if (!fabricIsInitialized) { // Make sure to return the error our API promises, not whatever storage @@ -725,14 +722,14 @@ CHIP_ERROR FabricTable::Delete(FabricIndex index) // and our fabric table was full, so there was no valid next index. We // have a single available index now, though; use it as // mNextAvailableFabricIndex. - mNextAvailableFabricIndex.SetValue(index); + mNextAvailableFabricIndex.SetValue(fabricIndex); } // If StoreFabricIndexInfo fails here, that's probably OK. When we try to // read things from storage later we will realize there is nothing for this // index. StoreFabricIndexInfo(); - if (mDelegate != nullptr) + if (mDelegateListRoot != nullptr) { if (mFabricCount == 0) { @@ -741,14 +738,15 @@ CHIP_ERROR FabricTable::Delete(FabricIndex index) else { mFabricCount--; - ChipLogProgress(Discovery, "Fabric (0x%x) deleted. Calling OnFabricDeletedFromStorage", static_cast(index)); + ChipLogProgress(Discovery, "Fabric (0x%x) deleted. Calling OnFabricDeletedFromStorage", + static_cast(fabricIndex)); } - FabricTableDelegate * delegate = mDelegate; + FabricTable::Delegate * delegate = mDelegateListRoot; while (delegate) { - delegate->OnFabricDeletedFromStorage(compressedFabricId, index); - delegate = delegate->mNext; + delegate->OnFabricDeletedFromStorage(*this, fabricIndex); + delegate = delegate->next; } } return CHIP_NO_ERROR; @@ -802,33 +800,63 @@ CHIP_ERROR FabricTable::Init(PersistentStorageDelegate * storage) return CHIP_NO_ERROR; } -CHIP_ERROR FabricTable::AddFabricDelegate(FabricTableDelegate * delegate) +CHIP_ERROR FabricTable::AddFabricDelegate(FabricTable::Delegate * delegate) { VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - for (FabricTableDelegate * iter = mDelegate; iter != nullptr; iter = iter->mNext) + for (FabricTable::Delegate * iter = mDelegateListRoot; iter != nullptr; iter = iter->next) { if (iter == delegate) { return CHIP_NO_ERROR; } } - delegate->mNext = mDelegate; - mDelegate = delegate; - ChipLogDetail(Discovery, "Add fabric pairing table delegate"); + delegate->next = mDelegateListRoot; + mDelegateListRoot = delegate; return CHIP_NO_ERROR; } +void FabricTable::RemoveFabricDelegate(FabricTable::Delegate * delegateToRemove) +{ + VerifyOrReturn(delegateToRemove != nullptr); + + if (delegateToRemove == mDelegateListRoot) + { + // Removing head of the list, keep link to next, may + // be nullptr. + mDelegateListRoot = mDelegateListRoot->next; + } + else + { + // Removing some other item: check if next, and + // remove the link, keeping its neighbour. + FabricTable::Delegate * currentNode = mDelegateListRoot; + + while (currentNode) + { + if (currentNode->next == delegateToRemove) + { + FabricTable::Delegate * temp = delegateToRemove->next; + currentNode->next = temp; + delegateToRemove->next = nullptr; + return; + } + + currentNode = currentNode->next; + } + } +} + namespace { // Increment a fabric index in a way that ensures that it stays in the valid // range [kMinValidFabricIndex, kMaxValidFabricIndex]. -FabricIndex NextFabricIndex(FabricIndex index) +FabricIndex NextFabricIndex(FabricIndex fabricIndex) { - if (index == kMaxValidFabricIndex) + if (fabricIndex == kMaxValidFabricIndex) { return kMinValidFabricIndex; } - return static_cast(index + 1); + return static_cast(fabricIndex + 1); } } // anonymous namespace diff --git a/src/credentials/FabricTable.h b/src/credentials/FabricTable.h index 5be4ca8fe10037..8fdacf020a5294 100644 --- a/src/credentials/FabricTable.h +++ b/src/credentials/FabricTable.h @@ -279,36 +279,6 @@ class DLL_EXPORT FabricInfo CHIP_ERROR SetCert(MutableByteSpan & dstCert, const ByteSpan & srcCert); }; -// Once attribute store has persistence implemented, FabricTable shoud be backed using -// attribute store so no need for this Delegate API anymore -// TODO: Reimplement FabricTable to only have one backing store. -class DLL_EXPORT FabricTableDelegate -{ - friend class FabricTable; - -public: - FabricTableDelegate(bool ownedByFabricTable = false) : mOwnedByFabricTable(ownedByFabricTable) {} - virtual ~FabricTableDelegate() {} - /** - * Gets called when a fabric is deleted from KVS store. - **/ - virtual void OnFabricDeletedFromStorage(CompressedFabricId compressedId, FabricIndex fabricIndex) = 0; - - /** - * Gets called when a fabric is loaded into Fabric Table from KVS store. - **/ - virtual void OnFabricRetrievedFromStorage(FabricInfo * fabricInfo) = 0; - - /** - * Gets called when a fabric in Fabric Table is persisted to KVS store. - **/ - virtual void OnFabricPersistedToStorage(FabricInfo * fabricInfo) = 0; - -private: - FabricTableDelegate * mNext = nullptr; - bool mOwnedByFabricTable = false; -}; - /** * Iterates over valid fabrics within a list */ @@ -378,15 +348,43 @@ class ConstFabricIterator class DLL_EXPORT FabricTable { +public: + class DLL_EXPORT Delegate + { + public: + Delegate() {} + virtual ~Delegate() {} + + /** + * Gets called when a fabric is deleted, such as on FabricTable::Delete(). + **/ + virtual void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) = 0; + + /** + * Gets called when a fabric is loaded into Fabric Table from storage, such as + * during FabricTable::Init(). + **/ + virtual void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) = 0; + + /** + * Gets called when a fabric in Fabric Table is persisted to storage, such as + * on FabricTable::AddNewFabric(). + **/ + virtual void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) = 0; + + // Intrusive list pointer for FabricTable to manage the entries. + Delegate * next = nullptr; + }; + public: FabricTable() {} ~FabricTable(); - CHIP_ERROR Store(FabricIndex index); + CHIP_ERROR Store(FabricIndex fabricIndex); CHIP_ERROR LoadFromStorage(FabricInfo * info); // Returns CHIP_ERROR_NOT_FOUND if there is no fabric for that index. - CHIP_ERROR Delete(FabricIndex index); + CHIP_ERROR Delete(FabricIndex fabricIndex); void DeleteAllFabrics(); /** @@ -410,7 +408,8 @@ class DLL_EXPORT FabricTable FabricInfo * FindFabricWithCompressedId(CompressedFabricId fabricId); CHIP_ERROR Init(PersistentStorageDelegate * storage); - CHIP_ERROR AddFabricDelegate(FabricTableDelegate * delegate); + CHIP_ERROR AddFabricDelegate(FabricTable::Delegate * delegate); + void RemoveFabricDelegate(FabricTable::Delegate * delegate); uint8_t FabricCount() const { return mFabricCount; } @@ -455,7 +454,9 @@ class DLL_EXPORT FabricTable FabricInfo mStates[CHIP_CONFIG_MAX_FABRICS]; PersistentStorageDelegate * mStorage = nullptr; - FabricTableDelegate * mDelegate = nullptr; + // FabricTable::Delegate link to first node, since FabricTable::Delegate is a form + // of intrusive linked-list item. + FabricTable::Delegate * mDelegateListRoot = nullptr; // We may not have an mNextAvailableFabricIndex if our table is as large as // it can go and is full.