Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cli options "3" - do not round up the RTT time. #540

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

liboabo
Copy link

@liboabo liboabo commented May 21, 2024

In original version ping, when the delay is >= 100ms the resulting time is rounded for milliseconds.

$ ping -c3 192.168.56.102
PING 192.168.56.102 (192.168.56.102) 56(84) bytes of data.
64 bytes from 192.168.56.102: icmp_seq=1 ttl=64 time=81.0 ms
64 bytes from 192.168.56.102: icmp_seq=2 ttl=64 time=130 ms
64 bytes from 192.168.56.102: icmp_seq=3 ttl=64 time=81.4 ms

To avoid this added new command line option "-3".

$ ping -c3 -3 192.168.56.102
PING 192.168.56.102 (192.168.56.102) 56(84) bytes of data.
64 bytes from 192.168.56.102: icmp_seq=1 ttl=64 time=138.515 ms
64 bytes from 192.168.56.102: icmp_seq=2 ttl=64 time=101.676 ms
64 bytes from 192.168.56.102: icmp_seq=3 ttl=64 time=107.434 ms

@pevik
Copy link
Contributor

pevik commented May 22, 2024

Simple question: why do you need it?

I see your -3 behavior is similar to the default for ping from busybox, fping behaves similarly to our ping from iputils.

There are formal things which would need to be fixed (no new README file, fixing formatting - wrong indent, missing a real explanation for the reason), but the main question is whether it's really needed to be included.

@@ -83,6 +83,8 @@ void usage(void)
" -V print version and exit\n"
" -w <deadline> reply wait <deadline> in seconds\n"
" -W <timeout> time to wait for response\n"
"\nRRI options (Huawei):\n"
Copy link
Contributor

@pevik pevik May 22, 2024

Choose a reason for hiding this comment

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

What RRI means? Why do you mention Huawei? And I would put this to among other options.

Copy link
Author

Choose a reason for hiding this comment

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

The reason is to have the -A and -3 options in the same binary to see the actual RTT deviation on the interface.

@pevik
Copy link
Contributor

pevik commented May 22, 2024

Please stash the changes to a single commit (git rebase -i and then use s for the second commit and then force push: git push -f). Or I'll do it during merge.

ping/ping.c Outdated
@@ -337,6 +337,7 @@ main(int argc, char **argv)
.source6.sin6_family = AF_INET6,
.ni.query = -1,
.ni.subject_type = -1,
.rtt_precision = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This has wrong indent, please fix it (and other wrong indents).

Copy link
Author

Choose a reason for hiding this comment

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

Done

@pevik pevik requested review from nmeyerhans, okias and a team May 22, 2024 08:02
Copy link
Contributor

@pevik pevik left a comment

Choose a reason for hiding this comment

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

@liboabo I'd like to know why you you need that precise option.

@iputils/maintainers FYI this feature LGTM, but I'd like to know your opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants