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

Add support for resolving non-service hostnames #192

Conversation

oysteintveit-nordicsemi
Copy link
Contributor

Resolves #191.

This adds several pieces of public API, for resolving mDNS hostnames as standalone.

  • ServiceDaemon::resolve(&self, hostname: &str) -> Result<Receiver<ResolutionEvent>>
  • ServiceDaemon::stop_resolve(&self, hostname: &str) -> Result<()>

as well as

pub enum ResolutionEvent {
    /// Started searching for the ip address of a hostname.
    SearchStarted(String),
    /// One or more addresses for a hostname has been found.
    HostnameAddressesFound(String, HashSet<IpAddr>),
    /// One or more addresses for a hostname has been removed.
    HostnameAddressesRemoved(String, HashSet<IpAddr>),
    /// Stopped searching for the ip address of a hostname.
    SearchStopped(String),
}

The internal implementation uses the dns cache for addresses, which should make it possible for service discovery and hostname discovery to benefit from each other in the case that you are running both at the same time.

The rest of the implementation is largely either copy paste of- or merging with the code that handles the service discovery, with some exceptions where the events are reporting an aggregate of ip-addresses rather than reporting one at a time.

Other notes

I refactored DnsCache::evict_expired to resolve some borrow checker issues. The current implementation will evict all expired records before it starts sending out events. It should not have any effect on any users of ServiceDaemon::browse, since they don't have direct access to the cache.

The current implementation is a bit loud, because it's missing Known Answer Suppression (RFC6762, sec. 7.1) (which is stated as a MUST requirement). As far as I understand, the current implementation for service browsing does not have this either, so I think it should be considered as a new issue.

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.

Thank you for your PR! With a quick look, it didn't feel quite right for me, even though it is based on what already discussed. I left one inline comment, but I need more time to think through and review other parts.

(I'm hoping to simplify the current solution and still meet your needs)

src/service_daemon.rs Outdated Show resolved Hide resolved
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.

I went through the whole diff 😅 . It looks good to me overall. Please see inline for some comments. The main thing is still regarding the public API & enum. (That's always the hard part for a library ;-) ).

Also, could you please add test cases for this new feature? Thanks!

src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Show resolved Hide resolved
@oysteintveit-nordicsemi oysteintveit-nordicsemi force-pushed the init-hostname-resolution-feature branch 2 times, most recently from 8540036 to 6e2762d Compare April 12, 2024 08:53
@oysteintveit-nordicsemi
Copy link
Contributor Author

Okay, so I've added a timeout: Option<u64> to ServiceDaemon:resolve_hostname. This adds the potential timeout to a table of timeouts, Zeroconf::hostname_resolver_timeouts: HashMap<String, u64> // (hostname, timeout), marks it in timers, and the Command::ResolveHostname continues to add retransmissions until it has reached the timeout. A block in the runloop will then clean up the tables, and notify the listener of both the timeout and the search stop.

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 the updates!

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

Btw, I added support for sending multiple questions in one query. You can try out the new send_query_vec .

@oysteintveit-nordicsemi oysteintveit-nordicsemi force-pushed the init-hostname-resolution-feature branch 2 times, most recently from 69e0b37 to 2b7ae71 Compare April 15, 2024 12:55
src/service_daemon.rs Outdated Show resolved Hide resolved
src/service_daemon.rs Outdated Show resolved Hide resolved
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.

Thank you for updating! Looks good! I only have one minor inline comment regarding naming.

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 7359cb3 into keepsimple1:main Apr 17, 2024
3 checks passed
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.

Utility function to resolve mDNS hostnames
2 participants