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

[posix] update Posix::Resolver to support IPv6 addresses for servers. #11325

Merged
merged 7 commits into from
Mar 13, 2025

Conversation

yangsong-cnyn
Copy link
Contributor

@yangsong-cnyn yangsong-cnyn commented Mar 5, 2025

This PR enhances the DNS resolver (resolver.cpp) to support IPv6 addresses in upstream DNS server configurations, allowing for greater flexibility and compatibility in IPv6-enabled environments.

Key Changes:

  • IPv6 Parsing in LoadDnsServerListFromConf:
    • The LoadDnsServerListFromConf function now parses both IPv4 and IPv6 addresses from /etc/resolv.conf using inet_pton with AF_INET and AF_INET6.
    • Parsed addresses are stored in mUpstreamDnsServerList.
  • IPv6 Handling in Query:

@yangsong-cnyn yangsong-cnyn force-pushed the ipv6_dns_resolver branch 2 times, most recently from a5e9eed to 70c3cbc Compare March 5, 2025 09:25
Copy link

size-report bot commented Mar 5, 2025

Size Report of OpenThread

Merging #11325 into main(4c0af02).

name branch text data bss total
ot-cli-ftd main 474840 860 66364 542064
#11325 474840 860 66364 542064
+/- 0 0 0 0
ot-ncp-ftd main 442844 764 61584 505192
#11325 442844 764 61584 505192
+/- 0 0 0 0
libopenthread-ftd.a main 242440 95 40142 282677
#11325 242440 95 40142 282677
+/- 0 0 0 0
libopenthread-cli-ftd.a main 59395 0 8083 67478
#11325 59395 0 8083 67478
+/- 0 0 0 0
libopenthread-ncp-ftd.a main 32911 0 5924 38835
#11325 32911 0 5924 38835
+/- 0 0 0 0
ot-cli-mtd main 368840 764 50844 420448
#11325 368840 764 50844 420448
+/- 0 0 0 0
ot-ncp-mtd main 351324 764 46088 398176
#11325 351324 764 46088 398176
+/- 0 0 0 0
libopenthread-mtd.a main 161313 0 24646 185959
#11325 161313 0 24646 185959
+/- 0 0 0 0
libopenthread-cli-mtd.a main 40296 0 8059 48355
#11325 40296 0 8059 48355
+/- 0 0 0 0
libopenthread-ncp-mtd.a main 25561 0 5924 31485
#11325 25561 0 5924 31485
+/- 0 0 0 0
ot-cli-ftd-br main 562688 868 133300 696856
#11325 562688 868 133300 696856
+/- 0 0 0 0
libopenthread-ftd-br.a main 335647 100 107046 442793
#11325 335647 100 107046 442793
+/- 0 0 0 0
libopenthread-cli-ftd-br.a main 73857 0 8115 81972
#11325 73857 0 8115 81972
+/- 0 0 0 0
ot-rcp main 63184 568 20804 84556
#11325 63184 568 20804 84556
+/- 0 0 0 0
libopenthread-rcp.a main 9920 0 5060 14980
#11325 9920 0 5060 14980
+/- 0 0 0 0
libopenthread-radio.a main 19249 0 238 19487
#11325 19249 0 238 19487
+/- 0 0 0 0

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 71.21212% with 19 lines in your changes missing coverage. Please review.

Project coverage is 76.35%. Comparing base (4c0af02) to head (1bdf409).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/posix/platform/resolver.cpp 71.21% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11325      +/-   ##
==========================================
+ Coverage   73.22%   76.35%   +3.12%     
==========================================
  Files         628      630       +2     
  Lines       97928    98717     +789     
==========================================
+ Hits        71711    75378    +3667     
+ Misses      26217    23339    -2878     
Files with missing lines Coverage Δ
src/posix/platform/resolver.hpp 50.00% <ø> (ø)
src/posix/platform/resolver.cpp 68.36% <71.21%> (-1.84%) ⬇️

... and 297 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yangsong-cnyn yangsong-cnyn marked this pull request as ready for review March 6, 2025 04:47
@abtink
Copy link
Member

abtink commented Mar 7, 2025

Thanks @yangsong-cnyn for adding this.

One suggestion about the title of the PR and commit message.

You have used [dns] dns solver to support ipv6 address.

  • It is changing the Resolver class (not solver 😄).
  • Using [dns] makes it sound like this is changing some DNS module within the OT core implementation.
  • But this change is only related to the POSIX platform implementation and not part of the OT core. So, it's better to use [posix] instead.

I would suggest changing the title to [posix] update Posix::Resolver to support IPv6 addresses for servers.

@yangsong-cnyn yangsong-cnyn changed the title [dns] dns solver to support ipv6 address [posix] update Posix::Resolver to support IPv6 addresses for servers. Mar 7, 2025
… code revise
@yangsong-cnyn yangsong-cnyn force-pushed the ipv6_dns_resolver branch 2 times, most recently from fd9cb6b to 0ad7128 Compare March 7, 2025 02:43
… code revise
… code revise
… add comment
@yangsong-cnyn yangsong-cnyn requested a review from superwhd March 7, 2025 03:39
Copy link
Contributor

@superwhd superwhd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

Thanks @yangsong-cnyn for the changed. Looks good overall.

Couple of smaller suggestions below:

@yangsong-cnyn yangsong-cnyn force-pushed the ipv6_dns_resolver branch 3 times, most recently from 5465bea to db0f060 Compare March 11, 2025 03:20
@yangsong-cnyn yangsong-cnyn requested a review from abtink March 11, 2025 04:08
@yangsong-cnyn yangsong-cnyn force-pushed the ipv6_dns_resolver branch 5 times, most recently from 250e72a to cb2c465 Compare March 11, 2025 10:17
@yangsong-cnyn yangsong-cnyn requested a review from abtink March 12, 2025 01:29
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.

LGTM. Thanks for all the changes.

Couple of smaller suggestions belwo:

@@ -228,7 +276,7 @@ Resolver::Transaction *Resolver::GetTransaction(int aFd)

for (Transaction &txn : mUpstreamTransaction)
{
if (txn.mThreadTxn != nullptr && txn.mUdpFd == aFd)
if (txn.mThreadTxn != nullptr && (txn.mUdpFd4 == aFd || txn.mUdpFd6 == aFd))
Copy link
Member

Choose a reason for hiding this comment

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

The Resolver::GetTransaction(int aFd) does not seem to be used at all. I'm not sure why it was added.

Perhaps in a future (separate) PR, we can remove this to make the code simpler/cleaner?

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 will remove it in next PR

@yangsong-cnyn yangsong-cnyn requested a review from jwhui March 12, 2025 04:56
@jwhui jwhui merged commit a7dbd9b into main Mar 13, 2025
192 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants