From d942ea779fce026d85770b7599cf27410c287b88 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 15 Sep 2023 16:06:28 -0400 Subject: [PATCH] Stop using ResolverProxy for resolving node lookups (#29264) * Stop using ResolverProxy for performing Operational node discovery * Switch avahi to be capable to keep track of contexts and handle stopping node resolution * Restyle * Remove obsolete todo * Make sure stop resolve actuall clears multiple entries if they exist * increase log level verbositity for openiot to see progress in pytest * A bit more rich logging in the openiot utils - show successes and failures * report failure on commissioning (likely timeout, but show it since it is not none/bool * Restyle * Switch Dns.cpp to use address resolver for dns resolution * Restyle * remove operational delegate functionality from ResolverDelegateProxy and more cleanups on members/methods * Restyle * Schedule next result to display all results of a discovery * Fix namespacing * Code review update --------- Co-authored-by: Andrei Litvin --- scripts/examples/openiotsdk_example.sh | 2 +- src/lib/dnssd/Discovery_ImplPlatform.cpp | 176 ++++++++---------- src/lib/dnssd/Discovery_ImplPlatform.h | 6 +- src/lib/dnssd/ResolverProxy.h | 63 +------ src/lib/dnssd/Resolver_ImplMinimalMdns.cpp | 19 -- src/lib/dnssd/Resolver_ImplNone.cpp | 7 - src/lib/shell/commands/BUILD.gn | 1 + src/lib/shell/commands/Dns.cpp | 74 +++++--- src/platform/Linux/DnssdImpl.cpp | 98 ++++++++-- src/platform/Linux/DnssdImpl.h | 26 ++- .../integration-tests/common/utils.py | 5 +- 11 files changed, 247 insertions(+), 230 deletions(-) diff --git a/scripts/examples/openiotsdk_example.sh b/scripts/examples/openiotsdk_example.sh index ea33d6026526e9..36c748cc8d1aa2 100755 --- a/scripts/examples/openiotsdk_example.sh +++ b/scripts/examples/openiotsdk_example.sh @@ -268,7 +268,7 @@ function run_test() { fi set +e - pytest --json-report --json-report-summary --json-report-file="$EXAMPLE_TEST_PATH"/test_report_"$EXAMPLE".json --binaryPath="$EXAMPLE_EXE_PATH" --fvp="$FVP_BIN" --fvpConfig="$FVP_CONFIG_FILE" "${TEST_OPTIONS[@]}" "$EXAMPLE_TEST_PATH"/test_app.py + pytest --log-level=INFO --json-report --json-report-summary --json-report-file="$EXAMPLE_TEST_PATH"/test_report_"$EXAMPLE".json --binaryPath="$EXAMPLE_EXE_PATH" --fvp="$FVP_BIN" --fvpConfig="$FVP_CONFIG_FILE" "${TEST_OPTIONS[@]}" "$EXAMPLE_TEST_PATH"/test_app.py set -e if [[ ! -f $EXAMPLE_TEST_PATH/test_report_$EXAMPLE.json ]]; then diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index cf6c127599bf25..a4361f03929f44 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -55,70 +55,6 @@ static void HandleNodeResolve(void * context, DnssdService * result, const Span< proxy->Release(); } -static void HandleNodeIdResolve(void * context, DnssdService * result, const Span & addresses, CHIP_ERROR error) -{ - ResolverDelegateProxy * proxy = static_cast(context); - if (CHIP_NO_ERROR != error) - { - proxy->OnOperationalNodeResolutionFailed(PeerId(), error); - proxy->Release(); - return; - } - - VerifyOrDie(proxy != nullptr); - - if (result == nullptr) - { - proxy->OnOperationalNodeResolutionFailed(PeerId(), CHIP_ERROR_UNKNOWN_RESOURCE_ID); - proxy->Release(); - return; - } - - VerifyOrDie(proxy != nullptr); - - PeerId peerId; - error = ExtractIdFromInstanceName(result->mName, &peerId); - if (CHIP_NO_ERROR != error) - { - proxy->OnOperationalNodeResolutionFailed(PeerId(), error); - proxy->Release(); - return; - } - - VerifyOrDie(proxy != nullptr); - - ResolvedNodeData nodeData; - Platform::CopyString(nodeData.resolutionData.hostName, result->mHostName); - nodeData.resolutionData.interfaceId = result->mInterface; - nodeData.resolutionData.port = result->mPort; - nodeData.operationalData.peerId = peerId; - - size_t addressesFound = 0; - for (auto & ip : addresses) - { - if (addressesFound == ArraySize(nodeData.resolutionData.ipAddress)) - { - // Out of space. - ChipLogProgress(Discovery, "Can't add more IPs to ResolvedNodeData"); - break; - } - nodeData.resolutionData.ipAddress[addressesFound] = ip; - ++addressesFound; - } - nodeData.resolutionData.numIPs = addressesFound; - - for (size_t i = 0; i < result->mTextEntrySize; ++i) - { - ByteSpan key(reinterpret_cast(result->mTextEntries[i].mKey), strlen(result->mTextEntries[i].mKey)); - ByteSpan val(result->mTextEntries[i].mData, result->mTextEntries[i].mDataSize); - FillNodeDataFromTxt(key, val, nodeData.resolutionData); - } - - nodeData.LogNodeIdResolved(); - proxy->OnOperationalNodeResolved(nodeData); - proxy->Release(); -} - static void HandleNodeBrowse(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse, CHIP_ERROR error) { ResolverDelegateProxy * proxy = static_cast(context); @@ -336,6 +272,68 @@ CHIP_ERROR AddTxtRecord(TxtFieldKey key, TextEntry * entries, size_t & entriesCo } // namespace +void DiscoveryImplPlatform::HandleNodeIdResolve(void * context, DnssdService * result, const Span & addresses, + CHIP_ERROR error) +{ + DiscoveryImplPlatform * impl = static_cast(context); + + if (impl->mOperationalDelegate == nullptr) + { + ChipLogError(Discovery, "No delegate to handle node resolution data."); + return; + } + + if (CHIP_NO_ERROR != error) + { + impl->mOperationalDelegate->OnOperationalNodeResolutionFailed(PeerId(), error); + return; + } + + if (result == nullptr) + { + impl->mOperationalDelegate->OnOperationalNodeResolutionFailed(PeerId(), CHIP_ERROR_UNKNOWN_RESOURCE_ID); + return; + } + + PeerId peerId; + error = ExtractIdFromInstanceName(result->mName, &peerId); + if (CHIP_NO_ERROR != error) + { + impl->mOperationalDelegate->OnOperationalNodeResolutionFailed(PeerId(), error); + return; + } + + ResolvedNodeData nodeData; + Platform::CopyString(nodeData.resolutionData.hostName, result->mHostName); + nodeData.resolutionData.interfaceId = result->mInterface; + nodeData.resolutionData.port = result->mPort; + nodeData.operationalData.peerId = peerId; + + size_t addressesFound = 0; + for (auto & ip : addresses) + { + if (addressesFound == ArraySize(nodeData.resolutionData.ipAddress)) + { + // Out of space. + ChipLogProgress(Discovery, "Can't add more IPs to ResolvedNodeData"); + break; + } + nodeData.resolutionData.ipAddress[addressesFound] = ip; + ++addressesFound; + } + nodeData.resolutionData.numIPs = addressesFound; + + for (size_t i = 0; i < result->mTextEntrySize; ++i) + { + ByteSpan key(reinterpret_cast(result->mTextEntries[i].mKey), strlen(result->mTextEntries[i].mKey)); + ByteSpan val(result->mTextEntries[i].mData, result->mTextEntries[i].mDataSize); + FillNodeDataFromTxt(key, val, nodeData.resolutionData); + } + + nodeData.LogNodeIdResolved(); + impl->mOperationalDelegate->OnOperationalNodeResolved(nodeData); +} + void DnssdService::ToDiscoveredNodeData(const Span & addresses, DiscoveredNodeData & nodeData) { auto & resolutionData = nodeData.resolutionData; @@ -633,15 +631,27 @@ bool DiscoveryImplPlatform::IsInitialized() CHIP_ERROR DiscoveryImplPlatform::ResolveNodeId(const PeerId & peerId) { - ReturnErrorOnFailure(InitImpl()); - return mResolverProxy.ResolveNodeId(peerId); + // Resolve requests can only be issued once DNSSD is initialized and there is + // no caching currently + VerifyOrReturnError(mState == State::kInitialized, CHIP_ERROR_INCORRECT_STATE); + + ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...", + ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); + + DnssdService service; + + ReturnErrorOnFailure(MakeInstanceName(service.mName, sizeof(service.mName), peerId)); + Platform::CopyString(service.mType, kOperationalServiceName); + service.mProtocol = DnssdServiceProtocol::kDnssdProtocolTcp; + service.mAddressType = Inet::IPAddressType::kAny; + + return ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, this); } void DiscoveryImplPlatform::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) { char name[Common::kInstanceNameMaxLength + 1]; ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId)); - ChipDnssdResolveNoLongerNeeded(name); } @@ -684,38 +694,6 @@ Resolver & chip::Dnssd::Resolver::Instance() return DiscoveryImplPlatform::GetInstance(); } -CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) -{ - VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); - - ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...", - ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); - - DnssdService service; - - ReturnErrorOnFailure(MakeInstanceName(service.mName, sizeof(service.mName), peerId)); - Platform::CopyString(service.mType, kOperationalServiceName); - service.mProtocol = DnssdServiceProtocol::kDnssdProtocolTcp; - service.mAddressType = Inet::IPAddressType::kAny; - - mDelegate->Retain(); - - CHIP_ERROR err = ChipDnssdResolve(&service, Inet::InterfaceId::Null(), HandleNodeIdResolve, mDelegate); - if (err != CHIP_NO_ERROR) - { - mDelegate->Release(); - } - return err; -} - -void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) -{ - char name[Common::kInstanceNameMaxLength + 1]; - ReturnOnFailure(MakeInstanceName(name, sizeof(name), peerId)); - - ChipDnssdResolveNoLongerNeeded(name); -} - ResolverProxy::~ResolverProxy() { Shutdown(); diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h index 1bc75f105d0efd..e483a589de8adb 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.h +++ b/src/lib/dnssd/Discovery_ImplPlatform.h @@ -50,7 +50,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver CHIP_ERROR UpdateCommissionableInstanceName() override; // Members that implement Resolver interface. - void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mResolverProxy.SetOperationalDelegate(delegate); } + void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mOperationalDelegate = delegate; } void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override { mResolverProxy.SetCommissioningDelegate(delegate); @@ -91,9 +91,13 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver size_t subTypeSize, uint16_t port, Inet::InterfaceId interfaceId, const chip::ByteSpan & mac, DnssdServiceProtocol procotol, PeerId peerId); + static void HandleNodeIdResolve(void * context, DnssdService * result, const Span & addresses, + CHIP_ERROR error); + State mState = State::kUninitialized; uint8_t mCommissionableInstanceName[sizeof(uint64_t)]; ResolverProxy mResolverProxy; + OperationalResolveDelegate * mOperationalDelegate = nullptr; static DiscoveryImplPlatform sManager; }; diff --git a/src/lib/dnssd/ResolverProxy.h b/src/lib/dnssd/ResolverProxy.h index feff0a6bb4da73..176316ef8cd053 100644 --- a/src/lib/dnssd/ResolverProxy.h +++ b/src/lib/dnssd/ResolverProxy.h @@ -23,40 +23,12 @@ namespace chip { namespace Dnssd { -class ResolverDelegateProxy : public ReferenceCounted, - public OperationalResolveDelegate, - public CommissioningResolveDelegate +class ResolverDelegateProxy : public ReferenceCounted, public CommissioningResolveDelegate { public: - void SetOperationalDelegate(OperationalResolveDelegate * delegate) { mOperationalDelegate = delegate; } void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) { mCommissioningDelegate = delegate; } - // OperationalResolveDelegate - void OnOperationalNodeResolved(const ResolvedNodeData & nodeData) override - { - if (mOperationalDelegate != nullptr) - { - mOperationalDelegate->OnOperationalNodeResolved(nodeData); - } - else - { - ChipLogError(Discovery, "Missing operational delegate. Data discarded."); - } - } - - void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override - { - if (mOperationalDelegate != nullptr) - { - mOperationalDelegate->OnOperationalNodeResolutionFailed(peerId, error); - } - else - { - ChipLogError(Discovery, "Missing operational delegate. Failure info discarded."); - } - } - // CommissioningResolveDelegate void OnNodeDiscovered(const DiscoveredNodeData & nodeData) override { @@ -71,7 +43,6 @@ class ResolverDelegateProxy : public ReferenceCounted, } private: - OperationalResolveDelegate * mOperationalDelegate = nullptr; CommissioningResolveDelegate * mCommissioningDelegate = nullptr; }; @@ -90,13 +61,6 @@ class ResolverProxy : public Resolver if (mDelegate != nullptr) { - if (mPreInitOperationalDelegate != nullptr) - { - ChipLogProgress(Discovery, "Setting operational delegate post init"); - mDelegate->SetOperationalDelegate(mPreInitOperationalDelegate); - mPreInitOperationalDelegate = nullptr; - } - if (mPreInitCommissioningDelegate != nullptr) { ChipLogProgress(Discovery, "Setting commissioning delegate post init"); @@ -112,18 +76,10 @@ class ResolverProxy : public Resolver void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { - if (mDelegate != nullptr) - { - mDelegate->SetOperationalDelegate(delegate); - } - else - { - if (delegate != nullptr) - { - ChipLogProgress(Discovery, "Delaying proxy of operational discovery: missing delegate"); - } - mPreInitOperationalDelegate = delegate; - } + /// 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 @@ -145,22 +101,23 @@ class ResolverProxy : public Resolver void Shutdown() override { VerifyOrReturn(mDelegate != nullptr); - mDelegate->SetOperationalDelegate(nullptr); mDelegate->SetCommissioningDelegate(nullptr); mDelegate->Release(); mDelegate = nullptr; } - 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 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 {} + private: ResolverDelegateProxy * mDelegate = nullptr; - OperationalResolveDelegate * mPreInitOperationalDelegate = nullptr; CommissioningResolveDelegate * mPreInitCommissioningDelegate = nullptr; // While discovery (commissionable or commissioner) is ongoing, diff --git a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp index 36ead8c9e5cb75..416c747d830cc6 100644 --- a/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp +++ b/src/lib/dnssd/Resolver_ImplMinimalMdns.cpp @@ -748,25 +748,6 @@ ResolverProxy::~ResolverProxy() Shutdown(); } -// Minimal implementation does not support associating a context to a request (while platforms implementations do). So keep -// updating the delegate that ends up being used by the server by calling 'SetOperationalDelegate'. -// This effectively allow minimal to have multiple controllers issuing requests as long the requests are serialized, but -// it won't work well if requests are issued in parallel. -CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) -{ - VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); - - ChipLogProgress(Discovery, "Resolving " ChipLogFormatX64 ":" ChipLogFormatX64 " ...", - ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); - chip::Dnssd::Resolver::Instance().SetOperationalDelegate(mDelegate); - return chip::Dnssd::Resolver::Instance().ResolveNodeId(peerId); -} - -void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) -{ - return chip::Dnssd::Resolver::Instance().NodeIdResolutionNoLongerNeeded(peerId); -} - CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) { VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE); diff --git a/src/lib/dnssd/Resolver_ImplNone.cpp b/src/lib/dnssd/Resolver_ImplNone.cpp index 2dd591aa4ad759..3d0d97f57cd449 100644 --- a/src/lib/dnssd/Resolver_ImplNone.cpp +++ b/src/lib/dnssd/Resolver_ImplNone.cpp @@ -68,13 +68,6 @@ ResolverProxy::~ResolverProxy() Shutdown(); } -CHIP_ERROR ResolverProxy::ResolveNodeId(const PeerId & peerId) -{ - return CHIP_ERROR_NOT_IMPLEMENTED; -} - -void ResolverProxy::NodeIdResolutionNoLongerNeeded(const PeerId & peerId) {} - CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter) { return CHIP_ERROR_NOT_IMPLEMENTED; diff --git a/src/lib/shell/commands/BUILD.gn b/src/lib/shell/commands/BUILD.gn index 20ec201984678b..05a6a0e2660ff4 100644 --- a/src/lib/shell/commands/BUILD.gn +++ b/src/lib/shell/commands/BUILD.gn @@ -50,6 +50,7 @@ source_set("commands") { if (chip_device_platform != "none") { sources += [ "Dns.cpp" ] + public_deps += [ "${chip_root}/src/lib/address_resolve" ] } if (chip_enable_ota_requestor && chip_device_platform != "none" && diff --git a/src/lib/shell/commands/Dns.cpp b/src/lib/shell/commands/Dns.cpp index 0717d4235f4d9b..83fd12fc0b3ff1 100644 --- a/src/lib/shell/commands/Dns.cpp +++ b/src/lib/shell/commands/Dns.cpp @@ -16,6 +16,7 @@ */ #include +#include #include #include #include @@ -39,40 +40,47 @@ Shell::Engine sShellDnsBrowseSubcommands; Shell::Engine sShellDnsSubcommands; Dnssd::ResolverProxy sResolverProxy; -class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, public Dnssd::CommissioningResolveDelegate +class DnsShellResolverDelegate : public Dnssd::CommissioningResolveDelegate, public AddressResolve::NodeListener { public: - void OnOperationalNodeResolved(const Dnssd::ResolvedNodeData & nodeData) override + DnsShellResolverDelegate() { mSelfHandle.SetListener(this); } + + void OnNodeAddressResolved(const PeerId & peerId, const AddressResolve::ResolveResult & result) override { - sResolverProxy.NodeIdResolutionNoLongerNeeded(nodeData.operationalData.peerId); streamer_printf(streamer_get(), "DNS resolve for " ChipLogFormatX64 "-" ChipLogFormatX64 " succeeded:\r\n", - ChipLogValueX64(nodeData.operationalData.peerId.GetCompressedFabricId()), - ChipLogValueX64(nodeData.operationalData.peerId.GetNodeId())); - streamer_printf(streamer_get(), " Hostname: %s\r\n", nodeData.resolutionData.hostName); - for (size_t i = 0; i < nodeData.resolutionData.numIPs; ++i) - { - streamer_printf(streamer_get(), " IP address: %s\r\n", nodeData.resolutionData.ipAddress[i].ToString(ipAddressBuf)); - } - streamer_printf(streamer_get(), " Port: %u\r\n", nodeData.resolutionData.port); - - auto retryInterval = nodeData.resolutionData.GetMrpRetryIntervalIdle(); - - if (retryInterval.HasValue()) - streamer_printf(streamer_get(), " MRP retry interval (idle): %" PRIu32 "ms\r\n", retryInterval.Value()); - - retryInterval = nodeData.resolutionData.GetMrpRetryIntervalActive(); - - if (retryInterval.HasValue()) - streamer_printf(streamer_get(), " MRP retry interval (active): %" PRIu32 "ms\r\n", retryInterval.Value()); - - streamer_printf(streamer_get(), " Supports TCP: %s\r\n", nodeData.resolutionData.supportsTcp ? "yes" : "no"); + ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId())); + + char addr_string[Transport::PeerAddress::kMaxToStringSize]; + result.address.ToString(addr_string); + + streamer_printf(streamer_get(), "Resolve completed: %s\r\n", addr_string); + streamer_printf(streamer_get(), " Supports TCP: %s\r\n", result.supportsTcp ? "YES" : "NO"); + streamer_printf(streamer_get(), " MRP IDLE retransmit timeout: %u ms\r\n", + result.mrpRemoteConfig.mIdleRetransTimeout.count()); + streamer_printf(streamer_get(), " MRP ACTIVE retransmit timeout: %u ms\r\n", + result.mrpRemoteConfig.mActiveRetransTimeout.count()); + streamer_printf(streamer_get(), " MRP ACTIVE Threshold timet: %u ms\r\n", + result.mrpRemoteConfig.mActiveThresholdTime.count()); + + // Schedule a retry. Not called directly so we do not recurse in OnNodeAddressResolved + DeviceLayer::SystemLayer().ScheduleLambda([this] { + CHIP_ERROR err = AddressResolve::Resolver::Instance().TryNextResult(Handle()); + if (err != CHIP_NO_ERROR && err != CHIP_ERROR_WELL_EMPTY) + { + ChipLogError(Discovery, "Failed to list next result: %" CHIP_ERROR_FORMAT, err.Format()); + } + }); } - void OnOperationalNodeResolutionFailed(const PeerId & peerId, CHIP_ERROR error) override + void OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason) override { - sResolverProxy.NodeIdResolutionNoLongerNeeded(peerId); + streamer_printf(streamer_get(), + "DNS resolve for " ChipLogFormatX64 "-" ChipLogFormatX64 " failed: %" CHIP_ERROR_FORMAT "\r\n", + ChipLogValueX64(peerId.GetCompressedFabricId()), ChipLogValueX64(peerId.GetNodeId()), reason.Format()); } + AddressResolve::NodeLookupHandle & Handle() { return mSelfHandle; } + void OnNodeDiscovered(const Dnssd::DiscoveredNodeData & nodeData) override { if (!nodeData.resolutionData.IsValid()) @@ -118,6 +126,7 @@ class DnsShellResolverDelegate : public Dnssd::OperationalResolveDelegate, publi private: char ipAddressBuf[Inet::IPAddress::kMaxStringLength]; + AddressResolve::NodeLookupHandle mSelfHandle; }; DnsShellResolverDelegate sDnsShellResolverDelegate; @@ -126,13 +135,19 @@ CHIP_ERROR ResolveHandler(int argc, char ** argv) { VerifyOrReturnError(argc == 2, CHIP_ERROR_INVALID_ARGUMENT); + if (sDnsShellResolverDelegate.Handle().IsActive()) + { + streamer_printf(streamer_get(), "Cancelling previous resolve...\r\n"); + LogErrorOnFailure(AddressResolve::Resolver::Instance().CancelLookup(sDnsShellResolverDelegate.Handle(), + AddressResolve::Resolver::FailureCallback::Call)); + } + streamer_printf(streamer_get(), "Resolving ...\r\n"); - PeerId peerId; - peerId.SetCompressedFabricId(strtoull(argv[0], nullptr, 10)); - peerId.SetNodeId(strtoull(argv[1], nullptr, 10)); + AddressResolve::NodeLookupRequest request( + PeerId().SetCompressedFabricId(strtoull(argv[0], nullptr, 10)).SetNodeId(strtoull(argv[1], nullptr, 10))); - return sResolverProxy.ResolveNodeId(peerId); + return AddressResolve::Resolver::Instance().LookupNode(request, sDnsShellResolverDelegate.Handle()); } bool ParseSubType(int argc, char ** argv, Dnssd::DiscoveryFilter & filter) @@ -230,7 +245,6 @@ CHIP_ERROR DnsHandler(int argc, char ** argv) } sResolverProxy.Init(DeviceLayer::UDPEndPointManager()); - sResolverProxy.SetOperationalDelegate(&sDnsShellResolverDelegate); sResolverProxy.SetCommissioningDelegate(&sDnsShellResolverDelegate); return sShellDnsSubcommands.ExecCommand(argc, argv); diff --git a/src/platform/Linux/DnssdImpl.cpp b/src/platform/Linux/DnssdImpl.cpp index fa72836ef8b160..5f46dd24d8728f 100644 --- a/src/platform/Linux/DnssdImpl.cpp +++ b/src/platform/Linux/DnssdImpl.cpp @@ -779,18 +779,69 @@ void MdnsAvahi::HandleBrowse(AvahiServiceBrowser * browser, AvahiIfIndex interfa } } +MdnsAvahi::ResolveContext * MdnsAvahi::AllocateResolveContext() +{ + ResolveContext * context = chip::Platform::New(); + if (context == nullptr) + { + return nullptr; + } + + context->mNumber = mResolveCount++; + mAllocatedResolves.push_back(context); + + return context; +} + +MdnsAvahi::ResolveContext * MdnsAvahi::ResolveContextForHandle(size_t handle) +{ + for (auto it : mAllocatedResolves) + { + if (it->mNumber == handle) + { + return it; + } + } + return nullptr; +} + +void MdnsAvahi::FreeResolveContext(size_t handle) +{ + for (auto it = mAllocatedResolves.begin(); it != mAllocatedResolves.end(); it++) + { + if ((*it)->mNumber == handle) + { + chip::Platform::Delete(*it); + mAllocatedResolves.erase(it); + return; + } + } +} + +void MdnsAvahi::StopResolve(const char * name) +{ + auto truncate_end = std::remove_if(mAllocatedResolves.begin(), mAllocatedResolves.end(), + [name](ResolveContext * ctx) { return strcmp(ctx->mName, name) == 0; }); + + for (auto it = truncate_end; it != mAllocatedResolves.end(); it++) + { + (*it)->mCallback((*it)->mContext, nullptr, Span(), CHIP_ERROR_CANCELLED); + chip::Platform::Delete(*it); + } + + mAllocatedResolves.erase(truncate_end, mAllocatedResolves.end()); +} + CHIP_ERROR MdnsAvahi::Resolve(const char * name, const char * type, DnssdServiceProtocol protocol, Inet::IPAddressType addressType, Inet::IPAddressType transportType, Inet::InterfaceId interface, DnssdResolveCallback callback, void * context) { - AvahiServiceResolver * resolver; AvahiIfIndex avahiInterface = static_cast(interface.GetPlatformInterface()); - ResolveContext * resolveContext = chip::Platform::New(); + ResolveContext * resolveContext = AllocateResolveContext(); CHIP_ERROR error = CHIP_NO_ERROR; - - resolveContext->mInstance = this; - resolveContext->mCallback = callback; - resolveContext->mContext = context; + resolveContext->mInstance = this; + resolveContext->mCallback = callback; + resolveContext->mContext = context; if (!interface.IsPresent()) { @@ -803,15 +854,17 @@ CHIP_ERROR MdnsAvahi::Resolve(const char * name, const char * type, DnssdService resolveContext->mAddressType = ToAvahiProtocol(addressType); resolveContext->mFullType = GetFullType(type, protocol); - resolver = avahi_service_resolver_new(mClient, avahiInterface, resolveContext->mTransport, name, - resolveContext->mFullType.c_str(), nullptr, resolveContext->mAddressType, - static_cast(0), HandleResolve, resolveContext); + AvahiServiceResolver * resolver = + avahi_service_resolver_new(mClient, avahiInterface, resolveContext->mTransport, name, resolveContext->mFullType.c_str(), + nullptr, resolveContext->mAddressType, static_cast(0), HandleResolve, + reinterpret_cast(resolveContext->mNumber)); // Otherwise the resolver will be freed in the callback if (resolver == nullptr) { error = CHIP_ERROR_INTERNAL; chip::Platform::Delete(resolveContext); } + resolveContext->mResolver = resolver; return error; } @@ -821,9 +874,16 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte const char * host_name, const AvahiAddress * address, uint16_t port, AvahiStringList * txt, AvahiLookupResultFlags flags, void * userdata) { - ResolveContext * context = reinterpret_cast(userdata); + size_t handle = reinterpret_cast(userdata); + ResolveContext * context = sInstance.ResolveContextForHandle(handle); std::vector textEntries; + if (context == nullptr) + { + ChipLogError(Discovery, "Invalid context for handling resolves: %ld", static_cast(handle)); + return; + } + switch (event) { case AVAHI_RESOLVER_FAILURE: @@ -831,14 +891,14 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte { ChipLogProgress(DeviceLayer, "Re-trying resolve"); avahi_service_resolver_free(resolver); - resolver = avahi_service_resolver_new(context->mInstance->mClient, context->mInterface, context->mTransport, - context->mName, context->mFullType.c_str(), nullptr, context->mAddressType, - static_cast(0), HandleResolve, context); - if (resolver == nullptr) + context->mResolver = avahi_service_resolver_new( + context->mInstance->mClient, context->mInterface, context->mTransport, context->mName, context->mFullType.c_str(), + nullptr, context->mAddressType, static_cast(0), HandleResolve, context); + if (context->mResolver == nullptr) { ChipLogError(DeviceLayer, "Avahi resolve failed on retry"); context->mCallback(context->mContext, nullptr, Span(), CHIP_ERROR_INTERNAL); - chip::Platform::Delete(context); + sInstance.FreeResolveContext(handle); } return; } @@ -930,8 +990,7 @@ void MdnsAvahi::HandleResolve(AvahiServiceResolver * resolver, AvahiIfIndex inte break; } - avahi_service_resolver_free(resolver); - chip::Platform::Delete(context); + sInstance.FreeResolveContext(handle); } CHIP_ERROR ChipDnssdInit(DnssdAsyncReturnCallback initCallback, DnssdAsyncReturnCallback errorCallback, void * context) @@ -988,7 +1047,10 @@ CHIP_ERROR ChipDnssdResolve(DnssdService * browseResult, chip::Inet::InterfaceId browseResult->mAddressType, Inet::IPAddressType::kAny, interface, callback, context); } -void ChipDnssdResolveNoLongerNeeded(const char * instanceName) {} +void ChipDnssdResolveNoLongerNeeded(const char * instanceName) +{ + MdnsAvahi::GetInstance().StopResolve(instanceName); +} CHIP_ERROR ChipDnssdReconfirmRecord(const char * hostname, chip::Inet::IPAddress address, chip::Inet::InterfaceId interface) { diff --git a/src/platform/Linux/DnssdImpl.h b/src/platform/Linux/DnssdImpl.h index e875e97830ebbf..2914a888c29381 100644 --- a/src/platform/Linux/DnssdImpl.h +++ b/src/platform/Linux/DnssdImpl.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -115,6 +116,7 @@ class MdnsAvahi CHIP_ERROR Resolve(const char * name, const char * type, DnssdServiceProtocol protocol, chip::Inet::IPAddressType addressType, chip::Inet::IPAddressType transportType, chip::Inet::InterfaceId interface, DnssdResolveCallback callback, void * context); + void StopResolve(const char * name); Poller & GetPoller() { return mPoller; } @@ -137,6 +139,7 @@ class MdnsAvahi struct ResolveContext { + size_t mNumber; // unique number for this context MdnsAvahi * mInstance; DnssdResolveCallback mCallback; void * mContext; @@ -145,12 +148,29 @@ class MdnsAvahi AvahiProtocol mTransport; AvahiProtocol mAddressType; std::string mFullType; - uint8_t mAttempts = 0; + uint8_t mAttempts = 0; + AvahiServiceResolver * mResolver = nullptr; + + ~ResolveContext() + { + if (mResolver != nullptr) + { + avahi_service_resolver_free(mResolver); + mResolver = nullptr; + } + } }; MdnsAvahi() : mClient(nullptr) {} static MdnsAvahi sInstance; + /// Allocates a new resolve context with a unique `mNumber` + ResolveContext * AllocateResolveContext(); + + ResolveContext * ResolveContextForHandle(size_t handle); + void FreeResolveContext(size_t handle); + void FreeResolveContext(const char * name); + static void HandleClientState(AvahiClient * client, AvahiClientState state, void * context); void HandleClientState(AvahiClient * client, AvahiClientState state); @@ -174,6 +194,10 @@ class MdnsAvahi std::map mPublishedGroups; Poller mPoller; static constexpr size_t kMaxBrowseRetries = 4; + + // Handling of allocated resolves + size_t mResolveCount = 0; + std::list mAllocatedResolves; }; } // namespace Dnssd diff --git a/src/test_driver/openiotsdk/integration-tests/common/utils.py b/src/test_driver/openiotsdk/integration-tests/common/utils.py index 5b6e5c7f3d3332..1865cf6274f6ed 100644 --- a/src/test_driver/openiotsdk/integration-tests/common/utils.py +++ b/src/test_driver/openiotsdk/integration-tests/common/utils.py @@ -72,6 +72,7 @@ def discover_device(devCtrl, setupPayload): if not res: log.info("Device not found") return None + log.info("Device found at %r" % res[0]) return res[0] @@ -87,6 +88,8 @@ def connect_device(devCtrl, setupPayload, commissionableDevice, nodeId=None): if nodeId is None: nodeId = random.randint(1, 1000000) + log.info("Connecting to device %d" % nodeId) + pincode = int(setupPayload.attributes['SetUpPINCode']) try: res = devCtrl.CommissionOnNetwork( @@ -95,7 +98,7 @@ def connect_device(devCtrl, setupPayload, commissionableDevice, nodeId=None): log.error("Commission discovered device failed {}".format(str(ex))) return None if not res: - log.info("Commission discovered device failed") + log.info("Commission discovered device failed: %r" % res) return None return nodeId