search

project-chip / rs-matter

Rust implementation of the Matter protocol. Status: Experimental
Apache License 2.0
303 stars 43 forks source link

mDNS: always set the "cache flush" bit #154

Closed lucasvr closed 4 days ago

lucasvr commented 3 months ago

Always broadcast mDNS messages with the "cache flush" bit set for TXT, SRV, A, and AAAA records. This is useful when the Matter device reboots and gets assigned a different IP address, for example. In such a case, hosts with an outdated cache may need to wait until the original TTL expires (up to 60 seconds, per the TTL definitions on mdns/builtin.rs).

ivmarkov commented 1 month ago

UPDATE: GH links to lines in PR do not work as I expected. I'll update with inline-comments

@lucasvr Sorry for replying so late, but after implementing and submitting this, I think I'm finally somewhat qualified to assess what you are doing here.

Yes, my interpretation matches yours now. Setting the cache-flush bits unconditionally might help (or at least not hurt) for all but those PTR records where we are not the authority (as in https://datatracker.ietf.org/doc/html/rfc6762#section-10.2).

One question though: I think we should also cache-flush those PTR answers where we redirect ("point") a service-type or a service subtype to its host record?

However, in your PR, I see two things which worry me a bit:

Please let me know if I misunderstood your changes, or the spec. In any case, once #186 is in, I plan to apply a variation of the changes you have suggested here. Thanks again for noticing this and sorry for the late reply! I should've at least left a note why I'm not replying immediately (as in - not being qualified enough to comment).

lucasvr commented 1 month ago
  • Contrary to what the PR title says, you are cache-flushing also PTR records. Is this by accident or by design?
  • You are also cache-flushing PTR records where we are not the authority (i.e. "what DNS-SD service-types and sub-types you have?") (I'll comment on these lines in the PR itself shortly.)

That was not by design. While it wouldn't hurt to set the cache-flush bit on those cases, it's indeed more sane to leave them out from PTR records. Thanks for the careful code review.

I've updated the PR to address your suggestions. Feel free to use this as a reference only (in case you want to apply a variation of this PR at a later point in time).