From 4d013e65f0bf5a99fcb118b7721ddf34320defa0 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 16 Jun 2023 15:19:38 -0400 Subject: [PATCH] MinMds - remove duplicate broadcasts at startup time (remove IP address looping) (#27268) * Reduce the boot time advertisement of addresses. MinMDNS advertises once for every IP address, but this makes no sense as a multicast is per interface not per external IP. Ensure we only broadcast once per interface. * Remove unused method * Update src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp Co-authored-by: Boris Zbarsky * Make loopback a method in IPAddress as it seems convenient and does not require string parsing * Clean up Broadcast IP address get now that we cleaned up loopback * Restyled by clang-format * VerifyOrDie if broadcast addresses are wrong --------- Co-authored-by: Andrei Litvin Co-authored-by: Boris Zbarsky Co-authored-by: Restyled.io --- src/inet/IPAddress.cpp | 23 ++++ src/inet/IPAddress.h | 8 ++ src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 130 +++++++------------ src/lib/dnssd/minimal_mdns/Server.cpp | 46 ++----- src/lib/dnssd/minimal_mdns/Server.h | 16 +-- 5 files changed, 90 insertions(+), 133 deletions(-) diff --git a/src/inet/IPAddress.cpp b/src/inet/IPAddress.cpp index 19da2acaee94f7..3546df5acb0657 100644 --- a/src/inet/IPAddress.cpp +++ b/src/inet/IPAddress.cpp @@ -508,5 +508,28 @@ IPAddress IPAddress::MakeIPv4Broadcast() return ipAddr; } +IPAddress IPAddress::Loopback(IPAddressType type) +{ + IPAddress address; +#if INET_CONFIG_ENABLE_IPV4 + if (type == IPAddressType::kIPv4) + { + address.Addr[0] = 0; + address.Addr[1] = 0; + address.Addr[2] = htonl(0xFFFF); + address.Addr[3] = htonl(0x7F000001); + } + else +#endif + { + address.Addr[0] = 0; + address.Addr[1] = 0; + address.Addr[2] = 0; + address.Addr[3] = htonl(1); + } + + return address; +} + } // namespace Inet } // namespace chip diff --git a/src/inet/IPAddress.h b/src/inet/IPAddress.h index c97f25de752d44..2b756ef5561b99 100644 --- a/src/inet/IPAddress.h +++ b/src/inet/IPAddress.h @@ -698,6 +698,14 @@ class DLL_EXPORT IPAddress * not be modified by users of the CHIP Inet Layer. */ static IPAddress Any; + + /** + * Creates a loopback of the specified type. Type MUST be IPv6/v4. + * + * If type is anything else (or IPv4 is not available) an IPv6 + * loopback will be created. + */ + static IPAddress Loopback(IPAddressType type); }; static_assert(std::is_trivial::value, "IPAddress is not trivial"); diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp index 2c9c021906cecf..33f037587858b5 100644 --- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp @@ -206,10 +206,6 @@ class AdvertiserMinMdns : public ServiceAdvertiser, /// removes all records by advertising a 0 TTL) void AdvertiseRecords(BroadcastAdvertiseType type); - /// Determine if advertisement on the specified interface/address is ok given the - /// interfaces on which the mDNS server is listening - bool ShouldAdvertiseOn(const chip::Inet::InterfaceId id, const chip::Inet::IPAddress & addr); - FullQName GetCommissioningTxtEntries(const CommissionAdvertisingParameters & params); FullQName GetOperationalTxtEntries(OperationalQueryAllocator::Allocator * allocator, const OperationalAdvertisingParameters & params); @@ -856,35 +852,6 @@ FullQName AdvertiserMinMdns::GetCommissioningTxtEntries(const CommissionAdvertis return allocator->AllocateQNameFromArray(txtFields, numTxtFields); } -bool AdvertiserMinMdns::ShouldAdvertiseOn(const chip::Inet::InterfaceId id, const chip::Inet::IPAddress & addr) -{ - auto & server = GlobalMinimalMdnsServer::Server(); - - bool result = false; - - server.ForEachEndPoints([&](auto * info) { - if (info->mListenUdp == nullptr) - { - return chip::Loop::Continue; - } - - if (info->mInterfaceId != id) - { - return chip::Loop::Continue; - } - - if (info->mAddressType != addr.Type()) - { - return chip::Loop::Continue; - } - - result = true; - return chip::Loop::Break; - }); - - return result; -} - void AdvertiserMinMdns::AdvertiseRecords(BroadcastAdvertiseType type) { ResponseConfiguration responseConfiguration; @@ -905,60 +872,53 @@ void AdvertiserMinMdns::AdvertiseRecords(BroadcastAdvertiseType type) UniquePtr allIps = GetAddressPolicy()->GetIpAddressesForEndpoint(interfaceId, addressType); VerifyOrDieWithMsg(allIps != nullptr, Discovery, "Failed to allocate memory for ip addresses."); - Inet::IPAddress ipAddress; - while (allIps->Next(ipAddress)) + chip::Inet::IPPacketInfo packetInfo; + + packetInfo.Clear(); + + // advertising on every interface requires a valid IP address + // since we use "BROADCAST" (unicast is false), we do not actually care about + // the source IP address value, just that it has the right "type" + // + // NOTE: cannot use Broadcast address as the source as they have the type kAny. + // + // TODO: ideally we may want to have a destination that is explicit as "unicast/destIp" + // vs "multicast/addressType". Such a change requires larger code updates. + packetInfo.SrcAddress = chip::Inet::IPAddress::Loopback(addressType); + packetInfo.DestAddress = BroadcastIpAddresses::Get(addressType); + packetInfo.SrcPort = kMdnsPort; + packetInfo.DestPort = kMdnsPort; + packetInfo.Interface = interfaceId; + + // Advertise all records + // + // TODO: Consider advertising delta changes. + // + // Current advertisement does not have a concept of "delta" to only + // advertise changes. Current implementation is to always + // 1. advertise TTL=0 (clear all caches) + // 2. advertise available records (with longer TTL) + // + // It would be nice if we could selectively advertise what changes, like + // send TTL=0 for anything removed/about to be removed (and only those), + // then only advertise new items added. + // + // This optimization likely will take more logic and state storage, so + // for now it is not done. + QueryData queryData(QType::PTR, QClass::IN, false /* unicast */); + queryData.SetIsInternalBroadcast(true); + + for (auto & it : mOperationalResponders) { - if (!ShouldAdvertiseOn(interfaceId, ipAddress)) - { - continue; - } - - chip::Inet::IPPacketInfo packetInfo; - - packetInfo.Clear(); - packetInfo.SrcAddress = ipAddress; - if (ipAddress.IsIPv4()) - { - BroadcastIpAddresses::GetIpv4Into(packetInfo.DestAddress); - } - else - { - BroadcastIpAddresses::GetIpv6Into(packetInfo.DestAddress); - } - packetInfo.SrcPort = kMdnsPort; - packetInfo.DestPort = kMdnsPort; - packetInfo.Interface = interfaceId; - - // Advertise all records - // - // TODO: Consider advertising delta changes. - // - // Current advertisement does not have a concept of "delta" to only - // advertise changes. Current implementation is to always - // 1. advertise TTL=0 (clear all caches) - // 2. advertise available records (with longer TTL) - // - // It would be nice if we could selectively advertise what changes, like - // send TTL=0 for anything removed/about to be removed (and only those), - // then only advertise new items added. - // - // This optimization likely will take more logic and state storage, so - // for now it is not done. - QueryData queryData(QType::PTR, QClass::IN, false /* unicast */); - queryData.SetIsInternalBroadcast(true); - - for (auto & it : mOperationalResponders) - { - it.GetAllocator()->GetQueryResponder()->ClearBroadcastThrottle(); - } - mQueryResponderAllocatorCommissionable.GetQueryResponder()->ClearBroadcastThrottle(); - mQueryResponderAllocatorCommissioner.GetQueryResponder()->ClearBroadcastThrottle(); + it.GetAllocator()->GetQueryResponder()->ClearBroadcastThrottle(); + } + mQueryResponderAllocatorCommissionable.GetQueryResponder()->ClearBroadcastThrottle(); + mQueryResponderAllocatorCommissioner.GetQueryResponder()->ClearBroadcastThrottle(); - CHIP_ERROR err = mResponseSender.Respond(0, queryData, &packetInfo, responseConfiguration); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to advertise records: %" CHIP_ERROR_FORMAT, err.Format()); - } + CHIP_ERROR err = mResponseSender.Respond(0, queryData, &packetInfo, responseConfiguration); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Discovery, "Failed to advertise records: %" CHIP_ERROR_FORMAT, err.Format()); } } diff --git a/src/lib/dnssd/minimal_mdns/Server.cpp b/src/lib/dnssd/minimal_mdns/Server.cpp index ccf041eb605d7c..f8a0971f5710e5 100644 --- a/src/lib/dnssd/minimal_mdns/Server.cpp +++ b/src/lib/dnssd/minimal_mdns/Server.cpp @@ -121,51 +121,26 @@ class InterfaceTypeFilterDelegate : public ServerBase::BroadcastSendDelegate namespace BroadcastIpAddresses { // Get standard mDNS Broadcast addresses - -void GetIpv6Into(chip::Inet::IPAddress & dest) +chip::Inet::IPAddress Get(chip::Inet::IPAddressType addressType) { - if (!chip::Inet::IPAddress::FromString("FF02::FB", dest)) + chip::Inet::IPAddress address; +#if INET_CONFIG_ENABLE_IPV4 + if (addressType == chip::Inet::IPAddressType::kIPv4) { - ChipLogError(Discovery, "Failed to parse standard IPv6 broadcast address"); + VerifyOrDie(chip::Inet::IPAddress::FromString("224.0.0.251", address)); } -} - -void GetIpv4Into(chip::Inet::IPAddress & dest) -{ - if (!chip::Inet::IPAddress::FromString("224.0.0.251", dest)) + else +#endif { - ChipLogError(Discovery, "Failed to parse standard IPv4 broadcast address"); + VerifyOrDie(chip::Inet::IPAddress::FromString("FF02::FB", address)); } + return address; } } // namespace BroadcastIpAddresses namespace { -CHIP_ERROR JoinMulticastGroup(chip::Inet::InterfaceId interfaceId, chip::Inet::UDPEndPoint * endpoint, - chip::Inet::IPAddressType addressType) -{ - - chip::Inet::IPAddress address; - - if (addressType == chip::Inet::IPAddressType::kIPv6) - { - BroadcastIpAddresses::GetIpv6Into(address); -#if INET_CONFIG_ENABLE_IPV4 - } - else if (addressType == chip::Inet::IPAddressType::kIPv4) - { - BroadcastIpAddresses::GetIpv4Into(address); -#endif // INET_CONFIG_ENABLE_IPV4 - } - else - { - return CHIP_ERROR_INVALID_ARGUMENT; - } - - return endpoint->JoinMulticastGroup(interfaceId, address); -} - #if CHIP_ERROR_LOGGING const char * AddressTypeStr(chip::Inet::IPAddressType addressType) { @@ -240,7 +215,8 @@ CHIP_ERROR ServerBase::Listen(chip::Inet::EndPointManagerListen(OnUdpPacketReceived, nullptr /*OnReceiveError*/, this)); - CHIP_ERROR err = JoinMulticastGroup(interfaceId, listenUdp, addressType); + CHIP_ERROR err = listenUdp->JoinMulticastGroup(interfaceId, BroadcastIpAddresses::Get(addressType)); + if (err != CHIP_NO_ERROR) { char interfaceName[chip::Inet::InterfaceId::kMaxIfNameLength]; diff --git a/src/lib/dnssd/minimal_mdns/Server.h b/src/lib/dnssd/minimal_mdns/Server.h index 150a5e44dc8e69..39e9ff2a06b345 100644 --- a/src/lib/dnssd/minimal_mdns/Server.h +++ b/src/lib/dnssd/minimal_mdns/Server.h @@ -32,9 +32,7 @@ namespace Minimal { namespace BroadcastIpAddresses { // Get standard mDNS Broadcast addresses - -void GetIpv6Into(chip::Inet::IPAddress & dest); -void GetIpv4Into(chip::Inet::IPAddress & dest); +chip::Inet::IPAddress Get(chip::Inet::IPAddressType addressType); } // namespace BroadcastIpAddresses @@ -130,10 +128,9 @@ class ServerBase ServerBase(EndpointInfoPoolType & pool) : mEndpoints(pool) { - BroadcastIpAddresses::GetIpv6Into(mIpv6BroadcastAddress); - + mIpv6BroadcastAddress = BroadcastIpAddresses::Get(chip::Inet::IPAddressType::kIPv6); #if INET_CONFIG_ENABLE_IPV4 - BroadcastIpAddresses::GetIpv4Into(mIpv4BroadcastAddress); + mIpv4BroadcastAddress = BroadcastIpAddresses::Get(chip::Inet::IPAddressType::kIPv4); #endif } virtual ~ServerBase(); @@ -178,13 +175,6 @@ class ServerBase return *this; } - /// Iterator through all Endpoints - template - chip::Loop ForEachEndPoints(Function && function) - { - return mEndpoints.ForEachActiveObject(std::forward(function)); - } - /// A server is considered listening if any UDP endpoint is active. /// /// This is expected to return false after any Shutdown() and will