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

Added the link-local feature to if_addrs in Cargo.toml #126

Merged
merged 1 commit into from
Sep 30, 2023

Conversation

hgomersall
Copy link
Contributor

if_addrs disables link-local addresses on windows if the "link-local" feature is not enabled. See:
https://github.com/messense/if-addrs/blob/947c6342681b047b48b5f53eb75049881d2dfa20/src/sockaddr.rs#L63C21-L63C21

Currently, the behaviour on Linux is to allow link-local IPv4 addresses, but IPv6 and windows is behind the feature gate.

This PR trivially adds the link-local feature to the if-addrs package.

It might be desirable to reflect the feature gating of link-local addresses instead of accepting this PR as-is.

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.

Looks good! And,

It might be desirable to reflect the feature gating of link-local addresses instead of accepting this PR as-is.

Agreed. Would you mind add that (e.g. in [features] , add link-local = ["if-addrs/link-local"] and enable it in default ) ?

Thanks for opening the PR!

@hgomersall
Copy link
Contributor Author

Agreed. Would you mind add that (e.g. in [features] , add link-local = ["if-addrs/link-local"] and enable it in default ) ?

It seems odd to me that under Linux this flag would be ignored entirely on IPv4. It's probably why the issue with if-addrs wasn't picked up sooner because AFAICT it impacts both browse and register - that is, link-local addresses are completely removed from the mdns-sd process except for Linux IPv4.

Is it desired behaviour that the flag will be ignored with IPv4 under linux (as per if-addrs)?

@keepsimple1
Copy link
Owner

It seems odd to me that under Linux this flag would be ignored entirely on IPv4. It's probably why the issue with if-addrs wasn't picked up sooner because AFAICT it impacts both browse and register - that is, link-local addresses are completely removed from the mdns-sd process except for Linux IPv4.

Is it desired behaviour that the flag will be ignored with IPv4 under linux (as per if-addrs)?

I see what you mean. It seems inconsistent to me. Probably only the authors of if-addrs knows about the reason / history behind this behavior.

Given that, I think we can just apply the change you have now. It will be a user visible change on Windows, so we will bump up the minor version in the next release. I think It's a good change as it means more consistent behavior between Linux/macOS and Windows.

For adding the feature gate in our crate, we can do it later if needed. It should be okay as the feature gate will be enabled by default, so it would not cause backward compatibility 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.

LGTM per our discussions above.

@keepsimple1 keepsimple1 merged commit bef8682 into keepsimple1:main Sep 30, 2023
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.

None yet

2 participants