Skip to content

Commit bd83c1a

Browse files
bzbarsky-applebzbarsky-apple
authored
Add an API to stop a DNS-SD browse operation. (#22823)
* Add an API to stop a DNS-SD browse operation. Most backends don't implement this yet. Darwin does, and no longer stops Browse operations itself. Fixes #19194 May provide a way toward fixing #13275 * Address review comments. * Address more review comments.
1 parent fa9f977 commit bd83c1a

29 files changed

+256
-22
lines changed

‎examples/chip-tool/commands/pairing/PairingCommand.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,8 @@ void PairingCommand::OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData &
237237
nodeData.resolutionData.ipAddress[0].ToString(buf);
238238
ChipLogProgress(chipTool, "Discovered Device: %s:%u", buf, port);
239239

240-
// Stop Mdns discovery. Is it the right method ?
240+
// Stop Mdns discovery.
241+
CurrentCommissioner().StopCommissionableDiscovery();
241242
CurrentCommissioner().RegisterDeviceDiscoveryDelegate(nullptr);
242243

243244
Inet::InterfaceId interfaceId =

‎src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ CHIP_ERROR DiscoveryCommands::SetupDiscoveryCommands()
125125

126126
CHIP_ERROR DiscoveryCommands::TearDownDiscoveryCommands()
127127
{
128+
mDNSResolver.StopDiscovery();
128129
mDNSResolver.SetOperationalDelegate(nullptr);
129130
mDNSResolver.SetCommissioningDelegate(nullptr);
130131
return CHIP_NO_ERROR;

‎src/controller/CHIPDeviceController.cpp

+5
Original file line numberDiff line numberDiff line change
@@ -1431,6 +1431,11 @@ CHIP_ERROR DeviceCommissioner::DiscoverCommissionableNodes(Dnssd::DiscoveryFilte
14311431
return mDNSResolver.DiscoverCommissionableNodes(filter);
14321432
}
14331433

1434+
CHIP_ERROR DeviceCommissioner::StopCommissionableDiscovery()
1435+
{
1436+
return mDNSResolver.StopDiscovery();
1437+
}
1438+
14341439
const Dnssd::DiscoveredNodeData * DeviceCommissioner::GetDiscoveredDevice(int idx)
14351440
{
14361441
return GetDiscoveredNode(idx);

‎src/controller/CHIPDeviceController.h

+6
Original file line numberDiff line numberDiff line change
@@ -597,6 +597,12 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
597597
*/
598598
CHIP_ERROR DiscoverCommissionableNodes(Dnssd::DiscoveryFilter filter);
599599

600+
/**
601+
* Stop commissionable discovery triggered by a previous
602+
* DiscoverCommissionableNodes call.
603+
*/
604+
CHIP_ERROR StopCommissionableDiscovery();
605+
600606
/**
601607
* @brief
602608
* Returns information about discovered devices.

‎src/controller/SetUpCodePairer.cpp

+2
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,8 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverIP()
187187

188188
mWaitingForDiscovery[kIPTransport] = false;
189189
currentFilter.type = Dnssd::DiscoveryFilterType::kNone;
190+
191+
mCommissioner->StopCommissionableDiscovery();
190192
return CHIP_NO_ERROR;
191193
}
192194

‎src/controller/tests/TestCommissionableNodeController.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ class MockResolver : public Resolver
4242
{
4343
return CHIP_ERROR_NOT_IMPLEMENTED;
4444
}
45+
CHIP_ERROR StopDiscovery() override { return CHIP_ERROR_NOT_IMPLEMENTED; }
4546

4647
CHIP_ERROR InitStatus = CHIP_NO_ERROR;
4748
CHIP_ERROR ResolveNodeIdStatus = CHIP_NO_ERROR;

‎src/lib/dnssd/ActiveResolveAttempts.cpp

+13
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,19 @@ void ActiveResolveAttempts::CompleteIpResolution(SerializedQNameIterator targetH
7979
}
8080
}
8181

82+
CHIP_ERROR ActiveResolveAttempts::CompleteAllBrowses()
83+
{
84+
for (auto & item : mRetryQueue)
85+
{
86+
if (item.attempt.IsBrowse())
87+
{
88+
item.attempt.Clear();
89+
}
90+
}
91+
92+
return CHIP_NO_ERROR;
93+
}
94+
8295
void ActiveResolveAttempts::MarkPending(const chip::PeerId & peerId)
8396
{
8497
MarkPending(ScheduledAttempt(peerId, /* firstSend */ true));

‎src/lib/dnssd/ActiveResolveAttempts.h

+4
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,10 @@ class ActiveResolveAttempts
204204
void Complete(const chip::Dnssd::DiscoveredNodeData & data);
205205
void CompleteIpResolution(SerializedQNameIterator targetHostName);
206206

207+
/// Mark all browse-type scheduled attemptes as a success, removing them
208+
/// from the internal list.
209+
CHIP_ERROR CompleteAllBrowses();
210+
207211
/// Mark that a resolution is pending, adding it to the internal list
208212
///
209213
/// Once this complete, this peer id will be returned immediately

‎src/lib/dnssd/Discovery_ImplPlatform.cpp

+49-4
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,14 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser
153153

154154
if (error != CHIP_NO_ERROR)
155155
{
156+
// We don't have access to the ResolverProxy here to clear out its
157+
// mDiscoveryContext. The underlying implementation of
158+
// ChipDnssdStopBrowse needs to handle a possibly-stale reference
159+
// safely, so this won't lead to crashes, but it can lead to
160+
// mis-behavior if a stale mDiscoveryContext happens to match a newer
161+
// browse operation.
162+
//
163+
// TODO: Have a way to clear that state here.
156164
proxy->Release();
157165
return;
158166
}
@@ -174,6 +182,14 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser
174182

175183
if (finalBrowse)
176184
{
185+
// We don't have access to the ResolverProxy here to clear out its
186+
// mDiscoveryContext. The underlying implementation of
187+
// ChipDnssdStopBrowse needs to handle a possibly-stale reference
188+
// safely, so this won't lead to crashes, but it can lead to
189+
// mis-behavior if a stale mDiscoveryContext happens to match a newer
190+
// browse operation.
191+
//
192+
// TODO: Have a way to clear that state here.
177193
proxy->Release();
178194
}
179195
}
@@ -616,6 +632,12 @@ CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissioners(DiscoveryFilter filter)
616632
return mResolverProxy.DiscoverCommissioners(filter);
617633
}
618634

