Address PR comments

project-chip · Jun 13, 2024 · ce6a71f · ce6a71f
1 parent 8c9ba74
commit ce6a71f
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 41 deletions.
diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp
@@ -128,23 +128,6 @@ CHIP_ERROR CommandSender::SendCommandRequestInternal(const SessionHandle & sessi
     return SendInvokeRequest();
 }
 
-void CommandSender::CreateBackupForRequestRollback(RollbackData & aRollbackData)
-{
-    VerifyOrReturn(mBufferAllocated);
-    VerifyOrReturn(mState == State::Idle || mState == State::AddedCommand);
-    VerifyOrReturn(mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR);
-    VerifyOrReturn(mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR);
-    aRollbackData.Checkpoint(*this);
-}
-
-void CommandSender::RollbackRequest(RollbackData & aRollbackData)
-{
-    VerifyOrReturn(aRollbackData.RollbackIsValid());
-    VerifyOrReturn(mState == State::AddingCommand);
-    ChipLogDetail(DataManagement, "Rolling back response");
-    LogErrorOnFailure(aRollbackData.Rollback(*this));
-}
-
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
 CHIP_ERROR CommandSender::TestOnlyCommandSenderTimedRequestFlagWithNoTimedInvoke(const SessionHandle & session,
                                                                                  Optional<System::Clock::Timeout> timeout)
@@ -560,7 +543,7 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath,
     ReturnErrorOnFailure(AllocateBuffer());
 
     RollbackData rollbackData;
-    CreateBackupForRequestRollback(rollbackData);
+    rollbackData.Checkpoint(*this);
     PrepareCommandParameters prepareCommandParams(aAddRequestDataParams);
     TLV::TLVWriter * writer = nullptr;
     CHIP_ERROR err          = CHIP_NO_ERROR;
@@ -575,7 +558,7 @@ CHIP_ERROR CommandSender::AddRequestData(const CommandPathParams & aCommandPath,
 exit:
     if (err != CHIP_NO_ERROR)
     {
-        RollbackRequest(rollbackData);
+        LogErrorOnFailure(rollbackData.Rollback(*this));
     }
     return err;
 }
@@ -690,17 +673,22 @@ void CommandSender::MoveToState(const State aTargetState)
 
 void CommandSender::RollbackData::Checkpoint(CommandSender & aCommandSender)
 {
-    // InvokeRequestMessage::Builder& aInvokeRequestBuilder, const State& aState) {
+    VerifyOrReturn(aCommandSender.mBufferAllocated);
+    VerifyOrReturn(aCommandSender.mState == State::Idle || aCommandSender.mState == State::AddedCommand);
+    VerifyOrReturn(aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().GetError() == CHIP_NO_ERROR);
+    VerifyOrReturn(aCommandSender.mInvokeRequestBuilder.GetError() == CHIP_NO_ERROR);
     aCommandSender.mInvokeRequestBuilder.Checkpoint(mBackupWriter);
     mBackupState     = aCommandSender.mState;
     mRollbackIsValid = true;
 }
 
 CHIP_ERROR CommandSender::RollbackData::Rollback(CommandSender & aCommandSender)
 {
+    VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(aCommandSender.mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE);
+    ChipLogDetail(DataManagement, "Rolling back response");
     // TODO(#30453): Rollback of mInvokeRequestBuilder should handle resetting
     // InvokeResponses.
-    VerifyOrReturnError(mRollbackIsValid, CHIP_ERROR_INCORRECT_STATE);
     aCommandSender.mInvokeRequestBuilder.GetInvokeRequests().ResetError();
     aCommandSender.mInvokeRequestBuilder.Rollback(mBackupWriter);
     aCommandSender.MoveToState(mBackupState);

diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h
@@ -520,7 +520,22 @@ class CommandSender final : public Messaging::ExchangeDelegate
     class RollbackData
     {
     public:
+        /**
+         * Creates a backup to enable rolling back CommandSender's buffer containing
+         * InvokeRequestMessage in case subsequent calls to add request fail.
+         *
+         * A successful backup will only be created if the InvokeRequestMessage is
+         * in a known good state.
+         *
+         * @param [in] aCommandSender reference to CommandSender.
+         */
         void Checkpoint(CommandSender & aCommandSender);
+        /**
+         * Rolls back CommandSender's buffer containing InvokeRequestMessage to a previously
+         * saved state. Must have previously called Checkpoint in a known good state.
+         *
+         * @param [in] aCommandSender reference to CommandSender.
+         */
         CHIP_ERROR Rollback(CommandSender & aCommandSender);
         bool RollbackIsValid() { return mRollbackIsValid; }
 
@@ -585,26 +600,6 @@ class CommandSender final : public Messaging::ExchangeDelegate
 
     CHIP_ERROR SendCommandRequestInternal(const SessionHandle & session, Optional<System::Clock::Timeout> timeout);
 
-    /**
-     * Creates a backup to enable rolling back buffer containing InvokeRequestMessage
-     * in case subsequent calls to add request fail.
-     *
-     * A successful backup will only be created if the InvokeRequestMessage is
-     * in a known good state.
-     *
-     * @param [in] aRollbackData reference to rollback data that can be on the stack.
-     */
-    void CreateBackupForRequestRollback(RollbackData & aRollbackData);
-
-    /**
-     * Rolls back buffer containing InvokeRequestMessage  to a previously saved state.
-     *
-     * @param [in] aRollbackData reference to rollback data that was previously provided
-     *             to CreateBackupForRequestRollback. This rollbackData must be valid for
-     *             a successful rollback.
-     */
-    void RollbackRequest(RollbackData & aRollbackData);
-
     void OnResponseCallback(const ResponseData & aResponseData)
     {
         // mpExtendableCallback and mpCallback are mutually exclusive.