From 86a6d62bdc3f41a034bee0ba85e93edc907392db Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 12 Apr 2023 12:50:48 -0400 Subject: [PATCH] Improve the Darwin logging when we cancel dns-sd browse/resolve operations. Right now we are treating those as "timeout" and logging that, which really confuses people reading the logs. We should log them as cancellations instead. There's no obvious kDNSServiceErr_* value for "cancelled", so allow doing GenericContext::Finalize with a CHIP_ERROR. --- src/platform/Darwin/DnssdContexts.cpp | 36 +++++++++++++++++---------- src/platform/Darwin/DnssdImpl.cpp | 14 +++++------ src/platform/Darwin/DnssdImpl.h | 15 +++++++---- 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/src/platform/Darwin/DnssdContexts.cpp b/src/platform/Darwin/DnssdContexts.cpp index ebc1079b3fb32b..832b33637d88fa 100644 --- a/src/platform/Darwin/DnssdContexts.cpp +++ b/src/platform/Darwin/DnssdContexts.cpp @@ -112,17 +112,17 @@ DNSServiceProtocol GetProtocol(const chip::Inet::IPAddressType & addressType) namespace chip { namespace Dnssd { -CHIP_ERROR GenericContext::Finalize(DNSServiceErrorType err) +CHIP_ERROR GenericContext::FinalizeInternal(const char * errorStr, CHIP_ERROR err) { if (MdnsContexts::GetInstance().Has(this) == CHIP_NO_ERROR) { - if (kDNSServiceErr_NoError == err) + if (CHIP_NO_ERROR == err) { DispatchSuccess(); } else { - DispatchFailure(err); + DispatchFailure(errorStr, err); } } else @@ -130,7 +130,17 @@ CHIP_ERROR GenericContext::Finalize(DNSServiceErrorType err) chip::Platform::Delete(this); } - return Error::ToChipError(err); + return err; +} + +CHIP_ERROR GenericContext::Finalize(CHIP_ERROR err) +{ + return FinalizeInternal(err.AsString(), err); +} + +CHIP_ERROR GenericContext::Finalize(DNSServiceErrorType err) +{ + return FinalizeInternal(Error::ToString(err), Error::ToChipError(err)); } MdnsContexts::~MdnsContexts() @@ -289,10 +299,10 @@ RegisterContext::RegisterContext(const char * sType, const char * instanceName, mInstanceName = instanceName; } -void RegisterContext::DispatchFailure(DNSServiceErrorType err) +void RegisterContext::DispatchFailure(const char * errorStr, CHIP_ERROR err) { - ChipLogError(Discovery, "Mdns: Register failure (%s)", Error::ToString(err)); - callback(context, nullptr, nullptr, Error::ToChipError(err)); + ChipLogError(Discovery, "Mdns: Register failure (%s)", errorStr); + callback(context, nullptr, nullptr, err); MdnsContexts::GetInstance().Remove(this); } @@ -316,10 +326,10 @@ BrowseContext::BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServ protocol = cbContextProtocol; } -void BrowseContext::DispatchFailure(DNSServiceErrorType err) +void BrowseContext::DispatchFailure(const char * errorStr, CHIP_ERROR err) { - ChipLogError(Discovery, "Mdns: Browse failure (%s)", Error::ToString(err)); - callback(context, nullptr, 0, true, Error::ToChipError(err)); + ChipLogError(Discovery, "Mdns: Browse failure (%s)", errorStr); + callback(context, nullptr, 0, true, err); MdnsContexts::GetInstance().Remove(this); } @@ -353,14 +363,14 @@ ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip:: ResolveContext::~ResolveContext() {} -void ResolveContext::DispatchFailure(DNSServiceErrorType err) +void ResolveContext::DispatchFailure(const char * errorStr, CHIP_ERROR err) { - ChipLogError(Discovery, "Mdns: Resolve failure (%s)", Error::ToString(err)); + ChipLogError(Discovery, "Mdns: Resolve failure (%s)", errorStr); // Remove before dispatching, so calls back into // ChipDnssdResolveNoLongerNeeded don't find us and try to also remove us. bool needDelete = MdnsContexts::GetInstance().RemoveWithoutDeleting(this); - callback(context, nullptr, Span(), Error::ToChipError(err)); + callback(context, nullptr, Span(), err); if (needDelete) { diff --git a/src/platform/Darwin/DnssdImpl.cpp b/src/platform/Darwin/DnssdImpl.cpp index d859f2b8d1391d..acf65e09511534 100644 --- a/src/platform/Darwin/DnssdImpl.cpp +++ b/src/platform/Darwin/DnssdImpl.cpp @@ -465,11 +465,11 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier) return CHIP_ERROR_NOT_FOUND; } - // Just treat this as a timeout error. Don't bother delivering the partial + // We have been canceled. Don't bother delivering the partial // results we have queued up in the BrowseContext, if any. In practice // there shouldn't be anything there long-term anyway. // - // Make sure to time out all the resolves first, before we time out the + // Make sure to cancel all the resolves first, before we cancel the // browse (just to avoid dangling pointers in the resolves, even though we // only use them for equality compares). std::vector resolves; @@ -481,10 +481,10 @@ CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier) for (auto & resolve : resolves) { - resolve->Finalize(kDNSServiceErr_Timeout); + resolve->Finalize(CHIP_ERROR_CANCELLED); } - ctx->Finalize(kDNSServiceErr_Timeout); + ctx->Finalize(CHIP_ERROR_CANCELLED); return CHIP_NO_ERROR; } @@ -511,11 +511,11 @@ void ChipDnssdResolveNoLongerNeeded(const char * instanceName) if (*existingCtx->consumerCounter == 0) { // No more consumers; clear out all of these resolves so they don't - // stick around. Dispatch timeout failure on all of them to make sure - // whatever kicked them off cleans up resources as needed. + // stick around. Dispatch a "cancelled" failure on all of them to make + // sure whatever kicked them off cleans up resources as needed. do { - existingCtx->Finalize(kDNSServiceErr_Timeout); + existingCtx->Finalize(CHIP_ERROR_CANCELLED); existingCtx = MdnsContexts::GetInstance().GetExistingResolveForInstanceName(instanceName); } while (existingCtx != nullptr); } diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index 42f5d246ecb6fc..d599b052cdb1f6 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -44,9 +44,14 @@ struct GenericContext virtual ~GenericContext() {} + CHIP_ERROR Finalize(CHIP_ERROR err); CHIP_ERROR Finalize(DNSServiceErrorType err = kDNSServiceErr_NoError); - virtual void DispatchFailure(DNSServiceErrorType err) = 0; - virtual void DispatchSuccess() = 0; + + virtual void DispatchFailure(const char * errorStr, CHIP_ERROR err) = 0; + virtual void DispatchSuccess() = 0; + +private: + CHIP_ERROR FinalizeInternal(const char * errorStr, CHIP_ERROR err); }; struct RegisterContext; @@ -130,7 +135,7 @@ struct RegisterContext : public GenericContext RegisterContext(const char * sType, const char * instanceName, DnssdPublishCallback cb, void * cbContext); virtual ~RegisterContext() { mHostNameRegistrar.Unregister(); } - void DispatchFailure(DNSServiceErrorType err) override; + void DispatchFailure(const char * errorStr, CHIP_ERROR err) override; void DispatchSuccess() override; bool matches(const char * sType) { return mType.compare(sType) == 0; } @@ -145,7 +150,7 @@ struct BrowseContext : public GenericContext BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServiceProtocol cbContextProtocol); virtual ~BrowseContext() {} - void DispatchFailure(DNSServiceErrorType err) override; + void DispatchFailure(const char * errorStr, CHIP_ERROR err) override; void DispatchSuccess() override; // Dispatch what we have found so far, but don't stop browsing. @@ -195,7 +200,7 @@ struct ResolveContext : public GenericContext std::shared_ptr && consumerCounterToUse); virtual ~ResolveContext(); - void DispatchFailure(DNSServiceErrorType err) override; + void DispatchFailure(const char * errorStr, CHIP_ERROR err) override; void DispatchSuccess() override; CHIP_ERROR OnNewAddress(uint32_t interfaceId, const struct sockaddr * address);