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.

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

Remove unneeded/ambiguous CompressedFabricId usages #18459

4 changes: 2 additions & 2 deletions src/app/CASESessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion src/app/CASESessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class CASESessionManager

void ReleaseSession(PeerId peerId);

void ReleaseSessionsForFabric(CompressedFabricId compressedFabricId);
void ReleaseSessionsForFabric(FabricIndex fabricIndex);

void ReleaseAllSessions();

Expand Down
6 changes: 3 additions & 3 deletions src/app/OperationalDeviceProxyPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Expand Down
17 changes: 10 additions & 7 deletions src/app/clusters/bindings/BindingManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/app/clusters/bindings/BindingManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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));
Expand All @@ -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);
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved

emberAfPrintln(EMBER_AF_PRINT_DEBUG,
"OpCreds: Fabric index 0x%x was retrieved from storage. FabricId 0x" ChipLogFormatX64
", NodeId 0x" ChipLogFormatX64 ", VendorId 0x%04X",
Expand All @@ -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",
Expand Down
42 changes: 35 additions & 7 deletions src/app/server/Server.h
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ class Server
ServerTransportMgr * mTransports;
};

class ServerFabricDelegate final : public FabricTableDelegate
class ServerFabricDelegate final : public chip::FabricTable::Delegate
{
public:
ServerFabricDelegate() {}
Expand All @@ -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;
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved
auto & sessionManager = mServer->GetSecureSessionManager();
sessionManager.FabricRemoved(fabricIndex);

// Remove all CASE session resumption state
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved
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());
}
}

{
Expand All @@ -324,15 +343,24 @@ class Server
{
while (count)
{
Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, --count);
(void) Access::GetAccessControl().DeleteEntry(nullptr, fabricIndex, --count);
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
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;
Expand Down
2 changes: 1 addition & 1 deletion src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,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
Expand Down
14 changes: 13 additions & 1 deletion src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
tcarmelveilleux marked this conversation as resolved.
Show resolved Hide resolved
stateParams.fabricTableDelegate = delegate.get();
ReturnErrorOnFailure(stateParams.fabricTable->AddFabricDelegate(stateParams.fabricTableDelegate));
delegate.release();

ReturnErrorOnFailure(stateParams.sessionMgr->Init(stateParams.systemLayer, stateParams.transportMgr,
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 14 additions & 6 deletions src/controller/CHIPDeviceControllerFactory.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion src/controller/CHIPDeviceControllerSystemState.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Loading