Skip to content

Support unsigned time_t#438

Closed
willmmiles wants to merge 2 commits intologrotate:masterfrom
willmmiles:use-difftime
Closed

Support unsigned time_t#438
willmmiles wants to merge 2 commits intologrotate:masterfrom
willmmiles:use-difftime

Conversation

@willmmiles
Copy link

On platforms with unsigned time_t simple subtraction may produce a large positive result instead of the expected negative value. Use difftime() instead of subtracting to ensure meaningful comparisons.

I ran in to this issue using logrotate on QNX 6.6. It may be the only POSIX-compliant platform in any use today with an unsigned time_t, so this fix may not be worth the churn, but I thought I'd offer it anyhow.

@kdudka
Copy link
Member

kdudka commented Feb 25, 2022

No objections to use difftime() where appropriate but why are you replacing time_t with int? On x86_64 these types have different width (64bit vs. 32bit) and this seems like a step backwards.

@willmmiles
Copy link
Author

For daysElapsed()? Because POSIX time_t may be unsigned (as it is on my platform) so it cannot be used when the return value could be negative. For a result in days, it didn't seem to me like it was worth explicitly overriding the system default width -- a 32-bit day count spans a range of about 5.8 million years. If that's not enough I can resubmit with int64_t.

@kdudka
Copy link
Member

kdudka commented Feb 25, 2022

We do not seem to use int64_t in logrotate yet, so I am not sure whether it is available on all the supported platforms. But we should at least use long rather than int to avoid narrowing of the type in the most common case.

On platforms with unsigned time_t simple subtraction may produce a large
positive result instead of the expected negative value.  Use difftime()
to ensure meaningful comparison.
@willmmiles
Copy link
Author

Updated. I've broken the changes down in to two commits (one for difftime, one for daysElapsed) for clarity.

@willmmiles willmmiles changed the title Use difftime to compare time_t Support unsigned time_t Mar 2, 2022
@kdudka
Copy link
Member

kdudka commented Mar 3, 2022

Merging, thanks for the contribution!

@kdudka kdudka closed this in 4eb3add Mar 3, 2022
kdudka pushed a commit that referenced this pull request Mar 3, 2022
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.

3 participants