Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName #3471

jonmill · 2015-09-25T20:56:59Z

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName. The calls to getaddrinfo are not 1-to-1 with the native calls due to
complexities around the native structs. Calls using sockaddr and
addrinfo have nested structs pointing to different data depending
on if the IP is IPv4 or IPv6. To mitigate the complexities around
this code and around the underlying struct differences on the
currently supported platforms, I kept the same calling pattern (as
in Get* and Free*) but converted them to use an intermediate data
structure that is defined in the PAL. We can then take in the
required data from the Managed World and determine which native
types are required, handle all the native structs and casts, and
give back only the information that the managed code needs in a
much simpler format.

/cc @nguerrera @stephentoub @jasonwilliams200OK @pgavlin

jonmill · 2015-09-25T20:57:17Z

Note - will run the formatting script for the unmanaged code before committing

jonmill · 2015-09-25T20:58:05Z

This should fix #2988 :)

ghost · 2015-09-25T21:17:09Z

Thank you! @sokket :)

This was supposed to be the last major stopper to build CoreFX on FreeBSD! Now BSD support should be completed. One minor note for those who would want to try to build on FreeBSD 10.2 (latest version, released Aug 2015); if you are using latest Mono from portsnap / pkg install mono, you should add TZ=UTC before ./build.sh#L132 like so: TZ=UTC Mono29679.. or using setenv TZ UTC before running ./build.sh; as pointed out by @saper here: https://github.com/dotnet/coreclr/issues/602#issuecomment-108672899. This is most probably fixed in Mono master (by mono/mono#1860 and mono/mono#1870, which is landed on master but the current mono master is still not building on FreeBSD as is, due to some linux specific GCC macro usage..), so until vNext of Mono is landed for FreeBSD, this patch is needed.

After that I tried cert-sync /usr/local/share/certs/ca-root-nss.crt as well as mozroots --import --sync and mozroots --import --sync --machine and certmgr -ssl <for all: nuget.org, myget.org, go.microsoft.com and nugetgallery.blob.core.windows.net>, but it fails on Restoring all packages... step:

BatchRestorePackages:
  Restoring all packages...
EXEC : error : SendFailure (Error writing headers) [/root/projects/corefx/build.proj]
EXEC : error : SendFailure (Error writing headers) [/root/projects/corefx/build.proj]
EXEC : error : SendFailure (The object was used after being disposed.) [/root/projects/corefx/build.proj]
...
...

(note that i have done this exercise 4 times with FreeBSD v10.1 and v10.2 since yesterday via Windows 10 Hyper-V, but running into the same issue)

maybe someone can assist how to get around this issue.. /cc @akoeplinger.

stephentoub · 2015-09-25T21:48:42Z

src/Common/src/Interop/Unix/System.Native/Interop.GetHostName.cs

@@ -27,11 +25,10 @@ internal static unsafe string gethostname()
                // which should only happen if the buffer we supply isn't big
                // enough, and we're using a buffer size that the man page
                // says is the max for POSIX (and larger than the max for Linux).
-                Debug.Fail("gethostname failed");
+                System.Diagnostics.Debug.Fail("gethostname failed");


Nit: Why remove the using and then fully qualify this?

Ah sorry this was a new file and I copy+paste the header and using's from a template. Will add the using back to keep it a simpler change to track

bartonjs · 2015-09-25T23:04:06Z

This one is failing with some segfaulting tests:

22:13:21 ./run-test.sh: line 172: 58393 Aborted                 (core dumped) ./corerun xunit.console.netcore.exe $testDllName -xml testResults.xml -notrait category=failing -notrait category=OuterLoop -notrait category=$xunitOSCategory
22:13:21    System.Diagnostics.FileVersionInfo.Tests  Total: 3, Errors: 0, Failed: 0, Skipped: 0, Time: 0.873s
22:13:21 ./run-test.sh: line 172: 58391 Aborted                 (core dumped) ./corerun xunit.console.netcore.exe $testDllName -xml testResults.xml -notrait category=failing -notrait category=OuterLoop -notrait category=$xunitOSCatego

http://dotnet-ci.cloudapp.net/job/dotnet_corefx_linux_debug_tst_prtest/2749/console

jonmill · 2015-09-25T23:13:27Z

I believe its the System.Net.Primitives tests due to a native static assert. Looks like our assumption of max IP address string lengths is different in managed than the define in system headers

Maxwe11 · 2015-09-26T06:59:12Z

src/Native/System.Native/pal_networking.cpp

+static_assert(PAL_NO_DATA == NO_DATA, "");
+static_assert(PAL_NO_ADDRESS == NO_ADDRESS, "");
+
+static void IpStringToAddressHelper(const char* address, const char* port, bool isIpV6, int32_t* err, std::function<void(const addrinfo& info)> lambda)


AFAIK std::function uses type erasure which may/should result in additional allocation via operator new so I think this parameter should be const& to avoid additional allocations during the copying.

Good to know, thanks! I added that fix

pgavlin · 2015-09-28T23:26:17Z

There are a number of places in this change that manually pad record types (incl. automatically-generated closures) out to avoid a compiler warning on 64-bit platforms. IIUC the warning is "the compiler is padding this structure out for alignment". Have we considered disabling this warning (esp. given that different platforms are going to require different alignments, so the decisions we make now may not be universally applicable)?

pgavlin · 2015-09-28T23:30:47Z

src/Native/System.Native/pal_networking.cpp

+        // walk since we would (most likely) need to realloc the array, which is more
+        // expensive than just incrementing the limited number of IPs twice.
+        he->Count++;
+        infos.push_back(ai);


I'd prefer that we avoid the vector here and just repeat the check for AF_INET and AF_INET6 in the loop below.

janhenke · 2015-09-29T19:31:14Z

@dotnet-bot test this please.

jonmill · 2015-09-29T20:03:45Z

Looks like more of the new socket tests are failing. I'm investigating now, the previous failures were fixed with trivial changes so hopefully these will be the same

jonmill · 2015-09-29T22:18:41Z

@pgavlin Have you seen these failures before?

pgavlin · 2015-09-29T23:02:18Z

@dotnet-bot test this please

pgavlin · 2015-09-29T23:02:23Z

Yes, I have.

pgavlin · 2015-09-29T23:46:57Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

-                    }
+                        sockaddr = new SocketAddress(AddressFamily.InterNetworkV6);
+                        factory = IPEndPointStatics.IPv6Any;
+                        bufferLength = SocketAddressPal.IPv6AddressSize;


Please add a comment or file an issue to track updating this once sockaddr et. al. are shimmed out.

Opened issue #3570

pgavlin · 2015-09-29T23:49:33Z

This is looking good aside from some comments re: PalIpAddress.

jonmill · 2015-10-01T00:45:22Z

@dotnet-bot test this please

The calls to getaddrinfo are not 1-to-1 with the native calls due to complexities around the native structs. Calls using sockaddr and addrinfo have nested structs pointing to different data depending on if the IP is IPv4 or IPv6. To mitigate the complexities around this code and around the underlying struct differences on the currently supported platforms, I kept the same calling pattern (as in Get* and Free*) but converted them to use an intermediate data structure that is defined in the PAL. We can then take in the required data from the Managed World and determine which native types are required, handle all the native structs and casts, and give back only the information that the managed code needs in a much simpler format.

jonmill · 2015-10-01T00:52:44Z

Looks like the test failure is unrelated and the assertions are a known issue that @pgavlin is investigating now. Once this build completes and I verify the same issues, this will be good to merge

jonmill · 2015-10-01T01:52:44Z

Same, expected, failures. Any further comments?

stephentoub · 2015-10-01T17:11:11Z

@dotnet-bot test this please (I disabled the failing AssemblyLoadContext test)

jonmill · 2015-10-01T17:38:49Z

@dotnet-bot test this please (looks like a Windows timer test failed...?)

