Fix status report handling in read handler (project-chip#21374)

* Fix status report handling in read handler * address comments * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/tests/TestReadInteraction.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Fix 1. Once message is passed to OnReadInitialRequest in read handler, if anything bad in incoming message, handler would send status report with invalid action, if there is not enough resource for paths in IM engines, we would send status report with exhansted path. 2. Fix fake build in linux, it seems this build don't have IM schema check. Usually without CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK, if schema error happens, we would get a little bit more detailed error, here, we add additional error check in TestReadHandlerInvalidAttributePath. * address comments Co-authored-by: Boris Zbarsky <[email protected]>
isiu-apple · Sep 16, 2022 · 4a8e698 · 4a8e698
1 parent 7b80ced
commit 4a8e698
Show file tree

Hide file tree

Showing 4 changed files with 339 additions and 52 deletions.
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
@@ -406,7 +406,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
         reader.Init(aPayload.Retain());
 
         ReadRequestMessage::Parser readRequestParser;
-        VerifyOrReturnError(readRequestParser.Init(reader) == CHIP_NO_ERROR, Status::Failure);
+        VerifyOrReturnError(readRequestParser.Init(reader) == CHIP_NO_ERROR, Status::InvalidAction);
 
         {
             size_t requestedAttributePathCount = 0;
@@ -458,15 +458,9 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
         return Status::ResourceExhausted;
     }
 
-    CHIP_ERROR err = handler->OnInitialRequest(std::move(aPayload));
-    if (err == CHIP_ERROR_NO_MEMORY)
-    {
-        return Status::ResourceExhausted;
-    }
+    handler->OnInitialRequest(std::move(aPayload));
 
-    // TODO: Should probably map various TLV errors into InvalidAction, here
-    // or inside the read handler.
-    return StatusIB(err).mStatus;
+    return Status::Success;
 }
 
 Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext,

diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp
@@ -36,6 +36,7 @@
 
 namespace chip {
 namespace app {
+using Status = Protocols::InteractionModel::Status;
 
 ReadHandler::ReadHandler(ManagementCallback & apCallback, Messaging::ExchangeContext * apExchangeContext,
                          InteractionType aInteractionType) :
@@ -87,7 +88,7 @@ void ReadHandler::Close()
     mManagementCallback.OnDone(*this);
 }
 
-CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
+void ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
     System::PacketBufferHandle response;
@@ -103,22 +104,30 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)
 
     if (err != CHIP_NO_ERROR)
     {
+        Status status = Status::InvalidAction;
+        if (err.IsIMStatus())
+        {
+            status = StatusIB(err).mStatus;
+        }
+        StatusResponse::Send(status, mExchangeCtx.Get(), /* aExpectResponse = */ false);
         Close();
     }
     else
     {
         // Force us to be in a dirty state so we get processed by the reporting
         mFlags.Set(ReadHandlerFlags::ForceDirty);
     }
-
-    return err;
 }
 
-CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
+CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload,
+                                         bool & aSendStatusResponse)
 {
     CHIP_ERROR err         = CHIP_NO_ERROR;
+    aSendStatusResponse    = true;
     CHIP_ERROR statusError = CHIP_NO_ERROR;
     SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
+    // Since this is a valid Status Response message, we don't have to send a Status Response in reply to it.
+    aSendStatusResponse = false;
     SuccessOrExit(err = statusError);
     switch (mState)
     {
@@ -162,11 +171,6 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
     }
 
 exit:
-    if (err != CHIP_NO_ERROR)
-    {
-        Close();
-    }
-
     return err;
 }
 
@@ -249,15 +253,26 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
 CHIP_ERROR ReadHandler::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
                                           System::PacketBufferHandle && aPayload)
 {
-    CHIP_ERROR err = CHIP_NO_ERROR;
-
+    CHIP_ERROR err          = CHIP_NO_ERROR;
+    bool sendStatusResponse = true;
     if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
     {
-        err = OnStatusResponse(apExchangeContext, std::move(aPayload));
+        err = OnStatusResponse(apExchangeContext, std::move(aPayload), sendStatusResponse);
     }
     else
     {
-        err = OnUnknownMsgType(apExchangeContext, aPayloadHeader, std::move(aPayload));
+        ChipLogDetail(DataManagement, "ReadHandler:: Msg type %d not supported", aPayloadHeader.GetMessageType());
+        err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
+    }
+
+    if (sendStatusResponse)
+    {
+        StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
+    }
+
+    if (err != CHIP_NO_ERROR)
+    {
+        Close();
     }
     return err;
 }
@@ -269,14 +284,6 @@ bool ReadHandler::IsFromSubscriber(Messaging::ExchangeContext & apExchangeContex
             GetAccessingFabricIndex() == apExchangeContext.GetSessionHandle()->GetFabricIndex());
 }
 
-CHIP_ERROR ReadHandler::OnUnknownMsgType(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
-                                         System::PacketBufferHandle && aPayload)
-{
-    ChipLogDetail(DataManagement, "Msg type %d not supported", aPayloadHeader.GetMessageType());
-    Close();
-    return CHIP_ERROR_INVALID_MESSAGE_TYPE;
-}
-
 void ReadHandler::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext)
 {
     ChipLogError(DataManagement, "Time out! failed to receive status response from Exchange: " ChipLogFormatExchange,
@@ -298,14 +305,7 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa
 
     ReturnErrorOnFailure(readRequestParser.Init(reader));
 #if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
-    err = readRequestParser.CheckSchemaValidity();
-    if (err != CHIP_NO_ERROR)
-    {
-        // The actual error we want to return to our consumer is an IM "invalid
-        // action" error, not whatever internal error CheckSchemaValidity
-        // happens to come up with.
-        return CHIP_IM_GLOBAL_STATUS(InvalidAction);
-    }
+    ReturnErrorOnFailure(readRequestParser.CheckSchemaValidity());
 #endif
 
     err = readRequestParser.GetAttributeRequests(&attributePathListParser);

diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h
@@ -240,7 +240,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
      *  @retval #CHIP_NO_ERROR On success.
      *
      */
-    CHIP_ERROR OnInitialRequest(System::PacketBufferHandle && aPayload);
+    void OnInitialRequest(System::PacketBufferHandle && aPayload);
 
     /**
      *  Send ReportData to initiator
@@ -369,12 +369,11 @@ class ReadHandler : public Messaging::ExchangeDelegate
     CHIP_ERROR ProcessAttributePathList(AttributePathIBs::Parser & aAttributePathListParser);
     CHIP_ERROR ProcessEventPaths(EventPathIBs::Parser & aEventPathsParser);
     CHIP_ERROR ProcessEventFilters(EventFilterIBs::Parser & aEventFiltersParser);
-    CHIP_ERROR OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload);
+    CHIP_ERROR OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload,
+                                bool & aSendStatusResponse);
     CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
                                  System::PacketBufferHandle && aPayload) override;
     void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;
-    CHIP_ERROR OnUnknownMsgType(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
-                                System::PacketBufferHandle && aPayload);
     void MoveToState(const HandlerState aTargetState);
 
     const char * GetStateStr() const;