Cleanup error handling in commissioner (#14282)

* Consolidate commissioning report sending * Fix session cleanups for errors. * Fix weird error variable that didn't get removed. * Update src/controller/AutoCommissioner.cpp * Line got lost in the merge
project-chip · Jun 22, 2023 · 50da333 · 50da333
1 parent 455d928
commit 50da333
Show file tree

Hide file tree

Showing 5 changed files with 115 additions and 93 deletions.
diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -249,56 +249,62 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Failed to perform commissioning step %d", static_cast<int>(report.stageCompleted));
-        return err;
     }
-    switch (report.stageCompleted)
+    else
     {
-    case CommissioningStage::kSendPAICertificateRequest:
-        SetPAI(report.Get<RequestedCertificate>().certificate);
-        break;
-    case CommissioningStage::kSendDACCertificateRequest:
-        SetDAC(report.Get<RequestedCertificate>().certificate);
-        break;
-    case CommissioningStage::kSendAttestationRequest:
-        // These don't need to be deep copied to local memory because they are used in this one step then never again.
-        mParams.SetAttestationElements(report.Get<AttestationResponse>().attestationElements)
-            .SetAttestationSignature(report.Get<AttestationResponse>().signature);
-        // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we woouldn't need to hold a
-        // reference to the operational credential delegate here
-        if (mOperationalCredentialsDelegate != nullptr)
+        switch (report.stageCompleted)
         {
-            MutableByteSpan nonce(mCSRNonce);
-            ReturnErrorOnFailure(mOperationalCredentialsDelegate->ObtainCsrNonce(nonce));
-            mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce)));
+        case CommissioningStage::kSendPAICertificateRequest:
+            SetPAI(report.Get<RequestedCertificate>().certificate);
+            break;
+        case CommissioningStage::kSendDACCertificateRequest:
+            SetDAC(report.Get<RequestedCertificate>().certificate);
+            break;
+        case CommissioningStage::kSendAttestationRequest:
+            // These don't need to be deep copied to local memory because they are used in this one step then never again.
+            mParams.SetAttestationElements(report.Get<AttestationResponse>().attestationElements)
+                .SetAttestationSignature(report.Get<AttestationResponse>().signature);
+            // TODO: Does this need to be done at runtime? Seems like this could be done earlier and we wouldn't need to hold a
+            // reference to the operational credential delegate here
+            if (mOperationalCredentialsDelegate != nullptr)
+            {
+                MutableByteSpan nonce(mCSRNonce);
+                ReturnErrorOnFailure(mOperationalCredentialsDelegate->ObtainCsrNonce(nonce));
+                mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce)));
+            }
+            break;
+        case CommissioningStage::kSendOpCertSigningRequest: {
+            NOCChainGenerationParameters nocParams;
+            nocParams.nocsrElements = report.Get<AttestationResponse>().attestationElements;
+            nocParams.signature     = report.Get<AttestationResponse>().signature;
+            mParams.SetNOCChainGenerationParameters(nocParams);
         }
         break;
-    case CommissioningStage::kSendOpCertSigningRequest: {
-        NOCChainGenerationParameters nocParams;
-        nocParams.nocsrElements = report.Get<AttestationResponse>().attestationElements;
-        nocParams.signature     = report.Get<AttestationResponse>().signature;
-        mParams.SetNOCChainGenerationParameters(nocParams);
-    }
-    break;
-    case CommissioningStage::kGenerateNOCChain:
-        // For NOC chain generation, we re-use the buffers. NOCChainGenerated triggers the next stage before
-        // storing the returned certs, so just return here without triggering the next stage.
-        return NOCChainGenerated(report.Get<NocChain>().noc, report.Get<NocChain>().icac, report.Get<NocChain>().rcac,
-                                 report.Get<NocChain>().ipk, report.Get<NocChain>().adminSubject);
-    case CommissioningStage::kFindOperational:
-        mOperationalDeviceProxy = report.Get<OperationalNodeFoundData>().operationalProxy;
-        break;
-    case CommissioningStage::kCleanup:
-        ReleasePAI();
-        ReleaseDAC();
-        mCommissioneeDeviceProxy = nullptr;
-        mOperationalDeviceProxy  = nullptr;
-        mParams                  = CommissioningParameters();
-        return CHIP_NO_ERROR;
-    default:
-        break;
+        case CommissioningStage::kGenerateNOCChain:
+            // For NOC chain generation, we re-use the buffers. NOCChainGenerated triggers the next stage before
+            // storing the returned certs, so just return here without triggering the next stage.
+            return NOCChainGenerated(report.Get<NocChain>().noc, report.Get<NocChain>().icac, report.Get<NocChain>().rcac,
+                                     report.Get<NocChain>().ipk, report.Get<NocChain>().adminSubject);
+        case CommissioningStage::kFindOperational:
+            mOperationalDeviceProxy = report.Get<OperationalNodeFoundData>().operationalProxy;
+            break;
+        case CommissioningStage::kCleanup:
+            ReleasePAI();
+            ReleaseDAC();
+            mCommissioneeDeviceProxy = nullptr;
+            mOperationalDeviceProxy  = nullptr;
+            mParams                  = CommissioningParameters();
+            return CHIP_NO_ERROR;
+        default:
+            break;
+        }
     }
 
     CommissioningStage nextStage = GetNextCommissioningStage(report.stageCompleted, err);
+    if (nextStage == CommissioningStage::kError)
+    {
+        return CHIP_ERROR_INCORRECT_STATE;
+    }
 
     DeviceProxy * proxy = mCommissioneeDeviceProxy;
     if (nextStage == CommissioningStage::kSendComplete ||

diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h
@@ -58,7 +58,6 @@ class AutoCommissioner : public CommissioningDelegate
     OperationalCredentialsDelegate * mOperationalCredentialsDelegate = nullptr;
     CommissioningParameters mParams                                  = CommissioningParameters();
     // Memory space for the commisisoning parameters that come in as ByteSpans - the caller is not guaranteed to retain this memory
-    // TODO(cecille): Include memory from CommissioneeDeviceProxy once BLE is moved over
     uint8_t mSsid[CommissioningParameters::kMaxSsidLen];
     uint8_t mCredentials[CommissioningParameters::kMaxCredentialsLen];
     uint8_t mThreadOperationalDataset[CommissioningParameters::kMaxThreadDatasetLen];