From b28415f0197bb1be4c4f56cf575a20addfba596d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Sat, 21 May 2022 00:22:38 -0400 Subject: [PATCH] Stop passing our addresses from resolve in two different places. We used to pass some in the DnssdService and some in a Span. Now just pass them all in the Span. Fixes https://github.com/project-chip/connectedhomeip/issues/15279 --- src/include/platform/ThreadStackManager.h | 2 +- src/lib/dnssd/Discovery_ImplPlatform.cpp | 25 ++++------- src/lib/dnssd/platform/Dnssd.h | 8 ++-- src/platform/Darwin/DnssdContexts.cpp | 3 -- src/platform/ESP32/DnssdImpl.cpp | 42 +++++++++---------- src/platform/ESP32/DnssdImpl.h | 12 +++--- src/platform/Linux/DnssdImpl.cpp | 8 ++-- ...nericThreadStackManagerImpl_OpenThread.cpp | 5 ++- src/platform/Tizen/DnssdImpl.cpp | 3 +- src/platform/android/DnssdImpl.cpp | 9 ++-- src/platform/tests/TestDnssd.cpp | 9 ++-- 11 files changed, 57 insertions(+), 69 deletions(-) diff --git a/src/include/platform/ThreadStackManager.h b/src/include/platform/ThreadStackManager.h index 5ce7ec1dd09ea6..fbe533df7c288d 100644 --- a/src/include/platform/ThreadStackManager.h +++ b/src/include/platform/ThreadStackManager.h @@ -67,7 +67,7 @@ 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, const Span & extraIPs, +using DnsResolveCallback = void (*)(void * context, chip::Dnssd::DnssdService * result, const Span & addresses, 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 78e003f4f05a69..45b7c1766e05f1 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -37,7 +37,7 @@ namespace Dnssd { namespace { -static void HandleNodeResolve(void * context, DnssdService * result, const Span & extraIPs, CHIP_ERROR error) +static void HandleNodeResolve(void * context, DnssdService * result, const Span & addresses, CHIP_ERROR error) { ResolverDelegateProxy * proxy = static_cast(context); @@ -52,15 +52,10 @@ static void HandleNodeResolve(void * context, DnssdService * result, const Span< Platform::CopyString(nodeData.resolutionData.hostName, result->mHostName); Platform::CopyString(nodeData.commissionData.instanceName, result->mName); - size_t addressesFound = 0; - if (result->mAddress.HasValue()) - { - nodeData.resolutionData.ipAddress[addressesFound] = result->mAddress.Value(); - nodeData.resolutionData.interfaceId = result->mInterface; - ++addressesFound; - } + nodeData.resolutionData.interfaceId = result->mInterface; - for (auto & ip : extraIPs) + size_t addressesFound = 0; + for (auto & ip : addresses) { if (addressesFound == ArraySize(nodeData.resolutionData.ipAddress)) { @@ -88,7 +83,7 @@ static void HandleNodeResolve(void * context, DnssdService * result, const Span< proxy->Release(); } -static void HandleNodeIdResolve(void * context, DnssdService * result, const Span & extraIPs, CHIP_ERROR error) +static void HandleNodeIdResolve(void * context, DnssdService * result, const Span & addresses, CHIP_ERROR error) { ResolverDelegateProxy * proxy = static_cast(context); if (CHIP_NO_ERROR != error) @@ -127,12 +122,7 @@ static void HandleNodeIdResolve(void * context, DnssdService * result, const Spa nodeData.operationalData.peerId = peerId; size_t addressesFound = 0; - if (result->mAddress.HasValue()) - { - nodeData.resolutionData.ipAddress[addressesFound] = result->mAddress.Value(); - ++addressesFound; - } - for (auto & ip : extraIPs) + for (auto & ip : addresses) { if (addressesFound == ArraySize(nodeData.resolutionData.ipAddress)) { @@ -171,7 +161,8 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser } else { - HandleNodeResolve(context, &services[i], Span(), error); + Inet::IPAddress * address = &(services[i].mAddress.Value()); + HandleNodeResolve(context, &services[i], Span(address, 1), error); } } proxy->Release(); diff --git a/src/lib/dnssd/platform/Dnssd.h b/src/lib/dnssd/platform/Dnssd.h index 719835d7ecc172..c306903b56ee12 100644 --- a/src/lib/dnssd/platform/Dnssd.h +++ b/src/lib/dnssd/platform/Dnssd.h @@ -87,13 +87,13 @@ struct DnssdService * any pointer inside this structure. * * @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] result The mdns resolve result, can be nullptr if error + * happens. The mAddress of this object will be ignored. + * @param[in] addresses IP addresses that we resolved. * @param[in] error The error code. * */ -using DnssdResolveCallback = void (*)(void * context, DnssdService * result, const Span & extraIPs, +using DnssdResolveCallback = void (*)(void * context, DnssdService * result, const Span & addresses, CHIP_ERROR error); /** diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index 777f9d8a98d67c..2699bec216983b 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -326,9 +326,6 @@ void ResolveContext::DispatchSuccess() continue; } - // Use the first IP we got for the DnssdService. - interface.second.service.mAddress.SetValue(ips.front()); - ips.erase(ips.begin()); callback(context, &interface.second.service, Span(ips.data(), ips.size()), CHIP_NO_ERROR); break; } diff --git a/src/platform/ESP32/DnssdImpl.cpp b/src/platform/ESP32/DnssdImpl.cpp index 91cd8a07ddf3df..0c55ecfa5c0a6b 100644 --- a/src/platform/ESP32/DnssdImpl.cpp +++ b/src/platform/ESP32/DnssdImpl.cpp @@ -321,7 +321,7 @@ CHIP_ERROR OnBrowseDone(BrowseContext * ctx) return RemoveMdnsQuery(reinterpret_cast(ctx)); } -size_t GetExtraIPsSize(mdns_ip_addr_t * addr) +size_t GetAddressCount(mdns_ip_addr_t * addr) { size_t ret = 0; while (addr) @@ -334,9 +334,8 @@ size_t GetExtraIPsSize(mdns_ip_addr_t * addr) CHIP_ERROR OnResolveQuerySrvDone(ResolveContext * ctx) { - CHIP_ERROR error = CHIP_NO_ERROR; - mdns_ip_addr_t * extraAddr = nullptr; - size_t extraIPIndex = 0; + CHIP_ERROR error = CHIP_NO_ERROR; + size_t addressIndex = 0; VerifyOrExit(ctx && ctx->mResolveCb, error = CHIP_ERROR_INVALID_ARGUMENT); VerifyOrExit(ctx->mService == nullptr && ctx->mResolveState == ResolveContext::ResolveState::QuerySrv, @@ -359,33 +358,30 @@ CHIP_ERROR OnResolveQuerySrvDone(ResolveContext * ctx) if (ctx->mResult->addr) { - Inet::IPAddress IPAddr; - GetIPAddress(IPAddr, ctx->mResult->addr); - ctx->mService->mAddress.SetValue(IPAddr); - extraAddr = ctx->mResult->addr->next; - ctx->mExtraIPSize = GetExtraIPsSize(extraAddr); - if (ctx->mExtraIPSize > 0) + ctx->mAddressCount = GetAddressCount(ctx->mResult->addr); + if (ctx->mAddressCount > 0) { - ctx->mExtraIPs = - static_cast(chip::Platform::MemoryCalloc(ctx->mExtraIPSize, sizeof(Inet::IPAddress))); - if (ctx->mExtraIPs == nullptr) + ctx->mAddresses = + static_cast(chip::Platform::MemoryCalloc(ctx->mAddressCount, sizeof(Inet::IPAddress))); + if (ctx->mAddresses == nullptr) { - ChipLogError(DeviceLayer, "Failed to alloc memory for ExtraIPs"); - error = CHIP_ERROR_NO_MEMORY; - ctx->mExtraIPSize = 0; + ChipLogError(DeviceLayer, "Failed to alloc memory for addresses"); + error = CHIP_ERROR_NO_MEMORY; + ctx->mAddressCount = 0; ExitNow(); } - while (extraAddr) + auto * addr = ctx->mResult->addr; + while (addr) { - GetIPAddress(ctx->mExtraIPs[extraIPIndex], extraAddr); - extraIPIndex++; - extraAddr = extraAddr->next; + GetIPAddress(ctx->mAddressCount[addressIndex], addr); + addressIndex++; + addr = addr->next; } } else { - ctx->mExtraIPs = nullptr; - ctx->mExtraIPSize = 0; + ctx->mAddresses = nullptr; + ctx->mAddressCount = 0; } } } @@ -429,7 +425,7 @@ CHIP_ERROR OnResolveQueryTxtDone(ResolveContext * ctx) } else { - ctx->mResolveCb(ctx->mCbContext, ctx->mService, Span(ctx->mExtraIPs, ctx->mExtraIPSize), error); + ctx->mResolveCb(ctx->mCbContext, ctx->mService, Span(ctx->mAddresses, ctx->mAddressCount), error); } RemoveMdnsQuery(reinterpret_cast(ctx)); return error; diff --git a/src/platform/ESP32/DnssdImpl.h b/src/platform/ESP32/DnssdImpl.h index c5bcccfb44c47a..302e1ea8e4e19c 100644 --- a/src/platform/ESP32/DnssdImpl.h +++ b/src/platform/ESP32/DnssdImpl.h @@ -92,8 +92,8 @@ struct ResolveContext : public GenericContext char mInstanceName[Common::kInstanceNameMaxLength + 1]; DnssdResolveCallback mResolveCb; DnssdService * mService; - Inet::IPAddress * mExtraIPs; - size_t mExtraIPSize; + Inet::IPAddress * mAddresses; + size_t mAddressCount; enum class ResolveState { @@ -119,8 +119,8 @@ struct ResolveContext : public GenericContext mResolveState = ResolveState::QuerySrv; mResult = nullptr; mService = nullptr; - mExtraIPs = nullptr; - mExtraIPSize = 0; + mAddresses = nullptr; + mAddressCount = 0; } ~ResolveContext() @@ -133,9 +133,9 @@ struct ResolveContext : public GenericContext } chip::Platform::MemoryFree(mService); } - if (mExtraIPs) + if (mAddresses) { - chip::Platform::MemoryFree(mExtraIPs); + chip::Platform::MemoryFree(mAddresses); } if (mResult) { diff --git a/src/platform/Linux/DnssdImpl.cpp b/src/platform/Linux/DnssdImpl.cpp index 3f5b5258c51344..f98c83adb118be 100644 --- a/src/platform/Linux/DnssdImpl.cpp +++ b/src/platform/Linux/DnssdImpl.cpp @@ -729,7 +729,6 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte case AVAHI_RESOLVER_FOUND: DnssdService result = {}; - result.mAddress.SetValue(chip::Inet::IPAddress()); ChipLogError(DeviceLayer, "Avahi resolve found"); Platform::CopyString(result.mName, name); @@ -753,6 +752,7 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte } CHIP_ERROR result_err = CHIP_ERROR_INVALID_ADDRESS; + chip::Inet::IPAddress ipAddress; // Will be set of result_err is set to CHIP_NO_ERROR if (address) { switch (address->proto) @@ -762,7 +762,7 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte struct in_addr addr4; memcpy(&addr4, &(address->data.ipv4), sizeof(addr4)); - result.mAddress.SetValue(chip::Inet::IPAddress(addr4)); + ipAddress = chip::Inet::IPAddress(addr4); result_err = CHIP_NO_ERROR; #else ChipLogError(Discovery, "Ignoring IPv4 mDNS address."); @@ -772,7 +772,7 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte struct in6_addr addr6; memcpy(&addr6, &(address->data.ipv6), sizeof(addr6)); - result.mAddress.SetValue(chip::Inet::IPAddress(addr6)); + ipAddress = chip::Inet::IPAddress(addr6); result_err = CHIP_NO_ERROR; break; default: @@ -802,7 +802,7 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte if (result_err == CHIP_NO_ERROR) { - context->mCallback(context->mContext, &result, Span(), CHIP_NO_ERROR); + context->mCallback(context->mContext, &result, Span(&ipAddress, 1), CHIP_NO_ERROR); } else { diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index b7a22d1234be76..5d5ce4f5fe77cc 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -2391,8 +2391,9 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons template void GenericThreadStackManagerImpl_OpenThread::DispatchResolve(intptr_t context) { - auto * dnsResult = reinterpret_cast(context); - ThreadStackMgrImpl().mDnsResolveCallback(dnsResult->context, &(dnsResult->mMdnsService), Span(), + auto * dnsResult = reinterpret_cast(context); + Inet::IPAddress * ipAddress = &(dnsResult->mMdnsService.mAddress.Value()); + ThreadStackMgrImpl().mDnsResolveCallback(dnsResult->context, &(dnsResult->mMdnsService), Span(ipAddress, 1), dnsResult->error); Platform::Delete(dnsResult); } diff --git a/src/platform/Tizen/DnssdImpl.cpp b/src/platform/Tizen/DnssdImpl.cpp index 2540e0ec03bae7..8631a306287592 100644 --- a/src/platform/Tizen/DnssdImpl.cpp +++ b/src/platform/Tizen/DnssdImpl.cpp @@ -459,8 +459,7 @@ void OnResolve(dnssd_error_e result, dnssd_service_h service, void * data) if (validIP) { - mdnsService.mAddress.SetValue(ipStr); - rCtx->callback(rCtx->cbContext, &mdnsService, chip::Span(), CHIP_NO_ERROR); + rCtx->callback(rCtx->cbContext, &mdnsService, chip::Span(&ipStr, 1), CHIP_NO_ERROR); StopResolve(rCtx); } else diff --git a/src/platform/android/DnssdImpl.cpp b/src/platform/android/DnssdImpl.cpp index fa5a9d30e589f2..0fa4397d8f39ad 100644 --- a/src/platform/android/DnssdImpl.cpp +++ b/src/platform/android/DnssdImpl.cpp @@ -217,10 +217,12 @@ void HandleResolve(jstring instanceName, jstring serviceType, jstring address, j { VerifyOrReturn(callbackHandle != 0, ChipLogError(Discovery, "HandleResolve called with callback equal to nullptr")); - const auto dispatch = [callbackHandle, contextHandle](CHIP_ERROR error, DnssdService * service = nullptr) { + const auto dispatch = [callbackHandle, contextHandle](CHIP_ERROR error, DnssdService * service = nullptr, + Inet::IPAddress * address = nullptr) { DeviceLayer::StackLock lock; DnssdResolveCallback callback = reinterpret_cast(callbackHandle); - callback(reinterpret_cast(contextHandle), service, Span(), error); + size_t addr_count = (address == nullptr) ? 0 : 1; + callback(reinterpret_cast(contextHandle), service, Span(address, addr_count), error); }; VerifyOrReturn(address != nullptr && port != 0, dispatch(CHIP_ERROR_UNKNOWN_RESOURCE_ID)); @@ -239,10 +241,9 @@ void HandleResolve(jstring instanceName, jstring serviceType, jstring address, j DnssdService service = {}; CopyString(service.mName, jniInstanceName.c_str()); CopyString(service.mType, jniServiceType.c_str()); - service.mAddress.SetValue(ipAddress); service.mPort = static_cast(port); - dispatch(CHIP_NO_ERROR, &service); + dispatch(CHIP_NO_ERROR, &service, &ipAddress); } } // namespace Dnssd diff --git a/src/platform/tests/TestDnssd.cpp b/src/platform/tests/TestDnssd.cpp index 7d7a9dfba32f1a..b66d51439344bb 100644 --- a/src/platform/tests/TestDnssd.cpp +++ b/src/platform/tests/TestDnssd.cpp @@ -14,7 +14,7 @@ using chip::Dnssd::DnssdService; using chip::Dnssd::DnssdServiceProtocol; using chip::Dnssd::TextEntry; -static void HandleResolve(void * context, DnssdService * result, const chip::Span & extraIPs, +static void HandleResolve(void * context, DnssdService * result, const chip::Span & addresses, CHIP_ERROR error) { char addrBuf[100]; @@ -22,8 +22,11 @@ static void HandleResolve(void * context, DnssdService * result, const chip::Spa NL_TEST_ASSERT(suite, result != nullptr); NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR); - result->mAddress.Value().ToString(addrBuf, sizeof(addrBuf)); - printf("Service at [%s]:%u\n", addrBuf, result->mPort); + if (!addresses.empty()) + { + addresses.data()[0].ToString(addrBuf, sizeof(addrBuf)); + printf("Service at [%s]:%u\n", addrBuf, result->mPort); + } NL_TEST_ASSERT(suite, result->mTextEntrySize == 1); NL_TEST_ASSERT(suite, strcmp(result->mTextEntries[0].mKey, "key") == 0); NL_TEST_ASSERT(suite, strcmp(reinterpret_cast(result->mTextEntries[0].mData), "val") == 0);