[dnssd-plat] integrate DnssdPlatform into Application

[superwhd]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 43.30%. Comparing base (2b41187) to head (b4d5157).
Report is 947 commits behind head on main.

Files with missing lines	Patch %	Lines
src/host/posix/dnssd.cpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2677       +/-   ##
===========================================
- Coverage   55.77%   43.30%   -12.48%     
===========================================
  Files          87      108       +21     
  Lines        6890    13403     +6513     
  Branches        0      964      +964     
===========================================
+ Hits         3843     5804     +1961     
- Misses       3047     7292     +4245     
- Partials        0      307      +307     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Irving-cl]

LGTM 👍

[superwhd]

The CI failure should be solved when #2702 is meregd.

[abtink]

LGTM. Thanks. 👍

[superwhd]

Met CI failures in test_mdns_restart.py which I couldn't reproduce locally. Debugging...

[superwhd]

I noticed SIGPIPE signal was triggred and it made the program exit when calling mDNSResponder API. Not sure if ignoring the signal is fine.

@abtink

[abtink]

It would be better to understand why/how this is being signaled. Ignoring it may cause more harm later on, especially if it is happening due to some module not being ready or set up.

It seems to originate from us calling the mDNSResponder APIs?
My guess is that maybe we call an API somehow when the underlying mDNSResponder is not yet ready (too early)?

[superwhd]

It got SIGPIPE at this line:

ot-br-posix/src/mdns/mdns_mdnssd.cpp

Line 748 in 3f28d4c

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

by calling DNSServiceRemoveRecord. At that time the underlying mDNSResponder wasn't ready.

IIUC mDNSResponder supports turning the SIGPIPE off on OSX: https://github.com/apple-oss-distributions/mDNSResponder/blob/71e6611203d57c78b26fd505d98cb57a33d00880/mDNSShared/dnssd_clientstub.c#L839. So I think it should be fine for us to ignore it? Though the scope is different (per process vs per socket).

[abtink]

I still think it would be better to investigate and understand this better.

We have State in Publisher to track when the underlying mDNS is ready or not, and there are explicit checks before all method calls, like this:

    if (mState!= State::kReady)
    {
        error = OTBR_ERROR_INVALID_STATE;
        std::move(aCallback)(error);
        ExitNow();
    }

So why do we get to this part of the code where it may not be ready? Do we not detect that State has changed and it is not ready? Or are there some other missing state checks somewhere?

[superwhd]

The scenario is

1. mdnsd gets restarted.
2. the Publisher noticed this via kDNSServiceErr_ServiceNotRunning error code, then it tries to restart:

   ot-br-posix/src/mdns/mdns_mdnssd.cpp

   Line 391 in e2f3c1f

   Stop(kStopOnServiceNotRunningError);

3. In Stop(), it wants to remove all registrations.

   ot-br-posix/src/mdns/mdns_mdnssd.cpp

   Line 260 in e2f3c1f

   mServiceRegistrations.clear();

4. In the destructor of key registration, it unregisters itself by calling DNSServiceRemoveRecord.

   ot-br-posix/src/mdns/mdns_mdnssd.cpp

   Line 748 in e2f3c1f

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

Per your idea I think we should add/move such checks into every registration type's Register or Unregister method. I can send a PR later.

However, I think this cannot fully solve the issue. The restart of mdnsd could happen at any time. When it restarts right before the DNSServiceRemoveRecord call, it will cause the SIGPIPE before Publisher handles the state change.

[abtink]

Thanks for the details. I think we should also change order in Stop() and set mState first before the list/entry deallocation calls.

[superwhd]

Sent #2725 to apply the suggested order of actions in Stop().