[OpenThread] Do not re-add existing services (#19803)

* [OpenThread] Do not re-add existing services Currently, when a single DNS-SD service is updated, all services are removed and re-added. This approach is acceptable in the case of mDNS-based implementations, but in the case of SRP it generates unnecessary traffic to the SRP server. Also, it may generate SRP updates bigger than MTU which has only been addressed in OpenThread recently and may result in SRP update parsing errors when using older OpenThread versions. * Add parentheses Co-authored-by: Jonathan Mégevand <[email protected]> Co-authored-by: Jonathan Mégevand <[email protected]>
project-chip · Feb 9, 2024 · 1372097 · 1372097
1 parent 915caec
commit 1372097
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 18 deletions.
diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp
@@ -1959,10 +1959,12 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnSrpClientNotificatio
                                                                                   const otSrpClientService * aRemovedServices,
                                                                                   void * aContext)
 {
+    const char * errorStr = nullptr;
+
     switch (aError)
     {
     case OT_ERROR_NONE: {
-        ChipLogDetail(DeviceLayer, "OnSrpClientNotification: Last requested operation completed successfully");
+        ChipLogDetail(DeviceLayer, "SRP update succeeded");
 
         if (aHostInfo)
         {
@@ -1995,36 +1997,41 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnSrpClientNotificatio
         break;
     }
     case OT_ERROR_PARSE:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Parsing operaton failed");
+        errorStr = "parsing operation failed";
         break;
     case OT_ERROR_NOT_FOUND:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Domain name or RRset does not exist");
+        errorStr = "domain name or RRset does not exist";
         break;
     case OT_ERROR_NOT_IMPLEMENTED:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Server does not support query type");
+        errorStr = "server does not support query type";
         break;
     case OT_ERROR_SECURITY:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Operation refused for security reasons");
+        errorStr = "operation refused for security reasons";
         break;
     case OT_ERROR_DUPLICATED:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Domain name or RRset is duplicated");
+        errorStr = "domain name or RRset is duplicated";
         break;
     case OT_ERROR_RESPONSE_TIMEOUT:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Timed out waiting on server response");
+        errorStr = "timed out waiting on server response";
         break;
     case OT_ERROR_INVALID_ARGS:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Invalid service structure detected");
+        errorStr = "invalid service structure detected";
         break;
     case OT_ERROR_NO_BUFS:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Insufficient buffer to handle message");
+        errorStr = "insufficient buffer to handle message";
         break;
     case OT_ERROR_FAILED:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Internal server error occurred");
+        errorStr = "internal server error";
         break;
     default:
-        ChipLogError(DeviceLayer, "OnSrpClientNotification: Unknown error occurred");
+        errorStr = "unknown error";
         break;
     }
+
+    if (errorStr != nullptr)
+    {
+        ChipLogError(DeviceLayer, "SRP update error: %s", errorStr);
+    }
 }
 
 template <class ImplClass>
@@ -2057,10 +2064,46 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnSrpClientStateChange
 }
 
 template <class ImplClass>
