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.

Sign up for GitHub

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

Jump to bottom

Support for IPv6 interfaces (#37) #41

Merged
merged 10 commits into from
Apr 21, 2023
Merged

Support for IPv6 interfaces (#37) #41

merged 10 commits into from
Apr 21, 2023

Conversation

bruinsg
Copy link
Contributor

@bruinsg bruinsg commented Mar 22, 2023

This merge request has the following changes:

  1. Removed support for out of support frameworks
  2. Support for IPv6 networks on Windows and Linux
  3. Fix for unsupported addresses that would show up when remote devices restart

@bruinsg
Add support for net60
c70ad5b
@bruinsg
Support interfaces that only have IPv6 (tmds#37)
7c9a40a
@bruinsg
Only support pure IPv6 interfaces on Linux
a23f429
@bruinsg
Support for IPv6 and bug fixes for multi interface systems
47aa779 
When a remote device restarts or broadcasts it information
this is received on all interfaces. This means that interfaces
will have unreachable IP addresses in the service announcement.
By checking the originating IP address filtering is perfomed
and unreachable addresses are ignored.
@bruinsg
Copy link
Contributor Author

bruinsg commented Mar 29, 2023

@tmds Any feed back or comments on this?

@tmds
Copy link
Owner

tmds commented Mar 29, 2023

Sorry, I had missed your PR.
I will make some time to look at this next week.

tmds
tmds reviewed Apr 6, 2023
View reviewed changes
src/Tmds.MDns/Tmds.MDns.csproj Outdated Show resolved Hide resolved
src/Tmds.MDns/ServiceBrowser.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/MdnsSocket.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/MdnsSocket.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/MdnsSocket.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/MdnsSocket.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/MdnsSocket.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/NetworkInterfaceHandler.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/NetworkInterfaceHandler.cs Outdated Show resolved Hide resolved
src/Tmds.MDns/LinuxHelper.cs Outdated Show resolved Hide resolved
@tmds
Copy link
Owner

tmds commented Apr 6, 2023 •
edited
Loading

@bruinsg I took a look at the PR and added some comments.

I don't want to drop support for .NET framework, so we should add back a tfm for that.

Instead of having a List<MdnsSocket>, can we change NetworkInterfaceHandler to be NetworkInterfaceHandlerSocket, and take a bool isIPv6 argument?
I think we'll reduce the lines of code changed, which will make it easier to review and avoid regressions.

@bruinsg
Addressing feed back on merge request
4395a94 
* Changed MdnsSocket to NetworkInterfaceHandlerSocket
* Use MulticastInterface that works for Linux and Windows
@bruinsg
Fix for IPv6 network not being ready when NetworkInterfaceHandler is …
69d855f 
…created

During a system reboot the IPv6 is not always available when the NetworkHandler
is created. Listening to the NetworkAvailabilityChanged helps detecting changes
in the network.
@bruinsg
Support for netstandard 1.3
fcd6e73
@bruinsg
Copy link
Contributor Author

bruinsg commented Apr 11, 2023

I have address your concerns and added back support for netstandard1.3. Please have a look if there are any further issues you would like me address

@tmds
Copy link
Owner

tmds commented Apr 15, 2023 •
edited
Loading

Sorry this is moving slowly. I've been very busy the last weeks.

I took a close look at the PR now, and pushed a commit to your branch.
I've mostly refactored the code in a way the diff is easier to review and understand.
I also moved from netstandard1.3 to netstandard2.0 (to be able to simplify some code).

Can you take a look, and add any feedback you may have?

@bruinsg
Fixes on refactoring
9328a7e 
StartReceive after receiving a message from non local addresses
@bruinsg
Fixing legacy code
c3181df 
Remove netstandard 1.3 and fix depreaction warnings
@bruinsg
Copy link
Contributor Author

bruinsg commented Apr 17, 2023

Thanks for your response!

There was one bug in the code where the start receive was not issues after receiving a message from a non local ip address.
There were also some deprecation warnings that I fixed, please check if this is ok with you.

@tmds
Copy link
Owner

tmds commented Apr 17, 2023

There was one bug in the code where the start receive was not issues after receiving a message from a non local ip address.

Thanks for the fix-ups.

There were also some deprecation warnings that I fixed, please check if this is ok with you.

I like using IPAddress.Address for IPv4 (though it's obsolete) because it doesn't allocate.
Adding a pragma that ignores the obsolete is ok for me.
Leaving it as-is, is fine with me too.

Overall, this looks good to me.
I'm going to merge this on Friday and then make a new release of the library on nuget.org.

@bruinsg
Copy link
Contributor Author

bruinsg commented Apr 17, 2023

Great, thanks. Looking forward to the new release

@tmds tmds merged commit 7458c0b into tmds:master Apr 21, 2023
@tmds
Copy link
Owner

tmds commented Apr 21, 2023

This is part of v0.8.0 which has just been uploaded to nuget.org.

@bruinsg
Copy link
Contributor Author

bruinsg commented Apr 24, 2023

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmds tmds tmds left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bruinsg @tmds