Address review comments

Author: yufengwangca

examples/fabric-admin/commands/common/CHIPCommand.cpp

@@ -182,7 +182,7 @@ CHIP_ERROR CHIPCommand::MaybeSetUpStack()
     mCredIssuerCmds->SetCredentialIssuerOption(CredentialIssuerCommands::CredentialIssuerOptions::kAllowTestCdSigningKey,
                                                allowTestCdSigningKey);
 
-    ReturnLogErrorOnFailure(PairingManager::Instance().Init(&CurrentCommissioner()));
+    PairingManager::Instance().Init(&CurrentCommissioner());
 
     return CHIP_NO_ERROR;
 }

examples/fabric-admin/commands/fabric-sync/FabricSyncCommand.cpp

@@ -344,7 +344,7 @@ CHIP_ERROR FabricSyncDeviceCommand::RunCommand(EndpointId remoteId)
         return CHIP_NO_ERROR;
     }
 
-    PairingManager::Instance().RegisterOpenCommissioningWindowDelegate(this);
+    PairingManager::Instance().SetOpenCommissioningWindowDelegate(this);
 
     DeviceMgr().OpenRemoteDeviceCommissioningWindow(remoteId);
 

examples/fabric-admin/device_manager/DeviceManager.cpp

@@ -134,7 +134,6 @@ void DeviceManager::OpenRemoteDeviceCommissioningWindow(EndpointId remoteEndpoin
     // Open the commissioning window of a device from another fabric via its fabric bridge.
     // This method constructs and sends a command to open the commissioning window for a device
     // that is part of a different fabric, accessed through a fabric bridge.
-    StringBuilder<kMaxCommandSize> commandBuilder;
 
     // Use random discriminator to have less chance of collision.
     uint16_t discriminator =

examples/fabric-admin/device_manager/PairingManager.cpp

@@ -24,26 +24,35 @@
 
 using namespace ::chip;
 
-Platform::UniquePtr<chip::Controller::CommissioningWindowOpener> PairingManager::mWindowOpener;
-
 PairingManager::PairingManager() :
     mOnOpenCommissioningWindowCallback(OnOpenCommissioningWindowResponse, this),
     mOnOpenCommissioningWindowVerifierCallback(OnOpenCommissioningWindowVerifierResponse, this)
 {}
 
-CHIP_ERROR PairingManager::Init(chip::Controller::DeviceCommissioner * commissioner)
+void PairingManager::Init(Controller::DeviceCommissioner * commissioner)
 {
+    VerifyOrDie(mCommissioner == nullptr);
     mCommissioner = commissioner;
-    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR PairingManager::OpenCommissioningWindow(NodeId nodeId, EndpointId endpointId, uint16_t commissioningTimeout,
                                                    uint32_t iterations, uint16_t discriminator, const ByteSpan & salt,
                                                    const ByteSpan & verifier)
 {
-    VerifyOrReturnError(mCommissioner != nullptr, CHIP_ERROR_INCORRECT_STATE);
+    if (mCommissioner == nullptr)
+    {
+        ChipLogError(NotSpecified, "Commissioner is null, cannot open commissioning window");
+        return CHIP_ERROR_INCORRECT_STATE;
+    }
+
+    // Check if a window is already open
+    if (mWindowOpener != nullptr)
+    {
+        ChipLogError(NotSpecified, "A commissioning window is already open");
+        return CHIP_ERROR_INCORRECT_STATE;
+    }
 
-    auto * params                      = new CommissioningWindowParams();
+    auto params                        = Platform::MakeUnique<CommissioningWindowParams>();
     params->nodeId                     = nodeId;
     params->endpointId                 = endpointId;
     params->commissioningWindowTimeout = commissioningTimeout;
@@ -52,96 +61,110 @@ CHIP_ERROR PairingManager::OpenCommissioningWindow(NodeId nodeId, EndpointId end
 
     if (!salt.empty())
     {
+        if (salt.size() > Crypto::kSpake2p_Max_PBKDF_Salt_Length)
+        {
+            ChipLogError(NotSpecified, "Salt size exceeds buffer capacity");
+            return CHIP_ERROR_BUFFER_TOO_SMALL;
+        }
+
         memcpy(params->saltBuffer, salt.data(), salt.size());
         params->salt = ByteSpan(params->saltBuffer, salt.size());
     }
 
     if (!verifier.empty())
     {
+        if (verifier.size() > Crypto::kSpake2p_VerifierSerialized_Length)
+        {
+            ChipLogError(NotSpecified, "Verifier size exceeds buffer capacity");
+            return CHIP_ERROR_BUFFER_TOO_SMALL;
+        }
+
         memcpy(params->verifierBuffer, verifier.data(), verifier.size());
         params->verifier = ByteSpan(params->verifierBuffer, verifier.size());
     }
 
     // Schedule work on the Matter thread
-    chip::DeviceLayer::PlatformMgr().ScheduleWork(OnOpenCommissioningWindow, reinterpret_cast<intptr_t>(params));
-
-    return CHIP_NO_ERROR;
+    return DeviceLayer::PlatformMgr().ScheduleWork(OnOpenCommissioningWindow, reinterpret_cast<intptr_t>(params.release()));
 }
 
 void PairingManager::OnOpenCommissioningWindow(intptr_t context)
 {
-    auto * params         = reinterpret_cast<CommissioningWindowParams *>(context);
+    Platform::UniquePtr<CommissioningWindowParams> params(reinterpret_cast<CommissioningWindowParams *>(context));
     PairingManager & self = PairingManager::Instance();
 
     if (self.mCommissioner == nullptr)
     {
-        ChipLogError(AppServer, "Commissioner is null, cannot open commissioning window");
+        ChipLogError(NotSpecified, "Commissioner is null, cannot open commissioning window");
         return;
     }
 
-    mWindowOpener = Platform::MakeUnique<Controller::CommissioningWindowOpener>(self.mCommissioner);
+    self.mWindowOpener = Platform::MakeUnique<Controller::CommissioningWindowOpener>(self.mCommissioner);
 
     if (!params->verifier.empty())
     {
         if (params->salt.empty())
         {
-            ChipLogError(AppServer, "Salt is required when verifier is set");
+            ChipLogError(NotSpecified, "Salt is required when verifier is set");
+            self.mWindowOpener.reset();
             return;
         }
 
-        CHIP_ERROR err = mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowVerifierParams()
-                                                                    .SetNodeId(params->nodeId)
-                                                                    .SetEndpointId(params->endpointId)
-                                                                    .SetTimeout(params->commissioningWindowTimeout)
-                                                                    .SetIteration(params->iteration)
-                                                                    .SetDiscriminator(params->discriminator)
-                                                                    .SetVerifier(params->verifier)
-                                                                    .SetSalt(params->salt)
-                                                                    .SetCallback(&self.mOnOpenCommissioningWindowVerifierCallback));
+        CHIP_ERROR err =
+            self.mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowVerifierParams()
+                                                            .SetNodeId(params->nodeId)
+                                                            .SetEndpointId(params->endpointId)
+                                                            .SetTimeout(params->commissioningWindowTimeout)
+                                                            .SetIteration(params->iteration)
+                                                            .SetDiscriminator(params->discriminator)
+                                                            .SetVerifier(params->verifier)
+                                                            .SetSalt(params->salt)
+                                                            .SetCallback(&self.mOnOpenCommissioningWindowVerifierCallback));
         if (err != CHIP_NO_ERROR)
         {
-            ChipLogError(AppServer, "Failed to open commissioning window with verifier: %s", ErrorStr(err));
+            ChipLogError(NotSpecified, "Failed to open commissioning window with verifier: %s", ErrorStr(err));
+            self.mWindowOpener.reset();
         }
     }
     else
     {
         SetupPayload ignored;
-        CHIP_ERROR err = mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowPasscodeParams()
-                                                                    .SetNodeId(params->nodeId)
-                                                                    .SetEndpointId(params->endpointId)
-                                                                    .SetTimeout(params->commissioningWindowTimeout)
-                                                                    .SetIteration(params->iteration)
-                                                                    .SetDiscriminator(params->discriminator)
-                                                                    .SetSetupPIN(NullOptional)
-                                                                    .SetSalt(NullOptional)
-                                                                    .SetCallback(&self.mOnOpenCommissioningWindowCallback),
-                                                                ignored);
+        CHIP_ERROR err = self.mWindowOpener->OpenCommissioningWindow(Controller::CommissioningWindowPasscodeParams()
+                                                                         .SetNodeId(params->nodeId)
+                                                                         .SetEndpointId(params->endpointId)
+                                                                         .SetTimeout(params->commissioningWindowTimeout)
+                                                                         .SetIteration(params->iteration)
+                                                                         .SetDiscriminator(params->discriminator)
+                                                                         .SetSetupPIN(NullOptional)
+                                                                         .SetSalt(NullOptional)
+                                                                         .SetCallback(&self.mOnOpenCommissioningWindowCallback),
+                                                                     ignored);
         if (err != CHIP_NO_ERROR)
         {
-            ChipLogError(AppServer, "Failed to open commissioning window with passcode: %s", ErrorStr(err));
+            ChipLogError(NotSpecified, "Failed to open commissioning window with passcode: %s", ErrorStr(err));
+            self.mWindowOpener.reset();
         }
     }
-
-    // Clean up params
-    delete params;
 }
 
