Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@yunhanw-google
yunhanw-google committed Jul 21, 2022
1 parent 4dd9914 commit 91a9782
Show file tree
Hide file tree
Showing 20 changed files with 226 additions and 256 deletions.
36 changes: 13 additions & 23 deletions src/app/CommandHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,8 @@ CHIP_ERROR CommandHandler::AllocateBuffer()
return CHIP_NO_ERROR;
}

Status CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload,
bool isTimedInvoke)
Status CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload, bool isTimedInvoke)
{
Status status = Status::Success;
System::PacketBufferHandle response;
Expand Down Expand Up @@ -97,7 +95,7 @@ Status CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * apExc
if (status != Status::Success)
{
CHIP_ERROR err = StatusResponse::Send(status, mpExchangeCtx,
/* aExpectResponse = */ false);
/* aExpectResponse = */ false);

if (err != CHIP_NO_ERROR)
{
Expand All @@ -110,7 +108,7 @@ Status CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * apExc
// SendCommandResponse() later (when our holdoff count drops to 0) it
// just fails and we don't double-respond.
mpExchangeCtx = nullptr;
status = Status::Success;
status = Status::Success;
}
return status;
}
Expand All @@ -125,16 +123,12 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa
reader.Init(std::move(payload));
VerifyOrReturnError(invokeRequestMessage.Init(reader) == CHIP_NO_ERROR, Status::InvalidAction);
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
VerifyOrReturnError(invokeRequestMessage.CheckSchemaValidity() == CHIP_NO_ERROR,
Status::InvalidAction);
VerifyOrReturnError(invokeRequestMessage.CheckSchemaValidity() == CHIP_NO_ERROR, Status::InvalidAction);
#endif

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(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);

Expand All @@ -148,16 +142,13 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa

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

if (mpExchangeCtx->IsGroupExchangeContext())
{
VerifyOrReturnError(ProcessGroupCommandDataIB(commandData) == CHIP_NO_ERROR,
Status::Failure);
VerifyOrReturnError(ProcessGroupCommandDataIB(commandData) == CHIP_NO_ERROR, Status::Failure);
}
else
{
Expand All @@ -178,15 +169,14 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa
CHIP_ERROR CommandHandler::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload)
{
ChipLogDetail(DataManagement, "Unexpected message type %d", aPayloadHeader.GetMessageType());
ChipLogDetail(DataManagement, "CommandHandler: Unexpected message type %d", aPayloadHeader.GetMessageType());
return OnUnknownMsgType();
}

CHIP_ERROR CommandHandler::OnUnknownMsgType()
{
VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
CHIP_ERROR err =
StatusResponse::Send(Status::InvalidAction, mpExchangeCtx, false /*aExpectResponse*/);
CHIP_ERROR err = StatusResponse::Send(Status::InvalidAction, mpExchangeCtx, false /*aExpectResponse*/);
if (err != CHIP_NO_ERROR)
{
// We have to manually close the exchange, because we called
Expand Down
25 changes: 11 additions & 14 deletions src/app/CommandSender.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,47 +155,44 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha
{
CHIP_ERROR statusError = CHIP_NO_ERROR;
SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
suppressErrorStatusResponse = true;
SuccessOrExit(err = statusError);
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}
else
{
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}

exit:
return ResponseMessageHandled(err, apExchangeContext, suppressErrorStatusResponse);
ResponseMessageHandled(err, apExchangeContext, suppressErrorStatusResponse);
return err;
}

CHIP_ERROR CommandSender::ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
bool aSuppressErrorStatusResponse)
void CommandSender::ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
bool aSuppressErrorStatusResponse)
{
CHIP_ERROR err = aError;
if (aError != CHIP_NO_ERROR)
{
if (!aSuppressErrorStatusResponse)
{
err = StatusResponse::Send(Protocols::InteractionModel::Status::InvalidAction, apExchangeContext,
false /*aExpectResponse*/);
CHIP_ERROR err = StatusResponse::Send(Protocols::InteractionModel::Status::InvalidAction, apExchangeContext,
false /*aExpectResponse*/);
if (err == CHIP_NO_ERROR)
{
mpExchangeCtx = nullptr;
}
}

if (mpCallback != nullptr)
{
mpCallback->OnError(this, aError);
if (mpCallback != nullptr)
{
mpCallback->OnError(this, aError);
}
}
}

if (mState != State::CommandSent)
{
Close();
}
// Else we got a response to a Timed Request and just sent the invoke.

return err;
}

