From 1d37eb09b0fea55efdfadcb8ad6bca575312d122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Thu, 23 Nov 2023 21:07:00 +0100 Subject: [PATCH] [dns-sd] Untangle dependencies between Resolver and ResolverProxy (#30563) * [dns-sd] Untangle dependencies between Resolver and ResolverProxy The relationship between Resolver and Resolver Proxy is unclear as both classes depend on each other and Resolver Proxy is implemented in a source file of concrete Resolver implementation. This makes it impossible to build Matter library with several Resolver implementations and select one to be used at runtime. The main reason for introducing Resolver Proxy was the need for running multiple parallel DNS-SD queries. Address this by adding Discovery Delegate parameter to the Resolver interface methods for starting and stopping the discovery. Then implement Resolver Proxy as a convenience class on top of the Resolver interface. Additionally, simplify the Commissionable Node Controller class by injecting a test Resolver into Resolver Proxy. * Fix shell commands * Rename DiscoveryDelegate to DiscoveryContext * Add comments --- src/app/CASESessionManager.h | 2 - .../commands/discovery/DiscoveryCommands.cpp | 2 - .../commands/discovery/DiscoveryCommands.h | 6 +- .../AbstractDnssdDiscoveryController.h | 3 +- .../CHIPCommissionableNodeController.cpp | 26 +-- .../CHIPCommissionableNodeController.h | 5 +- .../TestCommissionableNodeController.cpp | 7 +- src/lib/dnssd/BUILD.gn | 2 + src/lib/dnssd/Discovery_ImplPlatform.cpp | 199 ++++++++---------- src/lib/dnssd/Discovery_ImplPlatform.h | 12 +- src/lib/dnssd/Resolver.h | 65 ++++-- src/lib/dnssd/ResolverProxy.cpp | 66 ++++++ src/lib/dnssd/ResolverProxy.h | 116 +++------- src/lib/dnssd/Resolver_ImplMinimalMdns.cpp | 81 ++++--- src/lib/dnssd/Resolver_ImplNone.cpp | 36 +--- src/lib/shell/commands/Dns.cpp | 20 +- 16 files changed, 295 insertions(+), 353 deletions(-) create mode 100644 src/lib/dnssd/ResolverProxy.cpp diff --git a/src/app/CASESessionManager.h b/src/app/CASESessionManager.h index 8b803a0a10f0a2..7094c88f42e104 100644 --- a/src/app/CASESessionManager.h +++ b/src/app/CASESessionManager.h @@ -28,8 +28,6 @@ #include #include -#include - namespace chip { class OperationalSessionSetupPoolDelegate; diff --git a/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp b/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp index 5c7f3fc9f318d3..2313480351d803 100644 --- a/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp +++ b/src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp @@ -117,7 +117,6 @@ CHIP_ERROR DiscoveryCommands::SetupDiscoveryCommands() ReturnErrorOnFailure(mDNSResolver.Init(chip::DeviceLayer::UDPEndPointManager())); mReady = true; } - mDNSResolver.SetOperationalDelegate(this); mDNSResolver.SetCommissioningDelegate(this); return CHIP_NO_ERROR; } @@ -125,7 +124,6 @@ CHIP_ERROR DiscoveryCommands::SetupDiscoveryCommands() CHIP_ERROR DiscoveryCommands::TearDownDiscoveryCommands() { mDNSResolver.StopDiscovery(); - mDNSResolver.SetOperationalDelegate(nullptr); mDNSResolver.SetCommissioningDelegate(nullptr); return CHIP_NO_ERROR; } diff --git a/src/app/tests/suites/commands/discovery/DiscoveryCommands.h b/src/app/tests/suites/commands/discovery/DiscoveryCommands.h index 37fd9b4f8bf376..8425ef4ff1be81 100644 --- a/src/app/tests/suites/commands/discovery/DiscoveryCommands.h +++ b/src/app/tests/suites/commands/discovery/DiscoveryCommands.h @@ -24,7 +24,7 @@ #include -class DiscoveryCommands : public chip::Dnssd::CommissioningResolveDelegate, public chip::Dnssd::OperationalResolveDelegate +class DiscoveryCommands : public chip::Dnssd::CommissioningResolveDelegate { public: DiscoveryCommands(){}; @@ -64,10 +64,6 @@ class DiscoveryCommands : public chip::Dnssd::CommissioningResolveDelegate, publ /////////// CommissioningDelegate Interface ///////// void OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & nodeData) override; - /////////// OperationalDelegate Interface ///////// - void OnOperationalNodeResolved(const chip::Dnssd::ResolvedNodeData & nodeData) override{}; - void OnOperationalNodeResolutionFailed(const chip::PeerId & peerId, CHIP_ERROR error) override{}; - private: bool mReady = false; chip::Dnssd::ResolverProxy mDNSResolver; diff --git a/src/controller/AbstractDnssdDiscoveryController.h b/src/controller/AbstractDnssdDiscoveryController.h index 788102282bd86d..6e50d7b40c89ca 100644 --- a/src/controller/AbstractDnssdDiscoveryController.h +++ b/src/controller/AbstractDnssdDiscoveryController.h @@ -39,8 +39,7 @@ namespace Controller { class DLL_EXPORT AbstractDnssdDiscoveryController : public Dnssd::CommissioningResolveDelegate { public: - AbstractDnssdDiscoveryController() {} - ~AbstractDnssdDiscoveryController() override { mDNSResolver.Shutdown(); } + explicit AbstractDnssdDiscoveryController(Dnssd::Resolver * resolver = nullptr) : mDNSResolver(resolver) {} void OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & nodeData) override; CHIP_ERROR StopDiscovery() { return mDNSResolver.StopDiscovery(); }; diff --git a/src/controller/CHIPCommissionableNodeController.cpp b/src/controller/CHIPCommissionableNodeController.cpp index fdcf1f7e2649b9..d187cc1289a851 100644 --- a/src/controller/CHIPCommissionableNodeController.cpp +++ b/src/controller/CHIPCommissionableNodeController.cpp @@ -32,30 +32,12 @@ CHIP_ERROR CommissionableNodeController::DiscoverCommissioners(Dnssd::DiscoveryF { ReturnErrorOnFailure(SetUpNodeDiscovery()); - if (mResolver == nullptr) - { #if CONFIG_DEVICE_LAYER - mDNSResolver.Shutdown(); // reset if already inited - ReturnErrorOnFailure(mDNSResolver.Init(DeviceLayer::UDPEndPointManager())); + mDNSResolver.Shutdown(); // reset if already inited + ReturnErrorOnFailure(mDNSResolver.Init(DeviceLayer::UDPEndPointManager())); #endif - mDNSResolver.SetCommissioningDelegate(this); - return mDNSResolver.DiscoverCommissioners(discoveryFilter); - } - -#if CONFIG_DEVICE_LAYER - ReturnErrorOnFailure(mResolver->Init(DeviceLayer::UDPEndPointManager())); -#endif - return mResolver->DiscoverCommissioners(discoveryFilter); -} - -CHIP_ERROR CommissionableNodeController::StopDiscovery() -{ - if (mResolver == nullptr) - { - return AbstractDnssdDiscoveryController::StopDiscovery(); - } - - return mResolver->StopDiscovery(); + mDNSResolver.SetCommissioningDelegate(this); + return mDNSResolver.DiscoverCommissioners(discoveryFilter); } CommissionableNodeController::~CommissionableNodeController() diff --git a/src/controller/CHIPCommissionableNodeController.h b/src/controller/CHIPCommissionableNodeController.h index e573ba6fc4b5e9..5c9f57919a2897 100644 --- a/src/controller/CHIPCommissionableNodeController.h +++ b/src/controller/CHIPCommissionableNodeController.h @@ -36,14 +36,12 @@ namespace Controller { class DLL_EXPORT CommissionableNodeController : public AbstractDnssdDiscoveryController { public: - CommissionableNodeController(chip::Dnssd::Resolver * resolver = nullptr) : mResolver(resolver) {} + CommissionableNodeController(chip::Dnssd::Resolver * resolver = nullptr) : AbstractDnssdDiscoveryController(resolver) {} ~CommissionableNodeController() override; void RegisterDeviceDiscoveryDelegate(DeviceDiscoveryDelegate * delegate) { mDeviceDiscoveryDelegate = delegate; } CHIP_ERROR DiscoverCommissioners(Dnssd::DiscoveryFilter discoveryFilter = Dnssd::DiscoveryFilter()); - CHIP_ERROR StopDiscovery(); - /** * @return * Pointer to DiscoveredNodeData at index idx in the list of commissioners discovered @@ -57,7 +55,6 @@ class DLL_EXPORT CommissionableNodeController : public AbstractDnssdDiscoveryCon DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mDiscoveredCommissioners); } private: - Dnssd::Resolver * mResolver = nullptr; Dnssd::DiscoveredNodeData mDiscoveredCommissioners[CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES]; }; diff --git a/src/controller/tests/TestCommissionableNodeController.cpp b/src/controller/tests/TestCommissionableNodeController.cpp index d50a267172ec19..c2aafe500c293d 100644 --- a/src/controller/tests/TestCommissionableNodeController.cpp +++ b/src/controller/tests/TestCommissionableNodeController.cpp @@ -36,15 +36,14 @@ class MockResolver : public Resolver bool IsInitialized() override { return true; } void Shutdown() override {} void SetOperationalDelegate(OperationalResolveDelegate * delegate) override {} - void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override {} CHIP_ERROR ResolveNodeId(const PeerId & peerId) override { return ResolveNodeIdStatus; } void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override {} - CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return DiscoverCommissionersStatus; } - CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override + CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext &) override { return DiscoverCommissionersStatus; } + CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext &) override { return CHIP_ERROR_NOT_IMPLEMENTED; } - CHIP_ERROR StopDiscovery() override { return CHIP_ERROR_NOT_IMPLEMENTED; } + CHIP_ERROR StopDiscovery(DiscoveryContext &) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) override { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/lib/dnssd/BUILD.gn b/src/lib/dnssd/BUILD.gn index 313ae728688b80..68b90f1f8a2251 100644 --- a/src/lib/dnssd/BUILD.gn +++ b/src/lib/dnssd/BUILD.gn @@ -34,6 +34,8 @@ static_library("dnssd") { "IPAddressSorter.cpp", "IPAddressSorter.h", "Resolver.h", + "ResolverProxy.cpp", + "ResolverProxy.h", "ServiceNaming.cpp", "ServiceNaming.h", "TxtFields.cpp", diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index 9af17d2288024f..64eebbbb59962a 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -39,11 +39,11 @@ namespace { static void HandleNodeResolve(void * context, DnssdService * result, const Span & addresses, CHIP_ERROR error) { - ResolverDelegateProxy * proxy = static_cast(context); + DiscoveryContext * discoveryContext = static_cast(context); - if (CHIP_NO_ERROR != error) + if (error != CHIP_NO_ERROR) { - proxy->Release(); + discoveryContext->Release(); return; } @@ -51,31 +51,24 @@ static void HandleNodeResolve(void * context, DnssdService * result, const Span< result->ToDiscoveredNodeData(addresses, nodeData); nodeData.LogDetail(); - proxy->OnNodeDiscovered(nodeData); - proxy->Release(); + discoveryContext->OnNodeDiscovered(nodeData); + discoveryContext->Release(); } static void HandleNodeBrowse(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse, CHIP_ERROR error) { - ResolverDelegateProxy * proxy = static_cast(context); + DiscoveryContext * discoveryContext = static_cast(context); if (error != CHIP_NO_ERROR) { - // We don't have access to the ResolverProxy here to clear out its - // mDiscoveryContext. The underlying implementation of - // ChipDnssdStopBrowse needs to handle a possibly-stale reference - // safely, so this won't lead to crashes, but it can lead to - // mis-behavior if a stale mDiscoveryContext happens to match a newer - // browse operation. - // - // TODO: Have a way to clear that state here. - proxy->Release(); + discoveryContext->ClearBrowseIdentifier(); + discoveryContext->Release(); return; } for (size_t i = 0; i < servicesSize; ++i) { - proxy->Retain(); + discoveryContext->Retain(); // For some platforms browsed services are already resolved, so verify if resolve is really needed or call resolve callback // Check if SRV, TXT and AAAA records were received in DNS responses @@ -92,15 +85,8 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser if (finalBrowse) { - // We don't have access to the ResolverProxy here to clear out its - // mDiscoveryContext. The underlying implementation of - // ChipDnssdStopBrowse needs to handle a possibly-stale reference - // safely, so this won't lead to crashes, but it can lead to - // mis-behavior if a stale mDiscoveryContext happens to match a newer - // browse operation. - // - // TODO: Have a way to clear that state here. - proxy->Release(); + discoveryContext->ClearBrowseIdentifier(); + discoveryContext->Release(); } } @@ -398,7 +384,6 @@ CHIP_ERROR DiscoveryImplPlatform::InitImpl() void DiscoveryImplPlatform::Shutdown() { VerifyOrReturn(mState != State::kUninitialized); - mResolverProxy.Shutdown(); ChipDnssdShutdown(); mState = State::kUninitialized; } @@ -411,20 +396,6 @@ void DiscoveryImplPlatform::HandleDnssdInit(void * context, CHIP_ERROR initError { publisher->mState = State::kInitialized; - // TODO: this is wrong, however we need resolverproxy initialized - // otherwise DiscoveryImplPlatform is not usable. - // - // We rely on the fact that resolverproxy does not use the endpoint - // nor does DiscoveryImplPlatform use it (since init will be called - // twice now) - // - // The problem is that: - // - DiscoveryImplPlatform contains a ResolverProxy - // - ResolverProxy::Init calls Dnssd::Resolver::Instance().Init - // which results in a recursive dependency (proxy initializes the - // class that it is contained in). - publisher->mResolverProxy.Init(nullptr); - // Post an event that will start advertising DeviceLayer::ChipDeviceEvent event; event.Type = DeviceLayer::DeviceEventType::kDnssdInitialized; @@ -671,125 +642,131 @@ void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId ChipDnssdResolveNoLongerNeeded(name); } -CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissionableNodes(DiscoveryFilter filter) -{ - ReturnErrorOnFailure(InitImpl()); - return mResolverProxy.DiscoverCommissionableNodes(filter); -} - -CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissioners(DiscoveryFilter filter) -{ - ReturnErrorOnFailure(InitImpl()); - return mResolverProxy.DiscoverCommissioners(filter); -} - -CHIP_ERROR DiscoveryImplPlatform::StopDiscovery() +CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext & context) { ReturnErrorOnFailure(InitImpl()); - return mResolverProxy.StopDiscovery(); -} - -CHIP_ERROR DiscoveryImplPlatform::ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) -{ - ReturnErrorOnFailure(InitImpl()); - return mResolverProxy.ReconfirmRecord(hostname, address, interfaceId); -} - -DiscoveryImplPlatform & DiscoveryImplPlatform::GetInstance() -{ - return sManager.get(); -} - -ServiceAdvertiser & chip::Dnssd::ServiceAdvertiser::Instance() -{ - return DiscoveryImplPlatform::GetInstance(); -} - -Resolver & chip::Dnssd::Resolver::Instance() -{ - return DiscoveryImplPlatform::GetInstance(); -} - -ResolverProxy::~ResolverProxy() -{ - Shutdown(); -} - -CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) -{ - StopDiscovery(); - - VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); - mDelegate->Retain(); + StopDiscovery(context); if (filter.type == DiscoveryFilterType::kInstanceName) { - // when we have the instance name, no need to browse, only need to resolve + // When we have the instance name, no need to browse, only need to resolve. DnssdService service; ReturnErrorOnFailure(MakeServiceSubtype(service.mName, sizeof(service.mName), filter)); Platform::CopyString(service.mType, kCommissionableServiceName); service.mProtocol = DnssdServiceProtocol::kDnssdProtocolUdp; service.mAddressType = Inet::IPAddressType::kAny; - return ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeResolve, mDelegate); + + // Increase the reference count of the context to keep it alive until HandleNodeResolve is called back. + CHIP_ERROR error = ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeResolve, context.Retain()); + + if (error != CHIP_NO_ERROR) + { + context.Release(); + } + + return error; } char serviceName[kMaxCommissionableServiceNameSize]; ReturnErrorOnFailure(MakeServiceTypeName(serviceName, sizeof(serviceName), filter, DiscoveryType::kCommissionableNode)); intptr_t browseIdentifier; - ReturnErrorOnFailure(ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny, - Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate, &browseIdentifier)); - mDiscoveryContext.Emplace(browseIdentifier); - return CHIP_NO_ERROR; + // Increase the reference count of the context to keep it alive until HandleNodeBrowse is called back. + CHIP_ERROR error = ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny, + Inet::InterfaceId::Null(), HandleNodeBrowse, context.Retain(), &browseIdentifier); + + if (error == CHIP_NO_ERROR) + { + context.SetBrowseIdentifier(browseIdentifier); + } + else + { + context.Release(); + } + + return error; } -CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter) +CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext & context) { - StopDiscovery(); - - VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); - mDelegate->Retain(); + ReturnErrorOnFailure(InitImpl()); + StopDiscovery(context); if (filter.type == DiscoveryFilterType::kInstanceName) { - // when we have the instance name, no need to browse, only need to resolve + // When we have the instance name, no need to browse, only need to resolve. DnssdService service; ReturnErrorOnFailure(MakeServiceSubtype(service.mName, sizeof(service.mName), filter)); Platform::CopyString(service.mType, kCommissionerServiceName); service.mProtocol = DnssdServiceProtocol::kDnssdProtocolUdp; service.mAddressType = Inet::IPAddressType::kAny; - return ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeResolve, mDelegate); + + // Increase the reference count of the context to keep it alive until HandleNodeResolve is called back. + CHIP_ERROR error = ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeResolve, context.Retain()); + + if (error != CHIP_NO_ERROR) + { + context.Release(); + } } char serviceName[kMaxCommissionerServiceNameSize]; ReturnErrorOnFailure(MakeServiceTypeName(serviceName, sizeof(serviceName), filter, DiscoveryType::kCommissionerNode)); intptr_t browseIdentifier; - ReturnErrorOnFailure(ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny, - Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate, &browseIdentifier)); - mDiscoveryContext.Emplace(browseIdentifier); - return CHIP_NO_ERROR; + // Increase the reference count of the context to keep it alive until HandleNodeBrowse is called back. + CHIP_ERROR error = ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny, + Inet::InterfaceId::Null(), HandleNodeBrowse, context.Retain(), &browseIdentifier); + + if (error == CHIP_NO_ERROR) + { + context.SetBrowseIdentifier(browseIdentifier); + } + else + { + context.Release(); + } + + return error; } -CHIP_ERROR ResolverProxy::StopDiscovery() +CHIP_ERROR DiscoveryImplPlatform::StopDiscovery(DiscoveryContext & context) { - if (!mDiscoveryContext.HasValue()) + if (!context.GetBrowseIdentifier().HasValue()) { // No discovery going on. return CHIP_NO_ERROR; } - CHIP_ERROR err = ChipDnssdStopBrowse(mDiscoveryContext.Value()); - mDiscoveryContext.ClearValue(); - return err; + const auto browseIdentifier = context.GetBrowseIdentifier().Value(); + context.ClearBrowseIdentifier(); + + return ChipDnssdStopBrowse(browseIdentifier); } -CHIP_ERROR ResolverProxy::ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) +CHIP_ERROR DiscoveryImplPlatform::ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) { + ReturnErrorOnFailure(InitImpl()); + return ChipDnssdReconfirmRecord(hostname, address, interfaceId); } +DiscoveryImplPlatform & DiscoveryImplPlatform::GetInstance() +{ + return sManager.get(); +} + +ServiceAdvertiser & chip::Dnssd::ServiceAdvertiser::Instance() +{ + return DiscoveryImplPlatform::GetInstance(); +} + +Resolver & chip::Dnssd::Resolver::Instance() +{ + return DiscoveryImplPlatform::GetInstance(); +} + } // namespace Dnssd } // namespace chip diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h index 994112e9be1f8e..a3f79796ca535c 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.h +++ b/src/lib/dnssd/Discovery_ImplPlatform.h @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -52,15 +51,11 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver // Members that implement Resolver interface. void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mOperationalDelegate = delegate; } - void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override - { - mResolverProxy.SetCommissioningDelegate(delegate); - } CHIP_ERROR ResolveNodeId(const PeerId & peerId) override; void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override; - CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override; - CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override; - CHIP_ERROR StopDiscovery() override; + CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext & context) override; + CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext & context) override; + CHIP_ERROR StopDiscovery(DiscoveryContext & context) override; CHIP_ERROR ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) override; static DiscoveryImplPlatform & GetInstance(); @@ -97,7 +92,6 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver State mState = State::kUninitialized; uint8_t mCommissionableInstanceName[sizeof(uint64_t)]; - ResolverProxy mResolverProxy; OperationalResolveDelegate * mOperationalDelegate = nullptr; friend class Global; diff --git a/src/lib/dnssd/Resolver.h b/src/lib/dnssd/Resolver.h index 5623d4ad2fdb8e..ae4e95f4341b85 100644 --- a/src/lib/dnssd/Resolver.h +++ b/src/lib/dnssd/Resolver.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -352,6 +353,43 @@ class CommissioningResolveDelegate virtual void OnNodeDiscovered(const DiscoveredNodeData & nodeData) = 0; }; +/** + * Node discovery context class. + * + * This class enables multiple clients of the global DNS-SD resolver to start simultaneous + * discovery operations. + * + * An object of this class is shared between a resolver client and the concrete resolver + * implementation. The client is responsible for allocating the context and passing it to + * the resolver when initiating a discovery operation. The resolver, in turn, is supposed to retain + * the context until the operation is finished. This allows the client to release the ownership of + * the context at any time without putting the resolver at risk of using a deleted object. + */ +class DiscoveryContext : public ReferenceCounted +{ +public: + void SetBrowseIdentifier(intptr_t identifier) { mBrowseIdentifier.Emplace(identifier); } + void ClearBrowseIdentifier() { mBrowseIdentifier.ClearValue(); } + const Optional & GetBrowseIdentifier() const { return mBrowseIdentifier; } + + void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) { mCommissioningDelegate = delegate; } + void OnNodeDiscovered(const DiscoveredNodeData & nodeData) + { + if (mCommissioningDelegate != nullptr) + { + mCommissioningDelegate->OnNodeDiscovered(nodeData); + } + else + { + ChipLogError(Discovery, "Missing commissioning delegate. Data discarded"); + } + } + +private: + CommissioningResolveDelegate * mCommissioningDelegate = nullptr; + Optional mBrowseIdentifier; +}; + /** * Interface for resolving CHIP DNS-SD services */ @@ -385,11 +423,6 @@ class Resolver */ virtual void SetOperationalDelegate(OperationalResolveDelegate * delegate) = 0; - /** - * If nullptr is passed, the previously registered delegate is unregistered. - */ - virtual void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) = 0; - /** * Requests resolution of the given operational node service. * @@ -431,18 +464,26 @@ class Resolver /** * Finds all commissionable nodes matching the given filter. * - * Whenever a new matching node is found and a resolver delegate has been registered, - * the node information is passed to the delegate's `OnNodeDiscoveryComplete` method. + * Whenever a new matching node is found, the node information is passed to + * the `OnNodeDiscovered` method of the commissioning delegate configured + * in the context object. + * + * This method is expected to increase the reference count of the context + * object for as long as it takes to complete the discovery request. */ - virtual CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) = 0; + virtual CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext & context) = 0; /** * Finds all commissioner nodes matching the given filter. * - * Whenever a new matching node is found and a resolver delegate has been registered, - * the node information is passed to the delegate's `OnNodeDiscoveryComplete` method. + * Whenever a new matching node is found, the node information is passed to + * the `OnNodeDiscovered` method of the commissioning delegate configured + * in the context object. + * + * This method is expected to increase the reference count of the context + * object for as long as it takes to complete the discovery request. */ - virtual CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) = 0; + virtual CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext & context) = 0; /** * Stop discovery (of commissionable or commissioner nodes). @@ -450,7 +491,7 @@ class Resolver * Some back ends may not support stopping discovery, so consumers should * not assume they will stop getting callbacks after calling this. */ - virtual CHIP_ERROR StopDiscovery() = 0; + virtual CHIP_ERROR StopDiscovery(DiscoveryContext & context) = 0; /** * Verify the validity of an address that appears to be out of date (for example diff --git a/src/lib/dnssd/ResolverProxy.cpp b/src/lib/dnssd/ResolverProxy.cpp new file mode 100644 index 00000000000000..3e7446ebbbdc6b --- /dev/null +++ b/src/lib/dnssd/ResolverProxy.cpp @@ -0,0 +1,66 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "ResolverProxy.h" + +#include + +namespace chip { +namespace Dnssd { + +CHIP_ERROR ResolverProxy::Init(Inet::EndPointManager * udpEndPoint) +{ + VerifyOrReturnError(mContext == nullptr, CHIP_ERROR_INCORRECT_STATE); + + ReturnErrorOnFailure(mResolver.Init(udpEndPoint)); + mContext = Platform::New(); + VerifyOrReturnError(mContext != nullptr, CHIP_ERROR_NO_MEMORY); + + return CHIP_NO_ERROR; +} + +void ResolverProxy::Shutdown() +{ + VerifyOrReturn(mContext != nullptr); + mContext->SetCommissioningDelegate(nullptr); + mContext->Release(); + mContext = nullptr; +} + +CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) +{ + VerifyOrReturnError(mContext != nullptr, CHIP_ERROR_INCORRECT_STATE); + + return mResolver.DiscoverCommissionableNodes(filter, *mContext); +} + +CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter) +{ + VerifyOrReturnError(mContext != nullptr, CHIP_ERROR_INCORRECT_STATE); + + return mResolver.DiscoverCommissioners(filter, *mContext); +} + +CHIP_ERROR ResolverProxy::StopDiscovery() +{ + VerifyOrReturnError(mContext != nullptr, CHIP_ERROR_INCORRECT_STATE); + + return mResolver.StopDiscovery(*mContext); +} + +} // namespace Dnssd +} // namespace chip diff --git a/src/lib/dnssd/ResolverProxy.h b/src/lib/dnssd/ResolverProxy.h index 176316ef8cd053..09a50e89c1a209 100644 --- a/src/lib/dnssd/ResolverProxy.h +++ b/src/lib/dnssd/ResolverProxy.h @@ -17,112 +17,48 @@ #pragma once -#include #include namespace chip { namespace Dnssd { -class ResolverDelegateProxy : public ReferenceCounted, public CommissioningResolveDelegate - +/** + * Convenience class for discovering Matter commissioners and commissionable nodes. + * + * The class simplifies the usage of the global DNS-SD resolver to discover Matter nodes by managing + * the lifetime of the discovery context object. + * + * The proxy provides a method to register an external commissioning delegate whose + * `OnNodeDiscovered` method is called whenever a new node is discovered. The delegate is + * automatically unregistered when the proxy is shut down or destroyed. + * + * The proxy can be safely shut down or destroyed even if the last discovery operation has not + * finished yet. + */ +class ResolverProxy { public: - void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) { mCommissioningDelegate = delegate; } - - // CommissioningResolveDelegate - void OnNodeDiscovered(const DiscoveredNodeData & nodeData) override - { - if (mCommissioningDelegate != nullptr) - { - mCommissioningDelegate->OnNodeDiscovered(nodeData); - } - else - { - ChipLogError(Discovery, "Missing commissioning delegate. Data discarded."); - } - } + explicit ResolverProxy(Resolver * resolver = nullptr) : mResolver(resolver != nullptr ? *resolver : Resolver::Instance()) {} + ~ResolverProxy() { Shutdown(); } -private: - CommissioningResolveDelegate * mCommissioningDelegate = nullptr; -}; + CHIP_ERROR Init(Inet::EndPointManager * udpEndPoint = nullptr); + void Shutdown(); -class ResolverProxy : public Resolver -{ -public: - ResolverProxy() {} - ~ResolverProxy() override; - - // Resolver interface. - CHIP_ERROR Init(Inet::EndPointManager * udpEndPoint = nullptr) override + void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) { - ReturnErrorOnFailure(chip::Dnssd::Resolver::Instance().Init(udpEndPoint)); - VerifyOrReturnError(mDelegate == nullptr, CHIP_ERROR_INCORRECT_STATE); - mDelegate = chip::Platform::New(); - - if (mDelegate != nullptr) + if (mContext != nullptr) { - if (mPreInitCommissioningDelegate != nullptr) - { - ChipLogProgress(Discovery, "Setting commissioning delegate post init"); - mDelegate->SetCommissioningDelegate(mPreInitCommissioningDelegate); - mPreInitCommissioningDelegate = nullptr; - } + mContext->SetCommissioningDelegate(delegate); } - - return mDelegate != nullptr ? CHIP_NO_ERROR : CHIP_ERROR_NO_MEMORY; } - bool IsInitialized() override { return Resolver::Instance().IsInitialized(); } - - void SetOperationalDelegate(OperationalResolveDelegate * delegate) override - { - /// Unfortunately cannot remove this method since it is in a Resolver interface. - ChipLogError(Discovery, "!!! Operational proxy does NOT support operational discovery"); - ChipLogError(Discovery, "!!! Please use AddressResolver or DNSSD Resolver directly"); - chipDie(); // force detection of invalid usages. - } - - void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override - { - if (mDelegate != nullptr) - { - mDelegate->SetCommissioningDelegate(delegate); - } - else - { - if (delegate != nullptr) - { - ChipLogError(Discovery, "Delaying proxy of commissioning discovery: missing delegate"); - } - mPreInitCommissioningDelegate = delegate; - } - } - - void Shutdown() override - { - VerifyOrReturn(mDelegate != nullptr); - mDelegate->SetCommissioningDelegate(nullptr); - mDelegate->Release(); - mDelegate = nullptr; - } - - CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override; - CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override; - CHIP_ERROR StopDiscovery() override; - CHIP_ERROR ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) override; - - // TODO: ResolverProxy should not be used anymore to implement operational node resolution - // This method still here because Resolver interface requires it - CHIP_ERROR ResolveNodeId(const PeerId & peerId) override { return CHIP_ERROR_NOT_IMPLEMENTED; } - void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override {} + CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()); + CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()); + CHIP_ERROR StopDiscovery(); private: - ResolverDelegateProxy * mDelegate = nullptr; - CommissioningResolveDelegate * mPreInitCommissioningDelegate = nullptr; - - // While discovery (commissionable or commissioner) is ongoing, - // mDiscoveryContext may have a value to allow StopDiscovery to work. - Optional mDiscoveryContext; + Resolver & mResolver; + DiscoveryContext * mContext = nullptr; }; } // namespace Dnssd diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 416c747d830cc6..64461965367b13 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include #include #include @@ -274,6 +273,7 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate { GlobalMinimalMdnsServer::Instance().SetResponseDelegate(this); } + ~MinMdnsResolver() { SetDiscoveryContext(nullptr); } //// MdnsPacketDelegate implementation void OnMdnsPacketData(const BytesRange & data, const chip::Inet::IPPacketInfo * info) override; @@ -283,21 +283,21 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate bool IsInitialized() override; void Shutdown() override; void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mOperationalDelegate = delegate; } - void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override { mCommissioningDelegate = delegate; } CHIP_ERROR ResolveNodeId(const PeerId & peerId) override; void NodeIdResolutionNoLongerNeeded(const PeerId & peerId) override; - CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override; - CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override; - CHIP_ERROR StopDiscovery() override; + CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext & context) override; + CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext & context) override; + CHIP_ERROR StopDiscovery(DiscoveryContext & context) override; CHIP_ERROR ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) override; private: - OperationalResolveDelegate * mOperationalDelegate = nullptr; - CommissioningResolveDelegate * mCommissioningDelegate = nullptr; - System::Layer * mSystemLayer = nullptr; + OperationalResolveDelegate * mOperationalDelegate = nullptr; + DiscoveryContext * mDiscoveryContext = nullptr; + System::Layer * mSystemLayer = nullptr; ActiveResolveAttempts mActiveResolves; PacketParser mPacketParser; + void SetDiscoveryContext(DiscoveryContext * context); void ScheduleIpAddressResolve(SerializedQNameIterator hostName); CHIP_ERROR SendAllPendingQueries(); @@ -332,6 +332,21 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate char qnameStorage[kMaxQnameSize]; }; +void MinMdnsResolver::SetDiscoveryContext(DiscoveryContext * context) +{ + if (mDiscoveryContext != nullptr) + { + mDiscoveryContext->Release(); + } + + if (context != nullptr) + { + context->Retain(); + } + + mDiscoveryContext = context; +} + void MinMdnsResolver::ScheduleIpAddressResolve(SerializedQNameIterator hostName) { HeapQName target(hostName); @@ -409,9 +424,9 @@ void MinMdnsResolver::AdvancePendingResolverStates() if (discoveredNodeIsRelevant) { - if (mCommissioningDelegate != nullptr) + if (mDiscoveryContext != nullptr) { - mCommissioningDelegate->OnNodeDiscovered(nodeData); + mDiscoveryContext->OnNodeDiscovered(nodeData); } else { @@ -670,18 +685,26 @@ void MinMdnsResolver::ExpireIncrementalResolvers() } } -CHIP_ERROR MinMdnsResolver::DiscoverCommissionableNodes(DiscoveryFilter filter) +CHIP_ERROR MinMdnsResolver::DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext & context) { + // minmdns currently supports only one discovery context at a time so override the previous context + SetDiscoveryContext(&context); + return BrowseNodes(DiscoveryType::kCommissionableNode, filter); } -CHIP_ERROR MinMdnsResolver::DiscoverCommissioners(DiscoveryFilter filter) +CHIP_ERROR MinMdnsResolver::DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext & context) { + // minmdns currently supports only one discovery context at a time so override the previous context + SetDiscoveryContext(&context); + return BrowseNodes(DiscoveryType::kCommissionerNode, filter); } -CHIP_ERROR MinMdnsResolver::StopDiscovery() +CHIP_ERROR MinMdnsResolver::StopDiscovery(DiscoveryContext & context) { + SetDiscoveryContext(nullptr); + return mActiveResolves.CompleteAllBrowses(); } @@ -740,37 +763,5 @@ Resolver & chip::Dnssd::Resolver::Instance() return gResolver; } -ResolverProxy::~ResolverProxy() -{ - // TODO: this is a hack: resolver proxies used for commissionable discovery - // and they don't interact well with each other. - gResolver.SetCommissioningDelegate(nullptr); - Shutdown(); -} - -CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) -{ - VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); - chip::Dnssd::Resolver::Instance().SetCommissioningDelegate(mDelegate); - return chip::Dnssd::Resolver::Instance().DiscoverCommissionableNodes(filter); -} - -CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter) -{ - VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); - chip::Dnssd::Resolver::Instance().SetCommissioningDelegate(mDelegate); - return chip::Dnssd::Resolver::Instance().DiscoverCommissioners(filter); -} - -CHIP_ERROR ResolverProxy::StopDiscovery() -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - -CHIP_ERROR ResolverProxy::ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - } // namespace Dnssd } // namespace chip diff --git a/src/lib/dnssd/Resolver_ImplNone.cpp b/src/lib/dnssd/Resolver_ImplNone.cpp index 3d0d97f57cd449..9a1f40f3008dff 100644 --- a/src/lib/dnssd/Resolver_ImplNone.cpp +++ b/src/lib/dnssd/Resolver_ImplNone.cpp @@ -17,7 +17,6 @@ #include "Resolver.h" -#include #include namespace chip { @@ -31,7 +30,6 @@ class NoneResolver : public Resolver bool IsInitialized() override { return true; } void Shutdown() override {} void SetOperationalDelegate(OperationalResolveDelegate * delegate) override {} - void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override {} CHIP_ERROR ResolveNodeId(const PeerId & peerId) override { @@ -42,12 +40,15 @@ class NoneResolver : public Resolver { ChipLogError(Discovery, "Failed to stop resolving node ID: dnssd resolving not available"); } - CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override + CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter, DiscoveryContext & context) override { return CHIP_ERROR_NOT_IMPLEMENTED; } - CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; } - CHIP_ERROR StopDiscovery() override { return CHIP_ERROR_NOT_IMPLEMENTED; } + CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter, DiscoveryContext & context) override + { + return CHIP_ERROR_NOT_IMPLEMENTED; + } + CHIP_ERROR StopDiscovery(DiscoveryContext & context) override { return CHIP_ERROR_NOT_IMPLEMENTED; } CHIP_ERROR ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) override { return CHIP_ERROR_NOT_IMPLEMENTED; @@ -63,30 +64,5 @@ Resolver & chip::Dnssd::Resolver::Instance() return gResolver; } -ResolverProxy::~ResolverProxy() -{ - Shutdown(); -} - -CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - -CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter) -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - -CHIP_ERROR ResolverProxy::StopDiscovery() -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - -CHIP_ERROR ResolverProxy::ReconfirmRecord(const char * hostname, Inet::IPAddress address, Inet::InterfaceId interfaceId) -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - } // namespace Dnssd } // namespace chip diff --git a/src/lib/shell/commands/Dns.cpp b/src/lib/shell/commands/Dns.cpp index 50c6d1ad246cbb..69d1b478523893 100644 --- a/src/lib/shell/commands/Dns.cpp +++ b/src/lib/shell/commands/Dns.cpp @@ -211,12 +211,7 @@ bool ParseSubType(int argc, char ** argv, Dnssd::DiscoveryFilter & filter) CHIP_ERROR BrowseCommissionableHandler(int argc, char ** argv) { Dnssd::DiscoveryFilter filter; - - if (!ParseSubType(argc, argv, filter)) - { - streamer_printf(streamer_get(), "Invalid argument\r\n"); - return CHIP_ERROR_INVALID_ARGUMENT; - } + VerifyOrReturnError(ParseSubType(argc, argv, filter), CHIP_ERROR_INVALID_ARGUMENT); streamer_printf(streamer_get(), "Browsing commissionable nodes...\r\n"); @@ -226,12 +221,7 @@ CHIP_ERROR BrowseCommissionableHandler(int argc, char ** argv) CHIP_ERROR BrowseCommissionerHandler(int argc, char ** argv) { Dnssd::DiscoveryFilter filter; - - if (!ParseSubType(argc, argv, filter)) - { - streamer_printf(streamer_get(), "Invalid argument\r\n"); - return CHIP_ERROR_INVALID_ARGUMENT; - } + VerifyOrReturnError(ParseSubType(argc, argv, filter), CHIP_ERROR_INVALID_ARGUMENT); streamer_printf(streamer_get(), "Browsing commissioners...\r\n"); @@ -246,6 +236,9 @@ CHIP_ERROR BrowseHandler(int argc, char ** argv) return CHIP_NO_ERROR; } + sResolverProxy.Init(DeviceLayer::UDPEndPointManager()); + sResolverProxy.SetCommissioningDelegate(&sDnsShellResolverDelegate); + return sShellDnsBrowseSubcommands.ExecCommand(argc, argv); } @@ -257,9 +250,6 @@ CHIP_ERROR DnsHandler(int argc, char ** argv) return CHIP_NO_ERROR; } - sResolverProxy.Init(DeviceLayer::UDPEndPointManager()); - sResolverProxy.SetCommissioningDelegate(&sDnsShellResolverDelegate); - return sShellDnsSubcommands.ExecCommand(argc, argv); }