Skip to content

Conversation

keepsimple1
Copy link
Owner

@keepsimple1 keepsimple1 commented Jun 9, 2025

This is to address issue #358. The basic approach is:

  • Keep it backward compatible for ServiceEvent::ServiceResolved event, which returns ServiceInfo.
  • Introduce a new event ServiceEvent::ServiceDetailed to return ResolvedService. Update ResolvedService.addresses to be HashSet<HostIp>, where HostIp is a wrapper of IpAddr that includes scope_id for IPv6 addresses.

To opt in to receive ServiceEvent::ServiceDetailed instead of ServiceEvent::ServiceResolved, the user would call ServiceDaemon::use_service_detailed(true) of its client daemon.

So, in short, to use IPv6 address with scope (interface id), one needs to use the new ServiceDetailed event and ResolvedService struct instead of ServiceInfo. In the long term, I hope to deprecate the uses of ServiceInfo on the client (querier) side.

Also note, HostnameResolutionEvent is also updated to use HostIp instead of IpAddr. I think it's better to make this breaking change at the same time.

@hrzlgnm
Copy link
Contributor

hrzlgnm commented Jun 10, 2025

I guess we also should consider updating the query example to use the new proposed API.

@keepsimple1
Copy link
Owner Author

I guess we also should consider updating the query example to use the new proposed API.

I've updated the query example.

@hrzlgnm
Copy link
Contributor

hrzlgnm commented Jun 22, 2025

Can we make the struct dns_parser::InterfaceId pub in lib.rs? Otherwise consumers cannot implement a From trait for that struct if needed?

@keepsimple1 keepsimple1 force-pushed the new-resolved-service branch from eab38af to 2ebc146 Compare June 23, 2025 04:49
@keepsimple1
Copy link
Owner Author

Can we make the struct dns_parser::InterfaceId pub in lib.rs? Otherwise consumers cannot implement a From trait for that struct if needed?

Exported InterfaceId. I might missed something but I don't think Rust allows external consumers implement From trait for InterfaceId per the orphan rule.

@hrzlgnm
Copy link
Contributor

hrzlgnm commented Jun 23, 2025

Can we make the struct dns_parser::InterfaceId pub in lib.rs? Otherwise consumers cannot implement a From trait for that struct if needed?

Exported InterfaceId. I might missed something but I don't think Rust allows external consumers implement From trait for InterfaceId per the orphan rule.

Thanks, I was maybe a bit imprecise on which direction the trait goes. I wanted to implement a From<InterfaceId> for another struct in my crate which should be possible now.

HostIp::V4(v4) => write!(f, "{}", v4.addr),
HostIp::V6(v6) => {
if v6.scope_id.index != 0 {
write!(f, "{}%{}", v6.addr, v6.scope_id.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether address%index would be more feasible, as for example Windows does not use interface names but rather an interface index as a scope id.

Copy link
Owner Author

@keepsimple1 keepsimple1 Jul 1, 2025

Choose a reason for hiding this comment

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

I've updated it to use index. For myself reference: RFC 4007 section 11.3

@keepsimple1
Copy link
Owner Author

@oysteintveit-nordicsemi may I ask you help review the changes to HostnameResolutionEvent where AddressesFound and AddressesRemoved changes from using IpAddr to using HostIp ? Is it a positive change from HostnameResolutionEvent point of view?

This is a breaking change for HostnameResolutionEvent. Are there some methods that should be implemented for HostIp so that your code change is minimum?

@h7x4
Copy link

h7x4 commented Jul 4, 2025

Hello, this is @oysteintveit-nordicsemi from my non-work account, thank you for reaching out! I no longer work there so I don't have access to the code, but I can have a look at it and see if I can tell from memory.

I currently don't have access to a computer either (not until next week), but I had a quick skim through the diff and if the only thing that changed is the wrapping struct and the address inside is still accessible to the library user in some way, I think it'll be fine 😁

@keepsimple1
Copy link
Owner Author

I currently don't have access to a computer either (not until next week), but I had a quick skim through the diff and if the only thing that changed is the wrapping struct and the address inside is still accessible to the library user in some way, I think it'll be fine 😁

@h7x4 thank you! I appreciated that.

Copy link

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

I had a closer look today, the changes look good to me. Thank you for providing the to and from functions for HostIp and IpAddr, that should make it easy to migrate :)

Pinging @michaeljones for a heads up

@keepsimple1 keepsimple1 merged commit 1103ab2 into main Jul 9, 2025
3 checks passed
@keepsimple1 keepsimple1 deleted the new-resolved-service branch July 9, 2025 03:49
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.

3 participants