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

Rewrite NetworkManager and incorporate various fixes from open PRs #8147

Merged
merged 69 commits into from
Jul 3, 2023

Conversation

Shadowghost
Copy link
Contributor

@Shadowghost Shadowghost commented Jul 19, 2022

I tried debugging and fixing a problem but didn't understand all the custom code, so I reimplemented the NetworkManager functionality with native .NET IPAddress and IPNetwork and a way more simpler object to hold both (e.g. for interfaces or in case we allow IPs and Subnets in a config)

Changes

  • Migrate to native .NET IP objects and functions
  • Incorporate changes from Multiple IP fixes #5595
  • Fix LocalAddress resolution in System/Info/Public
  • Remove hack for .NET issues resolved in .NET 6
  • Remove invalid hack for X-Forwarded-For headers when IPv6 is enabled
  • Remove unused configuration values from network settings
  • Group network settings by functionality (this only affects how they appear in network.xml)
  • Handle subnets in KnownProxies
  • Removed UdpSocket and replaced it with native .NET sockets
  • DLNA/SSDP and AutoDiscovery now only bind to valid bind addresses (WIP)

Notes

  • DLNA does not require IPs in the Location prop but a lot of clients do not work with domains, therefore we force IP+port
  • DLNA and SSDP still only support IPv4

Issues

  • Additional testing is welcome, especially DLNA/SSDP and AutoDiscovery

Related
Fixes #6272
Fixes #9298
Fixes #9300
Partially fixes #8692
Might fix #4442
Might fix #7791
Might fix #9298
Might fix #9445
Might fix #9540
Might fix #9586

@Shadowghost Shadowghost marked this pull request as ready for review July 21, 2022 19:36
@jellyfin-bot jellyfin-bot added the merge conflict Merge conflicts should be resolved before a merge label Aug 22, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Merge conflicts should be resolved before a merge label Oct 1, 2022
@joshuaboniface
Copy link
Member

🚀

@joshuaboniface joshuaboniface merged commit 93b4003 into jellyfin:master Jul 3, 2023
18 checks passed
@Shadowghost
Copy link
Contributor Author

There goes the 1 year PR anniversary 😢

Copy link
Member

@Bond-009 Bond-009 left a comment

Choose a reason for hiding this comment

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

I'd also add constants for the number of bits and bytes in IPv4 and IPv6 addresses

Jellyfin.Networking/Manager/NetworkManager.cs Show resolved Hide resolved
MediaBrowser.Common/Net/NetworkExtensions.cs Show resolved Hide resolved
if (i is IPNetAddress nw)
var ipBlock = splitString.Current;
var address = IPAddress.None;
if (negated && ipBlock.StartsWith("!") && IPAddress.TryParse(ipBlock[1..], out var tmpAddress))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (negated && ipBlock.StartsWith("!") && IPAddress.TryParse(ipBlock[1..], out var tmpAddress))
if (negated && ipBlock.StartsWith('!') && IPAddress.TryParse(ipBlock[1..], out var tmpAddress))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not work: cannot convert from 'char' to 'System.ReadOnlySpan<char>'

MediaBrowser.Common/Net/NetworkExtensions.cs 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
9 participants