635+
CHIP_ERROR DiscoveryImplPlatform::StopDiscovery()
636+
{
637+
ReturnErrorOnFailure(InitImpl());
638+
return mResolverProxy.StopDiscovery();
639+
}
640+
619641
DiscoveryImplPlatform & DiscoveryImplPlatform::GetInstance()
620642
{
621643
return sManager;
@@ -656,6 +678,8 @@ ResolverProxy::~ResolverProxy()
656678

657679
CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
658680
{
681+
StopDiscovery();
682+
659683
VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
660684
mDelegate->Retain();
661685

@@ -674,12 +698,17 @@ CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
674698
char serviceName[kMaxCommissionableServiceNameSize];
675699
ReturnErrorOnFailure(MakeServiceTypeName(serviceName, sizeof(serviceName), filter, DiscoveryType::kCommissionableNode));
676700

677-
return ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
678-
Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate);
701+
intptr_t browseIdentifier;
702+
ReturnErrorOnFailure(ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
703+
Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate, &browseIdentifier));
704+
mDiscoveryContext.Emplace(browseIdentifier);
705+
return CHIP_NO_ERROR;
679706
}
680707

681708
CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
682709
{
710+
StopDiscovery();
711+
683712
VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
684713
mDelegate->Retain();
685714

@@ -698,8 +727,24 @@ CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
698727
char serviceName[kMaxCommissionerServiceNameSize];
699728
ReturnErrorOnFailure(MakeServiceTypeName(serviceName, sizeof(serviceName), filter, DiscoveryType::kCommissionerNode));
700729

701-
return ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
702-
Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate);
730+
intptr_t browseIdentifier;
731+
ReturnErrorOnFailure(ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
732+
Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate, &browseIdentifier));
733+
mDiscoveryContext.Emplace(browseIdentifier);
734+
return CHIP_NO_ERROR;
735+
}
736+
737+
CHIP_ERROR ResolverProxy::StopDiscovery()
738+
{
739+
if (!mDiscoveryContext.HasValue())
740+
{
741+
// No discovery going on.
742+
return CHIP_NO_ERROR;
743+
}
744+
745+
CHIP_ERROR err = ChipDnssdStopBrowse(mDiscoveryContext.Value());
746+
mDiscoveryContext.ClearValue();
747+
return err;
703748
}
704749

705750
} // namespace Dnssd

‎src/lib/dnssd/Discovery_ImplPlatform.h

+1
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
5757
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
5858
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
5959
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
60+
CHIP_ERROR StopDiscovery() override;
6061

6162
static DiscoveryImplPlatform & GetInstance();
6263

‎src/lib/dnssd/Resolver.h

+8
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,14 @@ class Resolver
390390
*/
391391
virtual CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) = 0;
392392

393+
/**
394+
* Stop discovery (of commissionable or commissioner nodes).
395+
*
396+
* Some back ends may not support stopping discovery, so consumers should
397+
* not assume they will stop getting callbacks after calling this.
398+
*/
399+
virtual CHIP_ERROR StopDiscovery() = 0;
400+
393401
/**
394402
* Provides the system-wide implementation of the service resolver
395403
*/

‎src/lib/dnssd/ResolverProxy.h

+5
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,16 @@ class ResolverProxy : public Resolver
152152
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
153153
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
154154
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
155+
CHIP_ERROR StopDiscovery() override;
155156

156157
private:
157158
ResolverDelegateProxy * mDelegate = nullptr;
158159
OperationalResolveDelegate * mPreInitOperationalDelegate = nullptr;
159160
CommissioningResolveDelegate * mPreInitCommissioningDelegate = nullptr;
161+
162+
// While discovery (commissionable or commissioner) is ongoing,
163+
// mDiscoveryContext may have a value to allow StopDiscovery to work.
164+
Optional<intptr_t> mDiscoveryContext;
160165
};
161166