stephentoub · 2015-10-01T17:40:10Z

looks like a Windows timer test failed...?

Did you open an issue for it? 😉

jonmill · 2015-10-01T17:42:21Z

Yup :) #3583

ghost · 2015-10-01T18:01:02Z

✅ 👍

jonmill · 2015-10-01T18:01:45Z

Green run - any more comments or can this be merged?

pgavlin · 2015-10-01T18:07:42Z

LGTM. We can address any other feedback during follow-up changes.

jonmill · 2015-10-01T18:10:20Z

Cool, merging. Thanks everyone!

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName Commit migrated from dotnet/corefx@5da58c0

jonmill added the 2 - In Progress label Sep 25, 2015

dnfclas added the cla-already-signed label Sep 25, 2015

stephentoub reviewed Sep 25, 2015
View reviewed changes

Maxwe11 reviewed Sep 26, 2015
View reviewed changes

pgavlin reviewed Sep 28, 2015
View reviewed changes

pgavlin reviewed Sep 29, 2015
View reviewed changes

jonmill pushed a commit that referenced this pull request Oct 1, 2015

Merge pull request #3471 from sokket/pal

5da58c0

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName

jonmill merged commit 5da58c0 into dotnet:master Oct 1, 2015

jonmill removed the 2 - In Progress label Oct 1, 2015

jonmill deleted the pal branch October 1, 2015 18:10

CIPop mentioned this pull request Oct 2, 2015

Porting System.Net.NetworkInformation and System.Net.Utilities #3600

Merged

karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#3471 from sokket/pal

233498d

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName Commit migrated from dotnet/corefx@5da58c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName #3471

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName #3471

jonmill commented Sep 25, 2015

jonmill commented Sep 25, 2015

jonmill commented Sep 25, 2015

ghost commented Sep 25, 2015

stephentoub Sep 25, 2015

jonmill Sep 25, 2015

bartonjs commented Sep 25, 2015

jonmill commented Sep 25, 2015

Maxwe11 Sep 26, 2015

jonmill Sep 28, 2015

pgavlin commented Sep 28, 2015

pgavlin Sep 28, 2015

jonmill Sep 28, 2015

janhenke commented Sep 29, 2015

jonmill commented Sep 29, 2015

jonmill commented Sep 29, 2015

pgavlin commented Sep 29, 2015

pgavlin commented Sep 29, 2015

pgavlin Sep 29, 2015

jonmill Oct 1, 2015

pgavlin commented Sep 29, 2015

jonmill commented Oct 1, 2015

jonmill commented Oct 1, 2015

jonmill commented Oct 1, 2015

stephentoub commented Oct 1, 2015

jonmill commented Oct 1, 2015

stephentoub commented Oct 1, 2015

jonmill commented Oct 1, 2015

ghost commented Oct 1, 2015

jonmill commented Oct 1, 2015

pgavlin commented Oct 1, 2015

jonmill commented Oct 1, 2015

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName #3471

Shim'ing out GetNameInfo, get/free addrinfo, and GetHostName #3471

Conversation

jonmill commented Sep 25, 2015

jonmill commented Sep 25, 2015

jonmill commented Sep 25, 2015

ghost commented Sep 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented Sep 25, 2015

jonmill commented Sep 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgavlin commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janhenke commented Sep 29, 2015

jonmill commented Sep 29, 2015

jonmill commented Sep 29, 2015

pgavlin commented Sep 29, 2015

pgavlin commented Sep 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgavlin commented Sep 29, 2015

jonmill commented Oct 1, 2015

jonmill commented Oct 1, 2015

jonmill commented Oct 1, 2015

stephentoub commented Oct 1, 2015

jonmill commented Oct 1, 2015

stephentoub commented Oct 1, 2015

jonmill commented Oct 1, 2015

ghost commented Oct 1, 2015

jonmill commented Oct 1, 2015

pgavlin commented Oct 1, 2015

jonmill commented Oct 1, 2015