-void PairingManager::OnOpenCommissioningWindowResponse(void * context, NodeId remoteId, CHIP_ERROR err, chip::SetupPayload payload)
+void PairingManager::OnOpenCommissioningWindowResponse(void * context, NodeId remoteId, CHIP_ERROR err, SetupPayload payload)
 {
+    VerifyOrDie(context != nullptr);
     PairingManager * self = static_cast<PairingManager *>(context);
     if (self->mCommissioningWindowDelegate)
     {
         self->mCommissioningWindowDelegate->OnCommissioningWindowOpened(remoteId, err, payload);
-        self->UnregisterOpenCommissioningWindowDelegate();
+        self->SetOpenCommissioningWindowDelegate(nullptr);
     }
 
     OnOpenCommissioningWindowVerifierResponse(context, remoteId, err);
 }
 
 void PairingManager::OnOpenCommissioningWindowVerifierResponse(void * context, NodeId remoteId, CHIP_ERROR err)
 {
+    VerifyOrDie(context != nullptr);
+    PairingManager * self = static_cast<PairingManager *>(context);
     LogErrorOnFailure(err);
 
-    PairingManager * self = reinterpret_cast<PairingManager *>(context);
-    VerifyOrReturn(self != nullptr, ChipLogError(NotSpecified, "OnOpenCommissioningWindowCommand: context is null"));
+    // Reset the window opener once the window operation is complete
+    self->mWindowOpener.reset();
 }

