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 <[email protected]>
project-chip · Sep 22, 2023 · db164d8 · db164d8
1 parent bdfbfe8
commit db164d8
Show file tree

Hide file tree

Showing 11 changed files with 247 additions and 230 deletions.
diff --git 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
@@ -55,70 +55,6 @@ static void HandleNodeResolve(void * context, DnssdService * result, const Span<
     proxy->Release();
 }
 
-static void HandleNodeIdResolve(void * context, DnssdService * result, const Span<Inet::IPAddress> & addresses, CHIP_ERROR error)
-{
-    ResolverDelegateProxy * proxy = static_cast<ResolverDelegateProxy *>(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<const uint8_t *>(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<ResolverDelegateProxy *>(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<Inet::IPAddress> & addresses,
+                                                CHIP_ERROR error)
+{
+    DiscoveryImplPlatform * impl = static_cast<DiscoveryImplPlatform *>(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<const uint8_t *>(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<Inet::IPAddress> & 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
@@ -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<Inet::IPAddress> & 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
@@ -23,40 +23,12 @@
 namespace chip {
 namespace Dnssd {
 
-class ResolverDelegateProxy : public ReferenceCounted<ResolverDelegateProxy>,
-                              public OperationalResolveDelegate,
-                              public CommissioningResolveDelegate
+class ResolverDelegateProxy : public ReferenceCounted<ResolverDelegateProxy>, 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<ResolverDelegateProxy>,
     }
 
 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
@@ -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
@@ -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;
-Original file line number
+Diff line change
@@ Expand Up / @@ -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
@@ Expand Down @@