162167
} // namespace Dnssd

‎src/lib/dnssd/Resolver_ImplMinimalMdns.cpp

+11
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate
281281
CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
282282
CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
283283
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
284+
CHIP_ERROR StopDiscovery() override;
284285

285286
private:
286287
OperationalResolveDelegate * mOperationalDelegate = nullptr;
@@ -633,6 +634,11 @@ CHIP_ERROR MinMdnsResolver::DiscoverCommissioners(DiscoveryFilter filter)
633634
return BrowseNodes(DiscoveryType::kCommissionerNode, filter);
634635
}
635636

637+
CHIP_ERROR MinMdnsResolver::StopDiscovery()
638+
{
639+
return mActiveResolves.CompleteAllBrowses();
640+
}
641+
636642
CHIP_ERROR MinMdnsResolver::BrowseNodes(DiscoveryType type, DiscoveryFilter filter)
637643
{
638644
mActiveResolves.MarkPending(filter, type);
@@ -712,5 +718,10 @@ CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
712718
return chip::Dnssd::Resolver::Instance().DiscoverCommissioners(filter);
713719
}
714720

721+
CHIP_ERROR ResolverProxy::StopDiscovery()
722+
{
723+
return CHIP_ERROR_NOT_IMPLEMENTED;
724+
}
725+
715726
} // namespace Dnssd
716727
} // namespace chip

‎src/lib/dnssd/Resolver_ImplNone.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ class NoneResolver : public Resolver
4242
return CHIP_ERROR_NOT_IMPLEMENTED;
4343
}
4444
CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
45+
CHIP_ERROR StopDiscovery() override { return CHIP_ERROR_NOT_IMPLEMENTED; }
4546
};
4647

4748
NoneResolver gResolver;
@@ -73,5 +74,10 @@ CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
7374
return CHIP_ERROR_NOT_IMPLEMENTED;
7475
}
7576

77+
CHIP_ERROR ResolverProxy::StopDiscovery()
78+
{
79+
return CHIP_ERROR_NOT_IMPLEMENTED;
80+
}
81+
7682
} // namespace Dnssd
7783
} // namespace chip

‎src/lib/dnssd/platform/Dnssd.h

+18-1
Original file line numberDiff line numberDiff line change
@@ -203,14 +203,31 @@ CHIP_ERROR ChipDnssdFinalizeServiceUpdate();
203203
* @param[in] interface The interface to send queries.
204204
* @param[in] callback The callback for found services.
205205
* @param[in] context The user context.
206+
* @param[out] browseIdentifier an identifier for this browse operation. This
207+
* can be used to call ChipDnssdStopBrowse. Only
208+
* set on success. This value remains valid until
209+
* the browse callback is called with an error or
210+
* is called with finalBrowse set to true.
206211
*
207212
* @retval CHIP_NO_ERROR The browse succeeds.
208213
* @retval CHIP_ERROR_INVALID_ARGUMENT The type or callback is nullptr.
209214
* @retval Error code The browse fails.
210215
*
211216
*/
212217
CHIP_ERROR ChipDnssdBrowse(const char * type, DnssdServiceProtocol protocol, chip::Inet::IPAddressType addressType,
213-
chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context);
218+
chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context,
219+
intptr_t * browseIdentifier);
220+
221+
/**
222+
* Stop an ongoing browse, if supported by this backend. If successful, this
223+
* will trigger a final callback, with either an error status or finalBrowse set
224+
* to true, to the DnssdBrowseCallback that was passed to the ChipDnssdBrowse
225+
* call that handed back this browseIdentifier.
226+
*
227+
* @param browseIdentifier an identifier for a browse operation that came from
228+
* ChipDnssdBrowse.
229+
*/
230+
CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier);
214231

215232
/**
216233
* This function resolves the services published by mDNS

‎src/platform/Ameba/DnssdImpl.cpp

100755100644
+7-1
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,13 @@ CHIP_ERROR ChipDnssdStopPublish()
9797
}
9898

9999
CHIP_ERROR ChipDnssdBrowse(const char * type, DnssdServiceProtocol protocol, chip::Inet::IPAddressType addressType,
100-
chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context);
100+
chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context,
101+
intptr_t * browseIdentifier);
102+
{
103+
return CHIP_ERROR_NOT_IMPLEMENTED;
104+
}
105+
106+
CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier)
101107
{
102108
return CHIP_ERROR_NOT_IMPLEMENTED;
103109
}

‎src/platform/Darwin/DnssdContexts.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,12 @@ void BrowseContext::DispatchSuccess()
299299
MdnsContexts::GetInstance().Remove(this);
300300
}
301301

302+
void BrowseContext::DispatchPartialSuccess()
303+
{
304+
callback(context, services.data(), services.size(), false, CHIP_NO_ERROR);
305+
services.clear();
306+
}
307+
302308
ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType)
303309
{
304310
type = ContextType::Resolve;

0 commit comments

Comments
 (0)