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

[dnssd-plat] integrate DnssdPlatform into Application #2677

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[dnssd-plat] integrate DnssdPlatform into Application #2677

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
28 changes: 27 additions & 1 deletion .github/workflows/border_router.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,59 +53,82 @@ jobs:
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=OFF -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 0
otbr_mdns: "mDNSResponder"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 1
- name: "Border Router (Avahi)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=OFF -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 0
otbr_mdns: "avahi"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 1
- name: "Border Router TREL (mDNSResponder)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=ON -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 0
otbr_mdns: "mDNSResponder"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 2
- name: "Border Router TREL (Avahi)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=ON -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 0
otbr_mdns: "avahi"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 2
- name: "Border Router MATN (mDNSResponder)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=OFF -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 0
otbr_mdns: "mDNSResponder"
cert_scripts: ./tests/scripts/thread-cert/border_router/MATN/*.py
packet_verification: 1
- name: "Border Router Internet Access Features (mDNSResponder)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=OFF -DOTBR_DNS_UPSTREAM_QUERY=ON -DOTBR_DHCP6_PD=ON"
border_routing: 1
internet: 1
dnssd_plat: 0
otbr_mdns: "mDNSResponder"
cert_scripts: ./tests/scripts/thread-cert/border_router/internet/*.py
packet_verification: 1
- name: "Backbone Router"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=OFF -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 0
internet: 0
dnssd_plat: 0
otbr_mdns: "mDNSResponder"
cert_scripts: ./tests/scripts/thread-cert/backbone/*.py
packet_verification: 1
- name: "Border Router TREL with FEATURE_FLAG (avahi)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_FEATURE_FLAGS=ON -DOTBR_TELEMETRY_DATA_API=ON -DOTBR_TREL=ON -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 0
otbr_mdns: "avahi"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 2

- name: "Border Router with OT Core Advertising Proxy (avahi)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=ON -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 1
otbr_mdns: "avahi"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 1
- name: "Border Router with OT Core Advertising Proxy (mDNSResponder)"
otbr_options: "-DOT_DUA=ON -DOT_ECDSA=ON -DOT_MLR=ON -DOT_SERVICE=ON -DOT_SRP_SERVER=ON -DOTBR_COVERAGE=ON -DOTBR_DUA_ROUTING=ON -DOTBR_TREL=ON -DOTBR_DNS_UPSTREAM_QUERY=ON"
border_routing: 1
internet: 0
dnssd_plat: 1
otbr_mdns: "mDNSResponder"
cert_scripts: ./tests/scripts/thread-cert/border_router/*.py
packet_verification: 1

name: ${{ matrix.name }}
env:
Expand All @@ -120,7 +143,9 @@ jobs:
INTER_OP_BBR: 0
BORDER_ROUTING: ${{ matrix.border_routing }}
NAT64: ${{ matrix.internet }}
DNSSD_PLAT: ${{ matrix.dnssd_plat }}
MAX_JOBS: 3
VERBOSE: 1
steps:
- uses: actions/checkout@v4
with:
Expand Down Expand Up @@ -153,6 +178,7 @@ jobs:
--build-arg NAT64_SERVICE=openthread \
--build-arg DNS64="${{ matrix.internet }}" \
--build-arg MDNS="${{ matrix.otbr_mdns }}" \
--build-arg DNSSD_PLAT="${{ matrix.dnssd_plat }}" \
--build-arg OTBR_OPTIONS="${otbr_options} -DCMAKE_CXX_FLAGS='-DOPENTHREAD_CONFIG_DNSSD_SERVER_BIND_UNSPECIFIED_NETIF=1'"
- name: Bootstrap OpenThread Test
if: ${{ success() && steps.check_cache_result.outputs.cache-hit != 'true' }}
Expand Down
7 changes: 7 additions & 0 deletions etc/cmake/options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,10 @@ if (OTBR_POWER_CALIBRATION)
else()
target_compile_definitions(otbr-config INTERFACE OTBR_ENABLE_POWER_CALIBRATION=0)
endif()

option(OTBR_DNSSD_PLAT "Enable OTBR DNS-SD platform implementation" OFF)
if (OTBR_DNSSD_PLAT)
target_compile_definitions(otbr-config INTERFACE OTBR_ENABLE_DNSSD_PLAT=1)
else()
target_compile_definitions(otbr-config INTERFACE OTBR_ENABLE_DNSSD_PLAT=0)
endif()
2 changes: 2 additions & 0 deletions etc/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ ARG REST_API
ARG WEB_GUI
ARG MDNS
ARG FIREWALL
ARG DNSSD_PLAT

ENV INFRA_IF_NAME=${INFRA_IF_NAME:-eth0}
ENV BORDER_ROUTING=${BORDER_ROUTING:-1}
Expand All @@ -61,6 +62,7 @@ ENV DNS64=${DNS64:-0}
ENV WEB_GUI=${WEB_GUI:-1}
ENV REST_API=${REST_API:-1}
ENV FIREWALL=${FIREWALL:-1}
ENV DNSSD_PLAT=${DNSSD_PLAT:-0}
ENV DOCKER 1

RUN env
Expand Down
11 changes: 10 additions & 1 deletion script/_otbr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ otbr_install()
"-DCMAKE_INSTALL_PREFIX=/usr"
"-DOTBR_DBUS=ON"
"-DOTBR_DNSSD_DISCOVERY_PROXY=ON"
"-DOTBR_SRP_ADVERTISING_PROXY=ON"
"-DOTBR_INFRA_IF_NAME=${INFRA_IF_NAME}"
"-DOTBR_MDNS=${OTBR_MDNS:=mDNSResponder}"
# Force re-evaluation of version strings
Expand All @@ -81,6 +80,16 @@ otbr_install()
"${otbr_options[@]}"
)

if with DNSSD_PLAT; then
otbr_options+=(
"-DOTBR_DNSSD_PLAT=ON"
)
else
otbr_options+=(
"-DOTBR_SRP_ADVERTISING_PROXY=ON"
)
fi

if with WEB_GUI; then
otbr_options+=("-DOTBR_WEB=ON")
fi
Expand Down
2 changes: 2 additions & 0 deletions script/bootstrap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ install_packages_apt()
# Avahi should be included for reference device builds.
if [[ ${OTBR_MDNS} == "avahi" || ${OT_BACKBONE_CI} == 1 || ${REFERENCE_DEVICE} == 1 ]]; then
sudo apt-get install --no-install-recommends -y avahi-daemon
# Increase the object number limit to rid of 'Too many objects' error
sudo sed -i 's/^#objects-per-client-max=[0-9]\+/objects-per-client-max=30000/' /etc/avahi/avahi-daemon.conf
fi

(MDNS_RESPONDER_SOURCE_NAME=mDNSResponder-1790.80.10 \
Expand Down
23 changes: 23 additions & 0 deletions src/agent/application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "agent/application.hpp"
#include "common/code_utils.hpp"
#include "common/mainloop_manager.hpp"
#include "host/posix/dnssd.hpp"
#include "utils/infra_link_selector.hpp"

namespace otbr {
Expand All @@ -63,6 +64,9 @@ Application::Application(Host::ThreadHost &aHost,
, mPublisher(
Mdns::Publisher::Create([this](Mdns::Publisher::State aState) { mMdnsStateSubject.UpdateState(aState); }))
#endif
#if OTBR_ENABLE_DNSSD_PLAT
, mDnssdPlatform(*mPublisher)
#endif
#if OTBR_ENABLE_DBUS_SERVER && OTBR_ENABLE_BORDER_AGENT
, mDBusAgent(MakeUnique<DBus::DBusAgent>(mHost, *mPublisher))
#endif
Expand Down Expand Up @@ -140,6 +144,9 @@ otbrError Application::Run(void)
// allow quitting elegantly
signal(SIGTERM, HandleSignal);

// avoid exiting on SIGPIPE
signal(SIGPIPE, SIG_IGN);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

@superwhd superwhd Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@superwhd superwhd Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


while (!sShouldTerminate)
{
otbr::MainloopContext mainloop;
Expand Down Expand Up @@ -221,6 +228,9 @@ void Application::CreateRcpMode(const std::string &aRestListenAddress, int aRest

void Application::InitRcpMode(void)
{
Host::RcpHost &rcpHost = static_cast<otbr::Host::RcpHost &>(mHost);
OTBR_UNUSED_VARIABLE(rcpHost);

#if OTBR_ENABLE_BORDER_AGENT
mMdnsStateSubject.AddObserver(*mBorderAgent);
#endif
Expand All @@ -233,6 +243,13 @@ void Application::InitRcpMode(void)
#if OTBR_ENABLE_TREL
mMdnsStateSubject.AddObserver(*mTrelDnssd);
#endif
#if OTBR_ENABLE_DNSSD_PLAT
mMdnsStateSubject.AddObserver(mDnssdPlatform);
mDnssdPlatform.SetDnssdStateChangedCallback(([&rcpHost](otPlatDnssdState aState) {
OTBR_UNUSED_VARIABLE(aState);
otPlatDnssdStateHandleStateChange(rcpHost.GetInstance());
}));
#endif

#if OTBR_ENABLE_MDNS
mPublisher->Start();
Expand Down Expand Up @@ -267,10 +284,16 @@ void Application::InitRcpMode(void)
#if OTBR_ENABLE_VENDOR_SERVER
mVendorServer->Init();
#endif
#if OTBR_ENABLE_DNSSD_PLAT
mDnssdPlatform.Start();
#endif
}

void Application::DeinitRcpMode(void)
{
#if OTBR_ENABLE_DNSSD_PLAT
mDnssdPlatform.Stop();
#endif
#if OTBR_ENABLE_SRP_ADVERTISING_PROXY
mAdvertisingProxy->SetEnabled(false);
#endif
Expand Down
6 changes: 6 additions & 0 deletions src/agent/application.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@
#if OTBR_ENABLE_VENDOR_SERVER
#include "agent/vendor.hpp"
#endif
#if OTBR_ENABLE_DNSSD_PLAT
#include "host/posix/dnssd.hpp"
#endif
#include "utils/infra_link_selector.hpp"

namespace otbr {
Expand Down Expand Up @@ -268,6 +271,9 @@ class Application : private NonCopyable
Mdns::StateSubject mMdnsStateSubject;
std::unique_ptr<Mdns::Publisher> mPublisher;
#endif
#if OTBR_ENABLE_DNSSD_PLAT
DnssdPlatform mDnssdPlatform;
#endif
#if OTBR_ENABLE_BORDER_AGENT
std::unique_ptr<BorderAgent> mBorderAgent;
#endif
Expand Down
5 changes: 4 additions & 1 deletion src/host/posix/dnssd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ static otbr::DnssdPlatform::RegisterCallback MakeRegisterCallback(otInstance
otPlatDnssdRegisterCallback aCallback)
{
return [aInstance, aCallback](otPlatDnssdRequestId aRequestId, otError aError) {
aCallback(aInstance, aRequestId, aError);
if (aCallback)
{
aCallback(aInstance, aRequestId, aError);
}
};
}

Expand Down
8 changes: 7 additions & 1 deletion third_party/openthread/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,22 +62,28 @@ set(OT_NAT64_TRANSLATOR ${OTBR_NAT64} CACHE STRING "enable NAT64 translator" FOR
set(OT_NETDATA_PUBLISHER ON CACHE STRING "enable netdata publisher" FORCE)
set(OT_NETDIAG_CLIENT ON CACHE STRING "enable Network Diagnostic client" FORCE)
set(OT_PLATFORM "posix" CACHE STRING "use posix platform" FORCE)
set(OT_PLATFORM_DNSSD ${OTBR_DNSSD_PLAT} CACHE STRING "enable platform DNSSD" FORCE)
set(OT_PLATFORM_NETIF ON CACHE STRING "enable platform netif" FORCE)
set(OT_PLATFORM_UDP ON CACHE STRING "enable platform UDP" FORCE)
set(OT_SERVICE ON CACHE STRING "enable service" FORCE)
set(OT_SLAAC ON CACHE STRING "enable SLAAC" FORCE)
set(OT_SRP_CLIENT ON CACHE STRING "enable SRP client" FORCE)
set(OT_SRP_ADV_PROXY ${OTBR_DNSSD_PLAT} CACHE STRING "enable SRP Advertising Proxy" FORCE)
set(OT_TARGET_OPENWRT ${OTBR_OPENWRT} CACHE STRING "target on OpenWRT" FORCE)
set(OT_TCP OFF CACHE STRING "disable TCP")
set(OT_TREL ${OTBR_TREL} CACHE STRING "enable TREL" FORCE)
set(OT_UDP_FORWARD OFF CACHE STRING "disable udp forward" FORCE)
set(OT_UPTIME ON CACHE STRING "enable uptime" FORCE)

if (OTBR_SRP_ADVERTISING_PROXY)
if (OTBR_DNSSD_PLAT OR OTBR_SRP_ADVERTISING_PROXY)
set(OT_SRP_SERVER ON CACHE STRING "enable SRP server" FORCE)
set(OT_EXTERNAL_HEAP ON CACHE STRING "enable external heap" FORCE)
endif()

if (OT_SRP_ADV_PROXY AND OTBR_SRP_ADVERTISING_PROXY)
message(FATAL_ERROR "Only one Advertising Proxy can be enabled. ${OTBR_DNSSD_PLAT} ")
endif()

if (NOT OT_THREAD_VERSION STREQUAL "1.1")
if (OT_REFERENCE_DEVICE)
set(OT_DUA ON CACHE STRING "Enable Thread 1.2 DUA for reference devices")
Expand Down
Loading