From 1372097b133629cd03f962747b8fea18fd6a52c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Wed, 22 Jun 2022 04:45:29 +0200 Subject: [PATCH] [OpenThread] Do not re-add existing services (#19803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 <77852424+jmeg-sfy@users.noreply.github.com> Co-authored-by: Jonathan Mégevand <77852424+jmeg-sfy@users.noreply.github.com> --- ...nericThreadStackManagerImpl_OpenThread.cpp | 80 +++++++++++++++---- ...GenericThreadStackManagerImpl_OpenThread.h | 4 +- .../Zephyr/CHIPDevicePlatformConfig.h | 1 - .../nrfconnect/CHIPDevicePlatformConfig.h | 1 - 4 files changed, 68 insertions(+), 18 deletions(-) diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp index 38cbf44b8408d0..d48a5ac5000105 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp @@ -1959,10 +1959,12 @@ void GenericThreadStackManagerImpl_OpenThread::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::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 @@ -2057,10 +2064,46 @@ void GenericThreadStackManagerImpl_OpenThread::OnSrpClientStateChange } template -bool GenericThreadStackManagerImpl_OpenThread::SrpClient::Service::Matches(const char * aInstanceName, - const char * aName) const +bool GenericThreadStackManagerImpl_OpenThread::SrpClient::Service::Matches(const char * instanceName, + const char * name) const +{ + return IsUsed() && (strcmp(mService.mInstanceName, instanceName) == 0) && (strcmp(mService.mName, name) == 0); +} + +template +bool GenericThreadStackManagerImpl_OpenThread::SrpClient::Service::Matches( + const char * instanceName, const char * name, uint16_t port, const Span & subTypes, + const Span & 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 @@ -2085,9 +2128,16 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_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 index 7a61600580acf5..9e0d9c0acac32f 100644 --- 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 & subTypes, + const Span & txtEntries) const; }; char mHostName[Dnssd::kHostNameMaxLength + 1]; diff --git a/src/platform/Zephyr/CHIPDevicePlatformConfig.h b/src/platform/Zephyr/CHIPDevicePlatformConfig.h index c591a51af4c070..6916dea46d5795 100644 --- 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 index fb6617d34d3dd8..73919004875c82 100644 --- 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