From f0ce64bac5042742796c10f141d9fce39392f5a3 Mon Sep 17 00:00:00 2001 From: Joonhaeng Heo Date: Thu, 23 May 2024 15:58:43 +0900 Subject: [PATCH 1/8] Add Compare IPAddress in discoveredNode --- .../AbstractDnssdDiscoveryController.cpp | 29 ++++- .../TestCommissionableNodeController.cpp | 101 ++++++++++++++++++ 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 3d3ca4c8097955..1d2af319181b20 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -25,6 +25,31 @@ namespace chip { namespace Controller { +bool compareIpAddresses(const size_t sourceNumIPs, const size_t destinationNumIPs, const Inet::IPAddress *source, const Inet::IPAddress *destination) +{ + size_t sameIpAddress = 0; + bool addressUsed[chip::Dnssd::CommonResolutionData::kMaxIPAddresses] = {false}; + if (sourceNumIPs != destinationNumIPs) + { + return false; + } + + for (size_t s = 0 ; s < sourceNumIPs ; s++) + { + for (size_t d = 0 ; d < destinationNumIPs ; d++) + { + if (!addressUsed[d] && source[s] == destination[d]) + { + // Change the user flag so that the compared target is no longer used + addressUsed[d] = true; + sameIpAddress++; + break; + } + } + } + return sameIpAddress == destinationNumIPs; +} + void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & discNodeData) { VerifyOrReturn(discNodeData.Is()); @@ -38,10 +63,8 @@ void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::Disco { continue; } - // TODO(#32576) Check if IP address are the same. Must account for `numIPs` in the list of `ipAddress`. - // Additionally, must NOT assume that the ordering is consistent. if (strcmp(discoveredNode.hostName, nodeData.hostName) == 0 && discoveredNode.port == nodeData.port && - discoveredNode.numIPs == nodeData.numIPs) + compareIpAddresses(discoveredNode.numIPs, nodeData.numIPs, discoveredNode.ipAddress, nodeData.ipAddress)) { discoveredNode = nodeData; if (mDeviceDiscoveryDelegate != nullptr) diff --git a/src/controller/tests/TestCommissionableNodeController.cpp b/src/controller/tests/TestCommissionableNodeController.cpp index b68d0748d62c42..6d9ef65db11e97 100644 --- a/src/controller/tests/TestCommissionableNodeController.cpp +++ b/src/controller/tests/TestCommissionableNodeController.cpp @@ -182,4 +182,105 @@ TEST_F(TestCommissionableNodeController, TestDiscoverCommissioners_DiscoverCommi EXPECT_NE(controller.DiscoverCommissioners(), CHIP_NO_ERROR); } +TEST_F(TestCommissionableNodeController, TestGetDiscoveredCommissioner_MultipleIPAddressDiscover) +{ + MockResolver resolver; + CommissionableNodeController controller(&resolver); + + // example 1 + chip::Dnssd::DiscoveredNodeData discNodeData1; + discNodeData1.Set(); + chip::Dnssd::CommissionNodeData & inNodeData1 = discNodeData1.Get(); + Platform::CopyString(inNodeData1.hostName, "mockHostName"); + Inet::IPAddress::FromString("fd11:1111:1122:2222:1111:2222:3333:4444", inNodeData1.ipAddress[0]); + inNodeData1.numIPs++; + Inet::IPAddress::FromString("fd11:1111:1122:2222:2222:3333:4444:5555", inNodeData1.ipAddress[1]); + inNodeData1.numIPs++; + inNodeData1.port = 5540; + + controller.OnNodeDiscovered(discNodeData1); + + // example 5 - exactly same as example 1 + chip::Dnssd::DiscoveredNodeData discNodeData5; + discNodeData5.Set(); + chip::Dnssd::CommissionNodeData & inNodeData5 = discNodeData5.Get(); + Platform::CopyString(inNodeData1.hostName, "mockHostName"); + Inet::IPAddress::FromString("fd11:1111:1122:2222:1111:2222:3333:4444", inNodeData5.ipAddress[0]); + inNodeData5.numIPs++; + Inet::IPAddress::FromString("fd11:1111:1122:2222:2222:3333:4444:5555", inNodeData5.ipAddress[1]); + inNodeData5.numIPs++; + inNodeData5.port = 5540; + + controller.OnNodeDiscovered(discNodeData5); + + // example 2 - same as example 1 (IPAdress sequence is only different.) + chip::Dnssd::DiscoveredNodeData discNodeData2; + discNodeData2.Set(); + chip::Dnssd::CommissionNodeData & inNodeData2 = discNodeData2.Get(); + Platform::CopyString(inNodeData2.hostName, "mockHostName"); + Inet::IPAddress::FromString("fd11:1111:1122:2222:2222:3333:4444:5555", inNodeData2.ipAddress[0]); + inNodeData2.numIPs++; + Inet::IPAddress::FromString("fd11:1111:1122:2222:1111:2222:3333:4444", inNodeData2.ipAddress[1]); + inNodeData2.numIPs++; + inNodeData2.port = 5540; + + controller.OnNodeDiscovered(discNodeData2); + + // example 3 - different example + chip::Dnssd::DiscoveredNodeData discNodeData3; + discNodeData3.Set(); + chip::Dnssd::CommissionNodeData & inNodeData3 = discNodeData3.Get(); + Platform::CopyString(inNodeData3.hostName, "mockHostName"); + Inet::IPAddress::FromString("fd11:1111:1122:2222:1111:2222:3333:4444", inNodeData3.ipAddress[0]); + inNodeData3.numIPs++; + Inet::IPAddress::FromString("fd11:1111:1122:2222:2222:3333:4444:6666", inNodeData3.ipAddress[1]); + inNodeData3.numIPs++; + inNodeData3.port = 5540; + + controller.OnNodeDiscovered(discNodeData3); + + // example 4 - different example (Different IP count) + chip::Dnssd::DiscoveredNodeData discNodeData4; + discNodeData4.Set(); + chip::Dnssd::CommissionNodeData & inNodeData4 = discNodeData4.Get(); + Platform::CopyString(inNodeData4.hostName, "mockHostName"); + Inet::IPAddress::FromString("fd11:1111:1122:2222:1111:2222:3333:4444", inNodeData4.ipAddress[0]); + inNodeData4.numIPs++; + Inet::IPAddress::FromString("fd11:1111:1122:2222:2222:3333:4444:6666", inNodeData4.ipAddress[1]); + inNodeData4.numIPs++; + Inet::IPAddress::FromString("fd11:1111:1122:2222:2222:3333:4444:7777", inNodeData4.ipAddress[2]); + inNodeData4.numIPs++; + inNodeData4.port = 5540; + + controller.OnNodeDiscovered(discNodeData4); + + // Example 2 result - example 1 is removed. (reason : duplicate) + ASSERT_NE(controller.GetDiscoveredCommissioner(0), nullptr); + EXPECT_STREQ(inNodeData1.hostName, controller.GetDiscoveredCommissioner(0)->hostName); + EXPECT_EQ(inNodeData2.ipAddress[0], controller.GetDiscoveredCommissioner(0)->ipAddress[0]); + EXPECT_EQ(inNodeData2.ipAddress[1], controller.GetDiscoveredCommissioner(0)->ipAddress[1]); + EXPECT_EQ(controller.GetDiscoveredCommissioner(0)->port, 5540); + EXPECT_EQ(controller.GetDiscoveredCommissioner(0)->numIPs, 2u); + + // Example 3 result + ASSERT_NE(controller.GetDiscoveredCommissioner(1), nullptr); + EXPECT_STREQ(inNodeData3.hostName, controller.GetDiscoveredCommissioner(1)->hostName); + EXPECT_EQ(inNodeData3.ipAddress[0], controller.GetDiscoveredCommissioner(1)->ipAddress[0]); + EXPECT_EQ(inNodeData3.ipAddress[1], controller.GetDiscoveredCommissioner(1)->ipAddress[1]); + EXPECT_EQ(controller.GetDiscoveredCommissioner(1)->port, 5540); + EXPECT_EQ(controller.GetDiscoveredCommissioner(1)->numIPs, 2u); + + // Example 4 result + ASSERT_NE(controller.GetDiscoveredCommissioner(2), nullptr); + EXPECT_STREQ(inNodeData4.hostName, controller.GetDiscoveredCommissioner(2)->hostName); + EXPECT_EQ(inNodeData4.ipAddress[0], controller.GetDiscoveredCommissioner(2)->ipAddress[0]); + EXPECT_EQ(inNodeData4.ipAddress[1], controller.GetDiscoveredCommissioner(2)->ipAddress[1]); + EXPECT_EQ(inNodeData4.ipAddress[2], controller.GetDiscoveredCommissioner(2)->ipAddress[2]); + EXPECT_EQ(controller.GetDiscoveredCommissioner(2)->port, 5540); + EXPECT_EQ(controller.GetDiscoveredCommissioner(2)->numIPs, 3u); + + // Total is 3. (Not 4) + ASSERT_EQ(controller.GetDiscoveredCommissioner(3), nullptr); +} + } // namespace From 19e0a8c2f4eb4a74535cf673a39f668de2f2db1d Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 24 May 2024 01:18:52 +0000 Subject: [PATCH 2/8] Restyled by clang-format --- src/controller/AbstractDnssdDiscoveryController.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 1d2af319181b20..a644f175321bb0 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -25,18 +25,19 @@ namespace chip { namespace Controller { -bool compareIpAddresses(const size_t sourceNumIPs, const size_t destinationNumIPs, const Inet::IPAddress *source, const Inet::IPAddress *destination) +bool compareIpAddresses(const size_t sourceNumIPs, const size_t destinationNumIPs, const Inet::IPAddress * source, + const Inet::IPAddress * destination) { - size_t sameIpAddress = 0; - bool addressUsed[chip::Dnssd::CommonResolutionData::kMaxIPAddresses] = {false}; + size_t sameIpAddress = 0; + bool addressUsed[chip::Dnssd::CommonResolutionData::kMaxIPAddresses] = { false }; if (sourceNumIPs != destinationNumIPs) { return false; } - for (size_t s = 0 ; s < sourceNumIPs ; s++) + for (size_t s = 0; s < sourceNumIPs; s++) { - for (size_t d = 0 ; d < destinationNumIPs ; d++) + for (size_t d = 0; d < destinationNumIPs; d++) { if (!addressUsed[d] && source[s] == destination[d]) { From b5c1fb2ecf170163776dbd43a517eca55f75950e Mon Sep 17 00:00:00 2001 From: Joonhaeng Heo Date: Fri, 24 May 2024 11:32:29 +0900 Subject: [PATCH 3/8] Fix function name, pointer -> array --- src/controller/AbstractDnssdDiscoveryController.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index a644f175321bb0..09ce8bf9013fef 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -25,8 +25,7 @@ namespace chip { namespace Controller { -bool compareIpAddresses(const size_t sourceNumIPs, const size_t destinationNumIPs, const Inet::IPAddress * source, - const Inet::IPAddress * destination) +bool AddressListsSameExceptOrder(const size_t sourceNumIPs, const size_t destinationNumIPs, const Inet::IPAddress source[Dnssd::CommissionNodeData::kMaxIPAddresses], const Inet::IPAddress destination[Dnssd::CommissionNodeData::kMaxIPAddresses]) { size_t sameIpAddress = 0; bool addressUsed[chip::Dnssd::CommonResolutionData::kMaxIPAddresses] = { false }; @@ -65,7 +64,7 @@ void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::Disco continue; } if (strcmp(discoveredNode.hostName, nodeData.hostName) == 0 && discoveredNode.port == nodeData.port && - compareIpAddresses(discoveredNode.numIPs, nodeData.numIPs, discoveredNode.ipAddress, nodeData.ipAddress)) + AddressListsSameExceptOrder(discoveredNode.numIPs, nodeData.numIPs, discoveredNode.ipAddress, nodeData.ipAddress)) { discoveredNode = nodeData; if (mDeviceDiscoveryDelegate != nullptr) From 64948d4d77896eaa9d54345d16242b8b54d7f0d6 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 24 May 2024 02:32:49 +0000 Subject: [PATCH 4/8] Restyled by clang-format --- src/controller/AbstractDnssdDiscoveryController.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 09ce8bf9013fef..44f59b68f6e6a2 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -25,7 +25,9 @@ namespace chip { namespace Controller { -bool AddressListsSameExceptOrder(const size_t sourceNumIPs, const size_t destinationNumIPs, const Inet::IPAddress source[Dnssd::CommissionNodeData::kMaxIPAddresses], const Inet::IPAddress destination[Dnssd::CommissionNodeData::kMaxIPAddresses]) +bool AddressListsSameExceptOrder(const size_t sourceNumIPs, const size_t destinationNumIPs, + const Inet::IPAddress source[Dnssd::CommissionNodeData::kMaxIPAddresses], + const Inet::IPAddress destination[Dnssd::CommissionNodeData::kMaxIPAddresses]) { size_t sameIpAddress = 0; bool addressUsed[chip::Dnssd::CommonResolutionData::kMaxIPAddresses] = { false }; From 1ef74e4a5bbb4da9ffe7998b770140098c2c5d38 Mon Sep 17 00:00:00 2001 From: Joonhaeng Heo Date: Mon, 27 May 2024 08:59:11 +0900 Subject: [PATCH 5/8] Modify from comment --- .../AbstractDnssdDiscoveryController.cpp | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 44f59b68f6e6a2..6d89ff00491ce9 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -22,34 +22,32 @@ #include #include +#include + namespace chip { namespace Controller { -bool AddressListsSameExceptOrder(const size_t sourceNumIPs, const size_t destinationNumIPs, - const Inet::IPAddress source[Dnssd::CommissionNodeData::kMaxIPAddresses], - const Inet::IPAddress destination[Dnssd::CommissionNodeData::kMaxIPAddresses]) +static bool SameExceptOrder(const chip::Span &source, const chip::Span &destination) { - size_t sameIpAddress = 0; - bool addressUsed[chip::Dnssd::CommonResolutionData::kMaxIPAddresses] = { false }; - if (sourceNumIPs != destinationNumIPs) + std::bitset addressUsed; + if (source.size() != destination.size()) { return false; } - for (size_t s = 0; s < sourceNumIPs; s++) + for (size_t s = 0; s < source.size(); s++) { - for (size_t d = 0; d < destinationNumIPs; d++) + for (size_t d = 0; d < destination.size(); d++) { if (!addressUsed[d] && source[s] == destination[d]) { // Change the user flag so that the compared target is no longer used - addressUsed[d] = true; - sameIpAddress++; + addressUsed.set(d, true); break; } } } - return sameIpAddress == destinationNumIPs; + return addressUsed.count() == destination.size(); } void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & discNodeData) @@ -65,8 +63,10 @@ void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::Disco { continue; } + chip::Span discoveredNodeIPAddressSpan(&discoveredNode.ipAddress[0], discoveredNode.numIPs); + chip::Span nodeDataIPAddressSpan(&nodeData.ipAddress[0], nodeData.numIPs); if (strcmp(discoveredNode.hostName, nodeData.hostName) == 0 && discoveredNode.port == nodeData.port && - AddressListsSameExceptOrder(discoveredNode.numIPs, nodeData.numIPs, discoveredNode.ipAddress, nodeData.ipAddress)) + SameExceptOrder(discoveredNodeIPAddressSpan, nodeDataIPAddressSpan)) { discoveredNode = nodeData; if (mDeviceDiscoveryDelegate != nullptr) From 54cc111fb54f2d7ffe2dc43b26bb90644ef4a6b7 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Sun, 26 May 2024 23:59:48 +0000 Subject: [PATCH 6/8] Restyled by clang-format --- src/controller/AbstractDnssdDiscoveryController.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 6d89ff00491ce9..9c624b88b9926e 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -27,7 +27,7 @@ namespace chip { namespace Controller { -static bool SameExceptOrder(const chip::Span &source, const chip::Span &destination) +static bool SameExceptOrder(const chip::Span & source, const chip::Span & destination) { std::bitset addressUsed; if (source.size() != destination.size()) From 70a563fc3a1bbb7059531e1782b43a36d76aa825 Mon Sep 17 00:00:00 2001 From: Joonhaeng Heo Date: Tue, 28 May 2024 09:23:59 +0900 Subject: [PATCH 7/8] Update from comments --- .../AbstractDnssdDiscoveryController.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 9c624b88b9926e..7c49e7f55ca814 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -27,19 +27,21 @@ namespace chip { namespace Controller { -static bool SameExceptOrder(const chip::Span & source, const chip::Span & destination) +static bool SameExceptOrder(const chip::Span & v1, const chip::Span & v2) { std::bitset addressUsed; - if (source.size() != destination.size()) + + VerifyOrDie(v1.size() <= Dnssd::CommissionNodeData::kMaxIPAddresses && v2.size() <= Dnssd::CommissionNodeData::kMaxIPAddresses); + if (v1.size() != v2.size()) { return false; } - for (size_t s = 0; s < source.size(); s++) + for (size_t s = 0; s < v1.size(); s++) { - for (size_t d = 0; d < destination.size(); d++) + for (size_t d = 0; d < v2.size(); d++) { - if (!addressUsed[d] && source[s] == destination[d]) + if (!addressUsed[d] && v1[s] == v2[d]) { // Change the user flag so that the compared target is no longer used addressUsed.set(d, true); @@ -47,7 +49,7 @@ static bool SameExceptOrder(const chip::Span & source, co } } } - return addressUsed.count() == destination.size(); + return addressUsed.count() == v2.size(); } void AbstractDnssdDiscoveryController::OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & discNodeData) From cc71cbbaf51a57b86b9a6fb2aa1011b95fbac05d Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Tue, 4 Jun 2024 10:33:08 -0700 Subject: [PATCH 8/8] Update src/controller/AbstractDnssdDiscoveryController.cpp Co-authored-by: Andrei Litvin --- src/controller/AbstractDnssdDiscoveryController.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/AbstractDnssdDiscoveryController.cpp b/src/controller/AbstractDnssdDiscoveryController.cpp index 7c49e7f55ca814..9d59e2dfdbca03 100644 --- a/src/controller/AbstractDnssdDiscoveryController.cpp +++ b/src/controller/AbstractDnssdDiscoveryController.cpp @@ -43,7 +43,7 @@ static bool SameExceptOrder(const chip::Span & v1, const { if (!addressUsed[d] && v1[s] == v2[d]) { - // Change the user flag so that the compared target is no longer used + // Change the used flag so that the compared target is no longer used addressUsed.set(d, true); break; }