From 0e027df934b52cdf54a3c436e25afb2ce8389a98 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 18 Feb 2022 21:48:48 -0500 Subject: [PATCH] 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 | 3 +- src/lib/dnssd/Discovery_ImplPlatform.cpp | 48 +++++++++++++++---- src/lib/dnssd/platform/Dnssd.h | 5 +- src/platform/Darwin/DnssdImpl.cpp | 32 ++++++++++--- src/platform/Darwin/DnssdImpl.h | 1 + src/platform/Linux/DnssdImpl.cpp | 8 ++-- ...nericThreadStackManagerImpl_OpenThread.cpp | 3 +- src/platform/Tizen/DnssdImpl.cpp | 6 +-- src/platform/android/DnssdImpl.cpp | 2 +- src/platform/tests/TestDnssd.cpp | 3 +- 10 files changed, 83 insertions(+), 28 deletions(-) diff --git a/src/include/platform/ThreadStackManager.h b/src/include/platform/ThreadStackManager.h index e4c0e4696ab3c0..8a231ca4a743ac 100644 --- a/src/include/platform/ThreadStackManager.h +++ b/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 & 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 diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index f9c09b92d88b98..a8c2be91c68c27 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -42,7 +42,7 @@ namespace { static DnssdCache sDnssdCache; #endif -static void HandleNodeResolve(void * context, DnssdService * result, CHIP_ERROR error) +static void HandleNodeResolve(void * context, DnssdService * result, const Span & extraIPs, CHIP_ERROR error) { ResolverDelegateProxy * proxy = static_cast(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 & extraIPs, CHIP_ERROR error) { ResolverDelegateProxy * proxy = static_cast(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(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(), error); } } proxy->Release(); diff --git a/src/lib/dnssd/platform/Dnssd.h b/src/lib/dnssd/platform/Dnssd.h index add47b59bd65bb..719835d7ecc172 100644 --- a/src/lib/dnssd/platform/Dnssd.h +++ b/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 & extraIPs, + CHIP_ERROR error); /** * The callback function for mDNS browse. diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index 186cde4fe47bda..413de03a7dbcfb 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -248,12 +248,12 @@ bool CheckForSuccess(GenericContext * context, const char * name, DNSServiceErro } case ContextType::Resolve: { ResolveContext * resolveContext = reinterpret_cast(context); - resolveContext->callback(resolveContext->context, nullptr, CHIP_ERROR_INTERNAL); + resolveContext->callback(resolveContext->context, nullptr, Span(), CHIP_ERROR_INTERNAL); break; } case ContextType::GetAddrInfo: { GetAddrInfoContext * resolveContext = reinterpret_cast(context); - resolveContext->callback(resolveContext->context, nullptr, CHIP_ERROR_INTERNAL); + resolveContext->callback(resolveContext->context, nullptr, Span(), 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(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); } diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index a0f1bdc4be78cb..0c002ba9de9dd6 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -91,6 +91,7 @@ struct ResolveContext : public GenericContext struct GetAddrInfoContext : public GenericContext { DnssdResolveCallback callback; + std::vector addresses; std::vector textEntries; char name[Common::kInstanceNameMaxLength + 1]; uint32_t interfaceId; diff --git a/src/platform/Linux/DnssdImpl.cpp b/src/platform/Linux/DnssdImpl.cpp index 5c25004fbd62e9..5fbb617490fef0 100644 --- a/src/platform/Linux/DnssdImpl.cpp +++ b/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(), 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(), 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(), CHIP_NO_ERROR); } else { - context->mCallback(context->mContext, nullptr, result_err); + context->mCallback(context->mContext, nullptr, Span(), result_err); } break; } diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 6312cfab08f1b1..7d88dc5141a484 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -2287,7 +2287,8 @@ template void GenericThreadStackManagerImpl_OpenThread::DispatchResolve(intptr_t context) { auto * dnsResult = reinterpret_cast(context); - ThreadStackMgrImpl().mDnsResolveCallback(dnsResult->context, &(dnsResult->mMdnsService), dnsResult->error); + ThreadStackMgrImpl().mDnsResolveCallback(dnsResult->context, &(dnsResult->mMdnsService), Span(), + dnsResult->error); Platform::Delete(dnsResult); } diff --git a/src/platform/Tizen/DnssdImpl.cpp b/src/platform/Tizen/DnssdImpl.cpp index 00b9177e1ba374..2c0ed610db8381 100644 --- a/src/platform/Tizen/DnssdImpl.cpp +++ b/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(context); - rCtx->callback(rCtx->context, nullptr, CHIP_ERROR_INTERNAL); + rCtx->callback(rCtx->context, nullptr, chip::Span(), 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_NO_ERROR); StopResolve(rCtx); } else { - rCtx->callback(rCtx->cbContext, nullptr, CHIP_ERROR_INTERNAL); + rCtx->callback(rCtx->cbContext, nullptr, chip::Span(), CHIP_ERROR_INTERNAL); RemoveContext(rCtx); } diff --git a/src/platform/android/DnssdImpl.cpp b/src/platform/android/DnssdImpl.cpp index 429ff609bf6dd2..52d20525e8ca04 100644 --- a/src/platform/android/DnssdImpl.cpp +++ b/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(callbackHandle); - callback(reinterpret_cast(contextHandle), service, error); + callback(reinterpret_cast(contextHandle), service, Span(), error); }; VerifyOrReturn(address != nullptr && port != 0, dispatch(CHIP_ERROR_UNKNOWN_RESOURCE_ID)); diff --git a/src/platform/tests/TestDnssd.cpp b/src/platform/tests/TestDnssd.cpp index 5a252f8ee1ca17..22ec8370a71d55 100644 --- a/src/platform/tests/TestDnssd.cpp +++ b/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 & extraIPs, + CHIP_ERROR error) { char addrBuf[100]; nlTestSuite * suite = static_cast(context);