diff --git a/src/lib/dnssd/ActiveResolveAttempts.cpp b/src/lib/dnssd/ActiveResolveAttempts.cpp index 2065dca3440d53..fe0cc16ebb26aa 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.cpp +++ b/src/lib/dnssd/ActiveResolveAttempts.cpp @@ -55,11 +55,11 @@ void ActiveResolveAttempts::Complete(const PeerId & peerId) #endif } -void ActiveResolveAttempts::Complete(const chip::Dnssd::DiscoveredNodeData & data) +void ActiveResolveAttempts::CompleteCommissioner(const chip::Dnssd::DiscoveredNodeData & data) { for (auto & item : mRetryQueue) { - if (item.attempt.Matches(data)) + if (item.attempt.Matches(data, chip::Dnssd::DiscoveryType::kCommissionerNode)) { item.attempt.Clear(); return; @@ -67,6 +67,36 @@ void ActiveResolveAttempts::Complete(const chip::Dnssd::DiscoveredNodeData & dat } } +void ActiveResolveAttempts::CompleteCommissionable(const chip::Dnssd::DiscoveredNodeData & data) +{ + for (auto & item : mRetryQueue) + { + if (item.attempt.Matches(data, chip::Dnssd::DiscoveryType::kCommissionableNode)) + { + item.attempt.Clear(); + return; + } + } +} + +bool ActiveResolveAttempts::HasBrowseFor(chip::Dnssd::DiscoveryType type) const +{ + for (auto & item : mRetryQueue) + { + if (!item.attempt.IsBrowse()) + { + continue; + } + + if (item.attempt.BrowseData().type == type) + { + return true; + } + } + + return false; +} + void ActiveResolveAttempts::CompleteIpResolution(SerializedQNameIterator targetHostName) { for (auto & item : mRetryQueue) diff --git a/src/lib/dnssd/ActiveResolveAttempts.h b/src/lib/dnssd/ActiveResolveAttempts.h index 4a4789364d3dff..15796abf85b20c 100644 --- a/src/lib/dnssd/ActiveResolveAttempts.h +++ b/src/lib/dnssd/ActiveResolveAttempts.h @@ -140,7 +140,7 @@ class ActiveResolveAttempts { return resolveData.Is() && (resolveData.Get().peerId == peer); } - bool Matches(const chip::Dnssd::DiscoveredNodeData & data) const + bool Matches(const chip::Dnssd::DiscoveredNodeData & data, chip::Dnssd::DiscoveryType type) const { if (!resolveData.Is()) { @@ -149,13 +149,11 @@ class ActiveResolveAttempts auto & browse = resolveData.Get(); - // TODO: we should mark returned node data based on the query - if (browse.type != chip::Dnssd::DiscoveryType::kCommissionableNode) + if (browse.type != type) { - // We don't currently have markers in the returned DiscoveredNodeData to differentiate these, so assume all returned - // packets match - return true; + return false; } + switch (browse.filter.type) { case chip::Dnssd::DiscoveryFilterType::kNone: @@ -251,7 +249,8 @@ class ActiveResolveAttempts /// Mark a resolution as a success, removing it from the internal list void Complete(const chip::PeerId & peerId); - void Complete(const chip::Dnssd::DiscoveredNodeData & data); + void CompleteCommissioner(const chip::Dnssd::DiscoveredNodeData & data); + void CompleteCommissionable(const chip::Dnssd::DiscoveredNodeData & data); void CompleteIpResolution(SerializedQNameIterator targetHostName); /// Mark all browse-type scheduled attemptes as a success, removing them @@ -289,6 +288,9 @@ class ActiveResolveAttempts /// IP resolution. bool IsWaitingForIpResolutionFor(SerializedQNameIterator hostName) const; + /// Check if a browse operation is active for the given discovery type + bool HasBrowseFor(chip::Dnssd::DiscoveryType type) const; + private: struct RetryEntry { diff --git a/src/lib/dnssd/IncrementalResolve.cpp b/src/lib/dnssd/IncrementalResolve.cpp index 69d475238b40ff..c36efbde177a6d 100644 --- a/src/lib/dnssd/IncrementalResolve.cpp +++ b/src/lib/dnssd/IncrementalResolve.cpp @@ -54,20 +54,12 @@ class TxtParser : public mdns::Minimal::TxtRecordDelegate DataType & mData; }; -enum class ServiceNameType -{ - kInvalid, // not a matter service name - kOperational, - kCommissioner, - kCommissionable, -}; - // Common prefix to check for all operational/commissioner/commissionable name parts constexpr QNamePart kOperationalSuffix[] = { kOperationalServiceName, kOperationalProtocol, kLocalDomain }; constexpr QNamePart kCommissionableSuffix[] = { kCommissionableServiceName, kCommissionProtocol, kLocalDomain }; constexpr QNamePart kCommissionerSuffix[] = { kCommissionerServiceName, kCommissionProtocol, kLocalDomain }; -ServiceNameType ComputeServiceNameType(SerializedQNameIterator name) +IncrementalResolver::ServiceNameType ComputeServiceNameType(SerializedQNameIterator name) { // SRV record names look like: // -._matter._tcp.local (operational) @@ -78,25 +70,25 @@ ServiceNameType ComputeServiceNameType(SerializedQNameIterator name) if (!name.Next() || !name.IsValid()) { // missing required components - empty service name - return ServiceNameType::kInvalid; + return IncrementalResolver::ServiceNameType::kInvalid; } if (name == kOperationalSuffix) { - return ServiceNameType::kOperational; + return IncrementalResolver::ServiceNameType::kOperational; } if (name == kCommissionableSuffix) { - return ServiceNameType::kCommissionable; + return IncrementalResolver::ServiceNameType::kCommissionable; } if (name == kCommissionerSuffix) { - return ServiceNameType::kCommissioner; + return IncrementalResolver::ServiceNameType::kCommissioner; } - return ServiceNameType::kInvalid; + return IncrementalResolver::ServiceNameType::kInvalid; } /// Automatically resets a IncrementalResolver to inactive in its destructor @@ -171,7 +163,9 @@ CHIP_ERROR IncrementalResolver::InitializeParsing(mdns::Minimal::SerializedQName Platform::CopyString(mCommonResolutionData.hostName, serverName.Value()); } - switch (ComputeServiceNameType(name)) + mServiceNameType = ComputeServiceNameType(name); + + switch (mServiceNameType) { case ServiceNameType::kOperational: mSpecificResolutionData.Set(); diff --git a/src/lib/dnssd/IncrementalResolve.h b/src/lib/dnssd/IncrementalResolve.h index eff4f04fd28b72..b28b3cb897b5f3 100644 --- a/src/lib/dnssd/IncrementalResolve.h +++ b/src/lib/dnssd/IncrementalResolve.h @@ -84,7 +84,16 @@ class IncrementalResolver }; using RequiredInformationFlags = BitFlags; - IncrementalResolver() {} + // Type of service name that is contained in this resolver + enum class ServiceNameType + { + kInvalid, // not a matter service name + kOperational, + kCommissioner, + kCommissionable, + }; + + IncrementalResolver() = default; /// Checks if object has been initialized using the `InitializeParsing` /// method. @@ -93,6 +102,8 @@ class IncrementalResolver bool IsActiveCommissionParse() const { return mSpecificResolutionData.Is(); } bool IsActiveOperationalParse() const { return mSpecificResolutionData.Is(); } + ServiceNameType GetCurrentType() const { return mServiceNameType; } + /// Start parsing a new record. SRV records are the records we are mainly /// interested on, after which TXT and A/AAAA are looked for. /// @@ -171,6 +182,7 @@ class IncrementalResolver StoredServerName mRecordName; // Record name for what is parsed (SRV/PTR/TXT) StoredServerName mTargetHostName; // `Target` for the SRV record + ServiceNameType mServiceNameType = ServiceNameType::kInvalid; CommonResolutionData mCommonResolutionData; ParsedRecordSpecificData mSpecificResolutionData; }; diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 5c09413f7a7246..36ead8c9e5cb75 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -380,18 +380,45 @@ void MinMdnsResolver::AdvancePendingResolverStates() if (err != CHIP_NO_ERROR) { ChipLogError(Discovery, "Failed to take discovery result: %" CHIP_ERROR_FORMAT, err.Format()); + continue; } - mActiveResolves.Complete(nodeData); - if (mCommissioningDelegate != nullptr) + // TODO: Ideally commissioning delegates should be aware of the + // node types they receive, however they are currently not + // so try to help out by only calling the delegate when an + // active browse exists. + // + // This is NOT ok and probably we should have separate comissioner + // or commissionable delegates or pass in a node type argument. + bool discoveredNodeIsRelevant = false; + + switch (resolver->GetCurrentType()) { - mCommissioningDelegate->OnNodeDiscovered(nodeData); + case IncrementalResolver::ServiceNameType::kCommissioner: + discoveredNodeIsRelevant = mActiveResolves.HasBrowseFor(chip::Dnssd::DiscoveryType::kCommissionerNode); + mActiveResolves.CompleteCommissioner(nodeData); + break; + case IncrementalResolver::ServiceNameType::kCommissionable: + discoveredNodeIsRelevant = mActiveResolves.HasBrowseFor(chip::Dnssd::DiscoveryType::kCommissionableNode); + mActiveResolves.CompleteCommissionable(nodeData); + break; + default: + ChipLogError(Discovery, "Unexpected type for commission data parsing"); + continue; } - else + + if (discoveredNodeIsRelevant) { + if (mCommissioningDelegate != nullptr) + { + mCommissioningDelegate->OnNodeDiscovered(nodeData); + } + else + { #if CHIP_MINMDNS_HIGH_VERBOSITY - ChipLogError(Discovery, "No delegate to report commissioning node discovery"); + ChipLogError(Discovery, "No delegate to report commissioning node discovery"); #endif + } } } else if (resolver->IsActiveOperationalParse()) diff --git a/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp b/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp index 097aa11fec1d05..574268bb3c8e55 100644 --- a/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp +++ b/src/lib/dnssd/tests/TestActiveResolveAttempts.cpp @@ -127,7 +127,7 @@ void TestSingleBrowseAddRemove(nlTestSuite * inSuite, void * inContext) // once complete, nothing to schedule Dnssd::DiscoveredNodeData data; data.commissionData.longDiscriminator = 1234; - attempts.Complete(data); + attempts.CompleteCommissionable(data); NL_TEST_ASSERT(inSuite, !attempts.GetTimeUntilNextExpectedResponse().HasValue()); NL_TEST_ASSERT(inSuite, !attempts.NextScheduled().HasValue()); } @@ -376,7 +376,7 @@ void TestCombination(nlTestSuite * inSuite, void * inContext) attempts.Complete(MakePeerId(1)); Dnssd::DiscoveredNodeData data; data.commissionData.longDiscriminator = 1234; - attempts.Complete(data); + attempts.CompleteCommissionable(data); NL_TEST_ASSERT(inSuite, !attempts.GetTimeUntilNextExpectedResponse().HasValue()); NL_TEST_ASSERT(inSuite, !attempts.NextScheduled().HasValue());