Improve ProcessStatusResponse (project-chip#21331)

* Improve ProcessStatusResponse -- Improve ProcessStatusResponse signature so that we can tell if the error is caused by malformed status response or the error is from stored value inside status response. * address comments * Fix unexpected non-StatusResponse handling. Co-authored-by: Boris Zbarsky <[email protected]>
isiu-apple · Sep 16, 2022 · 28bdc14 · 28bdc14
1 parent 1d2d700
commit 28bdc14
Show file tree

Hide file tree

Showing 10 changed files with 48 additions and 62 deletions.
diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp
@@ -120,6 +120,8 @@ CHIP_ERROR CommandSender::SendInvokeRequest()
 CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
                                             System::PacketBufferHandle && aPayload)
 {
+    using namespace Protocols::InteractionModel;
+
     if (mState == State::CommandSent)
     {
         MoveToState(State::ResponseReceived);
@@ -130,21 +132,27 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
 
     if (mState == State::AwaitingTimedStatus)
     {
-        err = HandleTimedStatus(aPayloadHeader, std::move(aPayload));
+        VerifyOrExit(aPayloadHeader.HasMessageType(MsgType::StatusResponse), err = CHIP_ERROR_INVALID_MESSAGE_TYPE);
+        CHIP_ERROR statusError = CHIP_NO_ERROR;
+        SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+        SuccessOrExit(err = statusError);
+        err = SendInvokeRequest();
         // Skip all other processing here (which is for the response to the
         // invoke request), no matter whether err is success or not.
         goto exit;
     }
 
-    if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandResponse))
+    if (aPayloadHeader.HasMessageType(MsgType::InvokeCommandResponse))
     {
         err = ProcessInvokeResponse(std::move(aPayload));
         SuccessOrExit(err);
     }
-    else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
+    else if (aPayloadHeader.HasMessageType(MsgType::StatusResponse))
     {
-        err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
-        SuccessOrExit(err);
+        CHIP_ERROR statusError = CHIP_NO_ERROR;
+        SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+        SuccessOrExit(err = statusError);
+        err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
     }
     else
     {
@@ -369,13 +377,6 @@ TLV::TLVWriter * CommandSender::GetCommandDataIBTLVWriter()
     return mInvokeRequestBuilder.GetInvokeRequests().GetCommandData().GetWriter();
 }
 
-CHIP_ERROR CommandSender::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
-{
-    ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload)));
-
-    return SendInvokeRequest();
-}
-
 CHIP_ERROR CommandSender::FinishCommand(const Optional<uint16_t> & aTimedInvokeTimeoutMs)
 {
     ReturnErrorOnFailure(FinishCommand(/* aEndDataStruct = */ false));

diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h
@@ -259,14 +259,6 @@ class CommandSender final : public Messaging::ExchangeDelegate
     CHIP_ERROR ProcessInvokeResponse(System::PacketBufferHandle && payload);
     CHIP_ERROR ProcessInvokeResponseIB(InvokeResponseIB::Parser & aInvokeResponse);
 
-    // Handle a message received when we are expecting a status response to a
-    // Timed Request.  The caller is assumed to have already checked that our
-    // exchange context member is the one the message came in on.
-    //
-    // If the server returned an error status, that will be returned as an error
-    // value of CHIP_ERROR.
-    CHIP_ERROR HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
-
     // Send our queued-up Invoke Request message.  Assumes the exchange is ready
     // and mPendingInvokeData is populated.
     CHIP_ERROR SendInvokeRequest();

diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
@@ -424,8 +424,10 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
     else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
         VerifyOrExit(apExchangeContext == mExchange.Get(), err = CHIP_ERROR_INCORRECT_STATE);
-        err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
-        SuccessOrExit(err);
+        CHIP_ERROR statusError = CHIP_NO_ERROR;
+        SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+        SuccessOrExit(err = statusError);
+        err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
     }
     else
     {

diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -116,9 +116,10 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
 
 CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
 {
-    CHIP_ERROR err = CHIP_NO_ERROR;
-    err            = StatusResponse::ProcessStatusResponse(std::move(aPayload));
-    SuccessOrExit(err);
+    CHIP_ERROR err         = CHIP_NO_ERROR;
+    CHIP_ERROR statusError = CHIP_NO_ERROR;
+    SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+    SuccessOrExit(err = statusError);
     switch (mState)
     {
     case HandlerState::AwaitingReportResponse:

diff --git a/src/app/StatusResponse.cpp b/src/app/StatusResponse.cpp
@@ -44,7 +44,7 @@ CHIP_ERROR StatusResponse::Send(Protocols::InteractionModel::Status aStatus, Mes
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload)
+CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload, CHIP_ERROR & aStatusError)
 {
     StatusResponseMessage::Parser response;
     System::PacketBufferTLVReader reader;
@@ -59,12 +59,8 @@ CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && a
                     ChipLogValueIMStatus(status.mStatus));
     ReturnErrorOnFailure(response.ExitContainer());
 
-    if (status.mStatus == Protocols::InteractionModel::Status::Success)
-    {
-        return CHIP_NO_ERROR;
-    }
-
-    return status.ToChipError();
+    aStatusError = status.ToChipError();
+    return CHIP_NO_ERROR;
 }
 } // namespace app
 } // namespace chip
