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

ping: use struct timespec to measure time #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kerolasa
Copy link
Collaborator

Use the struct timespec to measure how much is the time, and various related
intervals. Aim of this change is to be exact how time calculation happen,
so that it as easy as possible to understand what happens with them.

The exponentially weighted moving average changed slightly while writing
this change. Earlier implementation was so hard to follow that after this
change maths may have changed, but even if they did at least now one should
be able to understand the equation - and fix if needed.

Notice that there is a need for follow up to remove 'next' millisecond int
variable in pinger() and schedule_exit(). That change will be complicated
enough to be separate commit.

p.s. This commit is build on top of #225 Depending on what happens in that
pull request rebase, and rework may be needed. That in mind look only the
last commit within context of this pull request.

@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2020

This pull request introduces 4 alerts when merging d5b5930 into 4fd276c - view on LGTM.com

new alerts:

  • 4 for FIXME comment

@lgtm-com
Copy link

lgtm-com bot commented Aug 31, 2020

This pull request introduces 3 alerts when merging 59ea5ac into 4fd276c - view on LGTM.com

new alerts:

  • 3 for FIXME comment

Use the struct timespec to measure how much is the time, and various related
intervals.  Aim of this change is to be exact how time calculation happen,
so that it as easy as possible to understand what happens with them.

The exponentially weighted moving average changed slightly while writing
this change.  Earlier implementation was so hard to follow that after this
change maths may have changed, but even if they did at least now one should
be able to understand the equation - and fix if needed.

Notice that there is a need for follow up to remove 'next' millisecond int
variable in pinger() and schedule_exit().  That change will be complicated
enough to be separate commit.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2020

This pull request introduces 3 alerts when merging cf0de0a into 78e3d25 - view on LGTM.com

new alerts:

  • 3 for FIXME comment

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.

appart from detail with [ ] the rest LGTM on first look.

@@ -1372,7 +1377,7 @@ int ping4_receive_error_msg(struct ping_rts *rts, socket_st *sock)
if (rts->opt_flood) {
write_stdout("\bE", 2);
} else {
print_timestamp(rts);
print_timestamp("[", rts, "]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not this change output? Not aware of [ ... ].

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, looking you moved [ from print_timestamp(). Could you keep it there? Because than the formatting is duplicated on both places (ping IPv4 and IPv6), i.e. wrong direction.

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

Successfully merging this pull request may close these issues.

None yet

2 participants