From 42aef61c2833a910b07ec559637471384f23db12 Mon Sep 17 00:00:00 2001 From: Song GUO Date: Tue, 29 Mar 2022 22:54:57 +0800 Subject: [PATCH] [IM] Make CommandHandler::AddResponseData atomic (#15906) * Add encode failure tests * [IM] Make CommandHandler::AddResponseData atomic * Reset -> Rollback * Rename: AddResponseData -> AddResponse * Address comments * Update cluster code * 50 ms is too short on CI * Fix * Fix --- .../OTAProviderExample.cpp | 5 +- src/app/CommandHandler.cpp | 18 +++++ src/app/CommandHandler.h | 68 ++++++++++++++-- src/app/CommandResponseHelper.h | 9 +-- .../diagnostic-logs-server.cpp | 10 +-- .../door-lock-server/door-lock-server.cpp | 21 +++-- .../door-lock-server/door-lock-server.h | 16 ++-- .../general-commissioning-server.cpp | 8 +- .../group-key-mgmt-server.cpp | 13 +-- .../clusters/groups-server/groups-server.cpp | 28 ++----- .../network-commissioning.cpp | 10 +-- .../operational-credentials-server.cpp | 16 ++-- .../test-cluster-server.cpp | 44 +++------- src/app/tests/TestCommandInteraction.cpp | 81 ++++++++++++++++++- src/app/tests/TestTimedHandler.cpp | 2 +- .../tests/TestServerCommandDispatch.cpp | 2 +- .../tests/data_model/TestCommands.cpp | 2 +- 17 files changed, 224 insertions(+), 129 deletions(-) diff --git a/examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp b/examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp index c3f1e2754e2b61..7abfa8e3bb30a5 100644 --- a/examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp +++ b/examples/ota-provider-app/ota-provider-common/OTAProviderExample.cpp @@ -282,8 +282,7 @@ CHIP_ERROR OTAProviderExample::SendQueryImageResponse(chip::app::CommandHandler response.metadataForRequestor.Emplace(chip::ByteSpan()); } - ReturnErrorOnFailure(commandObj->AddResponseData(commandPath, response)); - + commandObj->AddResponse(commandPath, response); return CHIP_NO_ERROR; } @@ -390,7 +389,7 @@ EmberAfStatus OTAProviderExample::HandleApplyUpdateRequest(chip::app::CommandHan // Reset back to success case for subsequent uses mUpdateAction = OTAApplyUpdateAction::kProceed; - VerifyOrReturnError(commandObj->AddResponseData(commandPath, response) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE); + commandObj->AddResponse(commandPath, response); return EMBER_ZCL_STATUS_SUCCESS; } diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index ebbb4dd4607f99..bb898293cf4c39 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -481,10 +481,13 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct) { ReturnErrorOnFailure(AllocateBuffer()); + + mInvokeResponseBuilder.Checkpoint(mBackupWriter); // // We must not be in the middle of preparing a command, or having prepared or sent one. // VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE); + MoveToState(State::Preparing); InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses(); InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse(); ReturnErrorOnFailure(invokeResponses.GetError()); @@ -526,6 +529,7 @@ CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPat // We must not be in the middle of preparing a command, or having prepared or sent one. // VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE); + MoveToState(State::Preparing); InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses(); InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse(); ReturnErrorOnFailure(invokeResponses.GetError()); @@ -550,6 +554,17 @@ CHIP_ERROR CommandHandler::FinishStatus() return CHIP_NO_ERROR; } +CHIP_ERROR CommandHandler::RollbackResponse() +{ + VerifyOrReturnError(mState == State::Preparing || mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE); + mInvokeResponseBuilder.Rollback(mBackupWriter); + mInvokeResponseBuilder.ResetError(); + // Note: We only support one command per request, so we reset the state to Idle here, need to review the states when adding + // supports of having multiple requests in the same transaction. + MoveToState(State::Idle); + return CHIP_NO_ERROR; +} + TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter() { if (mState != State::AddingCommand) @@ -607,6 +622,9 @@ const char * CommandHandler::GetStateStr() const case State::Idle: return "Idle"; + case State::Preparing: + return "Preparing"; + case State::AddingCommand: return "AddingCommand"; diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h index 975fdf4b9ffbdd..ac76ac4de4d4c4 100644 --- a/src/app/CommandHandler.h +++ b/src/app/CommandHandler.h @@ -175,13 +175,42 @@ class CommandHandler template CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) { - ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; - ReturnErrorOnFailure(PrepareCommand(path, false)); - TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); - VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); - ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)), aData)); + // TryAddResponseData will ensure we are in the correct state when calling AddResponseData. + CHIP_ERROR err = TryAddResponseData(aRequestCommandPath, aData); + if (err != CHIP_NO_ERROR) + { + // The state guarantees that either we can rollback or we don't have to rollback the buffer, so we don't care about the + // return value of RollbackResponse. + RollbackResponse(); + } + return err; + } - return FinishCommand(/* aEndDataStruct = */ false); + /** + * API for adding a response. This will try to encode a data response (response command), and if that fails will encode a a + * Protocols::InteractionModel::Status::Failure status response instead. + * + * The template parameter T is generally expected to be a ClusterName::Commands::CommandName::Type struct, but any object that + * can be encoded using the DataModel::Encode machinery and exposes the right command id will work. + * + * Since the function will call AddStatus when it fails to encode the data, it cannot send any response when it fails to encode + * a status code since another AddStatus call will also fail. The error from AddStatus will just be logged. + * + * @param [in] aRequestCommandPath the concrete path of the command we are + * responding to. + * @param [in] aData the data for the response. + */ + template + void AddResponse(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) + { + if (CHIP_NO_ERROR != AddResponseData(aRequestCommandPath, aData)) + { + CHIP_ERROR err = AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::Failure); + if (err != CHIP_NO_ERROR) + { + ChipLogError(DataManagement, "Failed to encode status: %" CHIP_ERROR_FORMAT, err.Format()); + } + } } /** @@ -207,6 +236,7 @@ class CommandHandler enum class State { Idle, ///< Default state that the object starts out in, where no work has commenced + Preparing, ///< We are prepaing the command or status header. AddingCommand, ///< In the process of adding a command. AddedCommand, ///< A command has been completely encoded and is awaiting transmission. CommandSent, ///< The command has been sent successfully. @@ -216,6 +246,11 @@ class CommandHandler void MoveToState(const State aTargetState); const char * GetStateStr() const; + /** + * Rollback the state to before encoding the current ResponseData (before calling PrepareCommand / PrepareStatus) + */ + CHIP_ERROR RollbackResponse(); + /* * This forcibly closes the exchange context if a valid one is pointed to. Such a situation does * not arise during normal message processing flows that all normally call Close() above. This can only @@ -270,6 +305,26 @@ class CommandHandler CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, const Optional & aClusterStatus); + /** + * If this function fails, it may leave our TLV buffer in an inconsistent state. Callers should snapshot as needed before + * calling this function, and roll back as needed afterward. + * + * @param [in] aRequestCommandPath the concrete path of the command we are + * responding to. + * @param [in] aData the data for the response. + */ + template + CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData) + { + ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() }; + ReturnErrorOnFailure(PrepareCommand(path, false)); + TLV::TLVWriter * writer = GetCommandDataIBTLVWriter(); + VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE); + ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)), aData)); + + return FinishCommand(/* aEndDataStruct = */ false); + } + Messaging::ExchangeContext * mpExchangeCtx = nullptr; Callback * mpCallback = nullptr; InvokeResponseMessage::Builder mInvokeResponseBuilder; @@ -280,6 +335,7 @@ class CommandHandler State mState = State::Idle; chip::System::PacketBufferTLVWriter mCommandMessageWriter; + TLV::TLVWriter mBackupWriter; bool mBufferAllocated = false; }; diff --git a/src/app/CommandResponseHelper.h b/src/app/CommandResponseHelper.h index 7785f2b10aefb8..a85e92b72cc587 100644 --- a/src/app/CommandResponseHelper.h +++ b/src/app/CommandResponseHelper.h @@ -36,12 +36,9 @@ class CommandResponseHelper CHIP_ERROR Success(const CommandData & aResponse) { - CHIP_ERROR err = mCommandHandler->AddResponseData(mCommandPath, aResponse); - if (err == CHIP_NO_ERROR) - { - mSentResponse = true; - } - return err; + mCommandHandler->AddResponse(mCommandPath, aResponse); + mSentResponse = true; + return CHIP_NO_ERROR; }; CHIP_ERROR Success(ClusterStatus aClusterStatus) diff --git a/src/app/clusters/diagnostic-logs-server/diagnostic-logs-server.cpp b/src/app/clusters/diagnostic-logs-server/diagnostic-logs-server.cpp index f9a48939f20eec..9a918fc041343f 100644 --- a/src/app/clusters/diagnostic-logs-server/diagnostic-logs-server.cpp +++ b/src/app/clusters/diagnostic-logs-server/diagnostic-logs-server.cpp @@ -45,7 +45,7 @@ void DiagnosticLogsCommandHandler::InvokeCommand(HandlerContext & handlerContext if (mBuffer.IsEmpty()) { response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kNoLogs; - handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response); + handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response); break; } @@ -58,7 +58,7 @@ void DiagnosticLogsCommandHandler::InvokeCommand(HandlerContext & handlerContext if (!buf) { response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kBusy; - handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response); + handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response); break; } @@ -71,19 +71,19 @@ void DiagnosticLogsCommandHandler::InvokeCommand(HandlerContext & handlerContext response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kSuccess; response.content = chip::ByteSpan(buf.get() + sizeof(timeMs), logSize - sizeof(timeMs)); response.timeStamp = timeMs; - handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response); + handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response); } break; case chip::app::Clusters::DiagnosticLogs::LogsIntent::kNetworkDiag: { chip::app::Clusters::DiagnosticLogs::Commands::RetrieveLogsResponse::Type response; response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kNoLogs; - handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response); + handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response); } break; case chip::app::Clusters::DiagnosticLogs::LogsIntent::kCrashLogs: { chip::app::Clusters::DiagnosticLogs::Commands::RetrieveLogsResponse::Type response; response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kNoLogs; - handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response); + handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response); } break; } diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp index 70b092efce84b0..f0347d9cbd038d 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.cpp +++ b/src/app/clusters/door-lock-server/door-lock-server.cpp @@ -2169,11 +2169,10 @@ DlStatus DoorLockServer::clearSchedules(chip::EndpointId endpointId, uint16_t us return DlStatus::kSuccess; } -CHIP_ERROR DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj, - const chip::app::ConcreteCommandPath & commandPath, uint8_t weekdayIndex, - uint16_t userIndex, DlStatus status, DlDaysMaskMap daysMask, - uint8_t startHour, uint8_t startMinute, uint8_t endHour, - uint8_t endMinute) +void DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj, + const chip::app::ConcreteCommandPath & commandPath, uint8_t weekdayIndex, + uint16_t userIndex, DlStatus status, DlDaysMaskMap daysMask, uint8_t startHour, + uint8_t startMinute, uint8_t endHour, uint8_t endMinute) { VerifyOrDie(nullptr != commandObj); @@ -2190,7 +2189,7 @@ CHIP_ERROR DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHand response.endMinute = Optional(endMinute); } - return commandObj->AddResponseData(commandPath, response); + commandObj->AddResponse(commandPath, response); } bool DoorLockServer::yearDayIndexValid(chip::EndpointId endpointId, uint8_t yearDayIndex) @@ -2242,10 +2241,10 @@ DlStatus DoorLockServer::clearYearDaySchedules(chip::EndpointId endpointId, uint return DlStatus::kSuccess; } -CHIP_ERROR DoorLockServer::sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj, - const chip::app::ConcreteCommandPath & commandPath, uint8_t yearDayIndex, - uint16_t userIndex, DlStatus status, uint32_t localStartTime, - uint32_t localEndTime) +void DoorLockServer::sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj, + const chip::app::ConcreteCommandPath & commandPath, uint8_t yearDayIndex, + uint16_t userIndex, DlStatus status, uint32_t localStartTime, + uint32_t localEndTime) { VerifyOrDie(nullptr != commandObj); @@ -2259,7 +2258,7 @@ CHIP_ERROR DoorLockServer::sendGetYearDayScheduleResponse(chip::app::CommandHand response.localEndTime = Optional(localEndTime); } - return commandObj->AddResponseData(commandPath, response); + commandObj->AddResponse(commandPath, response); } EmberAfStatus DoorLockServer::clearCredential(chip::EndpointId endpointId, chip::FabricIndex modifier, chip::NodeId sourceNodeId, diff --git a/src/app/clusters/door-lock-server/door-lock-server.h b/src/app/clusters/door-lock-server/door-lock-server.h index 3e61471d5e4a7c..01fa48942eb6cc 100644 --- a/src/app/clusters/door-lock-server/door-lock-server.h +++ b/src/app/clusters/door-lock-server/door-lock-server.h @@ -238,21 +238,19 @@ class DoorLockServer DlStatus clearWeekDaySchedules(chip::EndpointId endpointId, uint16_t userIndex); DlStatus clearSchedules(chip::EndpointId endpointId, uint16_t userIndex); - CHIP_ERROR sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj, - const chip::app::ConcreteCommandPath & commandPath, uint8_t weekdayIndex, - uint16_t userIndex, DlStatus status, DlDaysMaskMap daysMask = DlDaysMaskMap(0), - uint8_t startHour = 0, uint8_t startMinute = 0, uint8_t endHour = 0, - uint8_t endMinute = 0); + void sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, + uint8_t weekdayIndex, uint16_t userIndex, DlStatus status, + DlDaysMaskMap daysMask = DlDaysMaskMap(0), uint8_t startHour = 0, uint8_t startMinute = 0, + uint8_t endHour = 0, uint8_t endMinute = 0); bool yearDayIndexValid(chip::EndpointId endpointId, uint8_t yearDayIndex); DlStatus clearYearDaySchedule(chip::EndpointId endpointId, uint16_t userIndex, uint8_t weekDayIndex); DlStatus clearYearDaySchedules(chip::EndpointId endpointId, uint16_t userIndex); - CHIP_ERROR sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj, - const chip::app::ConcreteCommandPath & commandPath, uint8_t yearDayIndex, - uint16_t userIndex, DlStatus status, uint32_t localStartTime = 0, - uint32_t localEndTime = 0); + void sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, + uint8_t yearDayIndex, uint16_t userIndex, DlStatus status, uint32_t localStartTime = 0, + uint32_t localEndTime = 0); bool sendRemoteLockUserChange(chip::EndpointId endpointId, DlLockDataType dataType, DlDataOperationType operation, chip::NodeId nodeId, chip::FabricIndex fabricIndex, uint16_t userIndex = 0, diff --git a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp index fb3db979b8f130..7817a6ea5f6e12 100644 --- a/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp +++ b/src/app/clusters/general-commissioning-server/general-commissioning-server.cpp @@ -151,12 +151,12 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler * CheckSuccess(failSafeContext.ArmFailSafe(accessingFabricIndex, System::Clock::Seconds16(commandData.expiryLengthSeconds)), Failure); response.errorCode = CommissioningError::kOk; - CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure); + commandObj->AddResponse(commandPath, response); } else { response.errorCode = CommissioningError::kBusyWithOtherAdmin; - CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure); + commandObj->AddResponse(commandPath, response); } return true; @@ -201,7 +201,7 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback( } } - CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure); + commandObj->AddResponse(commandPath, response); return true; } @@ -218,7 +218,7 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH Commands::SetRegulatoryConfigResponse::Type response; response.errorCode = CommissioningError::kOk; - CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure); + commandObj->AddResponse(commandPath, response); return true; } diff --git a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp index accc5c66c14398..e99b7d1334cf91 100644 --- a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp +++ b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp @@ -443,12 +443,7 @@ bool emberAfGroupKeyManagementClusterKeySetReadCallback( } response.groupKeySet.epochKey2.SetNull(); - CHIP_ERROR err = commandObj->AddResponseData(commandPath, response); - if (CHIP_NO_ERROR != err) - { - ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetRead failed: %s", ErrorStr(err)); - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } @@ -535,11 +530,7 @@ bool emberAfGroupKeyManagementClusterKeySetReadAllIndicesCallback( return true; } - CHIP_ERROR err = commandObj->AddResponseData(commandPath, KeySetReadAllIndicesResponse(keysIt)); - if (CHIP_NO_ERROR != err) - { - ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetReadAllIndices failed: %s", ErrorStr(err)); - } + commandObj->AddResponse(commandPath, KeySetReadAllIndicesResponse(keysIt)); keysIt->Release(); return true; } diff --git a/src/app/clusters/groups-server/groups-server.cpp b/src/app/clusters/groups-server/groups-server.cpp index 42ff2f2d5e1230..0d4ec94ca2a6a3 100644 --- a/src/app/clusters/groups-server/groups-server.cpp +++ b/src/app/clusters/groups-server/groups-server.cpp @@ -137,7 +137,6 @@ bool emberAfGroupsClusterAddGroupCallback(app::CommandHandler * commandObj, cons { auto fabricIndex = commandObj->GetAccessingFabricIndex(); Groups::Commands::AddGroupResponse::Type response; - CHIP_ERROR err = CHIP_NO_ERROR; // For all networks, Add Group commands are only responded to when // they are addressed to a single device. @@ -148,12 +147,7 @@ bool emberAfGroupsClusterAddGroupCallback(app::CommandHandler * commandObj, cons response.groupId = commandData.groupId; response.status = GroupAdd(fabricIndex, commandPath.mEndpointId, commandData.groupId, commandData.groupName); - err = commandObj->AddResponseData(commandPath, response); - if (CHIP_NO_ERROR != err) - { - ChipLogDetail(Zcl, "GroupsCluster: AddGroup failed: %s", err.AsString()); - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } @@ -185,12 +179,7 @@ bool emberAfGroupsClusterViewGroupCallback(app::CommandHandler * commandObj, con exit: response.groupId = groupId; response.status = status; - err = commandObj->AddResponseData(commandPath, response); - if (CHIP_NO_ERROR != err) - { - ChipLogDetail(Zcl, "GroupsCluster: ViewGroup failed: %s", err.AsString()); - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } @@ -284,14 +273,13 @@ bool emberAfGroupsClusterGetGroupMembershipCallback(app::CommandHandler * comman { GroupDataProvider::EndpointIterator * iter = nullptr; - CHIP_ERROR err = CHIP_NO_ERROR; iter = provider->IterateEndpoints(fabricIndex); VerifyOrExit(nullptr != iter, status = EMBER_ZCL_STATUS_FAILURE); - err = commandObj->AddResponseData(commandPath, GroupMembershipResponse(commandData, commandPath.mEndpointId, iter)); + commandObj->AddResponse(commandPath, GroupMembershipResponse(commandData, commandPath.mEndpointId, iter)); iter->Release(); - status = (CHIP_NO_ERROR == err) ? EMBER_ZCL_STATUS_SUCCESS : EMBER_ZCL_STATUS_FAILURE; + status = EMBER_ZCL_STATUS_SUCCESS; } exit: @@ -308,7 +296,6 @@ bool emberAfGroupsClusterRemoveGroupCallback(app::CommandHandler * commandObj, c { auto fabricIndex = commandObj->GetAccessingFabricIndex(); Groups::Commands::RemoveGroupResponse::Type response; - CHIP_ERROR err = CHIP_NO_ERROR; // For all networks, Remove Group commands are only responded to when // they are addressed to a single device. @@ -324,12 +311,7 @@ bool emberAfGroupsClusterRemoveGroupCallback(app::CommandHandler * commandObj, c response.groupId = commandData.groupId; response.status = GroupRemove(fabricIndex, commandPath.mEndpointId, commandData.groupId); - err = commandObj->AddResponseData(commandPath, response); - if (CHIP_NO_ERROR != err) - { - ChipLogDetail(Zcl, "GroupsCluster: RemoveGroup failed: %s", err.AsString()); - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index bae9bf2d94a334..f9695cd761a544 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -291,7 +291,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands MATTER_TRACE_EVENT_SCOPE("HandleAddOrUpdateWiFiNetwork", "NetworkCommissioning"); Commands::NetworkConfigResponse::Type response; response.networkingStatus = ToClusterObjectEnum(mpDriver.Get()->AddOrUpdateNetwork(req.ssid, req.credentials)); - ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response); + ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); } void Instance::HandleAddOrUpdateThreadNetwork(HandlerContext & ctx, const Commands::AddOrUpdateThreadNetwork::DecodableType & req) @@ -299,7 +299,7 @@ void Instance::HandleAddOrUpdateThreadNetwork(HandlerContext & ctx, const Comman MATTER_TRACE_EVENT_SCOPE("HandleAddOrUpdateThreadNetwork", "NetworkCommissioning"); Commands::NetworkConfigResponse::Type response; response.networkingStatus = ToClusterObjectEnum(mpDriver.Get()->AddOrUpdateNetwork(req.operationalDataset)); - ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response); + ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); } void Instance::HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveNetwork::DecodableType & req) @@ -307,7 +307,7 @@ void Instance::HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveN MATTER_TRACE_EVENT_SCOPE("HandleRemoveNetwork", "NetworkCommissioning"); Commands::NetworkConfigResponse::Type response; response.networkingStatus = ToClusterObjectEnum(mpWirelessDriver->RemoveNetwork(req.networkID)); - ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response); + ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); } void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::ConnectNetwork::DecodableType & req) @@ -331,7 +331,7 @@ void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::Reorde MATTER_TRACE_EVENT_SCOPE("HandleReorderNetwork", "NetworkCommissioning"); Commands::NetworkConfigResponse::Type response; response.networkingStatus = ToClusterObjectEnum(mpWirelessDriver->ReorderNetwork(req.networkID, req.networkIndex)); - ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response); + ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); } void Instance::OnResult(Status commissioningError, CharSpan errorText, int32_t interfaceStatus) @@ -349,7 +349,7 @@ void Instance::OnResult(Status commissioningError, CharSpan errorText, int32_t i response.networkingStatus = ToClusterObjectEnum(commissioningError); response.debugText = errorText; response.errorValue = interfaceStatus; - commandHandle->AddResponseData(mPath, response); + commandHandle->AddResponse(mPath, response); mLastNetworkIDLen = mConnectingNetworkIDLen; memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen); diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 7b26f8d9e090cf..3057f3446f8f44 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -61,8 +61,8 @@ using namespace chip::Protocols::InteractionModel; namespace { -CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status, - uint8_t index, const CharSpan & debug_text); +void SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status, + uint8_t index, const CharSpan & debug_text); OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err); constexpr uint8_t kDACCertificate = 1; @@ -519,8 +519,8 @@ namespace { // TODO: Manage ephemeral RCAC/ICAC/NOC storage to avoid a full FabricInfo being needed here. FabricInfo gFabricBeingCommissioned; -CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status, - uint8_t index, const CharSpan & debug_text) +void SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status, + uint8_t index, const CharSpan & debug_text) { Commands::NOCResponse::Type payload; payload.statusCode = status; @@ -535,7 +535,7 @@ CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteComma payload.debugText.Emplace(to_send); } - return commandObj->AddResponseData(path, payload); + commandObj->AddResponse(path, payload); } OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err) @@ -774,7 +774,7 @@ bool emberAfOperationalCredentialsClusterCertificateChainRequestCallback( } response.certificate = derBufSpan; - SuccessOrExit(err = commandObj->AddResponseData(commandPath, response)); + commandObj->AddResponse(commandPath, response); exit: if (err != CHIP_NO_ERROR) @@ -834,7 +834,7 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command response.attestationElements = attestationElementsSpan; response.signature = signatureSpan; - SuccessOrExit(err = commandObj->AddResponseData(commandPath, response)); + commandObj->AddResponse(commandPath, response); } exit: @@ -922,7 +922,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler response.NOCSRElements = nocsrElementsSpan; response.attestationSignature = signatureSpan; - SuccessOrExit(err = commandObj->AddResponseData(commandPath, response)); + commandObj->AddResponse(commandPath, response); } exit: diff --git a/src/app/clusters/test-cluster-server/test-cluster-server.cpp b/src/app/clusters/test-cluster-server/test-cluster-server.cpp index 2fe745b13ef912..ebbc8be7e70468 100644 --- a/src/app/clusters/test-cluster-server/test-cluster-server.cpp +++ b/src/app/clusters/test-cluster-server/test-cluster-server.cpp @@ -714,11 +714,7 @@ bool emberAfTestClusterClusterTestSpecificCallback(CommandHandler * apCommandObj { TestSpecificResponse::Type responseData; responseData.returnValue = 7; - CHIP_ERROR err = apCommandObj->AddResponseData(commandPath, responseData); - if (CHIP_NO_ERROR != err) - { - ChipLogError(Zcl, "Test Cluster: failed to send TestSpecific response: %" CHIP_ERROR_FORMAT, err.Format()); - } + apCommandObj->AddResponse(commandPath, responseData); return true; } @@ -738,11 +734,7 @@ bool emberAfTestClusterClusterTestAddArgumentsCallback(CommandHandler * apComman TestAddArgumentsResponse::Type responseData; responseData.returnValue = static_cast(commandData.arg1 + commandData.arg2); - CHIP_ERROR err = apCommandObj->AddResponseData(commandPath, responseData); - if (CHIP_NO_ERROR != err) - { - ChipLogError(Zcl, "Test Cluster: failed to send TestAddArguments response: %" CHIP_ERROR_FORMAT, err.Format()); - } + apCommandObj->AddResponse(commandPath, responseData); return true; } @@ -750,11 +742,7 @@ static bool SendBooleanResponse(CommandHandler * commandObj, const ConcreteComma { Commands::BooleanResponse::Type response; response.value = value; - CHIP_ERROR err = commandObj->AddResponseData(commandPath, response); - if (err != CHIP_NO_ERROR) - { - commandObj->AddStatus(commandPath, Protocols::InteractionModel::Status::Failure); - } + commandObj->AddResponse(commandPath, response); return true; } @@ -811,7 +799,7 @@ bool emberAfTestClusterClusterTestEmitTestEventRequestCallback( emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); return true; } - commandObj->AddResponseData(commandPath, responseData); + commandObj->AddResponse(commandPath, responseData); return true; } @@ -827,7 +815,7 @@ bool emberAfTestClusterClusterTestEmitTestFabricScopedEventRequestCallback( emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); return true; } - commandObj->AddResponseData(commandPath, responseData); + commandObj->AddResponse(commandPath, responseData); return true; } @@ -923,7 +911,7 @@ bool emberAfTestClusterClusterTestListInt8UReverseRequestCallback( Commands::TestListInt8UReverseResponse::Type responseData; if (count == 0) { - SuccessOrExit(commandObj->AddResponseData(commandPath, responseData)); + commandObj->AddResponse(commandPath, responseData); return true; } size_t cur = count; @@ -937,7 +925,7 @@ bool emberAfTestClusterClusterTestListInt8UReverseRequestCallback( VerifyOrExit(cur == 0, ); VerifyOrExit(iter.GetStatus() == CHIP_NO_ERROR, ); responseData.arg1 = DataModel::List(responseBuf.Get(), count); - SuccessOrExit(commandObj->AddResponseData(commandPath, responseData)); + commandObj->AddResponse(commandPath, responseData); return true; } @@ -953,11 +941,7 @@ bool emberAfTestClusterClusterTestEnumsRequestCallback(CommandHandler * commandO response.arg1 = commandData.arg1; response.arg2 = commandData.arg2; - CHIP_ERROR err = commandObj->AddResponseData(commandPath, response); - if (err != CHIP_NO_ERROR) - { - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } @@ -979,11 +963,7 @@ bool emberAfTestClusterClusterTestNullableOptionalRequestCallback( response.originalValue.Emplace(commandData.arg1.Value()); } - CHIP_ERROR err = commandObj->AddResponseData(commandPath, response); - if (err != CHIP_NO_ERROR) - { - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } @@ -1000,11 +980,7 @@ bool emberAfTestClusterClusterSimpleStructEchoRequestCallback(CommandHandler * c response.arg1.g = commandData.arg1.g; response.arg1.h = commandData.arg1.h; - CHIP_ERROR err = commandObj->AddResponseData(commandPath, response); - if (err != CHIP_NO_ERROR) - { - emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE); - } + commandObj->AddResponse(commandPath, response); return true; } diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 5aeb21815c51b8..bdd5c77d099f24 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -178,6 +179,8 @@ class TestCommandInteraction static void TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendSimpleCommandData(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext); + static void TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext); static void TestCommandHandlerWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext); @@ -515,6 +518,23 @@ struct Fields } }; +struct BadFields +{ + static constexpr chip::CommandId GetCommandId() { return 4; } + CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const + { + TLV::TLVType outerContainerType; + uint8_t data[36] = { 0 }; + ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType)); + // Just encode something bad to return a failure state here. + for (uint8_t i = 1; i < UINT8_MAX; i++) + { + ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(i), ByteSpan(data))); + } + return aWriter.EndContainer(outerContainerType); + } +}; + void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext) { TestContext & ctx = *static_cast(apContext); @@ -527,7 +547,64 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * auto path = MakeTestCommandPath(); - err = commandHandler.AddResponseData(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), Fields()); + commandHandler.AddResponse(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), Fields()); + err = commandHandler.Finalize(commandPacket); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + +#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK + chip::System::PacketBufferTLVReader reader; + InvokeResponseMessage::Parser invokeResponseMessageParser; + reader.Init(std::move(commandPacket)); + err = invokeResponseMessageParser.Init(reader); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + err = invokeResponseMessageParser.CheckSchemaValidity(); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); +#endif +} + +void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + app::CommandHandler commandHandler(nullptr); + System::PacketBufferHandle commandPacket; + + TestExchangeDelegate delegate; + commandHandler.mpExchangeCtx = ctx.NewExchangeToAlice(&delegate); + + auto path = MakeTestCommandPath(); + + commandHandler.AddResponse(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), BadFields()); + err = commandHandler.Finalize(commandPacket); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + +#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK + chip::System::PacketBufferTLVReader reader; + InvokeResponseMessage::Parser invokeResponseMessageParser; + reader.Init(std::move(commandPacket)); + err = invokeResponseMessageParser.Init(reader); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + err = invokeResponseMessageParser.CheckSchemaValidity(); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); +#endif +} + +void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + app::CommandHandler commandHandler(nullptr); + System::PacketBufferHandle commandPacket; + + TestExchangeDelegate delegate; + commandHandler.mpExchangeCtx = ctx.NewExchangeToAlice(&delegate); + + auto path = MakeTestCommandPath(); + + err = commandHandler.AddResponseData(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), BadFields()); + NL_TEST_ASSERT(apSuite, err != CHIP_NO_ERROR); + err = commandHandler.AddStatus(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), + Protocols::InteractionModel::Status::Failure); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); err = commandHandler.Finalize(commandPacket); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -826,6 +903,8 @@ const nlTest sTests[] = NL_TEST_DEF("TestCommandSenderWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandSenderWithProcessReceivedMsg), NL_TEST_DEF("TestCommandHandlerWithSendSimpleCommandData", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleCommandData), NL_TEST_DEF("TestCommandHandlerCommandDataEncoding", chip::app::TestCommandInteraction::TestCommandHandlerCommandDataEncoding), + NL_TEST_DEF("TestCommandHandlerCommandEncodeFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeFailure), + NL_TEST_DEF("TestCommandHandlerCommandEncodeExternalFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure), NL_TEST_DEF("TestCommandHandlerWithSendSimpleStatusCode", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg), NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand), diff --git a/src/app/tests/TestTimedHandler.cpp b/src/app/tests/TestTimedHandler.cpp index e92fedff593816..2af7563b2c3141 100644 --- a/src/app/tests/TestTimedHandler.cpp +++ b/src/app/tests/TestTimedHandler.cpp @@ -111,7 +111,7 @@ void TestTimedHandler::TestFollowingMessageFastEnough(nlTestSuite * aSuite, void TestContext & ctx = *static_cast(aContext); System::PacketBufferHandle payload; - GenerateTimedRequest(aSuite, 50, payload); + GenerateTimedRequest(aSuite, 500, payload); TestExchangeDelegate delegate; ExchangeContext * exchange = ctx.NewExchangeToAlice(&delegate); diff --git a/src/controller/tests/TestServerCommandDispatch.cpp b/src/controller/tests/TestServerCommandDispatch.cpp index a6515ddd7f6551..a824a57cd58912 100644 --- a/src/controller/tests/TestServerCommandDispatch.cpp +++ b/src/controller/tests/TestServerCommandDispatch.cpp @@ -93,7 +93,7 @@ void TestClusterCommandHandler::InvokeCommand(chip::app::CommandHandlerInterface dataResponse.arg1 = nestedStructList; dataResponse.arg6 = true; - ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, dataResponse); + ctx.mCommandHandler.AddResponse(ctx.mRequestPath, dataResponse); } return CHIP_NO_ERROR; diff --git a/src/controller/tests/data_model/TestCommands.cpp b/src/controller/tests/data_model/TestCommands.cpp index 393c1918f67b50..01d5e40a0e9768 100644 --- a/src/controller/tests/data_model/TestCommands.cpp +++ b/src/controller/tests/data_model/TestCommands.cpp @@ -106,7 +106,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip dataResponse.arg1 = nestedStructList; dataResponse.arg6 = true; - apCommandObj->AddResponseData(aCommandPath, dataResponse); + apCommandObj->AddResponse(aCommandPath, dataResponse); } else if (responseDirective == kSendSuccessStatusCode) {