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

IPv6 support for mDNS #2161

Merged
merged 11 commits into from Aug 3, 2021
Merged

IPv6 support for mDNS #2161

merged 11 commits into from Aug 3, 2021

Conversation

dvc94ch
Copy link
Contributor

@dvc94ch dvc94ch commented Jul 27, 2021

well it's multicast. so you are doubling the traffic (request sent to n peers and n responses to n peers is n + n2 packets. if you do the same thing on ip6 it's 2(n + n2) packets). if you are only listening on ip4 addresses, then it doesn't hurt as much as no one will respond to ipv6 mdns requests. (for the ip4 only case that would be 2n + n2)

cc @majchrzamemil I think the changes are reasonable, rebased your PR. But as I explained in #1989 enabling them both at the same time should remain optional and preferably only ip4 addresses should be broadcast on ip4 and ip6 addresses on ip6.

revived this effort as I have to do some work on mdns anyway. currently the timeout will expire on all machines at the same time flooding the network, but decided that that should be a separate PR.

Signed-off-by: Emil Majchrzak <majchrzakemil@gitlab.com>
Copy link
Member

@mxinden mxinden 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 to me overall. Thanks for picking this back up!

I wonder whether it makes sense to add a simple test with two in-process nodes discovering each other, once via v4, once via v6. That should be portable enough to work on most Laptops and our CI, right? What do you think @dvc94ch? Would be great to have some basic tests.

protocols/mdns/src/behaviour.rs Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Wow, that was fast. Thanks @dvc94ch for adding the tests!

protocols/mdns/tests/smoke.rs Outdated Show resolved Hide resolved
protocols/mdns/tests/smoke.rs Outdated Show resolved Hide resolved
protocols/mdns/tests/smoke.rs Outdated Show resolved Hide resolved
protocols/mdns/src/behaviour.rs Outdated Show resolved Hide resolved
dvc94ch and others added 4 commits August 2, 2021 21:32
Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Max Inden <mail@max-inden.de>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Great. Only needs a changelog entry, other than that this is good to go.

protocols/mdns/CHANGELOG.md Outdated Show resolved Hide resolved
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