Skip to content

Commit

Permalink
Make the error-handling in OnReadInitialRequest clearer. (#18706)
Browse files Browse the repository at this point in the history 
Instead of having both a status outparam and an error return, just use a status return.

Also fixes some bugs where we were not checking the results of some
TLV parsing operations, and not converting some TVL parsing failures
to the right status.

Fixes #11724

Fixes #17623
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Jul 17, 2023
1 parent aac852e commit 2412401
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 55 deletions.
103 changes: 54 additions & 49 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,10 @@ CHIP_ERROR InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeCon
return CHIP_NO_ERROR;
}

CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload,
ReadHandler::InteractionType aInteractionType,
Protocols::InteractionModel::Status & aStatus)
Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload,
ReadHandler::InteractionType aInteractionType)
{
ChipLogDetail(InteractionModel, "Received %s request",
aInteractionType == ReadHandler::InteractionType::Subscribe ? "Subscribe" : "Read");
Expand All @@ -301,55 +300,68 @@ CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeConte
if (apExchangeContext->GetSessionHandle()->GetFabricIndex() == kUndefinedFabricIndex)
{
// Subscriptions must be associated to a fabric.
aStatus = Protocols::InteractionModel::Status::UnsupportedAccess;
return CHIP_NO_ERROR;
return Status::UnsupportedAccess;
}

reader.Init(aPayload.Retain());

SubscribeRequestMessage::Parser subscribeRequestParser;
ReturnErrorOnFailure(subscribeRequestParser.Init(reader));
CHIP_ERROR err = subscribeRequestParser.Init(reader);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}

{
size_t requestedAttributePathCount = 0;
size_t requestedEventPathCount = 0;
AttributePathIBs::Parser attributePathListParser;
CHIP_ERROR err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
if (err == CHIP_NO_ERROR)
{
TLV::TLVReader pathReader;
attributePathListParser.GetReader(&pathReader);
TLV::Utilities::Count(pathReader, requestedAttributePathCount, false);
err = TLV::Utilities::Count(pathReader, requestedAttributePathCount, false);
}
else if (err != CHIP_ERROR_END_OF_TLV)
else if (err == CHIP_ERROR_END_OF_TLV)
{
aStatus = Protocols::InteractionModel::Status::InvalidAction;
return CHIP_NO_ERROR;
err = CHIP_NO_ERROR;
}
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}

EventPathIBs::Parser eventpathListParser;
err = subscribeRequestParser.GetEventRequests(&eventpathListParser);
if (err == CHIP_NO_ERROR)
{
TLV::TLVReader pathReader;
eventpathListParser.GetReader(&pathReader);
TLV::Utilities::Count(pathReader, requestedEventPathCount, false);
err = TLV::Utilities::Count(pathReader, requestedEventPathCount, false);
}
else if (err != CHIP_ERROR_END_OF_TLV)
else if (err == CHIP_ERROR_END_OF_TLV)
{
aStatus = Protocols::InteractionModel::Status::InvalidAction;
return CHIP_NO_ERROR;
err = CHIP_NO_ERROR;
}
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}

// The following cast is safe, since we can only hold a few tens of paths in one request.
if (!EnsureResourceForSubscription(apExchangeContext->GetSessionHandle()->GetFabricIndex(), requestedAttributePathCount,
requestedEventPathCount))
{
aStatus = Protocols::InteractionModel::Status::PathsExhausted;
return CHIP_NO_ERROR;
return Status::PathsExhausted;
}
}

ReturnErrorOnFailure(subscribeRequestParser.GetKeepSubscriptions(&keepExistingSubscriptions));
err = subscribeRequestParser.GetKeepSubscriptions(&keepExistingSubscriptions);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}

