Skip to content

Commit

Permalink
[dnssd] Replace invalid forced reset logic (project-chip#24698)
Browse files Browse the repository at this point in the history 
* [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
  • Loading branch information
@Damian-Nordic @kkasperczyk-no
Damian-Nordic authored and kkasperczyk-no committed Feb 1, 2023
1 parent e7b1fca commit 902b224
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 46 deletions.
12 changes: 8 additions & 4 deletions src/app/server/Dnssd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/include/platform/CHIPDeviceEvent.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
50 changes: 17 additions & 33 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,20 +358,21 @@ 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;
}

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)
Expand All @@ -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.
Expand All @@ -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<DiscoveryImplPlatform *>(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
{
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -607,10 +595,6 @@ CHIP_ERROR DiscoveryImplPlatform::RemoveServices()
{
ReturnErrorOnFailure(ChipDnssdRemoveServices());

mIsOperationalNodePublishing = false;
mIsCommissionableNodePublishing = false;
mIsCommissionerNodePublishing = false;

return CHIP_NO_ERROR;
}

Expand Down
17 changes: 8 additions & 9 deletions src/lib/dnssd/Discovery_ImplPlatform.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down

0 comments on commit 902b224

Please sign in to comment.