ping: Add units to SIGINFO's statistics
AbandonedPublic
Actions

Authored by jlduran on Feb 10 2023, 2:24 PM.

Details

Reviewers

asomers
markj

Summary

Printing numbers without units may not be of much help.
Also mention the fact that these times are round-trip.
Before:

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

After:

2/2 packets received (100.0%) round-trip min/avg/max = 0.068/0.118/0.168 ms

Test Plan

Should we drop the void casts?

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jlduran created this revision.Feb 10 2023, 2:24 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 10 2023, 2:24 PM

jlduran requested review of this revision.Feb 10 2023, 2:24 PM

jlduran added a parent revision: D38484: ping: Specify the sigaction(2) name.

I like it. And yes I think you should add stddev to match the regular program output at termination.

In D38485#875773, @asomers wrote:

I like it. And yes I think you should add stddev to match the regular program output at termination.

OK, will update the revision. OpenBSD reuses the same function, I'll try to come up with something similar.

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

Avoid a potential square root of a negative number calculation (based on an alternative to D38480, this change should not part of this revision)

I have a branch where it unifies the code into a single summary() function, however it changes a lot more of the code that I would like/have tests for at the moment. Besides, the code ends up being almost identical to ping6.c's. Let's leave the unification for later in the ping/ping6 series.
A few cosmetic things discovered from this exercise are:

ping's SIGINFO prints out to stderr (ping6 to stdout)
ping/ping6 should just become ping when displayed to the user
ping uses the expression "somebody's printing up packets!", I prefer ping6's "somebody's duplicating packets!" when a duplicate packet is received
ping calls the population standard deviation "stddev", while ping6 calls it "std-dev". I'm OK either way, but it should be the same for both

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

Avoid a potential square root of a negative number calculation (based on an alternative to D38480, this change should not part of this revision)

I have a branch where it unifies the code into a single summary() function, however it changes a lot more of the code that I would like/have tests for at the moment. Besides, the code ends up being almost identical to ping6.c's. Let's leave the unification for later in the ping/ping6 series.
A few cosmetic things discovered from this exercise are:

ping's SIGINFO prints out to stderr (ping6 to stdout)

It should be stderr.

ping/ping6 should just become ping when displayed to the user

I agree.

ping uses the expression "somebody's printing up packets!", I prefer ping6's "somebody's duplicating packets!" when a duplicate packet is received

I agree.

ping calls the population standard deviation "stddev", while ping6 calls it "std-dev". I'm OK either way, but it should be the same for both

"stddev" seems fine to me.

This revision is now accepted and ready to land.Mar 10 2023, 5:55 PM

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Yes, but probably not today (the tests may take some time).

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

jlduran mentioned this in D39126: ping: Unify ping/ping6 statistics section.Mar 16 2023, 5:05 PM

In D38485#888290, @jlduran_gmail.com wrote:

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

The code has been rebased on GitHub, and the proposal of this diff unified has been submitted as D39126.
Thank you!

In D38485#890556, @jlduran_gmail.com wrote:

In D38485#888290, @jlduran_gmail.com wrote:

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

The code has been rebased on GitHub, and the proposal of this diff unified has been submitted as D39126.
Thank you!

Do I understand correctly that this review is now obsolete in favour of D39126?

In D38485#891006, @markj wrote:

In D38485#890556, @jlduran_gmail.com wrote:

In D38485#888290, @jlduran_gmail.com wrote:

In D38485#888217, @markj wrote:

In D38485#888216, @jlduran_gmail.com wrote:

In D38485#888201, @markj wrote:

In D38485#877727, @jlduran_gmail.com wrote:

Address suggestions:
In this first iteration, simply duplicate the code from the final statistics section into SIGINFO's.

I think this can be committed as-is. Is there a review up to unify them?

I was planning on leaving that unification for last, but let me try to at least unify and test this user-facing part.
Maybe I should put all shared code in sbin/ping/utils.c... I'll work on that. Thank you!

Will you update this review with the result? I'll hold off on committing in that case.

Actually, I'll be submitting a separate review, it's a long diff.
I placed the unified code into main.c, as more elements are unified, they'll also be placed in main.c?
I'll wait over the weekend to let these changes soak in, before formally creating the review. But, if you want to take a preliminary look:
https://github.com/jlduran/freebsd-src/tree/ping-interrupts-ping6-unification

The code has been rebased on GitHub, and the proposal of this diff unified has been submitted as D39126.
Thank you!

Do I understand correctly that this review is now obsolete in favour of D39126?