From 3a7cbf8f524590773db32aa155c5d6563a09a7e9 Mon Sep 17 00:00:00 2001 From: Shubham Patil Date: Thu, 19 May 2022 00:39:13 +0530 Subject: [PATCH] [OTA] Handle invalid designator in OTA-P and reset OTA-R state on receiving BDX error (#18425) * [OTA-P] Abort the transfer if file designator is invalid [OTA-R] Reset ota-requestor state if BDX sends an error * Addressed review comments * Added comment explaining why it is okay to call Reset() * Replace if block with VerifyOrReturn --- .../ota-provider-common/BdxOtaSender.cpp | 49 +++++++++++++++---- .../clusters/ota-requestor/BDXDownloader.cpp | 22 ++++++--- .../clusters/ota-requestor/BDXDownloader.h | 1 + 3 files changed, 55 insertions(+), 17 deletions(-) diff --git a/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp b/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp index 1af2ea3dd690d2..f838b4bce98670 100644 --- a/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp +++ b/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp @@ -82,13 +82,24 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev // end of the transfer. sendFlags.Set(chip::Messaging::SendMessageFlags::kExpectResponse); } - VerifyOrReturn(mExchangeCtx != nullptr, ChipLogError(BDX, "%s: mExchangeCtx is null", __FUNCTION__)); + VerifyOrReturn(mExchangeCtx != nullptr); err = mExchangeCtx->SendMessage(event.msgTypeData.ProtocolId, event.msgTypeData.MessageType, std::move(event.MsgData), sendFlags); - if (err != CHIP_NO_ERROR) + + if (err == CHIP_NO_ERROR) + { + if (!sendFlags.Has(chip::Messaging::SendMessageFlags::kExpectResponse)) + { + // After sending the StatusReport, exchange context gets closed so, set mExchangeCtx to null + mExchangeCtx = nullptr; + } + } + else { - ChipLogError(BDX, "SendMessage failed: %s", chip::ErrorStr(err)); + ChipLogError(BDX, "SendMessage failed: %" CHIP_ERROR_FORMAT, err.Format()); + Reset(); } + break; } case TransferSession::OutputEventType::kInitReceived: { @@ -101,7 +112,7 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev acceptData.StartOffset = mTransfer.GetStartOffset(); acceptData.Length = mTransfer.GetTransferLength(); VerifyOrReturn(mTransfer.AcceptTransfer(acceptData) == CHIP_NO_ERROR, - ChipLogError(BDX, "%s: %s", __FUNCTION__, chip::ErrorStr(err))); + ChipLogError(BDX, "AcceptTransfer failed: %" CHIP_ERROR_FORMAT, err.Format())); // Store the file designator used during block query uint16_t fdl = 0; @@ -134,10 +145,21 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev } std::ifstream otaFile(mFileDesignator, std::ifstream::in); - VerifyOrReturn(otaFile.good(), ChipLogError(BDX, "%s: file read failed", __FUNCTION__)); + if (!otaFile.good()) + { + ChipLogError(BDX, "OTA file open failed"); + mTransfer.AbortTransfer(StatusCode::kFileDesignatorUnknown); + return; + } + otaFile.seekg(mNumBytesSent); otaFile.read(reinterpret_cast(blockBuf->Start()), bytesToRead); - VerifyOrReturn(otaFile.good() || otaFile.eof(), ChipLogError(BDX, "%s: file read failed", __FUNCTION__)); + if (!(otaFile.good() || otaFile.eof())) + { + ChipLogError(BDX, "OTA file read failed"); + mTransfer.AbortTransfer(StatusCode::kFileDesignatorUnknown); + return; + } blockData.Data = blockBuf->Start(); blockData.Length = static_cast(otaFile.gcount()); @@ -146,8 +168,12 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev mNumBytesSent = static_cast(mNumBytesSent + blockData.Length); otaFile.close(); - VerifyOrReturn(CHIP_NO_ERROR == mTransfer.PrepareBlock(blockData), - ChipLogError(BDX, "%s: PrepareBlock failed: %s", __FUNCTION__, chip::ErrorStr(err))); + err = mTransfer.PrepareBlock(blockData); + if (err != CHIP_NO_ERROR) + { + ChipLogError(BDX, "PrepareBlock failed: %" CHIP_ERROR_FORMAT, err.Format()); + mTransfer.AbortTransfer(StatusCode::kUnknown); + } break; } case TransferSession::OutputEventType::kAckReceived: @@ -172,10 +198,15 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev case TransferSession::OutputEventType::kBlockReceived: default: // TransferSession should prevent this case from happening. - ChipLogError(BDX, "%s: unsupported event type", __FUNCTION__); + ChipLogError(BDX, "Unsupported event type"); } } +/* Reset() calls bdx::TransferSession::Reset() which sets the output event type to + * TransferSession::OutputEventType::kNone. So, bdx::TransferFacilitator::PollForOutput() + * will call HandleTransferSessionOutput() with event TransferSession::OutputEventType::kNone. + * Since we are ignoring kNone events so, it is okay HandleTransferSessionOutput() being called with event kNone + */ void BdxOtaSender::Reset() { mFabricIndex.ClearValue(); diff --git a/src/app/clusters/ota-requestor/BDXDownloader.cpp b/src/app/clusters/ota-requestor/BDXDownloader.cpp index 8c1edbf8765040..8a00a382078bf7 100644 --- a/src/app/clusters/ota-requestor/BDXDownloader.cpp +++ b/src/app/clusters/ota-requestor/BDXDownloader.cpp @@ -226,6 +226,17 @@ void BDXDownloader::PollTransferSession() } while (outEvent.EventType != TransferSession::OutputEventType::kNone); } +void BDXDownloader::CleanupOnError(OTAChangeReasonEnum reason) +{ + Reset(); + mBdxTransfer.Reset(); + SetState(State::kIdle, reason); + if (mImageProcessor) + { + mImageProcessor->Abort(); + } +} + CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::OutputEvent & outEvent) { VerifyOrReturnError(mImageProcessor != nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -266,20 +277,15 @@ CHIP_ERROR BDXDownloader::HandleBdxEvent(const chip::bdx::TransferSession::Outpu } case TransferSession::OutputEventType::kStatusReceived: ChipLogError(BDX, "BDX StatusReport %x", static_cast(outEvent.statusData.statusCode)); - mBdxTransfer.Reset(); - ReturnErrorOnFailure(mImageProcessor->Abort()); + CleanupOnError(OTAChangeReasonEnum::kFailure); break; case TransferSession::OutputEventType::kInternalError: ChipLogError(BDX, "TransferSession error"); - Reset(); - mBdxTransfer.Reset(); - ReturnErrorOnFailure(mImageProcessor->Abort()); + CleanupOnError(OTAChangeReasonEnum::kFailure); break; case TransferSession::OutputEventType::kTransferTimeout: ChipLogError(BDX, "Transfer timed out"); - Reset(); - mBdxTransfer.Reset(); - ReturnErrorOnFailure(mImageProcessor->Abort()); + CleanupOnError(OTAChangeReasonEnum::kTimeOut); break; case TransferSession::OutputEventType::kInitReceived: case TransferSession::OutputEventType::kAckReceived: diff --git a/src/app/clusters/ota-requestor/BDXDownloader.h b/src/app/clusters/ota-requestor/BDXDownloader.h index a423f6cac9f728..121b59c027a704 100644 --- a/src/app/clusters/ota-requestor/BDXDownloader.h +++ b/src/app/clusters/ota-requestor/BDXDownloader.h @@ -82,6 +82,7 @@ class BDXDownloader : public chip::OTADownloader private: void PollTransferSession(); + void CleanupOnError(app::Clusters::OtaSoftwareUpdateRequestor::OTAChangeReasonEnum reason); CHIP_ERROR HandleBdxEvent(const chip::bdx::TransferSession::OutputEvent & outEvent); void SetState(State state, app::Clusters::OtaSoftwareUpdateRequestor::OTAChangeReasonEnum reason); void Reset();