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.

Sign up for GitHub

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

Jump to bottom

time-util: cleanups #2542

Merged
merged 1 commit into from
Feb 8, 2016
Merged

time-util: cleanups #2542

merged 1 commit into from
Feb 8, 2016

Conversation

0xAX
Copy link
Contributor

@0xAX 0xAX commented Feb 6, 2016

We have similar code in the dual_timestamp_from_realtime(), dual_timestamp_from_monotonic() and dual_timestamp_from_boottime_or_monotonic() functions which compares a ts with the given delta and return ts depends on delta value. Move this code to the separate inline function to prevent code duplication.

@keszybz keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Feb 7, 2016
@0xAX 0xAX force-pushed the get_ts_delta branch 2 times, most recently from a6218cc to 84aeb5f Compare February 7, 2016 15:30
@0xAX
Copy link
Contributor Author

0xAX commented Feb 7, 2016

@keszybz Thank you for feedback. I've updated commit.

@0xAX
time-util: introduce usec_sub()
5d634ca 
The dual_timestamp_from_realtime(), dual_timestamp_from_monotonic()
and dual_timestamp_from_boottime_or_monotonic() shares the same
code for comparison given ts with delta. Let's move it to the
separate inline function to prevent code duplication.
@0xAX
Copy link
Contributor Author

0xAX commented Feb 8, 2016

@poettering I've updated PR. What do you think about this variant? I've changed signed operation to unsigned, because I think it will be better as anyway we expect unsigned result from the usec_sub(). So If I didn't miss something, we envisage all options here.

poettering added a commit that referenced this pull request Feb 8, 2016
@poettering
Merge pull request #2542 from 0xAX/get_ts_delta
70b6596 
time-util: cleanups
@poettering poettering merged commit 70b6596 into systemd:master Feb 8, 2016
@benjarobin
Copy link
Contributor

@0xAX @poettering Is it OK of not handling USEC_INFINITY in the usec_sub utility function ?

@0xAX
Copy link
Contributor Author

0xAX commented Feb 9, 2016

@benjarobin In what case?

@benjarobin
Copy link
Contributor

@0xAX Currently none... But since this function is in time-util.h and not inside time-util.c somebody may want to use it elsewhere.
usec_sub shouldn't always return USEC_INFINITY if timestamp equals to USEC_INFINITY ?

@poettering
Copy link
Member

@benjarobin is right, we should make sure that USEC_INFINITY doesn't degrade to non-infinity when we subtract from it. I figure this is a lesson in that we better should have tests to catch corner-cases like this...

Will prep a fix...

poettering added a commit to poettering/systemd that referenced this pull request Feb 9, 2016
@poettering
util-lib: make sure usec_sub() doesn't degrade USEC_INFINITY
0c9d454 
As suggested in: systemd#2542 (comment)
@poettering
Copy link
Member

PR #2564 contains a fix that makes sure that USEC_INFINITY doesn't degrade in usec_sub(), and also adds a set of tests for this.

poettering added a commit to poettering/systemd that referenced this pull request Feb 10, 2016
@poettering
util-lib: make sure usec_sub() doesn't degrade USEC_INFINITY
04a1d84 
As suggested in: systemd#2542 (comment)
@0xAX 0xAX deleted the get_ts_delta branch February 10, 2016 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks util-lib
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@0xAX @benjarobin @poettering @keszybz