Review feedback

project-chip · May 3, 2021 · 4c95e74 · 4c95e74
1 parent 47e07e6
commit 4c95e74
Show file tree

Hide file tree

Showing 7 changed files with 25 additions and 65 deletions.
diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
@@ -69,7 +69,12 @@ EmberAfStatus writeFabricAttribute(uint8_t * buffer, int32_t index = -1)
     // at 1. In order to hide this to the rest of the code of this file, the element index is incremented by 1 here.
     // This also allows calling writeAttribute() with no index arg to mean "write the length".
 
-    return emAfReadOrWriteAttribute(&record, NULL, buffer, 0, true, index + 1);
+    return emAfReadOrWriteAttribute(&record, 
+                                    NULL, // metadata
+                                    buffer, 
+                                    0, // read length
+                                    true, // write ?
+                                    index + 1);
 }
 
 EmberAfStatus writeFabric(FabricId fabricId, NodeId nodeId, uint16_t vendorId, int32_t index)
@@ -89,7 +94,7 @@ EmberAfStatus writeFabric(FabricId fabricId, NodeId nodeId, uint16_t vendorId, i
     return status;
 }
 
-CHIP_ERROR writeAdminsIntoFabricsListAttribute(void)
+CHIP_ERROR writeAdminsIntoFabricsListAttribute()
 {
     emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Call to writeAdminsIntoFabricsListAttribute");
     CHIP_ERROR err = CHIP_NO_ERROR;
@@ -143,9 +148,9 @@ CHIP_ERROR writeAdminsIntoFabricsListAttribute(void)
 AdminPairingInfo * retrieveCurrentAdmin()
 {
     uint64_t fabricId = emberAfCurrentCommand()->source;
-    // TODO: Figure out how to get device node id so we can do FindAdmin(fabricId, nodeId)...
+    // TODO: Figure out how to get device node id so we can do FindAdminForNode(fabricId, nodeId)...
     emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: Finding admin with fabricId  %" PRIX64 ".", fabricId);
-    return GetGlobalAdminPairingTable().FindAdmin(fabricId);
+    return GetGlobalAdminPairingTable().FindAdminForNode(fabricId);
 }
 
 // TODO: The code currently has two sources of truths for admins, the pairing table + the attributes. There should only be one,
@@ -206,7 +211,7 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(chip::app::Command
     CHIP_ERROR err;
 
     // Fetch matching admin
-    admin = GetGlobalAdminPairingTable().FindAdmin(fabricId, nodeId, vendorId);
+    admin = GetGlobalAdminPairingTable().FindAdminForNode(fabricId, nodeId, vendorId);
     VerifyOrExit(admin != nullptr, status = EMBER_ZCL_STATUS_SUCCESS); // Admin has already been removed
 
     // Delete admin

diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp
@@ -282,7 +282,7 @@ class ServerRendezvousAdvertisementDelegate : public RendezvousAdvertisementDele
                 mDelegate->OnPairingWindowClosed();
         }
 
-        AdminPairingInfo * admin = gAdminPairings.FindAdmin(mAdmin);
+        AdminPairingInfo * admin = gAdminPairings.FindAdminWithId(mAdmin);
         if (admin != nullptr)
         {
             ReturnErrorOnFailure(PersistAdminPairingToKVS(admin, gNextAvailableAdminId));

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -686,7 +686,7 @@ CHIP_ERROR DeviceCommissioner::PairDevice(NodeId remoteDeviceId, RendezvousParam
     Device * device                    = nullptr;
     Transport::PeerAddress peerAddress = Transport::PeerAddress::UDP(Inet::IPAddress::Any);
 
-    Transport::AdminPairingInfo * admin = mAdmins.FindAdmin(mAdminId);
+    Transport::AdminPairingInfo * admin = mAdmins.FindAdminWithId(mAdminId);
 
     VerifyOrExit(remoteDeviceId != kAnyNodeId && remoteDeviceId != kUndefinedNodeId, err = CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);

diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h
@@ -137,7 +137,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
     {
         if (mExchangeACL == nullptr)
         {
-            Transport::AdminPairingInfo * admin = table.FindAdmin(mSecureSession.GetAdminId());
+            Transport::AdminPairingInfo * admin = table.FindAdminWithId(mSecureSession.GetAdminId());
             if (admin != nullptr)
             {
                 mExchangeACL = chip::Platform::New<CASEExchangeACL>(admin);

diff --git a/src/transport/AdminPairingTable.cpp b/src/transport/AdminPairingTable.cpp
@@ -26,7 +26,7 @@
 
 namespace chip {
 
-// TODO: Admin Pairing table should be backed by a single backing store (attribute store), remove delegate callbacks.
+// TODO: Admin Pairing table should be backed by a single backing store (attribute store), remove delegate callbacks #6419
 namespace {
 PersistentStorageDelegate * gStorage             = nullptr;
 Transport::AdminPairingTableDelegate * gDelegate = nullptr;
@@ -164,14 +164,14 @@ AdminPairingInfo * AdminPairingTable::AssignAdminId(AdminId adminId, NodeId node
 
 void AdminPairingTable::ReleaseAdminId(AdminId adminId)
 {
-    AdminPairingInfo * admin = FindAdmin(adminId);
+    AdminPairingInfo * admin = FindAdminWithId(adminId);
     if (admin != nullptr)
     {
         admin->Reset();
     }
 }
 
-AdminPairingInfo * AdminPairingTable::FindAdmin(AdminId adminId)
+AdminPairingInfo * AdminPairingTable::FindAdminWithId(AdminId adminId)
 {
     for (auto & state : mStates)
     {
@@ -184,50 +184,7 @@ AdminPairingInfo * AdminPairingTable::FindAdmin(AdminId adminId)
     return nullptr;
 }
 
-AdminPairingInfo * AdminPairingTable::FindAdmin(FabricId fabricId)
-{
-    uint32_t index = 0;
-    for (auto & state : mStates)
-    {
-        if (state.IsInitialized())
-        {
-            ChipLogProgress(Discovery, "Looking at index %d with fabricID %llu nodeID %llu to see if it matches fabricId %llu.",
-                            index, state.GetFabricId(), state.GetNodeId(), fabricId);
-        }
-        if (state.IsInitialized() && state.GetFabricId() == fabricId)
-        {
-            ChipLogProgress(Discovery, "Found a match!");
-            return &state;
-        }
-        index++;
-    }
-
-    return nullptr;
-}
-
-AdminPairingInfo * AdminPairingTable::FindAdmin(FabricId fabricId, NodeId nodeId)
-{
-    uint32_t index = 0;
-    for (auto & state : mStates)
-    {
-        if (state.IsInitialized())
-        {
-            ChipLogProgress(Discovery,
-                            "Looking at index %d with fabricID %llu nodeID %llu to see if it matches fabricId %llu nodeId %llu.",
-                            index, state.GetFabricId(), state.GetNodeId(), fabricId, nodeId);
-        }
-        if (state.IsInitialized() && state.GetFabricId() == fabricId && state.GetNodeId() == nodeId)
-        {
-            ChipLogProgress(Discovery, "Found a match!");
-            return &state;
-        }
-        index++;
-    }
-
-    return nullptr;
-}
-
-AdminPairingInfo * AdminPairingTable::FindAdmin(FabricId fabricId, NodeId nodeId, uint16_t vendorId)
+AdminPairingInfo * AdminPairingTable::FindAdminForNode(FabricId fabricId, NodeId nodeId, uint16_t vendorId)
 {
     uint32_t index = 0;
     for (auto & state : mStates)
@@ -239,8 +196,10 @@ AdminPairingInfo * AdminPairingTable::FindAdmin(FabricId fabricId, NodeId nodeId
                             "nodeId %llu vendorId %d.",
                             index, state.GetFabricId(), state.GetNodeId(), state.GetVendorId(), fabricId, nodeId, vendorId);
         }
-        if (state.IsInitialized() && state.GetFabricId() == fabricId && state.GetNodeId() == nodeId &&
-            state.GetVendorId() == vendorId)
+        if (state.IsInitialized() && 
+            state.GetFabricId() == fabricId && 
+            (nodeId == kUndefinedNodeId || state.GetNodeId() == nodeId) &&
+            (vendorId == kUndefinedVendorId || state.GetVendorId() == vendorId))
         {
             ChipLogProgress(Discovery, "Found a match!");
             return &state;

diff --git a/src/transport/AdminPairingTable.h b/src/transport/AdminPairingTable.h
@@ -226,13 +226,9 @@ class DLL_EXPORT AdminPairingTable
 
     void ReleaseAdminId(AdminId adminId);
 
-    AdminPairingInfo * FindAdmin(AdminId adminId);
+    AdminPairingInfo * FindAdminWithId(AdminId adminId);
 
-    AdminPairingInfo * FindAdmin(FabricId fabricId);
-
-    AdminPairingInfo * FindAdmin(FabricId fabricId, NodeId nodeId);
-
-    AdminPairingInfo * FindAdmin(FabricId fabricId, NodeId nodeId, uint16_t vendorId);
+    AdminPairingInfo * FindAdminForNode(FabricId fabricId, NodeId nodeId = kUndefinedNodeId, uint16_t vendorId = kUndefinedVendorId);
 
     void Reset();
 

diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp
@@ -171,7 +171,7 @@ CHIP_ERROR SecureSessionMgr::SendMessage(SecureSessionHandle session, PayloadHea
 
     // This marks any connection where we send data to as 'active'
     mPeerConnections.MarkConnectionActive(state);
-    admin = mAdmins->FindAdmin(state->GetAdminId());
+    admin = mAdmins->FindAdminWithId(state->GetAdminId());
     VerifyOrExit(admin != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
     localNodeId = admin->GetNodeId();
 
@@ -389,7 +389,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader,
         ExitNow(err = CHIP_ERROR_KEY_NOT_FOUND_FROM_PEER);
     }
 
-    admin = mAdmins->FindAdmin(state->GetAdminId());
+    admin = mAdmins->FindAdminWithId(state->GetAdminId());
     VerifyOrExit(admin != nullptr,
                  ChipLogError(Inet, "Secure transport received packet for unknown admin (%p, %d) pairing, discarding", state,
                               state->GetAdminId()));