-
Notifications
You must be signed in to change notification settings - Fork 428
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
Enable support for DHCP option 15 (domain name) #972
base: main
Are you sure you want to change the base?
Conversation
I see clippy is a bit angry with the size of the domain name and the impact it has. |
9db6953
to
e4a6d20
Compare
I added it in my latest commit. Also realized I was doing wrong in |
619dcdb
to
9778985
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #972 +/- ##
==========================================
+ Coverage 79.18% 79.29% +0.10%
==========================================
Files 81 81
Lines 26885 26944 +59
==========================================
+ Hits 21289 21365 +76
+ Misses 5596 5579 -17 ☔ View full report in Codecov by Sentry. |
5d908a7
to
ad63a7f
Compare
This PR builds on top of #974 to make |
the original design of DHCP was to provide the raw bytes in |
Hi @Dirbaio , I fully agree on the idea of having special options being handled outside as it is designed. The motivation is that without domain name the DNS implementation is only working on non-FQDN networks or with manual FQDN lookups, and the only discrepancy was networks with domain name and automatically forming non-FQDN -> FQDN. To me this warranted the direct inclusion in |
Another idea is to feature gate this so people do not pay for it unless they need it. |
I made it optional in my last commit just to show what I mean. I can drop the commit if it's considered as not needed. |
This makes `smoltcp` work in industrial networks with Windows DHCPs as they do not support option 119.
fd3c3ee
to
35bfdcb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am torn about feature-gating this. Low memory usage is important but we could end up with an unwieldy lot of feature flags when more DHCP options are added over time.
@@ -60,6 +60,7 @@ defmt = ["dep:defmt", "heapless/defmt-03"] | |||
"proto-sixlowpan" = ["proto-ipv6"] | |||
"proto-sixlowpan-fragmentation" = ["proto-sixlowpan", "_proto-fragmentation"] | |||
"proto-dns" = [] | |||
"proto-domainname" = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature's name could be confusing as "domainname protocol" sounds a lot like DNS. I suggest proto-dhcpv4-domainname
.
This makes
smoltcp
work in industrial networks with Windows DHCPs as they do not support option 119.I'm not sure if the result should be directly tied into the
dns
socket, but the IPs for DNS servers are manually supplied so I follow the same pattern here and assume that users will build the FQDN before doing a DNS request.Feedback welcome!