Skip to content

Commit ffecbc1

Browse files
Damian-NordicDamian-Nordic
authored
Simplify CASEClient initialization code (#24079)
* Remove CASEClientInitParams from CASEClient to save RAM It is only needed in EstablishSession so there is no point in keeping it as a class member. * Devirtualize CASEServer::GetSession Overriding it in unit tests does not seem to change the test results. * Merge DeviceProxyInitParams with CASEClientInitParams Both structures are almost the same and as we tend to pass more and more interfaces down the stack, translating between all the different structures becomes cumbersome.
1 parent 2f6eada commit ffecbc1

13 files changed

+53
-81
lines changed

‎src/app/CASEClient.cpp

+9-10
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,20 @@
1919

2020
namespace chip {
2121

22-
CASEClient::CASEClient(const CASEClientInitParams & params) : mInitParams(params) {}
23-
2422
void CASEClient::SetRemoteMRPIntervals(const ReliableMessageProtocolConfig & remoteMRPConfig)
2523
{
2624
mCASESession.SetRemoteMRPConfig(remoteMRPConfig);
2725
}
2826

29-
CHIP_ERROR CASEClient::EstablishSession(const ScopedNodeId & peer, const Transport::PeerAddress & peerAddress,
27+
CHIP_ERROR CASEClient::EstablishSession(const CASEClientInitParams & params, const ScopedNodeId & peer,
28+
const Transport::PeerAddress & peerAddress,
3029
const ReliableMessageProtocolConfig & remoteMRPConfig,
3130
SessionEstablishmentDelegate * delegate)
3231
{
33-
VerifyOrReturnError(mInitParams.fabricTable != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
32+
VerifyOrReturnError(params.fabricTable != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
3433

3534
// Create a UnauthenticatedSession for CASE pairing.
36-
Optional<SessionHandle> session = mInitParams.sessionManager->CreateUnauthenticatedSession(peerAddress, remoteMRPConfig);
35+
Optional<SessionHandle> session = params.sessionManager->CreateUnauthenticatedSession(peerAddress, remoteMRPConfig);
3736
VerifyOrReturnError(session.HasValue(), CHIP_ERROR_NO_MEMORY);
3837

3938
// Allocate the exchange immediately before calling CASESession::EstablishSession.
@@ -42,13 +41,13 @@ CHIP_ERROR CASEClient::EstablishSession(const ScopedNodeId & peer, const Transpo
4241
// free it on error, but can only do this if it is actually called.
4342
// Allocating the exchange context right before calling EstablishSession
4443
// ensures that if allocation succeeds, CASESession has taken ownership.
45-
Messaging::ExchangeContext * exchange = mInitParams.exchangeMgr->NewContext(session.Value(), &mCASESession);
44+
Messaging::ExchangeContext * exchange = params.exchangeMgr->NewContext(session.Value(), &mCASESession);
4645
VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);
4746

48-
mCASESession.SetGroupDataProvider(mInitParams.groupDataProvider);
49-
ReturnErrorOnFailure(mCASESession.EstablishSession(*mInitParams.sessionManager, mInitParams.fabricTable, peer, exchange,
50-
mInitParams.sessionResumptionStorage, mInitParams.certificateValidityPolicy,
51-
delegate, mInitParams.mrpLocalConfig));
47+
mCASESession.SetGroupDataProvider(params.groupDataProvider);
48+
ReturnErrorOnFailure(mCASESession.EstablishSession(*params.sessionManager, params.fabricTable, peer, exchange,
49+
params.sessionResumptionStorage, params.certificateValidityPolicy, delegate,
50+
params.mrpLocalConfig));
5251

5352
return CHIP_NO_ERROR;
5453
}

‎src/app/CASEClient.h

+15-7
Original file line numberDiff line numberDiff line change
@@ -34,23 +34,31 @@ struct CASEClientInitParams
3434
Messaging::ExchangeManager * exchangeMgr = nullptr;
3535
FabricTable * fabricTable = nullptr;
3636
Credentials::GroupDataProvider * groupDataProvider = nullptr;
37+
Optional<ReliableMessageProtocolConfig> mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Missing();
3738

38-
Optional<ReliableMessageProtocolConfig> mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Missing();
39+
CHIP_ERROR Validate() const
40+
{
41+
// sessionResumptionStorage can be nullptr when resumption is disabled.
42+
// certificateValidityPolicy is optional, too.
43+
ReturnErrorCodeIf(sessionManager == nullptr, CHIP_ERROR_INCORRECT_STATE);
44+
ReturnErrorCodeIf(exchangeMgr == nullptr, CHIP_ERROR_INCORRECT_STATE);
45+
ReturnErrorCodeIf(fabricTable == nullptr, CHIP_ERROR_INCORRECT_STATE);
46+
ReturnErrorCodeIf(groupDataProvider == nullptr, CHIP_ERROR_INCORRECT_STATE);
47+
48+
return CHIP_NO_ERROR;
49+
}
3950
};
4051

4152
class DLL_EXPORT CASEClient
4253
{
4354
public:
44-
CASEClient(const CASEClientInitParams & params);
45-
4655
void SetRemoteMRPIntervals(const ReliableMessageProtocolConfig & remoteMRPConfig);
4756

48-
CHIP_ERROR EstablishSession(const ScopedNodeId & peer, const Transport::PeerAddress & peerAddress,
49-
const ReliableMessageProtocolConfig & remoteMRPConfig, SessionEstablishmentDelegate * delegate);
57+
CHIP_ERROR EstablishSession(const CASEClientInitParams & params, const ScopedNodeId & peer,
58+
const Transport::PeerAddress & peerAddress, const ReliableMessageProtocolConfig & remoteMRPConfig,
59+
SessionEstablishmentDelegate * delegate);
5060

5161
private:
52-
CASEClientInitParams mInitParams;
53-
5462
CASESession mCASESession;
5563
};
5664

‎src/app/CASEClientPool.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ namespace chip {
2525
class CASEClientPoolDelegate
2626
{
2727
public:
28-
virtual CASEClient * Allocate(CASEClientInitParams params) = 0;
28+
virtual CASEClient * Allocate() = 0;
2929

3030
virtual void Release(CASEClient * client) = 0;
3131

@@ -38,7 +38,7 @@ class CASEClientPool : public CASEClientPoolDelegate
3838
public:
3939
~CASEClientPool() override { mClientPool.ReleaseAll(); }
4040

41-
CASEClient * Allocate(CASEClientInitParams params) override { return mClientPool.CreateObject(params); }
41+
CASEClient * Allocate() override { return mClientPool.CreateObject(); }
4242

4343
void Release(CASEClient * client) override { mClientPool.ReleaseObject(client); }
4444

‎src/app/CASESessionManager.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ void CASESessionManager::FindOrEstablishSession(const ScopedNodeId & peerId, Cal
4141
{
4242
ChipLogDetail(CASESessionManager, "FindOrEstablishSession: No existing OperationalSessionSetup instance found");
4343

44-
session = mConfig.sessionSetupPool->Allocate(mConfig.sessionInitParams, peerId, this);
44+
session = mConfig.sessionSetupPool->Allocate(mConfig.sessionInitParams, mConfig.clientPool, peerId, this);
4545

4646
if (session == nullptr)
4747
{
@@ -83,7 +83,7 @@ void CASESessionManager::UpdatePeerAddress(ScopedNodeId peerId)
8383
{
8484
ChipLogDetail(CASESessionManager, "UpdatePeerAddress: No existing OperationalSessionSetup instance found");
8585

86-
session = mConfig.sessionSetupPool->Allocate(mConfig.sessionInitParams, peerId, this);
86+
session = mConfig.sessionSetupPool->Allocate(mConfig.sessionInitParams, mConfig.clientPool, peerId, this);
8787
if (session == nullptr)
8888
{
8989
ChipLogDetail(CASESessionManager, "UpdatePeerAddress: Failed to allocate OperationalSessionSetup instance");

‎src/app/CASESessionManager.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ class OperationalSessionSetupPoolDelegate;
3636

3737
struct CASESessionManagerConfig
3838
{
39-
DeviceProxyInitParams sessionInitParams;
39+
CASEClientInitParams sessionInitParams;
40+
CASEClientPoolDelegate * clientPool = nullptr;
4041
OperationalSessionSetupPoolDelegate * sessionSetupPool = nullptr;
4142
};
4243

‎src/app/OperationalSessionSetup.cpp

+5-7
Original file line numberDiff line numberDiff line change
@@ -221,12 +221,10 @@ void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & ad
221221

222222
CHIP_ERROR OperationalSessionSetup::EstablishConnection(const ReliableMessageProtocolConfig & config)
223223
{
224-
mCASEClient = mInitParams.clientPool->Allocate(CASEClientInitParams{
225-
mInitParams.sessionManager, mInitParams.sessionResumptionStorage, mInitParams.certificateValidityPolicy,
226-
mInitParams.exchangeMgr, mFabricTable, mInitParams.groupDataProvider, mInitParams.mrpLocalConfig });
224+
mCASEClient = mClientPool->Allocate();
227225
ReturnErrorCodeIf(mCASEClient == nullptr, CHIP_ERROR_NO_MEMORY);
228226

229-
CHIP_ERROR err = mCASEClient->EstablishSession(mPeerId, mDeviceAddress, config, this);
227+
CHIP_ERROR err = mCASEClient->EstablishSession(mInitParams, mPeerId, mDeviceAddress, config, this);
230228
if (err != CHIP_NO_ERROR)
231229
{
232230
CleanupCASEClient();
@@ -330,7 +328,7 @@ void OperationalSessionSetup::CleanupCASEClient()
330328
{
331329
if (mCASEClient)
332330
{
333-
mInitParams.clientPool->Release(mCASEClient);
331+
mClientPool->Release(mCASEClient);
334332
mCASEClient = nullptr;
335333
}
336334
}
@@ -364,7 +362,7 @@ OperationalSessionSetup::~OperationalSessionSetup()
364362
if (mCASEClient)
365363
{
366364
// Make sure we don't leak it.
367-
mInitParams.clientPool->Release(mCASEClient);
365+
mClientPool->Release(mCASEClient);
368366
}
369367
}
370368

@@ -382,7 +380,7 @@ CHIP_ERROR OperationalSessionSetup::LookupPeerAddress()
382380
return CHIP_NO_ERROR;
383381
}
384382

385-
auto const * fabricInfo = mFabricTable->FindFabricWithIndex(mPeerId.GetFabricIndex());
383+
auto const * fabricInfo = mInitParams.fabricTable->FindFabricWithIndex(mPeerId.GetFabricIndex());
386384
VerifyOrReturnError(fabricInfo != nullptr, CHIP_ERROR_INVALID_FABRIC_INDEX);
387385

388386
PeerId peerId(fabricInfo->GetCompressedFabricId(), mPeerId.GetNodeId());

‎src/app/OperationalSessionSetup.h

+5-30
Original file line numberDiff line numberDiff line change
@@ -45,31 +45,6 @@
4545

4646
namespace chip {
4747

48-
struct DeviceProxyInitParams
49-
{
50-
SessionManager * sessionManager = nullptr;
51-
SessionResumptionStorage * sessionResumptionStorage = nullptr;
52-
Credentials::CertificateValidityPolicy * certificateValidityPolicy = nullptr;
53-
Messaging::ExchangeManager * exchangeMgr = nullptr;
54-
FabricTable * fabricTable = nullptr;
55-
CASEClientPoolDelegate * clientPool = nullptr;
56-
Credentials::GroupDataProvider * groupDataProvider = nullptr;
57-
58-
Optional<ReliableMessageProtocolConfig> mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Missing();
59-
60-
CHIP_ERROR Validate() const
61-
{
62-
ReturnErrorCodeIf(sessionManager == nullptr, CHIP_ERROR_INCORRECT_STATE);
63-
// sessionResumptionStorage can be nullptr when resumption is disabled
64-
ReturnErrorCodeIf(exchangeMgr == nullptr, CHIP_ERROR_INCORRECT_STATE);
65-
ReturnErrorCodeIf(fabricTable == nullptr, CHIP_ERROR_INCORRECT_STATE);
66-
ReturnErrorCodeIf(groupDataProvider == nullptr, CHIP_ERROR_INCORRECT_STATE);
67-
ReturnErrorCodeIf(clientPool == nullptr, CHIP_ERROR_INCORRECT_STATE);
68-
69-
return CHIP_NO_ERROR;
70-
}
71-
};
72-
7348
class OperationalSessionSetup;
7449

7550
/**
@@ -171,20 +146,20 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
171146
public:
172147
~OperationalSessionSetup() override;
173148

174-
OperationalSessionSetup(DeviceProxyInitParams & params, ScopedNodeId peerId,
149+
OperationalSessionSetup(const CASEClientInitParams & params, CASEClientPoolDelegate * clientPool, ScopedNodeId peerId,
175150
OperationalSessionReleaseDelegate * releaseDelegate) :
176151
mSecureSession(*this)
177152
{
178153
mInitParams = params;
179-
if (params.Validate() != CHIP_NO_ERROR || releaseDelegate == nullptr)
154+
if (params.Validate() != CHIP_NO_ERROR || clientPool == nullptr || releaseDelegate == nullptr)
180155
{
181156
mState = State::Uninitialized;
182157
return;
183158
}
184159

160+
mClientPool = clientPool;
185161
mSystemLayer = params.exchangeMgr->GetSessionManager()->SystemLayer();
186162
mPeerId = peerId;
187-
mFabricTable = params.fabricTable;
188163
mReleaseDelegate = releaseDelegate;
189164
mState = State::NeedsAddress;
190165
mAddressLookupHandle.SetListener(this);
@@ -260,8 +235,8 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
260235
SecureConnected, // CASE session established.
261236
};
262237

263-
DeviceProxyInitParams mInitParams;
264-
FabricTable * mFabricTable = nullptr;
238+
CASEClientInitParams mInitParams;
239+
CASEClientPoolDelegate * mClientPool = nullptr;
265240
System::Layer * mSystemLayer;
266241

267242
// mCASEClient is only non-null if we are in State::Connecting or just

‎src/app/OperationalSessionSetupPool.h

+5-5
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ namespace chip {
2727
class OperationalSessionSetupPoolDelegate
2828
{
2929
public:
30-
virtual OperationalSessionSetup * Allocate(DeviceProxyInitParams & params, ScopedNodeId peerId,
31-
OperationalSessionReleaseDelegate * releaseDelegate) = 0;
30+
virtual OperationalSessionSetup * Allocate(const CASEClientInitParams & params, CASEClientPoolDelegate * clientPool,
31+
ScopedNodeId peerId, OperationalSessionReleaseDelegate * releaseDelegate) = 0;
3232

3333
virtual void Release(OperationalSessionSetup * device) = 0;
3434

@@ -47,10 +47,10 @@ class OperationalSessionSetupPool : public OperationalSessionSetupPoolDelegate
4747
public:
4848
~OperationalSessionSetupPool() override { mSessionSetupPool.ReleaseAll(); }
4949

50-
OperationalSessionSetup * Allocate(DeviceProxyInitParams & params, ScopedNodeId peerId,
51-
OperationalSessionReleaseDelegate * releaseDelegate) override
50+
OperationalSessionSetup * Allocate(const CASEClientInitParams & params, CASEClientPoolDelegate * clientPool,
51+
ScopedNodeId peerId, OperationalSessionReleaseDelegate * releaseDelegate) override
5252
{
53-
return mSessionSetupPool.CreateObject(params, peerId, releaseDelegate);
53+
return mSessionSetupPool.CreateObject(params, clientPool, peerId, releaseDelegate);
5454
}
5555

5656
void Release(OperationalSessionSetup * device) override { mSessionSetupPool.ReleaseObject(device); }

‎src/app/server/Server.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -290,11 +290,11 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams)
290290
.certificateValidityPolicy = mCertificateValidityPolicy,
291291
.exchangeMgr = &mExchangeMgr,
292292
.fabricTable = &mFabrics,
293-
.clientPool = &mCASEClientPool,
294293
.groupDataProvider = mGroupsProvider,
295294
.mrpLocalConfig = GetLocalMRPConfig(),
296295
},
297-
.sessionSetupPool = &mSessionSetupPool,
296+
.clientPool = &mCASEClientPool,
297+
.sessionSetupPool = &mSessionSetupPool,
298298
};
299299

300300
err = mCASESessionManager.Init(&DeviceLayer::SystemLayer(), caseSessionManagerConfig);

‎src/app/tests/TestOperationalDeviceProxy.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ void TestOperationalDeviceProxy_EstablishSessionDirectly(nlTestSuite * inSuite,
6969
VerifyOrDie(groupDataProvider.Init() == CHIP_NO_ERROR);
7070
// TODO: Set IPK in groupDataProvider
7171

72-
DeviceProxyInitParams params = {
72+
CASEClientInitParams params = {
7373
.sessionManager = &sessionManager,
7474
.sessionResumptionStorage = &sessionResumptionStorage,
7575
.exchangeMgr = &exchangeMgr,

‎src/controller/CHIPDeviceControllerFactory.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -245,18 +245,18 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
245245
stateParams.sessionSetupPool = Platform::New<DeviceControllerSystemStateParams::SessionSetupPool>();
246246
stateParams.caseClientPool = Platform::New<DeviceControllerSystemStateParams::CASEClientPool>();
247247

248-
DeviceProxyInitParams deviceInitParams = {
248+
CASEClientInitParams sessionInitParams = {
249249
.sessionManager = stateParams.sessionMgr,
250250
.sessionResumptionStorage = stateParams.sessionResumptionStorage.get(),
251251
.exchangeMgr = stateParams.exchangeMgr,
252252
.fabricTable = stateParams.fabricTable,
253-
.clientPool = stateParams.caseClientPool,
254253
.groupDataProvider = stateParams.groupDataProvider,
255254
.mrpLocalConfig = GetLocalMRPConfig(),
256255
};
257256

258257
CASESessionManagerConfig sessionManagerConfig = {
259-
.sessionInitParams = deviceInitParams,
258+
.sessionInitParams = sessionInitParams,
259+
.clientPool = stateParams.caseClientPool,
260260
.sessionSetupPool = stateParams.sessionSetupPool,
261261
};
262262

‎src/protocols/secure_channel/CASEServer.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class CASEServer : public SessionEstablishmentDelegate,
6969
void OnResponseTimeout(Messaging::ExchangeContext * ec) override {}
7070
Messaging::ExchangeMessageDispatch & GetMessageDispatch() override { return GetSession().GetMessageDispatch(); }
7171

72-
virtual CASESession & GetSession() { return mPairingSession; }
72+
CASESession & GetSession() { return mPairingSession; }
7373

7474
private:
7575
Messaging::ExchangeManager * mExchangeManager = nullptr;

‎src/protocols/secure_channel/tests/TestCASESession.cpp

+1-10
Original file line numberDiff line numberDiff line change
@@ -124,15 +124,6 @@ class TestCASESecurePairingDelegate : public SessionEstablishmentDelegate
124124
uint32_t mNumPairingComplete = 0;
125125
};
126126

127-
class CASEServerForTest : public CASEServer
128-
{
129-
public:
130-
CASESession & GetSession() override { return mCaseSession; }
131-
132-
private:
133-
CASESession mCaseSession;
134-
};
135-
136127
class TestOperationalKeystore : public chip::Crypto::OperationalKeystore
137128
{
138129
public:
@@ -469,7 +460,7 @@ void TestCASESession::SecurePairingHandshakeTest(nlTestSuite * inSuite, void * i
469460
SecurePairingHandshakeTestCommon(inSuite, inContext, sessionManager, pairingCommissioner, delegateCommissioner);
470461
}
471462

472-
CASEServerForTest gPairingServer;
463+
CASEServer gPairingServer;
473464

474465
void TestCASESession::SecurePairingHandshakeServerTest(nlTestSuite * inSuite, void * inContext)
475466
{

0 commit comments

Comments
 (0)