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

feat: honor cache flush #201

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

lyager
Copy link
Contributor

@lyager lyager commented Apr 24, 2024

According to RFC 6762, when the cache-flush bit is set on a RR, all previously cache entries should be set to expire 1 second in the future.

This bit was previously called 'UNIQUE` which might have the same meaning, but cache-flush seems more in line with the RFC

@lyager
Copy link
Contributor Author

lyager commented Apr 24, 2024

I've had issues with embedded devices changing IP's and appearing with both the previous and current IP. I'm guessing this goes for other records too?

Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! Please see comments inline. And could you please add tests for the new feature?

src/dns_parser.rs Outdated Show resolved Hide resolved
src/dns_parser.rs Outdated Show resolved Hide resolved
src/dns_parser.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
@lyager
Copy link
Contributor Author

lyager commented Apr 25, 2024

Thanks for the response, I'll dive into it 👍

@lyager
Copy link
Contributor Author

lyager commented Apr 25, 2024

I had some trouble constructing a proper test. My idea was to start browsing, similar to the test service_with_temporarily_invalidated_ptr, but I had trouble constructing response packets. I'll give it another shot, but if you have some test code where MDNS response packets are constructed and send out, please let me know :)

@lyager lyager marked this pull request as draft April 25, 2024 13:56
@lyager
Copy link
Contributor Author

lyager commented Apr 25, 2024

Broke a test putting into draft while looking at it.

src/service_daemon.rs Outdated Show resolved Hide resolved
@keepsimple1
Copy link
Owner

keepsimple1 commented Apr 26, 2024

I had some trouble constructing a proper test. My idea was to start browsing, similar to the test service_with_temporarily_invalidated_ptr, but I had trouble constructing response packets. I'll give it another shot, but if you have some test code where MDNS response packets are constructed and send out, please let me know :)

service_with_temporarily_invalidated_ptr is a good sample to start. The response packet is constructed in to_packet_data called by broadcast_dns_on_intf . And you want to add CLASS_CACHE_FLUSH when creating a record (valid record instead of the invalid one in below):

mdns-sd/src/service_daemon.rs

Lines 2775 to 2784 in 1b9647d

let invalidate_ptr_packet = DnsPointer::new(
my_service.get_type(),
TYPE_PTR,
CLASS_IN,
0,
my_service.get_fullname().to_string(),
);
let mut packet_buffer = DnsOutgoing::new(FLAGS_QR_RESPONSE | FLAGS_AA);
packet_buffer.add_additional_answer(Box::new(invalidate_ptr_packet));

And a different, possible simpler approach is to construct a DnsCache directly, and then call its add_or_update and verify if the changes are applied.

@lyager lyager marked this pull request as ready for review April 26, 2024 09:11
@lyager
Copy link
Contributor Author

lyager commented Apr 26, 2024

I fixed the already existing test. Thanks for the help with understanding, I'll attempt writing a specific cache-flush test now.

@lyager
Copy link
Contributor Author

lyager commented Apr 26, 2024

Test added for cache flush - hope it's ok.

Copy link
Owner

@keepsimple1 keepsimple1 left a comment

Choose a reason for hiding this comment

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

Thanks for your updates. Some minor comments inline.

src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Show resolved Hide resolved
tests/mdns_test.rs Show resolved Hide resolved
src/service_daemon.rs Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
@lyager
Copy link
Contributor Author

lyager commented Apr 29, 2024

Thanks for the feedback, I've stoppede squashing the commit, if you prefer me to, let me know.

According to RFC 6762, when the cache-flush bit is set on a RR, all previously
cache entries should be set to expire 1 second in the future if the RR created
more than 1 second ago.

This bit was previously called 'UNIQUE` which might have the same meaning, but
cache-flush seems more in line with the RFC
Copy link
Owner

@keepsimple1 keepsimple1 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 !

@keepsimple1 keepsimple1 merged commit 16cb5cd into keepsimple1:main Apr 30, 2024
3 checks passed
@lyager lyager deleted the honor_flush_pr branch April 30, 2024 08:07
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.

2 participants