From 286017aa163e0c56015d108b1885d5f5d4df405b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 17 Mar 2022 22:54:21 -0400 Subject: [PATCH] Move CASESessionManager to CHIPDeviceControllerSystemState. (#16233) We should have one CASESessionManager, not one per DeviceController. Fixes https://github.com/project-chip/connectedhomeip/issues/16174 --- src/controller/CHIPDeviceController.cpp | 58 +++++-------------- src/controller/CHIPDeviceController.h | 13 +---- .../CHIPDeviceControllerFactory.cpp | 58 +++++++++++++++++++ .../CHIPDeviceControllerSystemState.h | 31 +++++++++- 4 files changed, 105 insertions(+), 55 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 36890f85cca211..c7553ba8a668bd 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -148,28 +148,6 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params) } } - DeviceProxyInitParams deviceInitParams = { - .sessionManager = params.systemState->SessionMgr(), - .exchangeMgr = params.systemState->ExchangeMgr(), - .idAllocator = &mIDAllocator, - .fabricTable = params.systemState->Fabrics(), - .clientPool = &mCASEClientPool, - .mrpLocalConfig = Optional::Value(GetLocalMRPConfig()), - }; - - CASESessionManagerConfig sessionManagerConfig = { - .sessionInitParams = deviceInitParams, -#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0 - .dnsCache = &mDNSCache, -#endif - .devicePool = &mDevicePool, - }; - - mCASESessionManager = chip::Platform::New(sessionManagerConfig); - VerifyOrReturnError(mCASESessionManager != nullptr, CHIP_ERROR_NO_MEMORY); - - ReturnErrorOnFailure(mCASESessionManager->Init(params.systemState->SystemLayer())); - mSystemState = params.systemState->Retain(); mState = State::Initialized; return CHIP_NO_ERROR; @@ -243,11 +221,9 @@ CHIP_ERROR DeviceController::Shutdown() if (mFabricInfo != nullptr) { - // Shut down any ongoing CASE session activity we have. Note that we - // own the mCASESessionManager, so shutting down everything on it is - // fine. If we ever end up sharing the CASE session manager with other - // DeviceController instances we may need to be more targeted here. - mCASESessionManager->ReleaseAllSessions(); + // 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()); // TODO: The CASE session manager does not shut down existing CASE // sessions. It just shuts down any ongoing CASE session establishment @@ -268,9 +244,6 @@ CHIP_ERROR DeviceController::Shutdown() mDNSResolver.Shutdown(); mDeviceDiscoveryDelegate = nullptr; - chip::Platform::Delete(mCASESessionManager); - mCASESessionManager = nullptr; - return CHIP_NO_ERROR; } @@ -288,14 +261,14 @@ void DeviceController::ReleaseOperationalDevice(NodeId remoteDeviceId) { VerifyOrReturn(mState == State::Initialized && mFabricInfo != nullptr, ChipLogError(Controller, "ReleaseOperationalDevice was called in incorrect state")); - mCASESessionManager->ReleaseSession(mFabricInfo->GetPeerIdForNode(remoteDeviceId)); + mSystemState->CASESessionMgr()->ReleaseSession(mFabricInfo->GetPeerIdForNode(remoteDeviceId)); } CHIP_ERROR DeviceController::DisconnectDevice(NodeId nodeId) { ChipLogProgress(Controller, "Force close session for node 0x%" PRIx64, nodeId); - OperationalDeviceProxy * proxy = mCASESessionManager->FindExistingSession(mFabricInfo->GetPeerIdForNode(nodeId)); + OperationalDeviceProxy * proxy = mSystemState->CASESessionMgr()->FindExistingSession(mFabricInfo->GetPeerIdForNode(nodeId)); if (proxy == nullptr) { ChipLogProgress(Controller, "Attempted to close a session that does not exist."); @@ -419,7 +392,7 @@ CHIP_ERROR DeviceController::GetPeerAddressAndPort(PeerId peerId, Inet::IPAddres { VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); Transport::PeerAddress peerAddr; - ReturnErrorOnFailure(mCASESessionManager->GetPeerAddress(peerId, peerAddr)); + ReturnErrorOnFailure(mSystemState->CASESessionMgr()->GetPeerAddress(peerId, peerAddr)); addr = peerAddr.GetIPAddress(); port = peerAddr.GetPort(); return CHIP_NO_ERROR; @@ -446,7 +419,7 @@ void DeviceController::OnVIDReadResponse(void * context, VendorId value) controller->mSetupPayload.vendorID = value; OperationalDeviceProxy * device = - controller->mCASESessionManager->FindExistingSession(controller->GetPeerIdWithCommissioningWindowOpen()); + controller->mSystemState->CASESessionMgr()->FindExistingSession(controller->GetPeerIdWithCommissioningWindowOpen()); if (device == nullptr) { ChipLogError(Controller, "Could not find device for opening commissioning window"); @@ -534,7 +507,8 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowWithCallback(NodeId deviceId if (callback != nullptr && mCommissioningWindowOption != CommissioningWindowOption::kOriginalSetupCode && readVIDPIDAttributes) { - OperationalDeviceProxy * device = mCASESessionManager->FindExistingSession(GetPeerIdWithCommissioningWindowOpen()); + OperationalDeviceProxy * device = + mSystemState->CASESessionMgr()->FindExistingSession(GetPeerIdWithCommissioningWindowOpen()); VerifyOrReturnError(device != nullptr, CHIP_ERROR_INVALID_ARGUMENT); constexpr EndpointId kBasicClusterEndpoint = 0; @@ -552,7 +526,7 @@ CHIP_ERROR DeviceController::OpenCommissioningWindowInternal() ChipLogProgress(Controller, "OpenCommissioningWindow for device ID %" PRIu64, mDeviceWithCommissioningWindowOpen); VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); - OperationalDeviceProxy * device = mCASESessionManager->FindExistingSession(GetPeerIdWithCommissioningWindowOpen()); + OperationalDeviceProxy * device = mSystemState->CASESessionMgr()->FindExistingSession(GetPeerIdWithCommissioningWindowOpen()); VerifyOrReturnError(device != nullptr, CHIP_ERROR_INVALID_ARGUMENT); constexpr EndpointId kAdministratorCommissioningClusterEndpoint = 0; @@ -619,7 +593,7 @@ ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams() .exchangeMgr = mSystemState->ExchangeMgr(), .udpEndPointManager = mSystemState->UDPEndPointManager(), .storageDelegate = mStorageDelegate, - .idAllocator = &mIDAllocator, + .idAllocator = mSystemState->SessionIDAlloc(), .fabricsTable = mSystemState->Fabrics(), }; } @@ -867,7 +841,7 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re session = mSystemState->SessionMgr()->CreateUnauthenticatedSession(params.GetPeerAddress(), device->GetMRPConfig()); VerifyOrExit(session.HasValue(), err = CHIP_ERROR_NO_MEMORY); - err = mIDAllocator.Allocate(keyID); + err = mSystemState->SessionIDAlloc()->Allocate(keyID); SuccessOrExit(err); // TODO - Remove use of SetActive/IsActive from CommissioneeDeviceProxy @@ -1522,7 +1496,7 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer error = CHIP_ERROR_INTERNAL; } - commissioner->mCASESessionManager->ReleaseSession(peerId); + commissioner->mSystemState->CASESessionMgr()->ReleaseSession(peerId); if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational && commissioner->mCommissioningDelegate != nullptr) { @@ -2105,13 +2079,13 @@ CHIP_ERROR DeviceController::UpdateDevice(NodeId deviceId) OperationalDeviceProxy * DeviceController::GetDeviceSession(const PeerId & peerId) { - return mCASESessionManager->FindExistingSession(peerId); + return mSystemState->CASESessionMgr()->FindExistingSession(peerId); } OperationalDeviceProxy * DeviceCommissioner::GetDeviceSession(const PeerId & peerId) { - CHIP_ERROR err = - mCASESessionManager->FindOrEstablishSession(peerId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback); + CHIP_ERROR err = mSystemState->CASESessionMgr()->FindOrEstablishSession(peerId, &mOnDeviceConnectedCallback, + &mOnDeviceConnectionFailureCallback); if (err != CHIP_NO_ERROR) { diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 72ed6f2416a432..b44010458184ba 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -211,7 +211,8 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr chip::Callback::Callback * onFailure) { VerifyOrReturnError(mState == State::Initialized && mFabricInfo != nullptr, CHIP_ERROR_INCORRECT_STATE); - return mCASESessionManager->FindOrEstablishSession(mFabricInfo->GetPeerIdForNode(deviceId), onConnection, onFailure); + return mSystemState->CASESessionMgr()->FindOrEstablishSession(mFabricInfo->GetPeerIdForNode(deviceId), onConnection, + onFailure); } /** @@ -350,14 +351,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr State mState; - CASESessionManager * mCASESessionManager = nullptr; - -#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0 - Dnssd::DnssdCache mDNSCache; -#endif - CASEClientPool mCASEClientPool; - OperationalDeviceProxyPool mDevicePool; - SerializableU64Set mPairedDevices; bool mPairedDevicesInitialized; @@ -377,8 +370,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr OperationalCredentialsDelegate * mOperationalCredentialsDelegate; - SessionIDAllocator mIDAllocator; - uint16_t mVendorId; /// Fetches the session to use for the current device. Allows overriding diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 4fc754997f3ee4..213ffddafb46e7 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -24,8 +24,10 @@ #include +#include #include #include +#include #if CONFIG_DEVICE_LAYER #include @@ -201,6 +203,31 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) chip::app::DnssdServer::Instance().StartServer(); } + stateParams.sessionIDAllocator = Platform::New(); + stateParams.operationalDevicePool = Platform::New(); + stateParams.caseClientPool = Platform::New(); + + DeviceProxyInitParams deviceInitParams = { + .sessionManager = stateParams.sessionMgr, + .exchangeMgr = stateParams.exchangeMgr, + .idAllocator = stateParams.sessionIDAllocator, + .fabricTable = stateParams.fabricTable, + .clientPool = stateParams.caseClientPool, + .mrpLocalConfig = Optional::Value(GetLocalMRPConfig()), + }; + + CASESessionManagerConfig sessionManagerConfig = { + .sessionInitParams = deviceInitParams, +#if CHIP_CONFIG_MDNS_CACHE_SIZE > 0 + .dnsCache = NoSuchThingWeWouldNeedToAddIt, +#endif + .devicePool = stateParams.operationalDevicePool, + }; + + // TODO: Need to be able to create a CASESessionManagerConfig here! + stateParams.caseSessionManager = Platform::New(sessionManagerConfig); + ReturnErrorOnFailure(stateParams.caseSessionManager->Init(stateParams.systemLayer)); + // store the system state mSystemState = chip::Platform::New(stateParams); ChipLogDetail(Controller, "System State Initialized..."); @@ -287,6 +314,33 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown() mCASEServer = nullptr; } + if (mCASESessionManager != nullptr) + { + mCASESessionManager->Shutdown(); + Platform::Delete(mCASESessionManager); + mCASESessionManager = nullptr; + } + + // mSessionIDAllocator, mCASEClientPool, and mDevicePool must be deallocated + // after mCASESessionManager, which uses them. + if (mSessionIDAllocator != nullptr) + { + Platform::Delete(mSessionIDAllocator); + mSessionIDAllocator = nullptr; + } + + if (mOperationalDevicePool != nullptr) + { + Platform::Delete(mOperationalDevicePool); + mOperationalDevicePool = nullptr; + } + + if (mCASEClientPool != nullptr) + { + Platform::Delete(mCASEClientPool); + mCASEClientPool = nullptr; + } + Dnssd::Resolver::Instance().Shutdown(); // Shut down the interaction model @@ -324,7 +378,11 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown() } mSystemLayer = nullptr; + mTCPEndPointManager = nullptr; mUDPEndPointManager = nullptr; +#if CONFIG_NETWORK_LAYER_BLE + mBleLayer = nullptr; +#endif // CONFIG_NETWORK_LAYER_BLE if (mMessageCounterManager != nullptr) { diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index ebb0c87be3ef8a..869d77c1ca68a1 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -29,9 +29,13 @@ #pragma once +#include +#include #include +#include #include #include +#include #include #include @@ -63,18 +67,29 @@ namespace Controller { struct DeviceControllerSystemStateParams { + using OperationalDevicePool = OperationalDeviceProxyPool; + using CASEClientPool = chip::CASEClientPool; + + // Params that can outlive the DeviceControllerSystemState System::Layer * systemLayer = nullptr; Inet::EndPointManager * tcpEndPointManager = nullptr; Inet::EndPointManager * udpEndPointManager = nullptr; #if CONFIG_NETWORK_LAYER_BLE Ble::BleLayer * bleLayer = nullptr; #endif + + // Params that will be deallocated via Platform::Delete in + // DeviceControllerSystemState::Shutdown. DeviceTransportMgr * transportMgr = nullptr; SessionManager * sessionMgr = nullptr; Messaging::ExchangeManager * exchangeMgr = nullptr; secure_channel::MessageCounterManager * messageCounterManager = nullptr; FabricTable * fabricTable = nullptr; CASEServer * caseServer = nullptr; + CASESessionManager * caseSessionManager = nullptr; + SessionIDAllocator * sessionIDAllocator = nullptr; + OperationalDevicePool * operationalDevicePool = nullptr; + CASEClientPool * caseClientPool = nullptr; }; // A representation of the internal state maintained by the DeviceControllerFactory @@ -82,13 +97,18 @@ struct DeviceControllerSystemStateParams // Expects that the creator of this object is the last one to release it. class DeviceControllerSystemState { + using OperationalDevicePool = DeviceControllerSystemStateParams::OperationalDevicePool; + using CASEClientPool = DeviceControllerSystemStateParams::CASEClientPool; + public: ~DeviceControllerSystemState(){}; DeviceControllerSystemState(DeviceControllerSystemStateParams params) : mSystemLayer(params.systemLayer), mTCPEndPointManager(params.tcpEndPointManager), mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr), mExchangeMgr(params.exchangeMgr), mMessageCounterManager(params.messageCounterManager), mFabrics(params.fabricTable), - mCASEServer(params.caseServer) + mCASEServer(params.caseServer), mCASESessionManager(params.caseSessionManager), + mSessionIDAllocator(params.sessionIDAllocator), mOperationalDevicePool(params.operationalDevicePool), + mCASEClientPool(params.caseClientPool) { #if CONFIG_NETWORK_LAYER_BLE mBleLayer = params.bleLayer; @@ -120,7 +140,8 @@ class DeviceControllerSystemState bool IsInitialized() { return mSystemLayer != nullptr && mUDPEndPointManager != nullptr && mTransportMgr != nullptr && mSessionMgr != nullptr && - mExchangeMgr != nullptr && mMessageCounterManager != nullptr && mFabrics != nullptr; + mExchangeMgr != nullptr && mMessageCounterManager != nullptr && mFabrics != nullptr && mCASESessionManager != nullptr && + mSessionIDAllocator != nullptr && mOperationalDevicePool != nullptr && mCASEClientPool != nullptr; }; System::Layer * SystemLayer() { return mSystemLayer; }; @@ -134,6 +155,8 @@ class DeviceControllerSystemState #if CONFIG_NETWORK_LAYER_BLE Ble::BleLayer * BleLayer() { return mBleLayer; }; #endif + CASESessionManager * CASESessionMgr() const { return mCASESessionManager; } + SessionIDAllocator * SessionIDAlloc() const { return mSessionIDAllocator; } private: DeviceControllerSystemState(){}; @@ -150,6 +173,10 @@ class DeviceControllerSystemState secure_channel::MessageCounterManager * mMessageCounterManager = nullptr; FabricTable * mFabrics = nullptr; CASEServer * mCASEServer = nullptr; + CASESessionManager * mCASESessionManager = nullptr; + SessionIDAllocator * mSessionIDAllocator = nullptr; + OperationalDevicePool * mOperationalDevicePool = nullptr; + CASEClientPool * mCASEClientPool = nullptr; std::atomic mRefCount{ 1 };