Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Feb 9, 2022

This is recent regression I bump to while running tests on FreeBSD.

#64625 assumes we would use ping utility only on Linux.
But that is how it works on pretty much on all UNIX like systems and macOS(like) is rare and recent exception.
(event if the list of actual variations is annoyingly long)

This PR generally flips the condition and it assumes we would use ping unless we know we don't have to.
That should make it work again for Solaris(like) and (Free)BSD(like) OSes.

cc: @am11 @Thefrank

@wfurt wfurt requested review from a team and rzikm February 9, 2022 08:28
@wfurt wfurt self-assigned this Feb 9, 2022
@ghost
Copy link

ghost commented Feb 9, 2022

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

Issue Details

This is recent regression I bump to while running tests on FreeBSD.

#64625 assumes we would use ping utility only on Linux.
But that is how it works on pretty much on all UNIX like systems and macOS(like) is rare and recent exception.
(event if the list of actual variations is annoyingly long)

This PR generally flips the condition and it assumes we would use ping unless we know we don't have to.
That should make it work again for Solaris(like) and (Free)BSD(like) OSes.

cc: @am11 @Thefrank

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net, os-freebsd

Milestone: -

@am11
Copy link
Member

am11 commented Feb 9, 2022

@wfurt, some network areas are quite involved and I didn't had enough time, so I previously asked community (#illumos channel on IRC) for help to port System.Net.* namespaces. The difference in /proc (we rely on procfs heavily for network statistics) alone is quite significant: on Linux it has text files that we conveniently read with System.IO.File.ReadAllLines/Text etc., whereas on illumos/Solaris it has binary files that requires read(struct)), so it is a large workitem for someone well-familiar with low-level networking APIs (that's not me 🙂).
Currently, only System.Runtime, corelib etc. works on illumos.

However, for ping utility, the manpage https://illumos.org/man/1m/ping says there are two forms. The first form supports timeout but not the count (npackets). The second form supports vice versa (uses default timeout of 20 sec). -c is traffic class, different than other Unices where it means count. e.g.

# First form: with default (1 sec)  timeout. simple "alive or not" reply
$ ping github.com

# First form: with specified timeout of 8 seconds
$ ping github.com 8

# Second form: send indefinite number of packets, with ttl 2 intervals
#         (default interval is 1 sec, can be set with -I with min value 0.011 sec):
#         (default size is 56 bytes and shown in output as 64 (56+8)):
$ ping -s -t 20 github.com

# Second form: send 4 packets of size 40 bytes (32 +8 which it will add), with minimum interval 0.011 sec:
$ ping -s -t 20 -I 0.011 github.com 32 4

Capability.CanUseRawSockets(TestSettings.GetLocalIPAddress().AddressFamily);

I think once System.Net is ported to illumos/Solaris, with capabilities queries, this path will work as well.

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@Thefrank
Copy link
Contributor

Thefrank commented Feb 9, 2022

LGTM.
This should be fine for now for what is currently un/supported, but as it was noted, this can get messy fast with all the different variations out there and their differing handling.

@wfurt
Copy link
Member Author

wfurt commented Feb 9, 2022

thanks for update @am11. When the time comes we may have more updates in production code that handles arguments.
This was really meant as test fix to sync up with reality. Since we don't have CI it is easy to miss the work-in-progress platforms.

@wfurt wfurt merged commit 55b5c98 into dotnet:main Feb 9, 2022
@wfurt wfurt deleted the unixPing branch February 9, 2022 18:48
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants