Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mdns] unify the behavior of Stop() between avahi and mDNSResponder #2725

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −6
Open

[mdns] unify the behavior of Stop() between avahi and mDNSResponder #2725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/mdns/mdns_avahi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,10 @@ bool PublisherAvahi::IsStarted(void) const

void PublisherAvahi::Stop(void)
{
VerifyOrExit(mState == State::kReady);

mState = State::kIdle;

mServiceRegistrations.clear();
mHostRegistrations.clear();

Expand All @@ -531,7 +535,10 @@ void PublisherAvahi::Stop(void)
mClient = nullptr;
}

mState = Mdns::Publisher::State::kIdle;
mStateCallback(mState);

exit:
return;
}

void PublisherAvahi::HandleClientState(AvahiClient *aClient, AvahiClientState aState, void *aContext)
Expand Down Expand Up @@ -664,8 +671,6 @@ void PublisherAvahi::HandleClientState(AvahiClient *aClient, AvahiClientState aS

case AVAHI_CLIENT_FAILURE:
otbrLogErr("Avahi client failed to start: %s", avahi_strerror(avahi_client_errno(aClient)));
mState = State::kIdle;
mStateCallback(mState);
Stop();
Start();
break;
Expand Down
7 changes: 5 additions & 2 deletions src/mdns/mdns_mdnssd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ void PublisherMDnsSd::Stop(StopMode aStopMode)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A general question about how the Stop() method is intended to be used:

  1. Stop() can be called when the underlying mDNS provider (e.g., mDNSResponder) is itself stopped.
  2. It may be called when the underlying mDNS provider is still active and running, but for some other reason, we are stopping all mDNS operations.

I noticed we have StopMode input with Stop(kNormalStop) or kStopOnServiceNotRunningError, which makes it seem that both cases are used and should be supported.

If we need to support both cases (which I think we need to), then I think we should handle them differently.

  • In the first case, it is okay/desired to set mState early to ensure the Clear() methods (destructors and Unregister() calls) won't call mDNSResponder APIs anymore, as it is no longer running.
  • In the second case, we do want to call mDNSResponder APIs to clear any previously registered entries (otherwise, it continues to publish the service/host info). But then setting mState early in the method will cause these to be skipped (which would be incorrect).

My suggestion is that we track whether the mDNSResponder is active in a separate variable other than mState, which we then check (along with mState) to decide if we can call any of the mDNSResponder (or Avahi) APIs. Thoughts?

VerifyOrExit(mState == State::kReady);

mState = State::kIdle;

// If we get a `kDNSServiceErr_ServiceNotRunning` and need to
// restart the `Publisher`, we should immediately de-allocate
// all `ServiceRef`. Otherwise, we first clear the `Registrations`
Expand All @@ -265,7 +267,7 @@ void PublisherMDnsSd::Stop(StopMode aStopMode)
mSubscribedServices.clear();
mSubscribedHosts.clear();

mState = State::kIdle;
mStateCallback(mState);

exit:
return;
Expand Down Expand Up @@ -756,7 +758,8 @@ void PublisherMDnsSd::DnssdKeyRegistration::Unregister(void)

VerifyOrExit(serviceRef != nullptr);

dnsError = DNSServiceRemoveRecord(serviceRef, mRecordRef, /* flags */ 0);
dnsError = DNSServiceRemoveRecord(serviceRef, mRecordRef, /* flags */ 0);
mRecordRef = nullptr;

otbrLogInfo("Unregistered key %s: error:%s", mName.c_str(), DNSErrorToString(dnsError));

Expand Down
10 changes: 9 additions & 1 deletion src/mdns/mdns_mdnssd.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,15 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
public:
using ServiceRegistration::ServiceRegistration; // Inherit base constructor

~DnssdServiceRegistration(void) override { Unregister(); }
~DnssdServiceRegistration(void) override
{
Unregister();
if (mServiceRef)
{
DNSServiceRefDeallocate(mServiceRef);
mServiceRef = nullptr;
}
}

void Update(MainloopContext &aMainloop) const;
void Process(const MainloopContext &aMainloop, std::vector<DNSServiceRef> &aReadyServices) const;
Expand Down
Loading