From bd924a86845d7808a747a1497000212fab12eb91 Mon Sep 17 00:00:00 2001 From: yunhanw Date: Sun, 7 Aug 2022 21:34:10 -0700 Subject: [PATCH] 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. --- src/app/InteractionModelEngine.cpp | 8 ++------ src/app/ReadHandler.cpp | 19 ++++++++----------- src/app/ReadHandler.h | 2 +- src/app/tests/TestReadInteraction.cpp | 16 +++++++++++----- 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 43a0d7f0052290..37549d5021a6c2 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -458,15 +458,11 @@ 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 index 01d55319d95402..4ede156be3084e 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -88,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; @@ -104,6 +104,12 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload) if (err != CHIP_NO_ERROR) { + Status status = Status::InvalidAction; + if (err == CHIP_IM_GLOBAL_STATUS(PathsExhausted)) + { + status = Status::PathsExhausted; + } + StatusResponse::Send(status, mExchangeCtx.Get(), /* aExpectResponse = */ false); Close(); } else @@ -111,8 +117,6 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload) // 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, @@ -301,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 index 1688f2e11c2204..3e192a2abf5f59 100644 --- 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 diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index e29ee3ef87f771..fc21f94f6a9ccc 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -521,7 +521,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex err = writer.Finalize(&readRequestbuf); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = readHandler.OnInitialRequest(std::move(readRequestbuf)); + err = readHandler.ProcessReadRequest(std::move(readRequestbuf)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } @@ -654,16 +654,21 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu attributePathListBuilder.EndOfAttributePathIBs(); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - readRequestBuilder.IsFabricFiltered(false).EndOfReadRequestMessage(); + readRequestBuilder.EndOfReadRequestMessage(); NL_TEST_ASSERT(apSuite, readRequestBuilder.GetError() == CHIP_NO_ERROR); err = writer.Finalize(&readRequestbuf); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - err = readHandler.OnInitialRequest(std::move(readRequestbuf)); - NL_TEST_ASSERT(apSuite, err == CHIP_IM_GLOBAL_STATUS(InvalidAction)); + err = readHandler.ProcessReadRequest(std::move(readRequestbuf)); + ChipLogError(DataManagement, "The error is %s", ErrorStr(err)); +#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB); +#else + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_END_OF_TLV); +#endif // CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK // - // In the call above to OnInitialRequest, the handler will not actually close out the EC since + // In the call above to ProcessReadRequest, the handler will not actually close out the EC since // it expects the ExchangeManager to do so automatically given it's not calling WillSend() on the EC, // and is not sending a response back. // @@ -3449,6 +3454,7 @@ void TestReadInteraction::TestReadHandlerMalformedReadRequest2(nlTestSuite * apS Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); + ChipLogError(DataManagement, "The error is %s", ErrorStr(delegate.mError)); NL_TEST_ASSERT(apSuite, delegate.mError == CHIP_IM_GLOBAL_STATUS(InvalidAction)); } NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);