Page MenuHomeFreeBSD

sbin/ping: Show the timeout message when echo reply is missing
AcceptedPublic

Authored by hrs on Jan 2 2023, 9:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 5 2024, 9:44 PM
Unknown Object (File)
Oct 5 2024, 12:55 PM
Unknown Object (File)
Oct 2 2024, 9:48 PM
Unknown Object (File)
Oct 2 2024, 7:58 PM
Unknown Object (File)
Oct 2 2024, 7:20 PM
Unknown Object (File)
Oct 1 2024, 7:03 AM
Unknown Object (File)
Sep 27 2024, 2:08 PM
Unknown Object (File)
Sep 25 2024, 11:54 AM
Subscribers

Details

Reviewers
cy
Group Reviewers
network
Summary

This change makes ping(8) show the timeout message when it does not receive an echo reply in time like this:

% ping 192.0.2.1
PING 192.0.2.1 (192.0.2.1): 56 data bytes
Request timeout for icmp_seq 0
Request timeout for icmp_seq 1
Request timeout for icmp_seq 2
Request timeout for icmp_seq 3
^C
--- 192.0.2.1 ping statistics ---
5 packets transmitted, 0 packets received, 100.0% packet loss

This is useful for diagnostics because the sequence number shows which packet was lost. The same behavior can be found ping(8) on macOS.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48940
Build 45829: arc lint + arc unit

Event Timeline

hrs held this revision as a draft.
hrs published this revision for review.Jan 2 2023, 9:26 PM
hrs edited the summary of this revision. (Show Details)
hrs added a reviewer: network.

I like this addition.

There is one thing, in my opinion, that needs to be fixed:

% ping -q 192.0.2.1

Should not produce that output.
Any reason why not to use the patch straight from Darwin?:
https://github.com/apple-oss-distributions/network_cmds/blob/6ccdc225ad5aa0d23ea5e7d374956245d2462427/ping.tproj/ping.c#L1071-L1076

Also failing this test:

% ping -c1 192.0.2.1
PING 192.0.2.1 (192.0.2.1): 56 data bytes
Request timeout for icmp_seq 0
Request timeout for icmp_seq 0

--- 192.0.2.1 ping statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss
cy requested changes to this revision.Jan 3 2023, 3:20 AM
cy added a subscriber: cy.
cy added inline comments.
sbin/ping/ping.c
990

Should we add a test for (options & F_QUIET) == 0 too? I think the timeout message is counter to the -q option.

sbin/ping/ping6.c
1257

Same as previously:

Should we add a test for (options & F_QUIET) == 0 too? I think the timeout message is counter to the -q option.

This revision now requires changes to proceed.Jan 3 2023, 3:20 AM
  • sbin/ping: Do not show "Request timeout" when -q is specified
  • sbin/ping: Fix the timeout message when -c or -l are specified

The -l option will send the specified number of packets quickly. The current implementation will display the message only when all of them are lost.

A bug when the -c option is specified and no reply packet arrives that caused an extra timeout message has been fixed.

hrs marked 2 inline comments as done.Jan 3 2023, 9:00 AM
In D37930#861889, @jlduran_gmail.com wrote:

Also failing this test:

% ping -c1 192.0.2.1
PING 192.0.2.1 (192.0.2.1): 56 data bytes
Request timeout for icmp_seq 0
Request timeout for icmp_seq 0

--- 192.0.2.1 ping statistics ---
1 packets transmitted, 0 packets received, 100.0% packet loss

Thanks. This should be fixed in the updated patch.

sbin/ping/ping.c
990

Thanks for the feedback. Yes, no message should be displayed when F_QUIET is true. Fixed.

This revision is now accepted and ready to land.Jan 3 2023, 5:31 PM

Thank you!
I am sorry to insist on the same issue, but I believe the message "Request timeout for icmp_seq..." should appear when -A beeps.
This seems to be the case for Darwin/macOS.
As stated in my first comment, I don't see any reason why not to use the code from Apple, it is simpler and achieves the desired functionality (without the case for -l). Or maybe I am missing something?

In D37930#861990, @jlduran_gmail.com wrote:

I believe the message "Request timeout for icmp_seq..." should appear when -A beeps.
Or maybe I am missing something?

😞 I believe ping -Ac1 192.0.2.1 should beep once, so there's a bug in -A?
Regardless, it would be tangential to this differential.