From 2389952147cb73d3ca45ccc8215ee63baca84d1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Mon, 30 Jan 2023 19:00:55 +0100 Subject: [PATCH] [dnssd] Replace invalid forced reset logic (#24698) * [dnssd] Replace invalid forced reset logic The platform implementation of DNS-SD contains a mechanism for restarting the advertising when the platform returns the CHIP_ERROR_FORCED_RESET error. This mechanism is broken as it assumes that only one operational service is used and it uses extra RAM although it is only used by Linux. Switch to an event-based approach that allows the DNS-SD server to restart the advertising when needed. * [dnssd] Make sure initialization is not run twice Currently, the platform implementation of DNS-SD uses a boolean member to indicate if the backend has already been initialized, but the initialization is asynchronous so it may happen that the initialization is invoked twice. Add another "initializing" state to handle that case. * Improve comment * Fix build --- src/app/server/Dnssd.cpp | 12 ++++-- src/include/platform/CHIPDeviceEvent.h | 5 +++ src/lib/dnssd/Discovery_ImplPlatform.cpp | 50 ++++++++---------------- src/lib/dnssd/Discovery_ImplPlatform.h | 17 ++++---- 4 files changed, 38 insertions(+), 46 deletions(-) diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index 29dd3d016daadf..40f0da6f8ccca5 100644 --- a/src/app/server/Dnssd.cpp +++ b/src/app/server/Dnssd.cpp @@ -46,13 +46,17 @@ namespace { void OnPlatformEvent(const DeviceLayer::ChipDeviceEvent * event) { - if (event->Type == DeviceLayer::DeviceEventType::kDnssdPlatformInitialized + switch (event->Type) + { + case DeviceLayer::DeviceEventType::kDnssdPlatformInitialized: + case DeviceLayer::DeviceEventType::kDnssdRestartNeeded: #if CHIP_DEVICE_CONFIG_ENABLE_SED - || event->Type == DeviceLayer::DeviceEventType::kSEDIntervalChange + case DeviceLayer::DeviceEventType::kSEDIntervalChange: #endif - ) - { app::DnssdServer::Instance().StartServer(); + break; + default: + break; } } diff --git a/src/include/platform/CHIPDeviceEvent.h b/src/include/platform/CHIPDeviceEvent.h index 4d3e8353e85e13..845751eae020e3 100644 --- a/src/include/platform/CHIPDeviceEvent.h +++ b/src/include/platform/CHIPDeviceEvent.h @@ -216,6 +216,11 @@ enum PublicEventTypes */ kDnssdPlatformInitialized, + /** + * Signals that DNS-SD backend was restarted and services must be published again. + */ + kDnssdRestartNeeded, + /** * Signals that bindings were updated. */ diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index aab39d78e524a4..8068a9babf47a3 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -358,9 +358,10 @@ DiscoveryImplPlatform::DiscoveryImplPlatform() = default; CHIP_ERROR DiscoveryImplPlatform::InitImpl() { - ReturnErrorCodeIf(mDnssdInitialized, CHIP_NO_ERROR); - ReturnErrorOnFailure(ChipDnssdInit(HandleDnssdInit, HandleDnssdError, this)); + VerifyOrReturnError(mState == State::kUninitialized, CHIP_NO_ERROR); + mState = State::kInitializing; + ReturnErrorOnFailure(ChipDnssdInit(HandleDnssdInit, HandleDnssdError, this)); UpdateCommissionableInstanceName(); return CHIP_NO_ERROR; @@ -368,10 +369,10 @@ CHIP_ERROR DiscoveryImplPlatform::InitImpl() void DiscoveryImplPlatform::Shutdown() { - VerifyOrReturn(mDnssdInitialized); + VerifyOrReturn(mState != State::kUninitialized); mResolverProxy.Shutdown(); ChipDnssdShutdown(); - mDnssdInitialized = false; + mState = State::kUninitialized; } void DiscoveryImplPlatform::HandleDnssdInit(void * context, CHIP_ERROR initError) @@ -380,7 +381,7 @@ void DiscoveryImplPlatform::HandleDnssdInit(void * context, CHIP_ERROR initError if (initError == CHIP_NO_ERROR) { - publisher->mDnssdInitialized = true; + publisher->mState = State::kInitialized; // TODO: this is wrong, however we need resolverproxy initialized // otherwise DiscoveryImplPlatform is not usable. @@ -396,46 +397,35 @@ void DiscoveryImplPlatform::HandleDnssdInit(void * context, CHIP_ERROR initError // class that it is contained in). publisher->mResolverProxy.Init(nullptr); -#if !CHIP_DEVICE_LAYER_NONE // Post an event that will start advertising - chip::DeviceLayer::ChipDeviceEvent event; - event.Type = chip::DeviceLayer::DeviceEventType::kDnssdPlatformInitialized; + DeviceLayer::ChipDeviceEvent event; + event.Type = DeviceLayer::DeviceEventType::kDnssdPlatformInitialized; - CHIP_ERROR error = chip::DeviceLayer::PlatformMgr().PostEvent(&event); + CHIP_ERROR error = DeviceLayer::PlatformMgr().PostEvent(&event); if (error != CHIP_NO_ERROR) { ChipLogError(Discovery, "Posting DNS-SD platform initialized event failed with %" CHIP_ERROR_FORMAT, error.Format()); } -#endif } else { ChipLogError(Discovery, "DNS-SD initialization failed with %" CHIP_ERROR_FORMAT, initError.Format()); - publisher->mDnssdInitialized = false; + publisher->mState = State::kUninitialized; } } void DiscoveryImplPlatform::HandleDnssdError(void * context, CHIP_ERROR error) { - DiscoveryImplPlatform * publisher = static_cast(context); if (error == CHIP_ERROR_FORCED_RESET) { - if (publisher->mIsOperationalNodePublishing) - { - publisher->Advertise(publisher->mOperationalNodeAdvertisingParams); - } + DeviceLayer::ChipDeviceEvent event; + event.Type = DeviceLayer::DeviceEventType::kDnssdRestartNeeded; + error = DeviceLayer::PlatformMgr().PostEvent(&event); - if (publisher->mIsCommissionableNodePublishing) - { - publisher->Advertise(publisher->mCommissionableNodeAdvertisingParams); - } - - if (publisher->mIsCommissionerNodePublishing) + if (error != CHIP_NO_ERROR) { - publisher->Advertise(publisher->mCommissionerNodeAdvertisingParams); + ChipLogError(Discovery, "Failed to post DNS-SD restart event: %" CHIP_ERROR_FORMAT, error.Format()); } - - publisher->FinalizeServiceUpdate(); } else { @@ -496,7 +486,7 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE Inet::InterfaceId interfaceId, const chip::ByteSpan & mac, DnssdServiceProtocol protocol, PeerId peerId) { - ReturnErrorCodeIf(mDnssdInitialized == false, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mState == State::kInitialized, CHIP_ERROR_INCORRECT_STATE); DnssdService service; ReturnErrorOnFailure(MakeHostName(service.mHostName, sizeof(service.mHostName), mac)); @@ -550,9 +540,7 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE sizeof(Name##SubTypeBuf), params.Get##Name())); #define PUBLISH_RECORDS(Type) \ - ReturnErrorOnFailure(PublishService(k##Type##ServiceName, textEntries, textEntrySize, subTypes, subTypeSize, params)); \ - m##Type##NodeAdvertisingParams = params; \ - mIs##Type##NodePublishing = true; + ReturnErrorOnFailure(PublishService(k##Type##ServiceName, textEntries, textEntrySize, subTypes, subTypeSize, params)); CHIP_ERROR DiscoveryImplPlatform::Advertise(const OperationalAdvertisingParameters & params) { @@ -607,10 +595,6 @@ CHIP_ERROR DiscoveryImplPlatform::RemoveServices() { ReturnErrorOnFailure(ChipDnssdRemoveServices()); - mIsOperationalNodePublishing = false; - mIsCommissionableNodePublishing = false; - mIsCommissionerNodePublishing = false; - return CHIP_NO_ERROR; } diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h index 36c191d714bcc7..565d4afaf9b33f 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.h +++ b/src/lib/dnssd/Discovery_ImplPlatform.h @@ -64,6 +64,13 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver static DiscoveryImplPlatform & GetInstance(); private: + enum class State : uint8_t + { + kUninitialized, + kInitializing, + kInitialized + }; + DiscoveryImplPlatform(); DiscoveryImplPlatform(const DiscoveryImplPlatform &) = delete; @@ -83,16 +90,8 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver size_t subTypeSize, uint16_t port, Inet::InterfaceId interfaceId, const chip::ByteSpan & mac, DnssdServiceProtocol procotol, PeerId peerId); - OperationalAdvertisingParameters mOperationalNodeAdvertisingParams; - CommissionAdvertisingParameters mCommissionableNodeAdvertisingParams; - CommissionAdvertisingParameters mCommissionerNodeAdvertisingParams; - bool mIsOperationalNodePublishing = false; - bool mIsCommissionableNodePublishing = false; - bool mIsCommissionerNodePublishing = false; + State mState = State::kUninitialized; uint8_t mCommissionableInstanceName[sizeof(uint64_t)]; - - bool mDnssdInitialized = false; - ResolverProxy mResolverProxy; static DiscoveryImplPlatform sManager;