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.

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

[ncp] integrate SRP Server and Advertising Proxy for NCP #2624

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

Irving-cl
Copy link
Contributor

@Irving-cl Irving-cl commented Nov 27, 2024

This PR integrates the mDNS Publisher with NcpHost to make advertising proxy work under NCP mode.

The PR adds an expect test to verify the advertising proxy function under NCP mode. To do the test, avahi-browser is used. So it's added into the bootstrap of the CI for ncp_mode.

As an abstraction:

  1. The PR adds NcpSpinel::SrpServerSetEnabled and NcpSpinel::SrpServerSetAutoEnableMode to control the SRP server on the NCP.
  2. The PR adds the handling of SPINEL_PROP_DNSSD_HOST, SPINEL_PROP_DNSSD_SERVICE and SPINEL_PROP_DNSSD_KEY_RECORD to help NCP to publish the dnssd entries (by using the mDNSPublisher on the host).

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 4.19580% with 137 lines in your changes missing coverage. Please review.

Project coverage is 45.88%. Comparing base (2b41187) to head (8cf6c5b).
Report is 891 commits behind head on main.

Files with missing lines Patch % Lines
src/ncp/ncp_spinel.cpp 3.84% 124 Missing and 1 partial ⚠️
src/agent/application.cpp 0.00% 5 Missing ⚠️
src/ncp/ncp_host.cpp 16.66% 5 Missing ⚠️
src/ncp/ncp_spinel.hpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2624      +/-   ##
==========================================
- Coverage   55.77%   45.88%   -9.89%     
==========================================
  Files          87      106      +19     
  Lines        6890    12851    +5961     
  Branches        0      928     +928     
==========================================
+ Hits         3843     5897    +2054     
- Misses       3047     6632    +3585     
- Partials        0      322     +322     

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

@Irving-cl Irving-cl force-pushed the integrate_ncp_adv_proxy branch 4 times, most recently from 315d152 to 53d7c11 Compare November 27, 2024 08:30
@Irving-cl Irving-cl force-pushed the integrate_ncp_adv_proxy branch 6 times, most recently from 500b3ab to 5eeee35 Compare November 27, 2024 11:31
@Irving-cl Irving-cl mentioned this pull request Nov 27, 2024
17 tasks
@Irving-cl Irving-cl requested review from superwhd and abtink November 27, 2024 11:46
@Irving-cl Irving-cl marked this pull request as ready for review November 27, 2024 11:47
Copy link
Member

@abtink abtink left a comment

Choose a reason for hiding this comment

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

Looks good overll.
Couple of smaller suggestions. and one question/suggestion about key name.

src/ncp/ncp_host.hpp Outdated Show resolved Hide resolved
name += ".";
name += aKey.mServiceType;
}
return name;
Copy link
Member

Choose a reason for hiding this comment

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

We should be careful here, as the first label in a "service instance name" may itself contain a dot (.) character. So, if we construct a full name by appending the labels, it may be parsed incorrectly later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check to only add the dot when the name doesn't contain one in the end.

Copy link
Member

Choose a reason for hiding this comment

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

The . can be in the middle and part of the instance name label itself (not a separator). The dot . is generally not used (or allowed) in DNS labels, but service instance labels allow this. For example, a service instance label can be "device.1234" as a single label (. being part of the label itself).

If we want to handle this, I can think of two options:

  1. Keep the first label separate and pass it as a separate "string" to all APIs (mDNS-related APIs).
  2. Add a new escaping mechanism so we can construct a full name as a single string, using an escape character to indicate when . is part of the label and not a separator.

I prefer option 1 (that's why we use this way to represent the names from the OT core).

I know this is rather tricky, so it may be fine to leave it for the future (accepting that the current code would not work with instance labels that include a .).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM! I added a comment in the code.

@Irving-cl Irving-cl force-pushed the integrate_ncp_adv_proxy branch 2 times, most recently from fa3c97b to b861990 Compare December 3, 2024 11:25
@Irving-cl Irving-cl force-pushed the integrate_ncp_adv_proxy branch from b861990 to 9e20aea Compare December 4, 2024 02:39
Copy link
Contributor

@zhanglongxia zhanglongxia left a comment

Choose a reason for hiding this comment

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

LGTM

@Irving-cl Irving-cl requested review from jwhui and removed request for superwhd December 4, 2024 09:41
src/ncp/ncp_spinel.hpp Outdated Show resolved Hide resolved
src/ncp/ncp_spinel.hpp Outdated Show resolved Hide resolved
src/ncp/ncp_spinel.hpp Outdated Show resolved Hide resolved
@Irving-cl Irving-cl force-pushed the integrate_ncp_adv_proxy branch from 9e20aea to 8cf6c5b Compare December 5, 2024 06:17
@Irving-cl Irving-cl requested a review from jwhui December 5, 2024 06:17
@jwhui jwhui merged commit ff4ea4e into openthread:main Dec 5, 2024
32 checks passed
@Irving-cl Irving-cl deleted the integrate_ncp_adv_proxy branch December 5, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants