Revert "Fix the dnssd code that browses on both the local and srp domains" (#32691)

This reverts commit 88afa33.

Author: woody-apple

src/platform/Darwin/DnssdContexts.cpp

@@ -458,35 +458,27 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::
                                std::shared_ptr<uint32_t> && 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<uint32_t> && 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<uint32_t>(interfaceIndex), std::string(kLocalDot)))
         {
             if (needDelete)
             {
@@ -543,7 +536,7 @@ void ResolveContext::DispatchSuccess()
             return;
         }
 
-        if (TryReportingResultsForInterfaceIndex(interfaceIndex))
+        if (TryReportingResultsForInterfaceIndex(static_cast<uint32_t>(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,60 +560,52 @@ void ResolveContext::DispatchSuccess()
     }
 }
 
-bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex)
+bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, std::string domainName)
 {
     if (interfaceIndex == 0)
     {
         // Not actually an interface we have.
         return false;
     }
 
-    std::map<std::pair<uint32_t, std::string>, InterfaceInfo>::iterator iter = interfaces.begin();
-    while (iter != interfaces.end())
-    {
-        std::pair<uint32_t, std::string> key = iter->first;
-        if (key.first == interfaceIndex)
-        {
-            auto & interface = interfaces[key];
-            auto & ips       = interface.addresses;
+    std::pair<uint32_t, std::string> 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<Inet::IPAddress>(ips.data(), ips.size());
-            if (nullptr == callback)
-            {
-                auto delegate = static_cast<CommissioningResolveDelegate *>(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<Inet::IPAddress>(ips.data(), ips.size());
+    if (nullptr == callback)
+    {
+        auto delegate = static_cast<CommissioningResolveDelegate *>(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<uint32_t, std::string> & interfaceKey, const struct sockaddr * address)
+CHIP_ERROR ResolveContext::OnNewAddress(const std::pair<uint32_t, std::string> 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.
     // This can happen if "local" is set up as a search domain in the DNS setup
     // 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<uint32_t, std::string> 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()

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> 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<ResolveContext *>(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<void *>(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<void *>(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<void *>(ctx));
 }
 
-Global<MdnsContexts> 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);