CHIP_ERROR CommandSender::ProcessInvokeResponse(System::PacketBufferHandle && payload)
Expand Down
3 changes: 2 additions & 1 deletion src/app/CommandSender.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ class CommandSender final : public Messaging::ExchangeDelegate
CHIP_ERROR SendInvokeRequest();

CHIP_ERROR Finalize(System::PacketBufferHandle & commandPacket);
CHIP_ERROR ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext, bool aSuppressErrorStatusResponse);
void ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
bool aSuppressErrorStatusResponse);

Messaging::ExchangeContext * mpExchangeCtx = nullptr;
Callback * mpCallback = nullptr;
Expand Down
27 changes: 13 additions & 14 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,8 @@ void InteractionModelEngine::OnDone(ReadHandler & apReadObj)
}

Status InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload,
bool aIsTimedInvoke)
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
bool aIsTimedInvoke)
{
CommandHandler * commandHandler = mCommandHandlerObjs.CreateObject(this);
if (commandHandler == nullptr)
Expand All @@ -280,9 +279,8 @@ Status InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext
}

Status InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload,
ReadHandler::InteractionType aInteractionType)
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload,
ReadHandler::InteractionType aInteractionType)
{
ChipLogDetail(InteractionModel, "Received %s request",
aInteractionType == ReadHandler::InteractionType::Subscribe ? "Subscribe" : "Read");
Expand Down Expand Up @@ -452,10 +450,8 @@ Status InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext *
return StatusIB(err).mStatus;
}

Status InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload,
bool aIsTimedWrite)
Status InteractionModelEngine::OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, bool aIsTimedWrite)
{
ChipLogDetail(InteractionModel, "Received Write request");

Expand Down Expand Up @@ -854,9 +850,8 @@ bool InteractionModelEngine::TrimFabricForRead(FabricIndex aFabricIndex)
return false;
}

Status InteractionModelEngine::EnsureResourceForRead(FabricIndex aFabricIndex,
size_t aRequestedAttributePathCount,
size_t aRequestedEventPathCount)
Status InteractionModelEngine::EnsureResourceForRead(FabricIndex aFabricIndex, size_t aRequestedAttributePathCount,
size_t aRequestedEventPathCount)
{
#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP && !CHIP_CONFIG_IM_FORCE_FABRIC_QUOTA_CHECK
#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
Expand Down Expand Up @@ -1332,7 +1327,11 @@ void InteractionModelEngine::OnTimedInvoke(TimedHandler * apTimedHandler, Messag
VerifyOrDie(aPayloadHeader.HasMessageType(MsgType::InvokeCommandRequest));
VerifyOrDie(!apExchangeContext->IsGroupExchangeContext());

OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ true);
Status status = OnInvokeCommandRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), /* aIsTimedInvoke = */ true);
if (status != Status::Success)
{
StatusResponse::Send(status, apExchangeContext, /* aExpectResponse = */ false);
}
}

void InteractionModelEngine::OnTimedWrite(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
Expand Down
7 changes: 3 additions & 4 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,11 +347,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,

/**
* Called when Interaction Model receives a Command Request message. Errors processing
* the Command Request are handled entirely within this function.
* If this returns a failure status the caller should send a status response with that status.
*/
Status OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload, bool aIsTimedInvoke);
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
30 changes: 10 additions & 20 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,7 @@ CHIP_ERROR ReadClient::GenerateDataVersionFilterList(DataVersionFilterIBs::Build
CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload)
{
CHIP_ERROR err = CHIP_NO_ERROR;
bool suppressErrorStatusResponse = false;
CHIP_ERROR err = CHIP_NO_ERROR;
VerifyOrExit(!IsIdle(), err = CHIP_ERROR_INCORRECT_STATE);

if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData))
Expand All @@ -405,41 +404,34 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange

CHIP_ERROR statusError = CHIP_NO_ERROR;
SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
suppressErrorStatusResponse = true;
SuccessOrExit(err = statusError);
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}
else
{
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}

exit:
return ResponseMessageHandled(err, apExchangeContext, suppressErrorStatusResponse);
ResponseMessageHandled(err, apExchangeContext);
return err;
}

