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

Bugfix Ping.cs #3949

Closed
wants to merge 2 commits into from
Closed

Bugfix Ping.cs #3949

wants to merge 2 commits into from

Conversation

jogibear9988
Copy link
Contributor

@monojenkins
Copy link
Contributor

Hello! I'm the build bot for the Mono project.

I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done.

Contributors can ignore this message.

@dnfclas
Copy link

dnfclas commented Nov 14, 2016

Hi @jogibear9988, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@@ -215,7 +215,7 @@ static IPAddress GetNonLoopbackIP ()
return addr;
#pragma warning restore 618

throw new InvalidOperationException ("Could not resolve non-loopback IP address for localhost");
return IPAddress.Loopback;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why we need a non-loopback address, but this is not the correct way of fixing it.
You need to modify SendPrivileged() so it doesn't rely on GetNonLoopbackIP().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the COde

Copy link
Contributor Author

@jogibear9988 jogibear9988 left a comment

Choose a reason for hiding this comment

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

I also don't know why, and why is the first one found the correct ine? But ping also works when I add 127.0.0.1 to hosts, so I thought this is ok.

@jogibear9988
Copy link
Contributor Author

When I look again on this, I think the whole Code is wrong! We need the IP wich was used for send, and not the first one with correct hostname.
But how can we find out?

@jogibear9988
Copy link
Contributor Author

@akoeplinger
Copy link
Member

Yeah, the corefx code looks better in that regard. You can try porting the relevant pieces.

@jogibear9988
Copy link
Contributor Author

I will try to use the corefx code. Is it not planed to migrate a this Assembly in Mono?

Which Editor can I use? I tried with VS2015, but it crashes with the Mono Solution.

@jogibear9988
Copy link
Contributor Author

With the new change it does work. And it's the same as in corefx. They also only use the destination IP

@jogibear9988
Copy link
Contributor Author

@akoeplinger is the patch so ok?

@jogibear9988
Copy link
Contributor Author

@akoeplinger - do i still need to do changes on my patch?

@jogibear9988 jogibear9988 mentioned this pull request May 28, 2017
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

4 participants