Skip to content

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Nov 15, 2021

Fixes #61465

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Net labels Nov 15, 2021
@ghost
Copy link

ghost commented Nov 15, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #61465

Needs more testing. The ICMPv4 code path was tested locally, ICMPv6 was not tested at all.

Author: filipnavara
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

@wfurt
Copy link
Member

wfurt commented Nov 17, 2021

LGTM

ifconfig en0
en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	options=10b<RXCSUM,TXCSUM,VLAN_HWTAGGING,AV>
	ether 68:5b:35:ce:4a:d8
	inet6 fe80::1c62:813b:8b0d:4b5%en0 prefixlen 64 secured scopeid 0x7
	inet 10.127.72.87 netmask 0xfffffc00 broadcast 10.127.75.255
	inet6 2001:4898:f0:20:1c02:4dba:a6b5:e6c0 prefixlen 64 autoconf secured

$  /Users/presenter/Downloads/ping/bin/Debug/net6.0/osx-x64/publish/ping
Status: TimeExceeded Address: 2001:4898:f0:20:ff::2

it would be great IMHO to add (outerloop) test. It would need to ignore cases when there is no answer and perhaps simply test that the address is not ANY.
I would add two entries (for v4 & v6) similar to

public static string PingHost => GetValue("DOTNET_TEST_NET_PING_HOST", "www.microsoft.com");
with 8.8.8.8 and 2001:4860:4860::8888 as defaults.

@filipnavara
Copy link
Member Author

Thanks for testing it. Apparently there's already similar test (SendPingToExternalHostWithLowTtlTest) in outer loop, it just doesn't validate ICMPv6 and the resulting Status/Address.

@filipnavara filipnavara marked this pull request as ready for review November 17, 2021 11:01
@karelz karelz requested a review from wfurt November 22, 2021 09:31
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
thanks @filipnavara

@wfurt wfurt merged commit 439ffd2 into dotnet:main Nov 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
@karelz karelz added this to the 7.0.0 milestone Jan 11, 2022
@filipnavara filipnavara deleted the ping-parse-fix branch June 5, 2025 07:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class Ping not reading response when ICMP Time-to-live exceeded
3 participants