-
Notifications
You must be signed in to change notification settings - Fork 37
fix: unrequired records should be sent as additional records #57
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
base: main
Are you sure you want to change the base?
Conversation
61fe298 to
28827a2
Compare
28827a2 to
85ca3e6
Compare
|
Thanks, I want to try and get a new release out this weekend. Apologies for the quiet. I appreciate the link to the spec, even so could you add a brief description of the changes you've made? Just to help me understand the diff a bit quicker. |
|
A DNS response can be divided into answers and additional answers. The answers should only include the domain name requested by the user (in the past, it included data not requested by the user), while the additional answers are used to include data not requested by the user but possibly needed in subsequent requests, to reduce the number of broadcast packets. |
9f9cf5f to
318fc70
Compare
318fc70 to
85ca3e6
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.
Thanks for this improvement, but I'm not quite prepared to merge it right now. I'm inclined to think I would prefer we collated a de-serialized DNS response as structs and then packet-ised it, rather than stream-producing something only a few hundred bytes long. That would be easier to test and reason about IMO
| } | ||
| let response = builder.build().unwrap_or_else(|x| x); | ||
| if question.qu { | ||
| unicast_builder = self.handle_question(&question, unicast_builder); |
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 can see why you've removed the collation here (as every single handle_question might move the builder to the additional section), but it does seem unfortunate.
When possible, a responder SHOULD, for the sake of network
efficiency, aggregate as many responses as possible into a single
Multicast DNS response message. For example, when a responder has
several responses it plans to send, each delayed by a different
interval, then earlier responses SHOULD be delayed by up to an
additional 500 ms if that will permit them to be aggregated with
other responses scheduled to go out a little later.
This library doesn't ever meaningfully delay responses atm, so sending all answers in a single packet seems perfectly sensible to me, and ideally I'd like to keep it that behaviour.
|
|
||
| /// Packet building helpers for `fsm` to respond with `ServiceData` | ||
| impl ServiceData { | ||
| pub fn add_ptr_rr(&self, builder: AnswerBuilder, ttl: u32) -> AnswerBuilder { |
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 like this move to pure functions from functions that take a builder, but the reality of the resulting handle_question function is just too difficult to read for me. There is so much repetition of add_answer and it's arguments I can barely keep up. There must be a better solution.
https://datatracker.ietf.org/doc/html/rfc6763#section-12