Skip to content

Commit

Permalink
Fix status response in command handler.
Browse files Browse the repository at this point in the history 
When there is issue in incoming request, OnInvokeCommandRequest in
CommandRequest would send status response.
  • Loading branch information
@yunhanw-google
yunhanw-google committed Aug 2, 2022
1 parent 97134c5 commit dbfcc99
Show file tree
Hide file tree
Showing 5 changed files with 138 additions and 95 deletions.
86 changes: 43 additions & 43 deletions src/app/CommandHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

namespace chip {
namespace app {
using Status = Protocols::InteractionModel::Status;

CommandHandler::CommandHandler(Callback * apCallback) : mExchangeCtx(*this), mpCallback(apCallback), mSuppressResponse(false) {}

Expand All @@ -65,11 +66,15 @@ CHIP_ERROR CommandHandler::AllocateBuffer()
return CHIP_NO_ERROR;
}

CHIP_ERROR CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload, bool isTimedInvoke)
void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload, bool isTimedInvoke)
{
System::PacketBufferHandle response;
VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE);
Status status = Status::Failure;
VerifyOrDieWithMsg(ec != nullptr, DataManagement,
"Incoming exchange context should not be null");
VerifyOrDieWithMsg(mState == State::Idle, DataManagement,
"state should be Idle");

// NOTE: we already know this is an InvokeCommand Request message because we explicitly registered with the
// Exchange Manager for unsolicited InvokeCommand Requests.
Expand All @@ -81,67 +86,56 @@ CHIP_ERROR CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * e
// response too. Figure out at what point it's our responsibility to
// handler errors vs our caller's.
Handle workHandle(this);
mExchangeCtx->WillSendMessage();
ReturnErrorOnFailure(ProcessInvokeRequest(std::move(payload), isTimedInvoke));

return CHIP_NO_ERROR;
mExchangeCtx->WillSendMessage();
status = ProcessInvokeRequest(std::move(payload), isTimedInvoke);
if (status != Status::Success)
{
StatusResponse::Send(status, mExchangeCtx.Get(), false /*aExpectResponse*/);
mSentStatusResponse = true;
}
}

CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke)
Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke)
{
CHIP_ERROR err = CHIP_NO_ERROR;
System::PacketBufferTLVReader reader;
TLV::TLVReader invokeRequestsReader;
InvokeRequestMessage::Parser invokeRequestMessage;
InvokeRequests::Parser invokeRequests;
reader.Init(std::move(payload));
ReturnErrorOnFailure(invokeRequestMessage.Init(reader));
VerifyOrReturnError(invokeRequestMessage.Init(reader) == CHIP_NO_ERROR, Status::InvalidAction);
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
ReturnErrorOnFailure(invokeRequestMessage.CheckSchemaValidity());
VerifyOrReturnError(invokeRequestMessage.CheckSchemaValidity() == CHIP_NO_ERROR, Status::InvalidAction);
#endif
ReturnErrorOnFailure(invokeRequestMessage.GetSuppressResponse(&mSuppressResponse));
ReturnErrorOnFailure(invokeRequestMessage.GetTimedRequest(&mTimedRequest));
ReturnErrorOnFailure(invokeRequestMessage.GetInvokeRequests(&invokeRequests));

VerifyOrReturnError(mExchangeCtx, CHIP_ERROR_INCORRECT_STATE);

if (mTimedRequest != isTimedInvoke)
{
// The message thinks it should be part of a timed interaction but it's
// not, or vice versa. Spec says to Respond with UNSUPPORTED_ACCESS.
err = StatusResponse::Send(Protocols::InteractionModel::Status::UnsupportedAccess, mExchangeCtx.Get(),
/* aExpectResponse = */ false);
if (err == CHIP_NO_ERROR)
{
mSentStatusResponse = true;
}

return err;
}

VerifyOrReturnError(invokeRequestMessage.GetSuppressResponse(&mSuppressResponse) == CHIP_NO_ERROR, Status::InvalidAction);
VerifyOrReturnError(invokeRequestMessage.GetTimedRequest(&mTimedRequest) == CHIP_NO_ERROR, Status::InvalidAction);
VerifyOrReturnError(invokeRequestMessage.GetInvokeRequests(&invokeRequests) == CHIP_NO_ERROR, Status::InvalidAction);
VerifyOrReturnError(mTimedRequest == isTimedInvoke, Status::UnsupportedAccess);
invokeRequests.GetReader(&invokeRequestsReader);

{
// We don't support handling multiple commands but the protocol is ready to support it in the future, reject all of them and
// IM Engine will send a status response.
size_t commandCount = 0;
TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */);
VerifyOrReturnError(commandCount == 1, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
VerifyOrReturnError(commandCount == 1, Status::Failure);
}

while (CHIP_NO_ERROR == (err = invokeRequestsReader.Next()))
{
VerifyOrReturnError(TLV::AnonymousTag() == invokeRequestsReader.GetTag(), CHIP_ERROR_INVALID_TLV_TAG);
VerifyOrReturnError(TLV::AnonymousTag() == invokeRequestsReader.GetTag(), Status::InvalidAction);
CommandDataIB::Parser commandData;
ReturnErrorOnFailure(commandData.Init(invokeRequestsReader));
VerifyOrReturnError(commandData.Init(invokeRequestsReader) == CHIP_NO_ERROR, Status::InvalidAction);

if (mExchangeCtx->IsGroupExchangeContext())
{
ReturnErrorOnFailure(ProcessGroupCommandDataIB(commandData));
VerifyOrReturnError(ProcessGroupCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure);
}
else
{
ReturnErrorOnFailure(ProcessCommandDataIB(commandData));
VerifyOrReturnError(ProcessCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure);
}
}

Expand All @@ -150,8 +144,17 @@ CHIP_ERROR CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && pa
{
err = CHIP_NO_ERROR;
}
ReturnErrorOnFailure(err);
return invokeRequestMessage.ExitContainer();
VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
VerifyOrReturnError(invokeRequestMessage.ExitContainer() == CHIP_NO_ERROR, Status::InvalidAction);
return Status::Success;
}