examples/fabric-admin/device_manager/PairingManager.h

@@ -29,6 +29,26 @@ class CommissioningWindowDelegate
     virtual ~CommissioningWindowDelegate()                                                                      = default;
 };
 
+/**
+ * The PairingManager class is responsible for managing the commissioning and pairing process
+ * of Matter devices. PairingManager is designed to be used as a singleton, meaning that there
+ * should only be one instance of it running at any given time.
+ *
+ * Usage:
+ *
+ * 1. The class should be initialized when the system starts up, typically by invoking the static
+ *    instance method to get the singleton.
+ * 2. To open a commissioning window, the appropriate method should be called on the PairingManager instance.
+ * 3. The PairingManager will handle the lifecycle of the CommissioningWindowOpener and ensure that
+ *    resources are cleaned up appropriately when pairing is complete or the process is aborted.
+ *
+ * Example:
+ *
+ * @code
+ * PairingManager& manager = PairingManager::Instance();
+ * manager.OpenCommissioningWindow();
+ * @endcode
+ */
 class PairingManager
 {
 public:
@@ -38,19 +58,28 @@ class PairingManager
         return instance;
     }
 
-    CHIP_ERROR Init(chip::Controller::DeviceCommissioner * commissioner);
+    void Init(chip::Controller::DeviceCommissioner * commissioner);
 
+    /**
+     * Opens a commissioning window on the specified node and endpoint.
+     * Only one commissioning window can be active at a time. If a commissioning
+     * window is already open, this function will return an error.
+     *
+     * @param nodeId The target node ID for commissioning.
+     * @param endpointId The target endpoint ID for commissioning.
+     * @param commissioningTimeout Timeout for the commissioning window in seconds.
+     * @param iterations Iterations for PBKDF calculations.
+     * @param discriminator Discriminator for commissioning.
+     * @param salt Optional salt for verifier-based commissioning.
+     * @param verifier Optional verifier for enhanced commissioning security.
+     *
+     * @return CHIP_ERROR_INCORRECT_STATE if a commissioning window is already open.
+     */
     CHIP_ERROR OpenCommissioningWindow(chip::NodeId nodeId, chip::EndpointId endpointId, uint16_t commissioningTimeout,
                                        uint32_t iterations, uint16_t discriminator, const chip::ByteSpan & salt,
                                        const chip::ByteSpan & verifier);
 
-    void RegisterOpenCommissioningWindowDelegate(CommissioningWindowDelegate * delegate)
-    {
-        ChipLogProgress(NotSpecified, "yujuan: PairingManager::RegisterOpenCommissioningWindowDelegate");
-        mCommissioningWindowDelegate = delegate;
-    }
-
-    void UnregisterOpenCommissioningWindowDelegate() { mCommissioningWindowDelegate = nullptr; }
+    void SetOpenCommissioningWindowDelegate(CommissioningWindowDelegate * delegate) { mCommissioningWindowDelegate = delegate; }
 
 private:
     PairingManager();
@@ -76,7 +105,13 @@ class PairingManager
 
     CommissioningWindowDelegate * mCommissioningWindowDelegate = nullptr;
 
-    static chip::Platform::UniquePtr<chip::Controller::CommissioningWindowOpener> mWindowOpener;
+    /**
+     * Holds the unique_ptr to the current CommissioningWindowOpener.
+     * Only one commissioning window opener can be active at a time.
+     * The pointer is reset when the commissioning window is closed or when an error occurs.
+     */
+    chip::Platform::UniquePtr<chip::Controller::CommissioningWindowOpener> mWindowOpener;
+
     static void OnOpenCommissioningWindow(intptr_t context);
     static void OnOpenCommissioningWindowResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status,
                                                   chip::SetupPayload payload);