Skip to content

Commit

Permalink
MinMds - remove duplicate broadcasts at startup time (remove IP addre…
Browse files Browse the repository at this point in the history 
…ss 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@andy31415 @andreilitvin
@bzbarsky-apple @restyled-commits @pull
4 people authored and pull[bot] committed Sep 25, 2023
1 parent 87c7f70 commit 3453192
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 133 deletions.
23 changes: 23 additions & 0 deletions src/inet/IPAddress.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
8 changes: 8 additions & 0 deletions src/inet/IPAddress.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<IPAddress>::value, "IPAddress is not trivial");
Expand Down
130 changes: 45 additions & 85 deletions src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -905,60 +872,53 @@ void AdvertiserMinMdns::AdvertiseRecords(BroadcastAdvertiseType type)
UniquePtr<IpAddressIterator> 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());
}
}

Expand Down
46 changes: 11 additions & 35 deletions src/lib/dnssd/minimal_mdns/Server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -240,7 +215,8 @@ CHIP_ERROR ServerBase::Listen(chip::Inet::EndPointManager<chip::Inet::UDPEndPoin

ReturnErrorOnFailure(listenUdp->Listen(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];
Expand Down
16 changes: 3 additions & 13 deletions src/lib/dnssd/minimal_mdns/Server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -178,13 +175,6 @@ class ServerBase
return *this;
}

/// Iterator through all Endpoints
template <typename Function>
chip::Loop ForEachEndPoints(Function && function)
{
return mEndpoints.ForEachActiveObject(std::forward<Function>(function));
}

/// A server is considered listening if any UDP endpoint is active.
///
/// This is expected to return false after any Shutdown() and will
Expand Down

0 comments on commit 3453192

Please sign in to comment.