Port System.Net.NetworkInformation to SunOS#125054
Conversation
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR ports portions of System.Net.NetworkInformation to SunOS (Solaris/illumos) by enabling interface enumeration via getifaddrs, preferring AF_LINK/PF_LINK over AF_PACKET, and wiring up SunOS-specific managed and native build plumbing.
Changes:
- Update native configuration/build logic for SunOS (CMake, address family mapping, interface MTU for
AF_LINK). - Add SunOS-specific managed implementations for
NetworkInterfacePal/IPGlobalPropertiesPaland related types. - Remove
UnsupportedOSPlatformannotations for illumos/solaris onNetworkInterface/IPGlobalProperties.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/configure.cmake | Adjusts configure-time linking for getifaddrs on SunOS. |
| src/native/libs/System.Native/CMakeLists.txt | Adds SunOS-native source selection (incl. SunOS network statistics stub). |
| src/native/libs/System.Native/pal_networkstatistics_sunos.c | Introduces SunOS stubbed network statistics entrypoints. |
| src/native/libs/System.Native/pal_networking.h | Adds PAL AF_LINK address family constant. |
| src/native/libs/System.Native/pal_networking.c | Adds AF_LINK conversions between platform and PAL. |
| src/native/libs/System.Native/pal_maphardwaretype.c | Avoids AF_PACKET on SunOS to prevent private-header dependencies. |
| src/native/libs/System.Native/pal_interfaceaddresses.c | Prefers AF_LINK, skips invalid alias indices, and fetches MTU for AF_LINK. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkInterface.cs | Removes illumos/solaris unsupported annotations. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/IPGlobalProperties.cs | Removes illumos/solaris unsupported annotations. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkInterfacePal.SunOS.cs | Implements NetworkInterfacePal for SunOS via getifaddrs interop. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs | Adds SunOS UnixNetworkInterface implementation. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPInterfaceProperties.cs | Adds SunOS IP interface properties type. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPv4InterfaceProperties.cs | Adds SunOS IPv4 properties (mostly unsupported beyond MTU). |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPv6InterfaceProperties.cs | Adds SunOS IPv6 properties (scope ID + MTU). |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/IPGlobalPropertiesPal.SunOS.cs | Wires SunOS IPGlobalPropertiesPal. |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPGlobalProperties.cs | Adds SunOS global properties implementation (currently throws for most APIs). |
| src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPGlobalStatistics.cs | Adds SunOS minimal global statistics implementation. |
| src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj | Includes SunOS-specific managed sources for illumos/solaris. |
Comments suppressed due to low confidence (5)
src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj:221
- Spelling in the comment: "uisng" should be "using".
<Compile Include="System\Net\NetworkInformation\NetworkAddressChange.UnknownUnix.cs" />
<!-- Could port NetworkAddressChange.Android.cs uisng a PF_ROUTE socket -->
</ItemGroup>
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs:123
- Spelling in comments: "iplement" should be "implement".
// Future: iplement via kstats
public override IPInterfaceStatistics GetIPStatistics()
=> throw new PlatformNotSupportedException(SR.net_InformationUnavailableOnPlatform);
// Future: iplement via kstats
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs:132
- Spelling in comments: "iplement" should be "implement".
// Future: iplement via kstats
// Just return zero for now.
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs:6
- Spelling: "Eveything" should be "Everything" in the file header comment.
// Similar to the BSD and Linux code in use of getifaddrs()
// Eveything else is different (properties and statistics via kstats)
src/native/libs/System.Native/pal_interfaceaddresses.c:476
- There are tab characters in this block (notably the closing brace indentation and the
#endifcomment alignment). Repo-wide .editorconfig specifiesindent_style = space, so these should be converted to spaces to match formatting conventions.
if (ioctl(socketfd, SIOCGIFMTU, &ifr) == 0)
{
nii->Mtu = ifr.ifr_mtu;
}
}
...s/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPGlobalProperties.cs
Outdated
Show resolved
Hide resolved
...ibraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/NetworkInterface.cs
Show resolved
Hide resolved
...s/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSIPGlobalProperties.cs
Show resolved
Hide resolved
Use PF_LINK on SunOS getifaddrs on illumos needs -lsocket -lnsl Get MTU for IF_LINK in SystemNative_GetNetworkInterfaces Add NetworkInterface support files for SunOS Passes most of: System.Net.NetworkInformation.Functional.Tests Co-authored-by: Copilot <Copilot@users.noreply.github.com>
Use |
Thanks. Working on that. |
|
Added SkipOnPlatform lines. Now I see: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs:66
- The local variable is named
lni(LinuxNetworkInterface) in a SunOS-specific implementation, which makes the code harder to read/maintain. Rename it (and the matchingTryGetValueout var below) to something SunOS-appropriate likesniorsunOsNi.
{
var lni = new SunOSNetworkInterface(Utf8StringMarshaller.ConvertToManaged(nii->Name)!, nii->InterfaceIndex);
lni._interfaceType = (NetworkInterfaceType)nii->HardwareType;
lni._speed = nii->Speed; // Note: not filled in (-1)
...ies/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs
Outdated
Show resolved
Hide resolved
...aries/System.Net.NetworkInformation/tests/FunctionalTests/NetworkAvailabilityChangedTests.cs
Show resolved
Hide resolved
src/libraries/System.Net.NetworkInformation/tests/FunctionalTests/NetworkInterfaceBasicTest.cs
Outdated
Show resolved
Hide resolved
…workInformation/SunOSNetworkInterface.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SunOSNetworkInterface.cs:69
- The local variable name
lniin SunOSNetworkInterface is a copy/paste artifact (it reads like "LinuxNetworkInterface") and makes the code harder to follow on this platform. Rename it (and the corresponding out-var) to something SunOS-specific likesnito avoid confusion.
var lni = new SunOSNetworkInterface(Utf8StringMarshaller.ConvertToManaged(nii->Name)!, nii->InterfaceIndex);
lni._interfaceType = (NetworkInterfaceType)nii->HardwareType;
lni._speed = nii->Speed; // Note: not filled in (-1)
lni._operationalStatus = (OperationalStatus)nii->OperationalState;
lni._mtu = nii->Mtu;
lni._supportsMulticast = nii->SupportsMulticast != 0;
|
@dotnet/ncl, I have added No Merge label as this is not a complete port. I suggest we shouldn't take incomplete changes which are more meaningless than saying NotSupported assembly as we have in main. |
Can you offer a list of what would be needed to let this be considered "complete"? Would you rather see something like #124728 until this network interface work can do more than show you the local interfaces etc? |
By implementing the functionality.. All currently failing (skipped) tests that are not skipped on FreeBSD should be passing for this PR to be considered complete port. If something is not available on the OS, we need to understand and decide whether to throw NotSupportedException, return some sentinel/default value or some such. We can't just mislabel stuff and move on like you did in 03882c1. You cannot just reason "Not supported on SunOS" on something like |
Use PF_LINK on SunOS
getifaddrs on illumos needs -lsocket -lnsl
Get MTU for IF_LINK in SystemNative_GetNetworkInterfaces Add NetworkInterface support files for SunOS
Test passes: System.Net.NetworkInformation.Functional.Tests
System.Net.NetworkInformation.Functional.Tests
Total: 239, Errors: 0, Failed: 20, Skipped: 1, Time: 2.088s
This PR should replace #124728