CHIP_ERROR ReadClient::ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext,
bool aSuppressErrorStatusResponse)
void ReadClient::ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext)
{
CHIP_ERROR err = aError;
if (aError != CHIP_NO_ERROR)
{
if (!aSuppressErrorStatusResponse)
CHIP_ERROR err = StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
if (err == CHIP_NO_ERROR)
{
err = StatusResponse::Send(Status::InvalidAction, apExchangeContext,
false /*aExpectResponse*/);
if (err == CHIP_NO_ERROR)
{
mpExchangeCtx = nullptr;
}
mpExchangeCtx = nullptr;
}
}

if ((!IsSubscriptionType() && !mPendingMoreChunks) || err != CHIP_NO_ERROR)
if ((!IsSubscriptionType() && !mPendingMoreChunks) || aError != CHIP_NO_ERROR)
{
Close(aError);
}

return err;
}

CHIP_ERROR ReadClient::OnUnsolicitedReportData(Messaging::ExchangeContext * apExchangeContext,
Expand Down Expand Up @@ -567,9 +559,7 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
if (!suppressResponse)
{
bool noResponseExpected = IsSubscriptionActive() && !mPendingMoreChunks;
err = StatusResponse::Send(err == CHIP_NO_ERROR ? Status::Success
: Status::Failure,
mExchange.Get(),, !noResponseExpected);
err = StatusResponse::Send(err == CHIP_NO_ERROR ? Status::Success : Status::Failure, mExchange.Get(), !noResponseExpected);
}

mIsPrimingReports = false;
Expand Down
2 changes: 1 addition & 1 deletion src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ class ReadClient : public Messaging::ExchangeDelegate
CHIP_ERROR ProcessAttributePath(AttributePathIB::Parser & aAttributePath, ConcreteDataAttributePath & aClusterInfo);
CHIP_ERROR ProcessReportData(System::PacketBufferHandle && aPayload);
const char * GetStateStr() const;
CHIP_ERROR ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext, bool aSuppressErrorStatusResponse);
void ResponseMessageHandled(CHIP_ERROR aError, Messaging::ExchangeContext * apExchangeContext);
/*
* Checks if we should re-subscribe based on the specified re-subscription policy. If we should, re-subscription is scheduled
* aNextResubscribeIntervalMsec is updated accordingly, and true is returned.
Expand Down
7 changes: 3 additions & 4 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,9 @@ CHIP_ERROR ReadHandler::OnInitialRequest(System::PacketBufferHandle && aPayload)

CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload)
{
CHIP_ERROR err = CHIP_NO_ERROR;
CHIP_ERROR err = CHIP_NO_ERROR;
bool suppressErrorStatusResponse = false;
CHIP_ERROR statusError = CHIP_NO_ERROR;
CHIP_ERROR statusError = CHIP_NO_ERROR;
SuccessOrExit(err = StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError));
suppressErrorStatusResponse = true;
SuccessOrExit(err = statusError);
Expand Down Expand Up @@ -169,8 +169,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
{
if (!suppressErrorStatusResponse)
{
err = StatusResponse::Send(Status::InvalidAction, apExchangeContext,
false /*aExpectResponse*/);
err = StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
if (err == CHIP_NO_ERROR)
{
mpExchangeCtx = nullptr;
Expand Down
1 change: 1 addition & 0 deletions src/app/StatusResponse.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ namespace app {
CHIP_ERROR StatusResponse::Send(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext,
bool aExpectResponse)
{
ChipLogProgress(InteractionModel, "send status response");
VerifyOrReturnError(apExchangeContext != nullptr, CHIP_ERROR_INCORRECT_STATE);
System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
VerifyOrReturnError(!msgBuf.IsNull(), CHIP_ERROR_NO_MEMORY);
Expand Down
3 changes: 3 additions & 0 deletions src/app/StatusResponse.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class StatusResponse
public:
static CHIP_ERROR Send(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext,
bool aExpectResponse);

// The return value indicates whether the StatusResponse was parsed properly, and if it is CHIP_NO_ERROR
// then aStatus has been set to the actual status, which might be success or failure.
static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload, CHIP_ERROR & aStatus);
};
} // namespace app
Expand Down
Loading

0 comments on commit 91a9782

Please sign in to comment.