-bool GenericThreadStackManagerImpl_OpenThread<ImplClass>::SrpClient::Service::Matches(const char * aInstanceName,
-                                                                                      const char * aName) const
+bool GenericThreadStackManagerImpl_OpenThread<ImplClass>::SrpClient::Service::Matches(const char * instanceName,
+                                                                                      const char * name) const
+{
+    return IsUsed() && (strcmp(mService.mInstanceName, instanceName) == 0) && (strcmp(mService.mName, name) == 0);
+}
+
+template <class ImplClass>
+bool GenericThreadStackManagerImpl_OpenThread<ImplClass>::SrpClient::Service::Matches(
+    const char * instanceName, const char * name, uint16_t port, const Span<const char * const> & subTypes,
+    const Span<const Dnssd::TextEntry> & txtEntries) const
 {
-    return IsUsed() && (strcmp(mService.mInstanceName, aInstanceName) == 0) && (strcmp(mService.mName, aName) == 0);
+    size_t myNumSubTypes = 0;
+
+    for (const char * const * mySubType = mService.mSubTypeLabels; (mySubType != nullptr) && (*mySubType != nullptr); ++mySubType)
+    {
+        myNumSubTypes++;
+    }
+
+    VerifyOrReturnError(Matches(instanceName, name) && mService.mPort == port, false);
+    VerifyOrReturnError(myNumSubTypes == subTypes.size() && mService.mNumTxtEntries == txtEntries.size(), false);
+
+    const char * const * mySubType = mService.mSubTypeLabels;
+
+    for (const char * subType : subTypes)
+    {
+        VerifyOrReturnError(strcmp(*mySubType, subType) == 0, false);
+        ++mySubType;
+    }
+
+    const otDnsTxtEntry * myTxtEntry = mService.mTxtEntries;
+
+    for (const Dnssd::TextEntry & txtEntry : txtEntries)
+    {
+        VerifyOrReturnError(strcmp(myTxtEntry->mKey, txtEntry.mKey) == 0, false);
+        VerifyOrReturnError(
+            ByteSpan(myTxtEntry->mValue, myTxtEntry->mValueLength).data_equal(ByteSpan(txtEntry.mData, txtEntry.mDataSize)), false);
+        ++myTxtEntry;
+    }
+
+    return true;
 }
 
 template <class ImplClass>
@@ -2085,9 +2128,16 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_AddSrpService(c
     // remove the possible existing entry from anywhere in the list
     for (typename SrpClient::Service & service : mSrpClient.mServices)
     {
-        // Remove possible existing entry
+        if (service.Matches(aInstanceName, aName, aPort, aSubTypes, aTxtEntries))
+        {
+            // Re-adding existing service without any changes
+            service.mIsInvalid = false;
+            ExitNow();
+        }
+
         if (service.Matches(aInstanceName, aName))
         {
+            // Updating existing service
             SuccessOrExit(error = MapOpenThreadError(otSrpClientClearService(mOTInst, &service.mService)));
             // Clear memory immediately, as OnSrpClientNotification will not be called.
             memset(&service, 0, sizeof(service));

diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h
@@ -194,7 +194,9 @@ class GenericThreadStackManagerImpl_OpenThread
             otDnsTxtEntry mTxtEntries[kTxtMaxNumber];
 
             bool IsUsed() const { return mService.mInstanceName != nullptr; }
-            bool Matches(const char * aInstanceName, const char * aName) const;
+            bool Matches(const char * instanceName, const char * name) const;
+            bool Matches(const char * instanceName, const char * name, uint16_t port, const Span<const char * const> & subTypes,
+                         const Span<const Dnssd::TextEntry> & txtEntries) const;
         };
 
         char mHostName[Dnssd::kHostNameMaxLength + 1];

diff --git a/src/platform/Zephyr/CHIPDevicePlatformConfig.h b/src/platform/Zephyr/CHIPDevicePlatformConfig.h
@@ -107,7 +107,6 @@
 
 #ifdef CONFIG_CHIP_ENABLE_DNSSD_SRP
 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1
-#define CHIP_DEVICE_CONFIG_THREAD_SRP_MAX_SERVICES (CHIP_CONFIG_MAX_FABRICS + 1)
 #ifdef CONFIG_CHIP_ENABLE_DNS_CLIENT
 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT 1
 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_COMMISSIONABLE_DISCOVERY 1

diff --git a/src/platform/nrfconnect/CHIPDevicePlatformConfig.h b/src/platform/nrfconnect/CHIPDevicePlatformConfig.h
@@ -151,7 +151,6 @@
 
 #ifdef CONFIG_CHIP_ENABLE_DNSSD_SRP
 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1
-#define CHIP_DEVICE_CONFIG_THREAD_SRP_MAX_SERVICES (CHIP_CONFIG_MAX_FABRICS + 1)
 #ifdef CONFIG_CHIP_ENABLE_DNS_CLIENT
 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT 1
 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_COMMISSIONABLE_DISCOVERY 1