Allow platform DNS-SD resolve to provide more than one IP address in …

…the ResolvedNodeData (#15239)

* Change the ChipDnssdResolve API to be able to provide multiple IPs.

* On Darwin, wait until GetAddrInfo returns all the results before calling the resolve callback.

* Add logs, address review comment.

src/include/platform/ThreadStackManager.h

@@ -63,7 +63,8 @@ class GenericThreadStackManagerImpl_FreeRTOS;
 
 #if CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT
 // Declaration of callback types corresponding to DnssdResolveCallback and DnssdBrowseCallback to avoid circular including.
-using DnsResolveCallback = void (*)(void * context, chip::Dnssd::DnssdService * result, CHIP_ERROR error);
+using DnsResolveCallback = void (*)(void * context, chip::Dnssd::DnssdService * result, const Span<Inet::IPAddress> & extraIPs,
+                                    CHIP_ERROR error);
 using DnsBrowseCallback  = void (*)(void * context, chip::Dnssd::DnssdService * services, size_t servicesSize, CHIP_ERROR error);
 #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT
 

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -42,7 +42,7 @@ namespace {
 static DnssdCache<CHIP_CONFIG_MDNS_CACHE_SIZE> sDnssdCache;
 #endif
 
-static void HandleNodeResolve(void * context, DnssdService * result, CHIP_ERROR error)
+static void HandleNodeResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & extraIPs, CHIP_ERROR error)
 {
     ResolverDelegateProxy * proxy = static_cast<ResolverDelegateProxy *>(context);
 
@@ -57,17 +57,28 @@ static void HandleNodeResolve(void * context, DnssdService * result, CHIP_ERROR
     Platform::CopyString(nodeData.hostName, result->mHostName);
     Platform::CopyString(nodeData.instanceName, result->mName);
 
+    size_t addressesFound = 0;
     if (result->mAddress.HasValue())
     {
-        nodeData.ipAddress[0] = result->mAddress.Value();
-        nodeData.interfaceId  = result->mInterface;
-        nodeData.numIPs       = 1;
+        nodeData.ipAddress[addressesFound] = result->mAddress.Value();
+        nodeData.interfaceId               = result->mInterface;
+        ++addressesFound;
     }
-    else
+
+    for (auto & ip : extraIPs)
     {
-        nodeData.numIPs = 0;
+        if (addressesFound == ArraySize(nodeData.ipAddress))
+        {
+            // Out of space.
+            ChipLogProgress(Discovery, "Can't add more IPs to DiscoveredNodeData");
+            break;
+        }
+        nodeData.ipAddress[addressesFound] = ip;
+        ++addressesFound;
     }
 
+    nodeData.numIPs = addressesFound;
+
     nodeData.port = result->mPort;
 
     for (size_t i = 0; i < result->mTextEntrySize; ++i)
@@ -81,7 +92,7 @@ static void HandleNodeResolve(void * context, DnssdService * result, CHIP_ERROR
     proxy->Release();
 }
 
-static void HandleNodeIdResolve(void * context, DnssdService * result, CHIP_ERROR error)
+static void HandleNodeIdResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & extraIPs, CHIP_ERROR error)
 {
     ResolverDelegateProxy * proxy = static_cast<ResolverDelegateProxy *>(context);
     if (CHIP_NO_ERROR != error)
@@ -116,15 +127,32 @@ static void HandleNodeIdResolve(void * context, DnssdService * result, CHIP_ERRO
     ResolvedNodeData nodeData;
     Platform::CopyString(nodeData.mHostName, result->mHostName);
     nodeData.mInterfaceId = result->mInterface;
-    nodeData.mAddress[0]  = result->mAddress.ValueOr({});
     nodeData.mPort        = result->mPort;
-    nodeData.mNumIPs      = 1;
     nodeData.mPeerId      = peerId;
     // TODO: Use seconds?
     const System::Clock::Timestamp currentTime = System::SystemClock().GetMonotonicTimestamp();
 
     nodeData.mExpiryTime = currentTime + System::Clock::Seconds16(result->mTtlSeconds);
 
+    size_t addressesFound = 0;
+    if (result->mAddress.HasValue())
+    {
+        nodeData.mAddress[addressesFound] = result->mAddress.Value();
+        ++addressesFound;
+    }
+    for (auto & ip : extraIPs)
+    {
+        if (addressesFound == ArraySize(nodeData.mAddress))
+        {
+            // Out of space.
+            ChipLogProgress(Discovery, "Can't add more IPs to ResolvedNodeData");
+            break;
+        }
+        nodeData.mAddress[addressesFound] = ip;
+        ++addressesFound;
+    }
+    nodeData.mNumIPs = addressesFound;
+
     for (size_t i = 0; i < result->mTextEntrySize; ++i)
     {
         ByteSpan key(reinterpret_cast<const uint8_t *>(result->mTextEntries[i].mKey), strlen(result->mTextEntries[i].mKey));
@@ -155,7 +183,7 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser
         }
         else
         {
-            HandleNodeResolve(context, &services[i], error);
+            HandleNodeResolve(context, &services[i], Span<Inet::IPAddress>(), error);
         }
     }
     proxy->Release();

src/lib/dnssd/platform/Dnssd.h

@@ -88,10 +88,13 @@ struct DnssdService
  *
  * @param[in] context     The context passed to ChipDnssdBrowse or ChipDnssdResolve.
  * @param[in] result      The mdns resolve result, can be nullptr if error happens.
+ * @param[in] extraIPs    IP addresses, in addition to the one in "result", for
+ *                        the same hostname.  Can be empty.
  * @param[in] error       The error code.
  *
  */
-using DnssdResolveCallback = void (*)(void * context, DnssdService * result, CHIP_ERROR error);
+using DnssdResolveCallback = void (*)(void * context, DnssdService * result, const Span<Inet::IPAddress> & extraIPs,
+                                      CHIP_ERROR error);
 
 /**
  * The callback function for mDNS browse.

src/platform/Darwin/DnssdImpl.cpp

@@ -248,12 +248,12 @@ bool CheckForSuccess(GenericContext * context, const char * name, DNSServiceErro
             }
             case ContextType::Resolve: {
                 ResolveContext * resolveContext = reinterpret_cast<ResolveContext *>(context);
-                resolveContext->callback(resolveContext->context, nullptr, CHIP_ERROR_INTERNAL);
+                resolveContext->callback(resolveContext->context, nullptr, Span<Inet::IPAddress>(), CHIP_ERROR_INTERNAL);
                 break;
             }
             case ContextType::GetAddrInfo: {
                 GetAddrInfoContext * resolveContext = reinterpret_cast<GetAddrInfoContext *>(context);
-                resolveContext->callback(resolveContext->context, nullptr, CHIP_ERROR_INTERNAL);
+                resolveContext->callback(resolveContext->context, nullptr, Span<Inet::IPAddress>(), CHIP_ERROR_INTERNAL);
                 break;
             }
             }
@@ -403,18 +403,35 @@ static void OnGetAddrInfo(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t i
 
     ChipLogDetail(DeviceLayer, "Mdns: %s hostname:%s", __func__, hostname);
 
+    chip::Inet::IPAddress ip;
+    CHIP_ERROR status = chip::Inet::IPAddress::GetIPAddressFromSockAddr(*address, ip);
+    if (status == CHIP_NO_ERROR)
+    {
+        sdCtx->addresses.push_back(ip);
+    }
+
+    if (flags & kDNSServiceFlagsMoreComing)
+    {
+        // Wait for that.
+        return;
+    }
+
     DnssdService service   = {};
     service.mPort          = sdCtx->port;
     service.mTextEntries   = sdCtx->textEntries.empty() ? nullptr : sdCtx->textEntries.data();
     service.mTextEntrySize = sdCtx->textEntries.empty() ? 0 : sdCtx->textEntries.size();
-    chip::Inet::IPAddress ip;
-    CHIP_ERROR status = chip::Inet::IPAddress::GetIPAddressFromSockAddr(*address, ip);
-    service.mAddress.SetValue(ip);
+    // Use the first IP we got for the DnssdService.
+    if (sdCtx->addresses.size() != 0)
+    {
+        service.mAddress.SetValue(sdCtx->addresses.front());
+        sdCtx->addresses.erase(sdCtx->addresses.begin());
+    }
     Platform::CopyString(service.mName, sdCtx->name);
     Platform::CopyString(service.mHostName, hostname);
     service.mInterface = Inet::InterfaceId(sdCtx->interfaceId);
 
-    sdCtx->callback(sdCtx->context, &service, status);
+    // TODO: Does it really make sense to pass in the status from our last "get the IP address" operation?
+    sdCtx->callback(sdCtx->context, &service, Span<Inet::IPAddress>(sdCtx->addresses.data(), sdCtx->addresses.size()), status);
     MdnsContexts::GetInstance().Remove(sdCtx);
 }
 
@@ -512,6 +529,9 @@ static void OnResolve(DNSServiceRef sdRef, DNSServiceFlags flags, uint32_t inter
 
     GetAddrInfo(sdCtx->context, sdCtx->callback, interfaceId, sdCtx->addressType, sdCtx->name, hostname, ntohs(port), txtLen,
                 txtRecord);
+
+    // TODO: If flags & kDNSServiceFlagsMoreComing should we keep waiting to see
+    // what else we resolve instead of calling Remove() here?
     MdnsContexts::GetInstance().Remove(sdCtx);
 }
 

src/platform/Darwin/DnssdImpl.h

@@ -91,6 +91,7 @@ struct ResolveContext : public GenericContext
 struct GetAddrInfoContext : public GenericContext
 {
     DnssdResolveCallback callback;
+    std::vector<Inet::IPAddress> addresses;
     std::vector<TextEntry> textEntries;
     char name[Common::kInstanceNameMaxLength + 1];
     uint32_t interfaceId;

src/platform/Linux/DnssdImpl.cpp

@@ -714,13 +714,13 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte
             if (resolver == nullptr)
             {
                 ChipLogError(DeviceLayer, "Avahi resolve failed on retry");
-                context->mCallback(context->mContext, nullptr, CHIP_ERROR_INTERNAL);
+                context->mCallback(context->mContext, nullptr, Span<Inet::IPAddress>(), CHIP_ERROR_INTERNAL);
                 chip::Platform::Delete(context);
             }
             return;
         }
         ChipLogError(DeviceLayer, "Avahi resolve failed");
-        context->mCallback(context->mContext, nullptr, CHIP_ERROR_INTERNAL);
+        context->mCallback(context->mContext, nullptr, Span<Inet::IPAddress>(), CHIP_ERROR_INTERNAL);
         break;
     case AVAHI_RESOLVER_FOUND:
         DnssdService result   = {};
@@ -797,11 +797,11 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte
 
         if (result_err == CHIP_NO_ERROR)
         {
-            context->mCallback(context->mContext, &result, CHIP_NO_ERROR);
+            context->mCallback(context->mContext, &result, Span<Inet::IPAddress>(), CHIP_NO_ERROR);
         }
         else
         {
-            context->mCallback(context->mContext, nullptr, result_err);
+            context->mCallback(context->mContext, nullptr, Span<Inet::IPAddress>(), result_err);
         }
         break;
     }

src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp

@@ -2287,7 +2287,8 @@ template <class ImplClass>
 void GenericThreadStackManagerImpl_OpenThread<ImplClass>::DispatchResolve(intptr_t context)
 {
     auto * dnsResult = reinterpret_cast<DnsResult *>(context);
-    ThreadStackMgrImpl().mDnsResolveCallback(dnsResult->context, &(dnsResult->mMdnsService), dnsResult->error);
+    ThreadStackMgrImpl().mDnsResolveCallback(dnsResult->context, &(dnsResult->mMdnsService), Span<Inet::IPAddress>(),
+                                             dnsResult->error);
     Platform::Delete<DnsResult>(dnsResult);
 }
 

src/platform/Tizen/DnssdImpl.cpp

@@ -84,7 +84,7 @@ bool CheckForSuccess(GenericContext * context, int err, const char * func, bool
             }
             case ContextType::Resolve: {
                 ResolveContext * rCtx = reinterpret_cast<ResolveContext *>(context);
-                rCtx->callback(rCtx->context, nullptr, CHIP_ERROR_INTERNAL);
+                rCtx->callback(rCtx->context, nullptr, chip::Span<chip::Inet::IPAddress>(), CHIP_ERROR_INTERNAL);
                 break;
             }
             }
@@ -459,12 +459,12 @@ void OnResolve(dnssd_error_e result, dnssd_service_h service, void * data)
     if (validIP)
     {
         mdnsService.mAddress.SetValue(ipStr);
-        rCtx->callback(rCtx->cbContext, &mdnsService, CHIP_NO_ERROR);
+        rCtx->callback(rCtx->cbContext, &mdnsService, chip::Span<chip::Inet::IPAddress>(), CHIP_NO_ERROR);
         StopResolve(rCtx);
     }
     else
     {
-        rCtx->callback(rCtx->cbContext, nullptr, CHIP_ERROR_INTERNAL);
+        rCtx->callback(rCtx->cbContext, nullptr, chip::Span<chip::Inet::IPAddress>(), CHIP_ERROR_INTERNAL);
         RemoveContext(rCtx);
     }
 

src/platform/android/DnssdImpl.cpp

@@ -220,7 +220,7 @@ void HandleResolve(jstring instanceName, jstring serviceType, jstring address, j
     const auto dispatch = [callbackHandle, contextHandle](CHIP_ERROR error, DnssdService * service = nullptr) {
         DeviceLayer::StackLock lock;
         DnssdResolveCallback callback = reinterpret_cast<DnssdResolveCallback>(callbackHandle);
-        callback(reinterpret_cast<void *>(contextHandle), service, error);
+        callback(reinterpret_cast<void *>(contextHandle), service, Span<Inet::IPAddress>(), error);
     };
 
     VerifyOrReturn(address != nullptr && port != 0, dispatch(CHIP_ERROR_UNKNOWN_RESOURCE_ID));

src/platform/tests/TestDnssd.cpp

@@ -14,7 +14,8 @@ using chip::Dnssd::DnssdService;
 using chip::Dnssd::DnssdServiceProtocol;
 using chip::Dnssd::TextEntry;
 
-static void HandleResolve(void * context, DnssdService * result, CHIP_ERROR error)
+static void HandleResolve(void * context, DnssdService * result, const chip::Span<chip::Inet::IPAddress> & extraIPs,
+                          CHIP_ERROR error)
 {
     char addrBuf[100];
     nlTestSuite * suite = static_cast<nlTestSuite *>(context);