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

[System.Net] Replace use of obsolete SupportsIPv6 property. #7584

Merged
merged 1 commit into from Mar 16, 2018

Conversation

alexischr
Copy link
Contributor

@alexischr alexischr commented Mar 12, 2018

The obsolete property was always set to false. Fixes #6940

@akoeplinger
Copy link
Member

The codepath quoted on the issue which always sets the obsolete property to false isn't used on Mono so I don't understand why this happens.

@alexischr
Copy link
Contributor Author

alexischr commented Mar 13, 2018

@akoeplinger we define FEATURE_PAL:

REFERENCE_SOURCES_FLAGS = -d:FEATURE_PAL,SYSTEM_NAMESPACE,MONO,PLATFORM_UNIX

@marek-safar
Copy link
Member

I think the fix should be to move the code we have to location which is compiled

to

@akoeplinger
Copy link
Member

@alexischr you missed this little nugget at the top of the file 😄 :

#if MONO
#undef FEATURE_PAL
#endif

@alexischr
Copy link
Contributor Author

@akoeplinger Sorry, I saw that today 🤦‍♂️

That got me confused and had to verify that the change in Dns.cs fixes the behavior (it does, and the other change is not needed). I have to figure how OSSupportsIPv6 can possibly be true when SupportsIPv6 apparently wasn't.

@alexischr
Copy link
Contributor Author

oh:

ipv6 = SettingsSectionInternal.Section.Ipv6Enabled;

@akoeplinger
Copy link
Member

Yeah but that should just return true if the config section isn't there:

internal bool Ipv6Enabled {
get {
#if CONFIGURATION_DEP && !MOBILE
try {
var config = (SettingsSection) System.Configuration.ConfigurationManager.GetSection ("system.net/settings");
if (config != null)
return config.Ipv6.Enabled;
} catch {
}
#endif
return true;
}

... and looking at our machine.config file we're not setting it.

@@ -321,7 +321,7 @@ private static IPHostEntry hostent_to_IPHostEntry(string originalHostName, strin
IPAddress newAddress = IPAddress.Parse(h_addrlist[i]);

#pragma warning disable 618
Copy link
Contributor

Choose a reason for hiding this comment

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

The pragma should no longer be necessary.

@alexischr
Copy link
Contributor Author

@akoeplinger "system.net/settings" section is declared here:

<section name="settings" type="System.Net.Configuration.SettingsSection, System, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089" />

And the "ipv6/enabled" defaults to false:

[ConfigurationProperty ("enabled", DefaultValue = "False")]

@alexischr alexischr force-pushed the fix-gh-6940 branch 3 times, most recently from b13fd68 to ceedc06 Compare March 15, 2018 16:30
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Ok, so you're right that the config setting defaults to false - but in the MS referencesource code nothing uses that config setting anymore except a few old APIs and everything else switched over to OSSupportsIPv6.

Two consistency comments to fix and we're good to go :)

@@ -318,7 +318,7 @@ private static IPHostEntry GetLocalHost()
// old IPv4 gethostbyname(null). Instead we need
// to do a more complete lookup.
//
if (Socket.SupportsIPv6)
if (Socket.OSSupportsIPv6)
Copy link
Member

Choose a reason for hiding this comment

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

please revert this change, we're not using this code.

@@ -320,11 +320,9 @@ private static IPHostEntry hostent_to_IPHostEntry(string originalHostName, strin
try {
IPAddress newAddress = IPAddress.Parse(h_addrlist[i]);

#pragma warning disable 618
if( (Socket.SupportsIPv6 && newAddress.AddressFamily == AddressFamily.InterNetworkV6) ||
if( (Socket.OSSupportsIPv6 && newAddress.AddressFamily == AddressFamily.InterNetworkV6) ||
(Socket.SupportsIPv4 && newAddress.AddressFamily == AddressFamily.InterNetwork) )
Copy link
Member

Choose a reason for hiding this comment

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

please use Socket.OSSupportsIPv4 here so we're consistent.

The obsolete property was always set to false. Fixes mono#6940
@marek-safar
Copy link
Member

@monojenkins backport 2017-12

@marek-safar
Copy link
Member

@monojenkins backport 2018-02

@monojenkins
Copy link
Contributor

@marek-safar backporting to 2017-12 failed, the patch results in conflicts:

Applying: Replace use of obsolete `SupportsIPv6` property.
error: patch failed: mcs/class/System/Test/System.Net/DnsTest.cs:209
error: mcs/class/System/Test/System.Net/DnsTest.cs: patch does not apply
Patch failed at 0001 Replace use of obsolete `SupportsIPv6` property.

Please backport manually!

@monojenkins
Copy link
Contributor

@marek-safar backporting to 2018-02 failed, the patch results in conflicts:

Applying: Replace use of obsolete `SupportsIPv6` property.
error: patch failed: mcs/class/System/Test/System.Net/DnsTest.cs:209
error: mcs/class/System/Test/System.Net/DnsTest.cs: patch does not apply
Patch failed at 0001 Replace use of obsolete `SupportsIPv6` property.

Please backport manually!

@marek-safar
Copy link
Member

@alexischr please backport manually

alexischr referenced this pull request Mar 20, 2018
[System.Net] Replace use of obsolete `SupportsIPv6` property.
alexischr referenced this pull request Mar 20, 2018
[System.Net] Replace use of obsolete `SupportsIPv6` property.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
[System.Net] Replace use of obsolete `SupportsIPv6` property.

Commit migrated from mono/mono@2f21177
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

5 participants