Remove unneeded/ambiguous CompressedFabricId usages (#18459)

* Remove unneeded/ambiguous CompressedFabricId usages Within the SDK, FabricIndex is the only unambiguous identifier. Many methods that emanated from FabricTableDelegate::OnFabricDeletedFromStorage(chip::CompressedFabricId, chip::FabricIndex) perpetuated use of the compressed fabric ID, because at the bottom, we have CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId) - Replace CASESessionManager::ReleaseSessionsForFabric argument from CompressedFabricId to FabricIndex. - Rework FabricTableDelegate to remove need to pass any CompressedFabricId. - Replace all downstream usages of CompressedFabricId with FabricIndex and FabricTable reference. - Make FabricTableDelegate calls symmetrical in arguments - Make FabricTableDelegate an inner class of FabricTable to remove a friend relationship - Clarify when adding a FabricTableDelegate causes its deletion due to ownership changes - Add session resumption state clearing on fabric removal Fixes #18435 Issue #18436 * Restyled by clang-format * Fix clang CI * Apply review comments * Restyled by clang-format * Address review comments * Remove missed field * Please the gods of Tidy Co-authored-by: Restyled.io <[email protected]>
project-chip · May 17, 2022 · 4de9692 · 4de9692
1 parent 51cbb53
commit 4de9692
Show file tree

Hide file tree

Showing 13 changed files with 196 additions and 106 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<unsigned>(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
@@ -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<unsigned>(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<unsigned>(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
@@ -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
@@ -167,7 +167,8 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
 
     auto delegate = chip::Platform::MakeUnique<ControllerFabricDelegate>();
     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
@@ -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
@@ -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