Use ICMP for ping command

[amclain]

Resolves #81

This PR replaces the ping code that used TCP with an ICMP implementation. The interface maintains backwards compatibility with the former implementation.

[fhunleth]

Thanks!

I assume that you've only been testing on Linux. I'm trying this out on MacOS right now without any luck. All unit tests fail - usually with time outs. I haven't debugged yet. I just wanted to confirm.

Also, I'm thinking about what to do with tping. The intention behind the t was to stand for TCP. Since that's not the case any more, there's no reason to keep the t. Another option would be to keep the TCP "ping" functionality, but rename the helper to tcp_ping to be more clear. Then there's the question of giving help to users since at the moment, ICMP ping won't work on any Nerves device since the sysctl update to the systems hasn't been release, plus many people won't update their Nerves systems when it is released. They'll have to switch to a TCP-based ping.

[amclain]

I assume that you've only been testing on Linux. I'm trying this out on MacOS right now without any luck. All unit tests fail - usually with time outs. I haven't debugged yet. I just wanted to confirm.

That's correct, I've been testing on my Linux dev machine and the target hardware. Unfortunately I don't have access to a Mac to debug the issue you're seeing. I tried to add unit tests that were simple and had a low impact on refactoring since most of what the code is doing is orchestrating low level function calls. However, based on your experience on MacOS and what I saw in CI, should I convert the tests to using mocks? The drawback I could see to that is Toolshed's ping still wouldn't work on MacOS if it's expected to be able to run on the host.

Also, I'm thinking about what to do with tping. The intention behind the t was to stand for TCP.

I'm glad you brought this up; I meant to ask about it. I was wondering if t meant TCP, so thanks for clarifying that. I noticed tping's behavior is to send a single ping and return, rather than how ping sends repeatedly. Since tping is a public function I was hesitant to remove it and wanted to hear your thoughts on backwards compatibility for it. Although the function's name may be inaccurate now, is there a need for a function that can send a single ping to do a simple check if a host is up? An alternative could be to add a :count option to ping. An explicit host_up? function that returns true/false may be another approach to this, but may be out of scope of this issue.

If tping (or tcp_ping) remains, is it supposed to retain the single ping behavior, or is it supposed to behave like ping?

Another option would be to keep the TCP "ping" functionality, but rename the helper to tcp_ping to be more clear.

Could there be a use case for this when pinging hosts that block ICMP but expose a web service?

[fhunleth]

Don't worry about mocks or about breaking the public APIs. Regarding the API, this project is only supposed to be called by human users typing into the IEx prompt after running use Toolshed. I consider typing Toolshed.anything as unsupported and ok to remove/change without warning. Of course, if it's not nice to users then we shouldn't do it.

To replace the current ping command, this really needs to work more places. MacOS seems important. I'm not sure what the deal is with CI. Maybe CircleCI is locked down unusually tightly with regards to ICMP. At any rate, the current TCP-based ping works everywhere, so even though using ICMP may be more "correct", I think it sets a bar for functionality. Just to clarify, you don't need to run a web service for TCP pings to work. They work for checking connectivity to any computer that's not behind a firewall that drops TCP connection attempts. That's a similar restriction to ICMP since firewalls can drop ICMP pings as well.

[fhunleth]

I ran Wireshark. It might not be working on MacOS since the ICMP checksum isn't right.

I tried https://github.com/hauleth/gen_icmp and that works on MacOS. Wireshark also says that the checksum is correct.

It might be something else, but that's my guess for right now.

[amclain]

I ran Wireshark. It might not be working on MacOS since the ICMP checksum isn't right.

Ah, I know how to fix that. When I wiresharked Linux it was creating the checksum automatically for :dgram sockets so I took that code out to make the PR smaller. I'll add it back in.

[amclain]

I've pushed the checksum change in 95c16fc. Hopefully that works now!

[fhunleth]

Thanks! I got busy at work. I'll try out again soon.

[fhunleth]

I merged and made some updates. I removed IPv6 support for now due to complications with computing the ICMPv6 checksum.

[fhunleth]

Thanks for submitting this and sorry for the long wait to get it merged!