Fix Darwin dns-sd handling of results on interface index 0. (#32872)
https://github.com/project-chip/connectedhomeip/pull/32513 introduced a bug, in
that we would ignore all results associated with interface index 0.
But that's perfectly valid for dns-sd results in general. What we want to
ignore are cases where if_nametoindex returned 0, since those correspond to "no
such interface name".
The fix is to move "interfaceIndex == 0" check to the right spot.
The rest of the changes are just there to improve logging. Specifically, log
what's going on with our SRP resolve timer, and include the instance name being
resolved in various logs so we can tell which logs have to do with which node
being resolved.
diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp
index dfdcaa5..27a9d81 100644
--- a/src/platform/Darwin/DnssdContexts.cpp
+++ b/src/platform/Darwin/DnssdContexts.cpp
@@ -526,6 +526,26 @@
// ChipDnssdResolveNoLongerNeeded don't find us and try to also remove us.
bool needDelete = MdnsContexts::GetInstance().RemoveWithoutDeleting(this);
+ class AutoSelfDeleter
+ {
+ public:
+ AutoSelfDeleter(bool needDelete, ResolveContext * self) : mNeedDelete(needDelete), mSelf(self) {}
+
+ ~AutoSelfDeleter()
+ {
+ if (mNeedDelete)
+ {
+ MdnsContexts::GetInstance().Delete(mSelf);
+ }
+ }
+
+ private:
+ bool mNeedDelete;
+ ResolveContext * mSelf;
+ };
+
+ AutoSelfDeleter selfDeleter(needDelete, this);
+
#if TARGET_OS_TV
// On tvOS, prioritize results from en0, en1, ir0 in that order, if those
// interfaces are present, since those will generally have more up-to-date
@@ -544,12 +564,15 @@
for (auto interfaceIndex : priorityInterfaceIndices)
{
+ if (interfaceIndex == 0)
+ {
+ // Not actually an interface we have, since if_nametoindex
+ // returned 0.
+ continue;
+ }
+
if (TryReportingResultsForInterfaceIndex(static_cast<uint32_t>(interfaceIndex)))
{
- if (needDelete)
- {
- MdnsContexts::GetInstance().Delete(this);
- }
return;
}
}
@@ -559,24 +582,16 @@
if (TryReportingResultsForInterfaceIndex(interface.first.interfaceId, interface.first.hostname,
interface.first.isSRPResult))
{
- break;
+ return;
}
}
- if (needDelete)
- {
- MdnsContexts::GetInstance().Delete(this);
- }
+ ChipLogError(Discovery, "Successfully finalizing resolve for %s without finding any actual IP addresses.",
+ instanceName.c_str());
}
bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceIndex, const std::string & hostname, bool isSRPResult)
{
- if (interfaceIndex == 0)
- {
- // Not actually an interface we have.
- return false;
- }
-
InterfaceKey interfaceKey = { interfaceIndex, hostname, isSRPResult };
auto & interface = interfaces[interfaceKey];
auto & ips = interface.addresses;
@@ -626,12 +641,16 @@
{
auto sdCtx = static_cast<ResolveContext *>(callbackContext);
VerifyOrDie(sdCtx != nullptr);
+ sdCtx->isSRPTimerRunning = false;
+
+ ChipLogProgress(Discovery, "SRP resolve timer for %s expired; completing resolve", sdCtx->instanceName.c_str());
sdCtx->Finalize();
}
void ResolveContext::CancelSRPTimer()
{
DeviceLayer::SystemLayer().CancelTimer(SRPTimerExpiredCallback, static_cast<void *>(this));
+ ChipLogProgress(Discovery, "SRP resolve timer for %s cancelled; resolve timed out", instanceName.c_str());
}
CHIP_ERROR ResolveContext::OnNewAddress(const InterfaceKey & interfaceKey, const struct sockaddr * address)
@@ -655,7 +674,8 @@
#ifdef CHIP_PROGRESS_LOGGING
char addrStr[INET6_ADDRSTRLEN];
ip.ToString(addrStr, sizeof(addrStr));
- ChipLogProgress(Discovery, "Mdns: %s interface: %" PRIu32 " ip:%s", __func__, interfaceId, addrStr);
+ ChipLogProgress(Discovery, "Mdns: %s instance: %s interface: %" PRIu32 " ip: %s", __func__, instanceName.c_str(), interfaceId,
+ addrStr);
#endif // CHIP_PROGRESS_LOGGING
if (ip.IsIPv6LinkLocal() && interfaceId == kDNSServiceInterfaceIndexLocalOnly)
diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp
index 13f369b..e9314f7 100644
--- a/src/platform/Darwin/DnssdImpl.cpp
+++ b/src/platform/Darwin/DnssdImpl.cpp
@@ -77,6 +77,7 @@
CHIP_ERROR StartSRPTimer(uint16_t timeoutInMSecs, ResolveContext * ctx)
{
VerifyOrReturnValue(ctx != nullptr, CHIP_ERROR_INCORRECT_STATE);
+ ChipLogProgress(Discovery, "Starting timer to wait for possible SRP resolve results for %s", ctx->instanceName.c_str());
return chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Milliseconds16(timeoutInMSecs),
ResolveContext::SRPTimerExpiredCallback, static_cast<void *>(ctx));
}
@@ -280,6 +281,7 @@
// we are done. Otherwise start the timer to give the resolve on SRP domain some extra time to complete.
if (!sdCtx->shouldStartSRPTimerForResolve)
{
+ ChipLogDetail(Discovery, "No need to start SRP resolve timer for %s; completing resolve", sdCtx->instanceName.c_str());
sdCtx->Finalize();
}
else
@@ -363,8 +365,8 @@
static CHIP_ERROR Resolve(ResolveContext * sdCtx, uint32_t interfaceId, chip::Inet::IPAddressType addressType, const char * type,
const char * name, const char * domain)
{
- ChipLogProgress(Discovery, "Resolve type=%s name=%s interface=%" PRIu32, StringOrNullMarker(type), StringOrNullMarker(name),
- interfaceId);
+ ChipLogProgress(Discovery, "Resolve type=%s name=%s domain=%s interface=%" PRIu32, StringOrNullMarker(type),
+ StringOrNullMarker(name), StringOrNullMarker(domain), interfaceId);
auto err = DNSServiceCreateConnection(&sdCtx->serviceRef);
VerifyOrReturnError(kDNSServiceErr_NoError == err, sdCtx->Finalize(err));
