From 1f24589b931c27bfa82c26e3e3c5f4cbfbf57f64 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 16 Aug 2022 13:02:41 +0000 Subject: [PATCH 1/2] Remove mRemoteMRPConfig member from OperationalSessionSetup --- src/app/OperationalSessionSetup.cpp | 21 +++++++++++++-------- src/app/OperationalSessionSetup.h | 6 +----- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp index 11ceb8bb6c0e26..5917a4c4b9bcdf 100644 --- a/src/app/OperationalSessionSetup.cpp +++ b/src/app/OperationalSessionSetup.cpp @@ -125,7 +125,11 @@ void OperationalSessionSetup::Connect(Callback::Callback * on isConnected = AttachToExistingSecureSession(); if (!isConnected) { - err = EstablishConnection(); + // We should really be in State::HasAddress since in the same call + // we move to State::HasAddress we then moved to State::Connecting + // or call DequeueConnectionCallbacks with an error thus releasing + // ourselve. + err = CHIP_ERROR_INCORRECT_STATE; } break; @@ -179,13 +183,11 @@ void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & ad CHIP_ERROR err = CHIP_NO_ERROR; mDeviceAddress = addr; - mRemoteMRPConfig = config; - // Initialize CASE session state with any MRP parameters that DNS-SD has provided. // It can be overridden by CASE session protocol messages that include MRP parameters. if (mCASEClient) { - mCASEClient->SetRemoteMRPIntervals(mRemoteMRPConfig); + mCASEClient->SetRemoteMRPIntervals(config); } if (mState == State::ResolvingAddress) @@ -194,7 +196,7 @@ void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & ad mInitParams.sessionManager->UpdateAllSessionsPeerAddress(mPeerId, addr); if (!mPerformingAddressUpdate) { - err = EstablishConnection(); + err = EstablishConnection(config); if (err != CHIP_NO_ERROR) { DequeueConnectionCallbacks(err); @@ -216,14 +218,14 @@ void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & ad // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } -CHIP_ERROR OperationalSessionSetup::EstablishConnection() +CHIP_ERROR OperationalSessionSetup::EstablishConnection(const ReliableMessageProtocolConfig & config) { mCASEClient = mInitParams.clientPool->Allocate(CASEClientInitParams{ mInitParams.sessionManager, mInitParams.sessionResumptionStorage, mInitParams.certificateValidityPolicy, mInitParams.exchangeMgr, mFabricTable, mInitParams.groupDataProvider, mInitParams.mrpLocalConfig }); ReturnErrorCodeIf(mCASEClient == nullptr, CHIP_ERROR_NO_MEMORY); - CHIP_ERROR err = mCASEClient->EstablishSession(mPeerId, mDeviceAddress, mRemoteMRPConfig, this); + CHIP_ERROR err = mCASEClient->EstablishSession(mPeerId, mDeviceAddress, config, this); if (err != CHIP_NO_ERROR) { CleanupCASEClient(); @@ -334,7 +336,10 @@ void OperationalSessionSetup::CleanupCASEClient() void OperationalSessionSetup::OnSessionReleased() { - MoveToState(State::HasAddress); + // This is unlikely to be called since within the same call that we get SessionHandle we + // then call DequeueConnectionCallbacks which releases `this`. If this is called, and we + // we have any callbacks we will just send an error. + DequeueConnectionCallbacks(CHIP_ERROR_INCORRECT_STATE); } OperationalSessionSetup::~OperationalSessionSetup() diff --git a/src/app/OperationalSessionSetup.h b/src/app/OperationalSessionSetup.h index d0db84f6c69ce1..cde2fddc6dac0b 100644 --- a/src/app/OperationalSessionSetup.h +++ b/src/app/OperationalSessionSetup.h @@ -208,8 +208,6 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, */ void Connect(Callback::Callback * onConnection, Callback::Callback * onFailure); - bool IsConnecting() const { return mState == State::Connecting; } - bool IsForAddressUpdate() const { return mPerformingAddressUpdate; } //////////// SessionEstablishmentDelegate Implementation /////////////// @@ -288,11 +286,9 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate, /// This is used when a node address is required. chip::AddressResolve::NodeLookupHandle mAddressLookupHandle; - ReliableMessageProtocolConfig mRemoteMRPConfig = GetDefaultMRPConfig(); - bool mPerformingAddressUpdate = false; - CHIP_ERROR EstablishConnection(); + CHIP_ERROR EstablishConnection(const ReliableMessageProtocolConfig & config); /* * This checks to see if an existing CASE session exists to the peer within the SessionManager From 7790e7e088a206efdc898421e094c11b48640e5e Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 16 Aug 2022 13:10:33 +0000 Subject: [PATCH 2/2] Fix comment phrasing to be more descriptive --- src/app/OperationalSessionSetup.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp index 5917a4c4b9bcdf..3a0d124297a091 100644 --- a/src/app/OperationalSessionSetup.cpp +++ b/src/app/OperationalSessionSetup.cpp @@ -125,10 +125,11 @@ void OperationalSessionSetup::Connect(Callback::Callback * on isConnected = AttachToExistingSecureSession(); if (!isConnected) { - // We should really be in State::HasAddress since in the same call - // we move to State::HasAddress we then moved to State::Connecting - // or call DequeueConnectionCallbacks with an error thus releasing - // ourselve. + // We should not actually every be in be in State::HasAddress. This + // is because in the same call that we moved to State::HasAddress + // we either move to State::Connecting or call + // DequeueConnectionCallbacks with an error thus releasing + // ourselves before any call would reach this section of code. err = CHIP_ERROR_INCORRECT_STATE; }