[minimal mDNS] Fix sending mDNS goodbye packets (#23661)
* [minimal mDNS] Fix sending mDNS goodbye packets
mDNS clients such as avahi would keep stale commissionable
node services in their caches because minimal mDNS did not
send "Goodbye" packets properly on service removal. That is,
it attempted to send records with TTL=0 in Advertise()
methods, called after RemoveServices() which clears all
allocated responders.
It could be reproduce by opening and closing the
commissioning window on the device because each open
operation regenerates commissionable node service instance
name.
Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
* Code review
Signed-off-by: Damian Krolik <damian.krolik@nordicsemi.no>
diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
index 1997518..52a8be7 100644
--- a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
+++ b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -177,7 +177,7 @@
ChipLogError(Discovery, "Failed to set up commissioner responder: %" CHIP_ERROR_FORMAT, err.Format());
}
}
- ~AdvertiserMinMdns() override { RemoveServices(); }
+ ~AdvertiserMinMdns() override { ClearServices(); }
// Service advertiser
CHIP_ERROR Init(chip::Inet::EndPointManager<chip::Inet::UDPEndPoint> * udpEndPointManager) override;
@@ -281,6 +281,8 @@
OperationalQueryAllocator::Allocator * FindOperationalAllocator(const FullQName & qname);
OperationalQueryAllocator::Allocator * FindEmptyOperationalAllocator();
+ void ClearServices();
+
ResponseSender mResponseSender;
uint8_t mCommissionableInstanceName[sizeof(uint64_t)];
@@ -367,6 +369,18 @@
CHIP_ERROR AdvertiserMinMdns::RemoveServices()
{
+ // Send a "goodbye" packet for each RR being removed, as defined in RFC 6762.
+ // This allows mDNS clients to remove stale cached records which may not be re-added with
+ // subsequent Advertise() calls. In the case the same records are re-added, this extra
+ // is not harmful though suboptimal, so this is a subject to improvement in the future.
+ AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);
+ ClearServices();
+
+ return CHIP_NO_ERROR;
+}
+
+void AdvertiserMinMdns::ClearServices()
+{
while (mOperationalResponders.begin() != mOperationalResponders.end())
{
auto it = mOperationalResponders.begin();
@@ -391,7 +405,6 @@
mQueryResponderAllocatorCommissionable.Clear();
mQueryResponderAllocatorCommissioner.Clear();
- return CHIP_NO_ERROR;
}
OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindOperationalAllocator(const FullQName & qname)
@@ -431,17 +444,11 @@
CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters & params)
{
-
char nameBuffer[Operational::kInstanceNameMaxLength + 1] = "";
// need to set server name
ReturnErrorOnFailure(MakeInstanceName(nameBuffer, sizeof(nameBuffer), params.GetPeerId()));
- // Advertising data changed. Send a TTL=0 for everything as a refresh,
- // which will clear caches (including things we are about to remove). Once this is done
- // we will re-advertise available records with a longer TTL again.
- AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);
-
QNamePart nameCheckParts[] = { nameBuffer, kOperationalServiceName, kOperationalProtocol, kLocalDomain };
FullQName nameCheck = FullQName(nameCheckParts);
auto * operationalAllocator = FindOperationalAllocator(nameCheck);
@@ -558,11 +565,6 @@
CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters & params)
{
- // Advertising data changed. Send a TTL=0 for everything as a refresh,
- // which will clear caches (including things we are about to remove). Once this is done
- // we will re-advertise available records with a longer TTL again.
- AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);
-
if (params.GetCommissionAdvertiseMode() == CommssionAdvertiseMode::kCommissionableNode)
{
mQueryResponderAllocatorCommissionable.Clear();