CHIP_ERROR CommandHandler::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload)
{
ChipLogDetail(DataManagement, "CommandHandler: Unexpected message type %d", aPayloadHeader.GetMessageType());
StatusResponse::Send(Status::InvalidAction, mExchangeCtx.Get(), false /*aExpectResponse*/);
return CHIP_ERROR_INVALID_MESSAGE_TYPE;
}

void CommandHandler::Close()
Expand Down Expand Up @@ -186,7 +189,7 @@ void CommandHandler::DecrementHoldOff()
return;
}

if (!mExchangeCtx->IsGroupExchangeContext() && !mSentStatusResponse)
if (!mSentStatusResponse && !mExchangeCtx->IsGroupExchangeContext())
{
CHIP_ERROR err = SendCommandResponse();
if (err != CHIP_NO_ERROR)
Expand Down Expand Up @@ -250,7 +253,6 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
err = commandPath.GetEndpointId(&concretePath.mEndpointId);
SuccessOrExit(err);

using Protocols::InteractionModel::Status;
{
Status commandExists = mpCallback->CommandExists(concretePath);
if (commandExists != Status::Success)
Expand Down Expand Up @@ -284,7 +286,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand
{
// TODO: when wildcard invokes are supported, discard a
// wildcard-expanded path instead of returning a status.
return AddStatus(concretePath, Protocols::InteractionModel::Status::NeedsTimedInteraction);
return AddStatus(concretePath, Status::NeedsTimedInteraction);
}

if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId))
Expand Down Expand Up @@ -399,7 +401,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo

const ConcreteCommandPath concretePath(mapping.endpoint_id, clusterId, commandId);

if (mpCallback->CommandExists(concretePath) != Protocols::InteractionModel::Status::Success)
if (mpCallback->CommandExists(concretePath) != Status::Success)
{
ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x",
ChipLogValueMEI(commandId), ChipLogValueMEI(clusterId), mapping.endpoint_id);
Expand Down Expand Up @@ -452,20 +454,18 @@ CHIP_ERROR CommandHandler::AddStatusInternal(const ConcreteCommandPath & aComman
return FinishStatus();
}

CHIP_ERROR CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus)
CHIP_ERROR CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, const Status aStatus)
{
return AddStatusInternal(aCommandPath, StatusIB(aStatus));
}

CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)
{
using Protocols::InteractionModel::Status;
return AddStatusInternal(aCommandPath, StatusIB(Status::Success, aClusterStatus));
}

CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)
{
using Protocols::InteractionModel::Status;
return AddStatusInternal(aCommandPath, StatusIB(Status::Failure, aClusterStatus));
}

Expand Down Expand Up @@ -635,7 +635,7 @@ const char * CommandHandler::GetStateStr() const
void CommandHandler::MoveToState(const State aTargetState)
{
mState = aTargetState;
ChipLogDetail(DataManagement, "ICR moving to [%10.10s]", GetStateStr());
ChipLogDetail(DataManagement, "Command handler moving to [%10.10s]", GetStateStr());
}

} // namespace app
Expand Down
14 changes: 4 additions & 10 deletions src/app/CommandHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,15 @@ class CommandHandler : public Messaging::ExchangeDelegate
* transaction (i.e. was preceded by a Timed Request). If we reach here,
* the timer verification has already been done.
*/
CHIP_ERROR OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload, bool isTimedInvoke);
void OnInvokeCommandRequest(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload, bool isTimedInvoke);
CHIP_ERROR AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus);

CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);

CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);

CHIP_ERROR ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke);
Protocols::InteractionModel::Status ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke);
CHIP_ERROR PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct = true);
CHIP_ERROR FinishCommand(bool aEndDataStruct = true);
CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath);
Expand Down Expand Up @@ -250,13 +250,7 @@ class CommandHandler : public Messaging::ExchangeDelegate
friend class CommandHandler::Handle;

CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload) override
{
//
// We shouldn't be receiving any further messages on this exchange.
//
return CHIP_ERROR_INCORRECT_STATE;
}
System::PacketBufferHandle && payload) override;

void OnResponseTimeout(Messaging::ExchangeContext * ec) override
{
Expand Down
21 changes: 8 additions & 13 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,22 +278,18 @@ void InteractionModelEngine::OnDone(ReadHandler & apReadObj)
mReadHandlers.ReleaseObject(&apReadObj);
}

CHIP_ERROR InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, bool aIsTimedInvoke,
Protocols::InteractionModel::Status & aStatus)
Status InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
bool aIsTimedInvoke)
{
CommandHandler * commandHandler = mCommandHandlerObjs.CreateObject(this);
if (commandHandler == nullptr)
{
ChipLogProgress(InteractionModel, "no resource for Invoke interaction");
aStatus = Status::Busy;
return CHIP_ERROR_NO_MEMORY;
return Status::Busy;
}
ReturnErrorOnFailure(
commandHandler->OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), aIsTimedInvoke));
aStatus = Status::Success;
return CHIP_NO_ERROR;
commandHandler->OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), aIsTimedInvoke);
return Status::Success;
}

Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
Expand Down Expand Up @@ -567,7 +563,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext

if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandRequest))
{
OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ false, status);
status = OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ false);
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReadRequest))
{
Expand Down Expand Up @@ -1349,8 +1345,7 @@ void InteractionModelEngine::OnTimedInvoke(TimedHandler * apTimedHandler, Messag
VerifyOrDie(aPayloadHeader.HasMessageType(MsgType::InvokeCommandRequest));
VerifyOrDie(!apExchangeContext->IsGroupExchangeContext());

Status status = Status::Failure;
OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ true, status);
Status status = OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ true);
if (status != Status::Success)
{
StatusResponse::Send(status, apExchangeContext, /* aExpectResponse = */ false);
Expand Down
9 changes: 3 additions & 6 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,13 +357,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
CHIP_ERROR OnUnsolicitedMessageReceived(const PayloadHeader & payloadHeader, ExchangeDelegate *& newDelegate) override;

/**
* Called when Interaction Model receives a Command Request message. Errors processing
* the Command Request are handled entirely within this function. The caller pre-sets status to failure and the callee is
* expected to set it to success if it does not want an automatic status response message to be sent.
* Called when Interaction Model receives a Command Request message.
*/
CHIP_ERROR OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, bool aIsTimedInvoke,
Protocols::InteractionModel::Status & aStatus);
Status OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, bool aIsTimedInvoke);
CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload) override;
void OnResponseTimeout(Messaging::ExchangeContext * ec) override;
Expand Down
Loading

0 comments on commit dbfcc99

Please sign in to comment.