From 35fb1781ccd19dbcea6f1eab622024e454152aef Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Fri, 22 Mar 2024 13:22:38 -0700 Subject: [PATCH] Revert "Fix the dnssd code that browses on both the local and srp domains" (#32691) This reverts commit 88afa3361bbb89b1999624016d79b4eb92245427. --- src/platform/Darwin/DnssdContexts.cpp | 105 ++++++++++------------ src/platform/Darwin/DnssdImpl.cpp | 121 ++++++++++++-------------- src/platform/Darwin/DnssdImpl.h | 20 +++-- 3 files changed, 110 insertions(+), 136 deletions(-) diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index a8ae75e34d2eb5..c6067a55168e12 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -458,35 +458,27 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip:: std::shared_ptr && consumerCounterToUse) : browseThatCausedResolve(browseCausingResolve) { - type = ContextType::Resolve; - context = cbContext; - callback = cb; - protocol = GetProtocol(cbAddressType); - instanceName = instanceNameToResolve; - consumerCounter = std::move(consumerCounterToUse); - hasSrpTimerStarted = false; + type = ContextType::Resolve; + context = cbContext; + callback = cb; + protocol = GetProtocol(cbAddressType); + instanceName = instanceNameToResolve; + consumerCounter = std::move(consumerCounterToUse); } ResolveContext::ResolveContext(CommissioningResolveDelegate * delegate, chip::Inet::IPAddressType cbAddressType, const char * instanceNameToResolve, std::shared_ptr && consumerCounterToUse) : browseThatCausedResolve(nullptr) { - type = ContextType::Resolve; - context = delegate; - callback = nullptr; - protocol = GetProtocol(cbAddressType); - instanceName = instanceNameToResolve; - consumerCounter = std::move(consumerCounterToUse); - hasSrpTimerStarted = false; + type = ContextType::Resolve; + context = delegate; + callback = nullptr; + protocol = GetProtocol(cbAddressType); + instanceName = instanceNameToResolve; + consumerCounter = std::move(consumerCounterToUse); } -ResolveContext::~ResolveContext() -{ - if (this->hasSrpTimerStarted) - { - CancelSrpTimer(this); - } -} +ResolveContext::~ResolveContext() {} void ResolveContext::DispatchFailure(const char * errorStr, CHIP_ERROR err) { @@ -534,7 +526,8 @@ void ResolveContext::DispatchSuccess() for (auto interfaceIndex : priorityInterfaceIndices) { - if (TryReportingResultsForInterfaceIndex(interfaceIndex)) + // Try finding interfaces for domains kLocalDot and kOpenThreadDot and delete them. + if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex), std::string(kLocalDot))) { if (needDelete) { @@ -543,7 +536,7 @@ void ResolveContext::DispatchSuccess() return; } - if (TryReportingResultsForInterfaceIndex(interfaceIndex)) + if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex), std::string(kOpenThreadDot))) { if (needDelete) { @@ -555,8 +548,7 @@ void ResolveContext::DispatchSuccess() for (auto & interface : interfaces) { - auto interfaceId = interface.first.first; - if (TryReportingResultsForInterfaceIndex(interfaceId)) + if (TryReportingResultsForInterfaceIndex(interface.first.first, interface.first.second)) { break; } @@ -568,7 +560,7 @@ void ResolveContext::DispatchSuccess() } } -bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex) +bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, std::string domainName) { if (interfaceIndex == 0) { @@ -576,44 +568,36 @@ bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceInde return false; } - std::map, InterfaceInfo>::iterator iter = interfaces.begin(); - while (iter != interfaces.end()) - { - std::pair key = iter->first; - if (key.first == interfaceIndex) - { - auto & interface = interfaces[key]; - auto & ips = interface.addresses; + std::pair interfaceKey = std::make_pair(interfaceIndex, domainName); + auto & interface = interfaces[interfaceKey]; + auto & ips = interface.addresses; - // Some interface may not have any ips, just ignore them. - if (ips.size() == 0) - { - return false; - } + // Some interface may not have any ips, just ignore them. + if (ips.size() == 0) + { + return false; + } - ChipLogProgress(Discovery, "Mdns: Resolve success on interface %" PRIu32, interfaceIndex); + ChipLogProgress(Discovery, "Mdns: Resolve success on interface %" PRIu32, interfaceIndex); - auto & service = interface.service; - auto addresses = Span(ips.data(), ips.size()); - if (nullptr == callback) - { - auto delegate = static_cast(context); - DiscoveredNodeData nodeData; - service.ToDiscoveredNodeData(addresses, nodeData); - delegate->OnNodeDiscovered(nodeData); - } - else - { - callback(context, &service, addresses, CHIP_NO_ERROR); - } - - return true; - } + auto & service = interface.service; + auto addresses = Span(ips.data(), ips.size()); + if (nullptr == callback) + { + auto delegate = static_cast(context); + DiscoveredNodeData nodeData; + service.ToDiscoveredNodeData(addresses, nodeData); + delegate->OnNodeDiscovered(nodeData); } - return false; + else + { + callback(context, &service, addresses, CHIP_NO_ERROR); + } + + return true; } -CHIP_ERROR ResolveContext::OnNewAddress(const std::pair & interfaceKey, const struct sockaddr * address) +CHIP_ERROR ResolveContext::OnNewAddress(const std::pair interfaceKey, const struct sockaddr * address) { // If we don't have any information about this interfaceId, just ignore the // address, since it won't be usable anyway without things like the port. @@ -621,7 +605,7 @@ CHIP_ERROR ResolveContext::OnNewAddress(const std::pair & // on the system, because the hostnames we are looking up all end in // ".local". In other words, we can get regular DNS results in here, not // just DNS-SD ones. - auto interfaceId = interfaceKey.first; + uint32_t interfaceId = interfaceKey.first; if (interfaces.find(interfaceKey) == interfaces.end()) { @@ -736,7 +720,8 @@ void ResolveContext::OnNewInterface(uint32_t interfaceId, const char * fullname, } std::pair interfaceKey = std::make_pair(interfaceId, domainFromHostname); - interfaces.insert(std::make_pair(std::move(interfaceKey), std::move(interface))); + + interfaces.insert(std::make_pair(interfaceKey, std::move(interface))); } bool ResolveContext::HasInterface() diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index 94bbdc477d9b64..bf0fc96dc7cc4f 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -32,12 +32,8 @@ using namespace chip::Dnssd::Internal; namespace { -constexpr char kLocalDot[] = "local."; - -constexpr char kSrpDot[] = "default.service.arpa."; - -// The extra time in milliseconds that we will wait for the resolution on the srp domain to complete. -constexpr uint16_t kSrpTimeoutInMsec = 250; +// The extra time in milliseconds that we will wait for the resolution on the open thread domain to complete. +constexpr uint16_t kOpenThreadTimeoutInMsec = 250; constexpr DNSServiceFlags kRegisterFlags = kDNSServiceFlagsNoAutoRename; constexpr DNSServiceFlags kBrowseFlags = kDNSServiceFlagsShareConnection; @@ -148,57 +144,59 @@ std::string GetDomainFromHostName(const char * hostnameWithDomain) { std::string hostname = std::string(hostnameWithDomain); - // Find the first occurence of '.' - size_t first_pos = hostname.find("."); - - // if not found, return empty string - VerifyOrReturnValue(first_pos != std::string::npos, std::string()); + // Find the last occurence of '.' + size_t last_pos = hostname.find_last_of("."); + if (last_pos != std::string::npos) + { + // Get a substring without last '.' + std::string substring = hostname.substr(0, last_pos); - // Get a substring after the first occurence of '.' to the end of the string - return hostname.substr(first_pos + 1, hostname.size()); + // Find the last occurence of '.' in the substring created above. + size_t pos = substring.find_last_of("."); + if (pos != std::string::npos) + { + // Return the domain name between the last 2 occurences of '.' including the trailing dot'.'. + return std::string(hostname.substr(pos + 1, last_pos)); + } + } + return std::string(); } +Global MdnsContexts::sInstance; + +namespace { + /** - * @brief Callback that is called when the timeout for resolving on the kSrpDot domain has expired. + * @brief Callback that is called when the timeout for resolving on the kOpenThreadDot domain has expired. * * @param[in] systemLayer The system layer. * @param[in] callbackContext The context passed to the timer callback. */ -void SrpTimerExpiredCallback(System::Layer * systemLayer, void * callbackContext) +void OpenThreadTimerExpiredCallback(System::Layer * systemLayer, void * callbackContext) { - ChipLogProgress(Discovery, "Mdns: Timer expired for resolve to complete on the srp domain."); + ChipLogProgress(Discovery, "Mdns: Timer expired for resolve to complete on the open thread domain."); auto sdCtx = static_cast(callbackContext); VerifyOrDie(sdCtx != nullptr); - sdCtx->Finalize(); -} -/** - * @brief Starts a timer to wait for the resolution on the kSrpDot domain to happen. - * - * @param[in] timeoutSeconds The timeout in seconds. - * @param[in] ResolveContext The resolve context. - */ -CHIP_ERROR StartSrpTimer(uint16_t timeoutInMSecs, ResolveContext * ctx) -{ - VerifyOrReturnValue(ctx != nullptr, CHIP_ERROR_INCORRECT_STATE); - return DeviceLayer::SystemLayer().StartTimer(System::Clock::Milliseconds16(timeoutInMSecs), SrpTimerExpiredCallback, - reinterpret_cast(ctx)); + if (sdCtx->hasOpenThreadTimerStarted) + { + sdCtx->Finalize(); + } } /** - * @brief Cancels the timer that was started to wait for the resolution on the kSrpDot domain to happen. + * @brief Starts a timer to wait for the resolution on the kOpenThreadDot domain to happen. * + * @param[in] timeoutSeconds The timeout in seconds. * @param[in] ResolveContext The resolve context. */ -void CancelSrpTimer(ResolveContext * ctx) +void StartOpenThreadTimer(uint16_t timeoutInMSecs, ResolveContext * ctx) { - DeviceLayer::SystemLayer().CancelTimer(SrpTimerExpiredCallback, reinterpret_cast(ctx)); + VerifyOrReturn(ctx != nullptr, ChipLogError(Discovery, "Can't schedule open thread timer since context is null")); + DeviceLayer::SystemLayer().StartTimer(System::Clock::Milliseconds16(timeoutInMSecs), OpenThreadTimerExpiredCallback, + reinterpret_cast(ctx)); } -Global MdnsContexts::sInstance; - -namespace { - static void OnRegister(DNSServiceRef sdRef, DNSServiceFlags flags, DNSServiceErrorType err, const char * name, const char * type, const char * domain, void * context) { @@ -250,17 +248,17 @@ CHIP_ERROR Browse(BrowseHandler * sdCtx, uint32_t interfaceId, const char * type auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - // We will browse on both the local domain and the srp domain. + // We will browse on both the local domain and the open thread domain. ChipLogProgress(Discovery, "Browsing for: %s on domain %s", StringOrNullMarker(type), kLocalDot); auto sdRefLocal = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection err = DNSServiceBrowse(&sdRefLocal, kBrowseFlags, interfaceId, type, kLocalDot, OnBrowse, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - ChipLogProgress(Discovery, "Browsing for: %s on domain %s", StringOrNullMarker(type), kSrpDot); + ChipLogProgress(Discovery, "Browsing for: %s on domain %s", StringOrNullMarker(type), kOpenThreadDot); - auto sdRefSrp = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - err = DNSServiceBrowse(&sdRefSrp, kBrowseFlags, interfaceId, type, kSrpDot, OnBrowse, sdCtx); + DNSServiceRef sdRefOpenThread = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection + err = DNSServiceBrowse(&sdRefOpenThread, kBrowseFlags, interfaceId, type, kOpenThreadDot, OnBrowse, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); return MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); @@ -309,37 +307,25 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i { VerifyOrReturn(sdCtx->HasAddress(), sdCtx->Finalize(kDNSServiceErr_BadState)); - if (domainName.compare(kSrpDot) == 0) + if (domainName.compare(kOpenThreadDot) == 0) { - ChipLogProgress(Discovery, "Mdns: Resolve completed on the srp domain."); - - // Cancel the timer if one has been started - if (sdCtx->hasSrpTimerStarted) - { - CancelSrpTimer(sdCtx); - } + ChipLogProgress(Discovery, "Mdns: Resolve completed on the open thread domain."); sdCtx->Finalize(); } else if (domainName.compare(kLocalDot) == 0) { - ChipLogProgress(Discovery, - "Mdns: Resolve completed on the local domain. Starting a timer for the srp resolve to come back"); + ChipLogProgress( + Discovery, + "Mdns: Resolve completed on the local domain. Starting a timer for the open thread resolve to come back"); - // Usually the resolution on the local domain is quicker than on the srp domain. We would like to give the - // resolution on the srp domain around 250 millisecs more to give it a chance to resolve before finalizing + // Usually the resolution on the local domain is quicker than on the open thread domain. We would like to give the + // resolution on the open thread domain around 250 millisecs more to give it a chance to resolve before finalizing // the resolution. - if (!sdCtx->hasSrpTimerStarted) + if (!sdCtx->hasOpenThreadTimerStarted) { - // Schedule a timer to allow the resolve on Srp domain to complete. - CHIP_ERROR error = StartSrpTimer(kSrpTimeoutInMsec, sdCtx); - - // If the timer fails to start, finalize the context and return. - if (error != CHIP_NO_ERROR) - { - sdCtx->Finalize(); - return; - } - sdCtx->hasSrpTimerStarted = true; + // Schedule a timer to allow the resolve on OpenThread domain to complete. + StartOpenThreadTimer(kOpenThreadTimeoutInMsec, sdCtx); + sdCtx->hasOpenThreadTimerStarted = true; } } } @@ -381,7 +367,8 @@ static void OnResolve(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t inter if (!sdCtx->isResolveRequested) { GetAddrInfo(sdCtx); - sdCtx->isResolveRequested = true; + sdCtx->isResolveRequested = true; + sdCtx->hasOpenThreadTimerStarted = false; } } } @@ -395,13 +382,13 @@ static CHIP_ERROR Resolve(ResolveContext * sdCtx, uint32_t interfaceId, chip::In auto err = DNSServiceCreateConnection(&sdCtx->serviceRef); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - // Similar to browse, will try to resolve using both the local domain and the srp domain. + // Similar to browse, will try to resolve using both the local domain and the open thread domain. auto sdRefLocal = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection err = DNSServiceResolve(&sdRefLocal, kResolveFlags, interfaceId, name, type, kLocalDot, OnResolve, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); - auto sdRefSrp = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection - err = DNSServiceResolve(&sdRefSrp, kResolveFlags, interfaceId, name, type, kSrpDot, OnResolve, sdCtx); + auto sdRefOpenThread = sdCtx->serviceRef; // Mandatory copy because of kDNSServiceFlagsShareConnection + err = DNSServiceResolve(&sdRefOpenThread, kResolveFlags, interfaceId, name, type, kOpenThreadDot, OnResolve, sdCtx); VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err)); auto retval = MdnsContexts::GetInstance().Add(sdCtx, sdCtx->serviceRef); diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index 8a9853082b59ee..713a465c236afc 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -27,17 +27,15 @@ #include #include +constexpr char kLocalDot[] = "local."; + +constexpr char kOpenThreadDot[] = "default.service.arpa."; + namespace chip { namespace Dnssd { -struct BrowseWithDelegateContext; -struct RegisterContext; -struct ResolveContext; - std::string GetDomainFromHostName(const char * hostname); -void CancelSrpTimer(ResolveContext * ctx); - enum class ContextType { Register, @@ -64,6 +62,10 @@ struct GenericContext CHIP_ERROR FinalizeInternal(const char * errorStr, CHIP_ERROR err); }; +struct BrowseWithDelegateContext; +struct RegisterContext; +struct ResolveContext; + class MdnsContexts { public: @@ -237,7 +239,7 @@ struct ResolveContext : public GenericContext bool isResolveRequested = false; std::shared_ptr consumerCounter; BrowseContext * const browseThatCausedResolve; // Can be null - bool hasSrpTimerStarted = false; + bool hasOpenThreadTimerStarted = false; // browseCausingResolve can be null. ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, @@ -250,7 +252,7 @@ struct ResolveContext : public GenericContext void DispatchFailure(const char * errorStr, CHIP_ERROR err) override; void DispatchSuccess() override; - CHIP_ERROR OnNewAddress(const std::pair & interfaceKey, const struct sockaddr * address); + CHIP_ERROR OnNewAddress(const std::pair interfaceKey, const struct sockaddr * address); bool HasAddress(); void OnNewInterface(uint32_t interfaceId, const char * fullname, const char * hostname, uint16_t port, uint16_t txtLen, @@ -264,7 +266,7 @@ struct ResolveContext : public GenericContext * Returns true if information was reported, false if not (e.g. if there * were no IP addresses, etc). */ - bool TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex); + bool TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, std::string domainName); }; } // namespace Dnssd