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

mDNS changes to Trie (no SOA) and Query (multi-question, etc.) #35

Merged
merged 24 commits into from Jan 26, 2015

Conversation

@lcdunstan
Copy link
Contributor

@lcdunstan lcdunstan commented Dec 31, 2014

This is part of #28

The changes are:

  1. mDNS doesn't use SOA nor delegation (RFC 6762 section 12), so some minor changes to Trie are required to handle this.
  2. mDNS doesn't echo the questions in the response (RFC 6762 section 6), except in legacy mode, so a bool argument was added to Query.response_of_answer. Perhaps there are more elegant solutions?
  3. Query.answer still exists but now Query.answer_multiple is also available for answering multiple questions in one query to produce a single answer (RFC 6762 section 5.3). One caveat is that responses may exceed the maximum message length, but that is not really specific to mDNS. Also, in theory multiple questions might require multiple separate response messages in unusual cases, but that is complicated so I am not dealing with it yet.
  4. Query.answer_multiple takes an optional function to allow the caller to control the cache-flush bit. This bit is only set for records that have been "confirmed as unique". Using a callback requires minimal changes here but puts the burden of maintaining uniqueness state elsewhere. Do you think that it would be preferable to move such information into the dnsnode, even though it is mDNS-specific?
  5. Query.answer_multiple takes an optional function to filter the answer, in order to support "known answer suppression" (RFC 6762 section 7.1). Again, using a callback requires minimal change to the core, but later on the mDNS-specific known answer suppression logic could move into the Query module if that turns out to be simpler.
  6. A query for PTR returns additional records for SRV and TXT, to support efficient service discovery.
  7. Trie.iter was added to support mDNS announcements.
  8. Unit tests were added for some of the changes above. Let me know if you can see any specific gaps that need to be addressed for this PR.
@@ -47,6 +47,15 @@ let bad_node = { data = None; edge = ""; byte = -1;
children = [||]; ch_key = ""; least_child = '\255';
flags = Nothing; }

let local_dnsnode = { owner = hashcons_domainname (string_to_domain_name "local."); rrsets = [] }
Copy link
Member

@avsm avsm Dec 31, 2014

Does local. have reserved status, or should we make it possible to override its name?

Copy link
Contributor Author

@lcdunstan lcdunstan Dec 31, 2014

.local is reserved for mDNS, which is why I used it, but in fact this value is just a placeholder to be passed to `NXDomain for mDNS, and won't be transmitted in any packet, so the value could be anything. I can think of alternatives to make this clearer if you like.

@avsm
Copy link
Member

@avsm avsm commented Jan 26, 2015

Am reviewing these patches. There's a lot of mdns-specific stuff (e.g. the argument added to Query.response_of_answer, but I'm happy enough to follow the style of adding protocol-specific exceptions there for now (we have some dnssec specific bits too). A bigger refactoring can happen once the functionality is in place and we release a 1.0.

@avsm
Copy link
Member

@avsm avsm commented Jan 26, 2015

Using a callback requires minimal changes here but puts the burden of maintaining uniqueness state elsewhere. Do you think that it would be preferable to move such information into the dnsnode, even though it is mDNS-specific?

Are there multiple ways a client might maintain uniqueness state (for example via Irmin synchronization so that less broadcast traffic is required)? For now I'd keep the callback but bear in mind that if only one mechanism is used, this can be moved into dnsnode.

avsm added a commit to avsm/ocaml-dns that referenced this issue Jan 26, 2015
avsm added a commit to avsm/ocaml-dns that referenced this issue Jan 26, 2015
@avsm avsm mentioned this pull request Jan 26, 2015
@avsm avsm merged commit 358a6fe into mirage:master Jan 26, 2015
1 check passed
@samoht
Copy link
Member

@samoht samoht commented Jan 26, 2015

yay! Impressive PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants