ping: Unify ping/ping6 statistics section

[jlduran_gmail.com]

Summary

This is a first step towards a unification/simplification of ping/ping6 (internally). The end goal is to produce a standardized user-facing output.
Before (ping6):

PING6(56=40+8+8 bytes) 2001:db8::1 --> 2001:db8::2
16 bytes from ::1, icmp_seq=0 hlim=64 time=0.168 ms
16 bytes from ::1, icmp_seq=1 hlim=64 time=0.068 ms
 
--- 2001:db8::2 ping6 statistics ---
round-trip min/avg/max/std-dev = 0.068/0.118/0.168/0.050 ms

After (ping6):

PING(56=40+8+8 bytes) 2001:db8::1 --> 2001:db8::2
16 bytes from ::1, icmp_seq=0 hlim=64 time=0.168 ms
16 bytes from ::1, icmp_seq=1 hlim=64 time=0.068 ms
 
--- 2001:db8::2 ping statistics ---
round-trip min/avg/max/stddev = 0.068/0.118/0.168/0.050 ms

This has the nice side-effect of adding units to SIGINFO's statistics, as printing numbers without units may not be of much help. Also mentions the fact that these times are round-trip.

Before (ping/ping6 SIGINFO):

2/2 packets received (100.0%) 0.068 min / 0.118 avg / 0.168 max

After (ping/ping6 SIGINFO):

--- <ipv4/ipv6 address> ping statistics ---
2 packets transmitted, 2 packets received, 0.0% packet loss
round-trip min/avg/max/stddev = 0.068/0.118/0.168/0.050 ms

In the case of a SIGINFO, the output will be printed to stderr, for both ping and ping6.

Test Plan

This is the follow-up (per request) of D38485.

[jlduran_gmail.com]

Sigh, use the right base for the diff.

[markj]

Looks good, just a couple of comments.

sbin/ping/ping.c

862–863

I don't see any particular reason to conditionalize SIGINFO support?

1429–1430

These ARGSUSED annotations are obsolete and shouldn't be added to new code. The compiler will complain if parameters are unused.

Actually, why bother having this parameter at all? onint() isn't a signal handler, it just gets called in the event loop when the signal handler latched SIGINT.

[jlduran_gmail.com]

Address suggestions:

• Remove SIGINFO guards
• Remove onint()

sbin/ping/ping.c

862–863

Removed! I believe these guards came from the KAME project, probably to maximize portability (chiefly with Linux?).

1429–1430

Yes, seems like a byproduct. Removed!

[jlduran_gmail.com]

• Do not use D38483 as a parent revision

[jlduran_gmail.com]

• Reset seenint and seeninfo counters for SIGINT and SIGINFO respectively, just like ping6.c

[markj]

sbin/ping/ping.c

909

The loop condition is seenint == 0 - how can this condition be true?

913

But now we only handle signals in the main loop, so the comment applies to this check as well. In other words, removing the exit() call from the signal handler means that the DNS lookup has to complete before ping exits, which seems undesirable.

[jlduran_gmail.com]

sbin/ping/ping.c

909

I guess when we receive a SIGALRM. We increment the seenint counter with both, SIGALRM and SIGINFO. OpenBSD uses a separate seenalrm, I'm thinking about adding it, as this is indeed confusing.

[jlduran_gmail.com]

sbin/ping/ping.c

909

I guess when we receive a SIGALRM. We increment the seenint counter with both, SIGALRM and SIGINFO. OpenBSD uses a separate seenalrm, I'm thinking about adding it, as this is indeed confusing.

Sorry, my comment makes no sense.
It seems unnecessary. It was taken from ping6.c I'll resubmit.

[jlduran_gmail.com]

sbin/ping/ping.c

909

The original change stems from 0ecaa7ed053a7fb83748cae283ecee81e85a85bb. I'll see if I can recreate the PR.

[jlduran_gmail.com]

sbin/ping/ping.c

913

Right, it will likely produce a regression of PR 4696.

[jlduran_gmail.com]

Address issues:

• Move the interrupt handler function onsignal() to main.c. When a SIGINT is received, check to avoid PR 4696, although I was not able to reproduce it. I am currently working on adding tests for interrupts and this will become a test. I suspect cap_gethostbyname2 is not bound to the same threading issues as gethostbyname2 (back when the bug was filed)
• Unify F_NUMERIC and F_HOSTNAME constants, given numeric output is the default, use F_HOSTNAME (-n and -H should be mutually exclusive?)
• Reduce diffs between ping.c and ping6.c whenever possible

[jlduran_gmail.com]

sbin/ping/ping6.c

1122

To me, this looks like leftover from debugging: PING((40 + N)=40+8+(N - 8)), where N is pingerlen, typically 16 bytes.

1124

This arrow, on the other hand, could be somewhat useful. OpenBSD has opted to display it only if the ping is verbose (-v) for both ping and ping6. This would be the last of the "common" user-facing differences between ping and ping6.

[markj]

This is looking ok to me with the nits resolved.

sbin/ping/ping.c

982

The indentation on this comment is wrong.

991

The indentation on the continuing line is wrong.

[jlduran_gmail.com]

In D39126#896811, @markj wrote:

This is looking ok to me with the nits resolved.

Thank you for taking the time of reviewing it!
I plan to resume submitting patches and fixes after 13.2-RELEASE (hopefully next week).

[jlduran_gmail.com]

• Fix indentation

[markj]

In D39126#896814, @jlduran_gmail.com wrote:

In D39126#896811, @markj wrote:

This is looking ok to me with the nits resolved.

Thank you for taking the time of reviewing it!
I plan to resume submitting patches and fixes after 13.2-RELEASE (hopefully next week).

Great, thank you. This patch doesn't apply cleanly and I can see you have some other unsubmitted patches. I'll hold off on committing for now then.