diff --git a/src/app/StatusResponse.h b/src/app/StatusResponse.h
@@ -32,7 +32,10 @@ class StatusResponse
 public:
     static CHIP_ERROR Send(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext,
                            bool aExpectResponse);
-    static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload);
+
+    // The return value indicates whether the StatusResponse was parsed properly, and if it is CHIP_NO_ERROR
+    // then aStatus has been set to the actual status, which might be success or failure.
+    static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload, CHIP_ERROR & aStatus);
 };
 } // namespace app
 } // namespace chip
diff --git a/src/app/TimedRequest.cpp b/src/app/TimedRequest.cpp
@@ -53,12 +53,5 @@ CHIP_ERROR TimedRequest::Send(ExchangeContext * aExchangeContext, uint16_t aTime
     return aExchangeContext->SendMessage(MsgType::TimedRequest, std::move(payload), SendMessageFlags::kExpectResponse);
 }
 
-CHIP_ERROR TimedRequest::HandleResponse(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
-{
-    VerifyOrReturnError(aPayloadHeader.HasMessageType(MsgType::StatusResponse), CHIP_ERROR_INVALID_MESSAGE_TYPE);
-
-    return StatusResponse::ProcessStatusResponse(std::move(aPayload));
-}
-
 } // namespace app
 } // namespace chip
diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp
@@ -414,6 +414,8 @@ CHIP_ERROR WriteClient::SendWriteRequest()
 CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
                                           System::PacketBufferHandle && aPayload)
 {
+    using namespace Protocols::InteractionModel;
+
     if (mState == State::AwaitingResponse &&
         // We had sent the last chunk of data, and received all responses
         mChunks.IsNull())
@@ -431,13 +433,18 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
 
     if (mState == State::AwaitingTimedStatus)
     {
-        err = HandleTimedStatus(aPayloadHeader, std::move(aPayload));
+        VerifyOrExit(aPayloadHeader.HasMessageType(MsgType::StatusResponse), err = CHIP_ERROR_INVALID_MESSAGE_TYPE);
+        CHIP_ERROR statusError = CHIP_NO_ERROR;
+        SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+        SuccessOrExit(err = statusError);
+        err = SendWriteRequest();
+
         // Skip all other processing here (which is for the response to the
         // write request), no matter whether err is success or not.
         goto exit;
     }
 
-    if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteResponse))
+    if (aPayloadHeader.HasMessageType(MsgType::WriteResponse))
     {
         err = ProcessWriteResponseMessage(std::move(aPayload));
         SuccessOrExit(err);
@@ -447,10 +454,12 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang
             SuccessOrExit(SendWriteRequest());
         }
     }
-    else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
+    else if (aPayloadHeader.HasMessageType(MsgType::StatusResponse))
     {
-        err = StatusResponse::ProcessStatusResponse(std::move(aPayload));
-        SuccessOrExit(err);
+        CHIP_ERROR statusError = CHIP_NO_ERROR;
+        SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+        SuccessOrExit(err = statusError);
+        err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
     }
     else
     {
@@ -522,12 +531,5 @@ CHIP_ERROR WriteClient::ProcessAttributeStatusIB(AttributeStatusIB::Parser & aAt
     return err;
 }
 
-CHIP_ERROR WriteClient::HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
-{
-    ReturnErrorOnFailure(TimedRequest::HandleResponse(aPayloadHeader, std::move(aPayload)));
-
-    return SendWriteRequest();
-}
-
 } // namespace app
 } // namespace chip
diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h
@@ -340,15 +340,6 @@ class WriteClient : public Messaging::ExchangeDelegate
      */
     void Abort();
 
-    // Handle a message received when we are expecting a status response to a
-    // Timed Request.  The caller is assumed to have already checked that our
-    // exchange context member is the one the message came in on.
-    //
-    // If the server returned an error status response its status will be
-    // encapsulated in the CHIP_ERROR this returns.  In that case,
-    // StatusIB::InitFromChipError can be used to extract the status.
-    CHIP_ERROR HandleTimedStatus(const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
-
     // Send our queued-up Write Request message.  Assumes the exchange is ready
     // and mPendingWriteData is populated.
     CHIP_ERROR SendWriteRequest();

diff --git a/src/app/tests/TestTimedHandler.cpp b/src/app/tests/TestTimedHandler.cpp
@@ -68,7 +68,12 @@ class TestExchangeDelegate : public Messaging::ExchangeDelegate
         mLastMessageWasStatus = aPayloadHeader.HasMessageType(MsgType::StatusResponse);
         if (mLastMessageWasStatus)
         {
-            mError = StatusResponse::ProcessStatusResponse(std::move(aPayload));
+            CHIP_ERROR statusError = CHIP_NO_ERROR;
+            mError                 = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError);
+            if (mError == CHIP_NO_ERROR)
+            {
+                mError = statusError;
+            }
         }
         if (mKeepExchangeOpen)
         {