if (!keepExistingSubscriptions)
{
Expand Down Expand Up @@ -381,33 +393,29 @@ CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeConte

if ((handlerPoolCapacity - GetNumActiveReadHandlers()) == 0)
{
aStatus = Protocols::InteractionModel::Status::ResourceExhausted;
return CHIP_NO_ERROR;
return Status::ResourceExhausted;
}
#endif

// We have already reserved enough resources for read requests, and have granted enough resources for current subscriptions, so
// we should be able to allocate resources requested by this request.
ReadHandler * handler = mReadHandlers.CreateObject(*this, apExchangeContext, aInteractionType);
if (handler)
if (handler == nullptr)
{
CHIP_ERROR err = handler->OnInitialRequest(std::move(aPayload));
if (err == CHIP_ERROR_NO_MEMORY)
{
aStatus = Protocols::InteractionModel::Status::ResourceExhausted;
}
else
{
aStatus = StatusIB(err).mStatus;
}
return err;
ChipLogProgress(InteractionModel, "no resource for %s interaction",
aInteractionType == ReadHandler::InteractionType::Subscribe ? "Subscribe" : "Read");
return Status::ResourceExhausted;
}

ChipLogProgress(InteractionModel, "no resource for %s interaction",
aInteractionType == ReadHandler::InteractionType::Subscribe ? "Subscribe" : "Read");
aStatus = Protocols::InteractionModel::Status::ResourceExhausted;
CHIP_ERROR err = handler->OnInitialRequest(std::move(aPayload));
if (err == CHIP_ERROR_NO_MEMORY)
{
return Status::ResourceExhausted;
}

return CHIP_NO_ERROR;
// TODO: Should probably map various TLV errors into InvalidAction, here
// or inside the read handler.
return StatusIB(err).mStatus;
}

Protocols::InteractionModel::Status InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext,
Expand Down Expand Up @@ -494,7 +502,6 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
{
using namespace Protocols::InteractionModel;

CHIP_ERROR err = CHIP_NO_ERROR;
Protocols::InteractionModel::Status status = Protocols::InteractionModel::Status::Failure;

// Group Message can only be an InvokeCommandRequest or WriteRequest
Expand All @@ -503,27 +510,25 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest))
{
ChipLogProgress(InteractionModel, "Msg type %d not supported for group message", aPayloadHeader.GetMessageType());
return err;
return CHIP_NO_ERROR;
}

if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandRequest))
{
SuccessOrExit(
OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ false, status));
OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ false, status);
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReadRequest))
{
SuccessOrExit(OnReadInitialRequest(apExchangeContext, aPayloadHeader, std::move(aPayload),
ReadHandler::InteractionType::Read, status));
status = OnReadInitialRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), ReadHandler::InteractionType::Read);
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest))
{
status = OnWriteRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedWrite = */ false);
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::SubscribeRequest))
{
SuccessOrExit(OnReadInitialRequest(apExchangeContext, aPayloadHeader, std::move(aPayload),
ReadHandler::InteractionType::Subscribe, status));
status =
OnReadInitialRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), ReadHandler::InteractionType::Subscribe);
}
else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData))
{
Expand All @@ -532,19 +537,19 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext
}
else if (aPayloadHeader.HasMessageType(MsgType::TimedRequest))
{
SuccessOrExit(OnTimedRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), status));
OnTimedRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), status);
}
else
{
ChipLogProgress(InteractionModel, "Msg type %d not supported", aPayloadHeader.GetMessageType());
}

exit:
if (status != Protocols::InteractionModel::Status::Success && !apExchangeContext->IsGroupExchangeContext())
{
err = StatusResponse::Send(status, apExchangeContext, false /*aExpectResponse*/);
return StatusResponse::Send(status, apExchangeContext, false /*aExpectResponse*/);
}
return err;

return CHIP_NO_ERROR;
}

void InteractionModelEngine::OnResponseTimeout(Messaging::ExchangeContext * ec)
Expand Down
11 changes: 5 additions & 6 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,13 +377,12 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,

/**
* Called when Interaction Model receives a Read Request message. Errors processing
* the Read 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.
* the Read Request are handled entirely within this function. If the
* status returned is not Status::Success, the caller will send a status
* response message with that status.
*/

CHIP_ERROR OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, ReadHandler::InteractionType aInteractionType,
Protocols::InteractionModel::Status & aStatus);
Status OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, ReadHandler::InteractionType aInteractionType);

/**
* Called when Interaction Model receives a Write Request message. Errors processing
Expand Down

0 comments on commit 2412401

Please sign in to comment.