From e50da9b4a371c73bb5aa00329792fed45e8624a2 Mon Sep 17 00:00:00 2001 From: Nivedita Sarkar Date: Wed, 20 Mar 2024 14:29:21 -0700 Subject: [PATCH] Addressed review comments --- src/platform/Darwin/DnssdContexts.cpp | 22 +++++--- src/platform/Darwin/DnssdImpl.cpp | 78 ++++++++++++++------------- src/platform/Darwin/DnssdImpl.h | 7 +-- 3 files changed, 61 insertions(+), 46 deletions(-) diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index 0146bf3a835d42..c6067a55168e12 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -463,7 +463,6 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip:: callback = cb; protocol = GetProtocol(cbAddressType); instanceName = instanceNameToResolve; - domainName = std::string(kLocalDot); consumerCounter = std::move(consumerCounterToUse); } @@ -476,7 +475,6 @@ ResolveContext::ResolveContext(CommissioningResolveDelegate * delegate, chip::In callback = nullptr; protocol = GetProtocol(cbAddressType); instanceName = instanceNameToResolve; - domainName = std::string(kLocalDot); consumerCounter = std::move(consumerCounterToUse); } @@ -528,7 +526,17 @@ void ResolveContext::DispatchSuccess() for (auto interfaceIndex : priorityInterfaceIndices) { - if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex))) + // Try finding interfaces for domains kLocalDot and kOpenThreadDot and delete them. + if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex), std::string(kLocalDot))) + { + if (needDelete) + { + MdnsContexts::GetInstance().Delete(this); + } + return; + } + + if (TryReportingResultsForInterfaceIndex(static_cast(interfaceIndex), std::string(kOpenThreadDot))) { if (needDelete) { @@ -540,7 +548,7 @@ void ResolveContext::DispatchSuccess() for (auto & interface : interfaces) { - if (TryReportingResultsForInterfaceIndex(interface.first.first)) + if (TryReportingResultsForInterfaceIndex(interface.first.first, interface.first.second)) { break; } @@ -552,7 +560,7 @@ void ResolveContext::DispatchSuccess() } } -bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex) +bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, std::string domainName) { if (interfaceIndex == 0) { @@ -560,7 +568,7 @@ bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceInde return false; } - std::pair interfaceKey = std::make_pair(interfaceIndex, this->domainName); + std::pair interfaceKey = std::make_pair(interfaceIndex, domainName); auto & interface = interfaces[interfaceKey]; auto & ips = interface.addresses; @@ -707,7 +715,7 @@ void ResolveContext::OnNewInterface(uint32_t interfaceId, const char * fullname, std::string domainFromHostname = GetDomainFromHostName(hostnameWithDomain); if (domainFromHostname.empty()) { - ChipLogError(Discovery, "Mdns: Domain from hostname is empty"); + ChipLogError(Discovery, "Mdns: No domain set in hostname %s", hostnameWithDomain); return; } diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index d30114349b74b8..2028320fa8a4e1 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -35,8 +35,6 @@ namespace { // The extra time in milliseconds that we will wait for the resolution on the open thread domain to complete. constexpr uint16_t kOpenThreadTimeoutInMsec = 250; -static bool hasOpenThreadTimerStarted = false; - constexpr DNSServiceFlags kRegisterFlags = kDNSServiceFlagsNoAutoRename; constexpr DNSServiceFlags kBrowseFlags = kDNSServiceFlagsShareConnection; constexpr DNSServiceFlags kGetAddrInfoFlags = kDNSServiceFlagsTimeout | kDNSServiceFlagsShareConnection; @@ -58,22 +56,6 @@ std::string GetHostNameWithLocalDomain(const char * hostname) return std::string(hostname) + '.' + kLocalDot; } -bool HostNameHasDomain(const char * hostname, const char * domain) -{ - size_t domainLength = strlen(domain); - size_t hostnameLength = strlen(hostname); - if (domainLength > hostnameLength) - { - return false; - } - const char * found = strstr(hostname, domain); - if (found == nullptr) - { - return false; - } - return (strncmp(found, domain, domainLength) == 0); -} - void LogOnFailure(const char * name, DNSServiceErrorType err) { if (kDNSServiceErr_NoError != err) @@ -151,15 +133,31 @@ std::shared_ptr GetCounterHolder(const char * name) namespace chip { namespace Dnssd { -std::string GetDomainFromHostName(const char * hostname) +/** + * @brief Returns the domain name from a given hostname with domain. + * The assumption here is that the hostname comprises of "hostnameWithoutDomain.." + * The domainName returned from this API is "." + * + * @param[in] hostname The hostname with domain. + */ +std::string GetDomainFromHostName(const char * hostnameWithDomain) { - if (HostNameHasDomain(hostname, kLocalDot)) - { - return std::string(kLocalDot); - } - else if (HostNameHasDomain(hostname, kOpenThreadDot)) + std::string hostname = std::string(hostnameWithDomain); + + // Find the last occurence of '.' + size_t last_pos = hostname.find_last_of("."); + if (last_pos != std::string::npos) { - return std::string(kOpenThreadDot); + // Get a substring without last '.' + std::string substring = hostname.substr(0, last_pos); + + // 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(); } @@ -176,11 +174,14 @@ namespace { */ void OpenThreadTimerExpiredCallback(System::Layer * systemLayer, void * callbackContext) { - ChipLogProgress(Discovery, "Mdns: Resolve completed on the open thread 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(); - hasOpenThreadTimerStarted = false; + + if (sdCtx->hasOpenThreadTimerStarted) + { + sdCtx->Finalize(); + } } /** @@ -290,15 +291,15 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i ReturnOnFailure(MdnsContexts::GetInstance().Has(sdCtx)); LogOnFailure(__func__, err); - sdCtx->domainName = GetDomainFromHostName(hostname); - if (sdCtx->domainName.empty()) + std::string domainName = GetDomainFromHostName(hostname); + if (domainName.empty()) { - ChipLogError(Discovery, "Mdns: Domain name is not set"); + ChipLogError(Discovery, "Mdns: Domain name is not set in hostname %s", hostname); return; } if (kDNSServiceErr_NoError == err) { - std::pair key = std::make_pair(interfaceId, sdCtx->domainName); + std::pair key = std::make_pair(interfaceId, domainName); sdCtx->OnNewAddress(key, address); } @@ -306,21 +307,25 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i { VerifyOrReturn(sdCtx->HasAddress(), sdCtx->Finalize(kDNSServiceErr_BadState)); - if (sdCtx->domainName.compare(kOpenThreadDot) == 0) + if (domainName.compare(kOpenThreadDot) == 0) { ChipLogProgress(Discovery, "Mdns: Resolve completed on the open thread domain."); sdCtx->Finalize(); } - else if (sdCtx->domainName.compare(kLocalDot) == 0) + else if (domainName.compare(kLocalDot) == 0) { ChipLogProgress( Discovery, "Mdns: Resolve completed on the local domain. Starting a timer for the open thread resolve to come back"); - if (!hasOpenThreadTimerStarted) + + // 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->hasOpenThreadTimerStarted) { // Schedule a timer to allow the resolve on OpenThread domain to complete. StartOpenThreadTimer(kOpenThreadTimeoutInMsec, sdCtx); - hasOpenThreadTimerStarted = true; + sdCtx->hasOpenThreadTimerStarted = true; } } } @@ -363,6 +368,7 @@ static void OnResolve(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t inter { GetAddrInfo(sdCtx); sdCtx->isResolveRequested = true; + sdCtx->hasOpenThreadTimerStarted = false; } } } diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index 760e43947f6200..6ef81a3321cff3 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -29,7 +29,7 @@ constexpr char kLocalDot[] = "local."; -constexpr char kOpenThreadDot[] = "openthread.thread.home.arpa"; +constexpr char kOpenThreadDot[] = "default.service.arpa."; namespace chip { namespace Dnssd { @@ -236,10 +236,10 @@ struct ResolveContext : public GenericContext std::map, InterfaceInfo> interfaces; DNSServiceProtocol protocol; std::string instanceName; - std::string domainName; bool isResolveRequested = false; std::shared_ptr consumerCounter; BrowseContext * const browseThatCausedResolve; // Can be null + bool hasOpenThreadTimerStarted = false; // browseCausingResolve can be null. ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType, @@ -261,12 +261,13 @@ struct ResolveContext : public GenericContext bool Matches(const char * otherInstanceName) const { return instanceName == otherInstanceName; } private: + /** * Try reporting the results we got on the provided interface index. * 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