[OTA] Handle invalid designator in OTA-P and reset OTA-R state on rec…

…eiving 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
project-chip · May 18, 2022 · 3a7cbf8 · 3a7cbf8
1 parent 7c6e1a2
commit 3a7cbf8
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 17 deletions.
diff --git 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<char *>(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<size_t>(otaFile.gcount());
@@ -146,8 +168,12 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev
         mNumBytesSent = static_cast<uint32_t>(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
@@ -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<uint16_t>(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
@@ -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();