[dbus] add dbus API to set border agent Id by Irving-cl · Pull Request #3136 · openthread/ot-br-posix

Irving-cl · 2025-11-20T03:08:17Z

This PR adds a DBUS API to set the Border Agent Id.

This is required for GTBR implementation. The PR also adds a build option OTBR_BORDER_AGENT_ID to control whether to enable the Border Agent Id in openthread and the related code in ot-br-posix.

zhanglongxia

LGTM

codecov · 2025-11-20T03:23:14Z

Codecov Report

❌ Patch coverage is 38.88889% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.29%. Comparing base (2b41187) to head (9a970f5).
⚠️ Report is 1297 commits behind head on main.

Files with missing lines	Patch %	Lines
src/dbus/server/dbus_thread_object_rcp.cpp	22.22%	7 Missing ⚠️
tests/dbus/test_dbus_client.cpp	42.85%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3136       +/-   ##
===========================================
- Coverage   55.77%   35.29%   -20.49%     
===========================================
  Files          87      144       +57     
  Lines        6890    17422    +10532     
  Branches        0     1429     +1429     
===========================================
+ Hits         3843     6149     +2306     
- Misses       3047    10959     +7912     
- Partials        0      314      +314

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Irving-cl · 2025-11-20T03:33:28Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new DBus API for setting the Border Agent ID, controlled by the OTBR_BORDER_AGENT_ID build option. The implementation across the client, server, and build system is well-executed. My review includes a couple of suggestions to enhance code style and improve test coverage for the new functionality.

zhanglongxia · 2025-11-20T05:39:27Z

+    VerifyOrExit(DBusMessageExtractFromVariant(&aIter, data) == OTBR_ERROR_NONE, error = OT_ERROR_INVALID_ARGS);
+    memcpy(&borderAgentId.mId, &data.front(), sizeof(borderAgentId.mId));
+
+    error = otBorderAgentSetId(mHost.GetInstance(), &borderAgentId);


Will this conflict with the #3132?

I didn't sync code so didn't see #3132 yesterday.

#3132 can work but it seems a little unnatural to me to set it in UpdateMeshCopTxtHandler because UpdateMeshCopTxtHandler is used to update the external fields of the meshcop txt records while borderAgentId is an internal field (though it can be set by an API). The fields set in UpdateMeshCopTxtHandler will be taken to integrate into the txt data that provided by OT. But borderAgentId shouldn't be handled like that here because it's already in the txt data provided by OT. And in current code, a workaround is taken to do so. So it's like, we avail this API UpdateMeshCopTxtHandler and call a OT API here.

So I prefer having a dedicated API to set border agent Id. This PR will ONLY conflict with #3132 when both APIs are called to set the border agent Id. If we:

Set all vendor fields and border agent Id in UpdateMeshCopTxtHandler, it's ok

Set all vendor fields except border agent Id in UpdateMeshCopTxtHandler and set border agent id using this dedicated API, it's also ok.

So we can have both PRs.

Based on the description of the function otBorderAgentSetVendorTxtData(), the vendor-specific Key-Value TXT data pairs use TXT keys starting with v. The function UpdateMeshCopTxtHandler() is used to process the vendor TXT data. The key id is not the vendor-specific Key. I'm fine with this PR.

I am not sure about it.

As from spec An ID to uniquely identify the device hosting the Border Agent among multiple such devices., if manufacture want to manage ID themselves, it might also be vendor specific data.

Overall it's fine to add a separate dbus API but it should be well documented as in OT, people should be aware of the requirement of id by spec

The ID MUST remain identical if the device reboots. It MUST NOT persist after a BR factory-reset. It MUST be computed using a random function to minimize the ID collision probability.

librasungirl · 2025-11-21T03:41:28Z

    target_compile_definitions(otbr-config INTERFACE OTBR_ENABLE_BORDER_AGENT_MESHCOP_SERVICE=1)
 endif()

+option(OTBR_BORDER_AGENT_ID "Enable Border Agent ID support" ON)


is it necessary?

Since openthread has an option OT_BORDER_AGENT_ID to turn on/off Border Agent Id, I think it's necessary to add an option in ot-br-posix as well to avoid the case where OT_BORDER_AGENT_ID is OFF.

BTW, I'm thinking if border agent id is a MUST, why not removing the macro in openthread as well. Thoughts?

It's indeed MUST requirement per specification,

TXT record inclusion is REQUIRED. The ID MUST remain identical if the device reboots. It MUST NOT persist after a BR factory-reset. It MUST be computed using a random function to minimize the ID collision probability.

But the spec doesn't define who is to maintain it. OT core provides the option to help generate/set/store it and when OT is responsible for meshcop service, OPENTHREAD_CONFIG_BORDER_AGENT_MESHCOP_SERVICE_ENABLE, it will report error if it's not ON.

https://github.com/openthread/openthread/blob/main/src/core/meshcop/border_agent.hpp#L80

For BR, application can specify it via UpdateMeshCop UpdateMeshCopTxt or use this new dbus API you are introducing if it want to manage the id

librasungirl · 2025-11-21T03:46:41Z

    ClientError SetBorderAgentEnabled(bool aEnabled);

+    /**
+     * This method sets the 16 byte Border Agent Id.


pls add notes as https://github.com/openthread/openthread/blob/main/include/openthread/border_agent.h#L340 and highlight this API should be used with awareness and caution.

librasungirl · 2025-11-21T03:53:28Z

+    VerifyOrExit(DBusMessageExtractFromVariant(&aIter, data) == OTBR_ERROR_NONE, error = OT_ERROR_INVALID_ARGS);
+    memcpy(&borderAgentId.mId, &data.front(), sizeof(borderAgentId.mId));
+
+    error = otBorderAgentSetId(mHost.GetInstance(), &borderAgentId);


I am not sure about it.

As from spec An ID to uniquely identify the device hosting the Border Agent among multiple such devices., if manufacture want to manage ID themselves, it might also be vendor specific data.

Overall it's fine to add a separate dbus API but it should be well documented as in OT, people should be aware of the requirement of id by spec

The ID MUST remain identical if the device reboots. It MUST NOT persist after a BR factory-reset. It MUST be computed using a random function to minimize the ID collision probability.

Irving-cl added P2 comp: dbus comp: border agent labels Nov 20, 2025

zhanglongxia approved these changes Nov 20, 2025

View reviewed changes

Comment thread src/dbus/client/thread_api_dbus.cpp

Irving-cl force-pushed the dbus_set_border_agent_id branch from e5c54fd to 96e4a22 Compare November 20, 2025 03:33

Irving-cl marked this pull request as ready for review November 20, 2025 03:33

gemini-code-assist Bot reviewed Nov 20, 2025

View reviewed changes

Comment thread src/dbus/server/dbus_thread_object_rcp.cpp Outdated

Comment thread tests/dbus/test_dbus_client.cpp Outdated

zhanglongxia reviewed Nov 20, 2025

View reviewed changes

zhanglongxia requested a review from librasungirl November 20, 2025 06:59

[dbus] add dbus API to set border agent Id

9a970f5

Irving-cl force-pushed the dbus_set_border_agent_id branch from 96e4a22 to 9a970f5 Compare November 21, 2025 03:49

librasungirl reviewed Nov 21, 2025

View reviewed changes

Irving-cl marked this pull request as draft November 21, 2025 08:51

Uh oh!

Conversation

Irving-cl commented Nov 20, 2025

Uh oh!

zhanglongxia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Irving-cl commented Nov 20, 2025

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 20, 2025 •

edited

Loading