Overhaul Utility::FormatDateTime() #10165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Al2Klimov
commented
Sep 19, 2024
•
Al2Klimov
commented
- Backport of Overhaul Utility::FormatDateTime() #10112
Al2Klimov
force-pushed
the
formatdatetime-2.13
branch
from
4f26a38
to
771c2e7
Compare
|
I don't think we should backport this to 2.13.x as it requires some codes that were not backported for their own reasons. I mean, do we really need this for a 2.13.10 bugfix release at all? |
|
@Al2Klimov Why did you need to pull in the other PRs, i.e. what needs C++14? |
– https://github.com/Icinga/icinga2/actions/runs/10940187059/job/30372035371#step:4:2275 |
|
That function is only used in the tests three times for the error messages and can easily be replaced by adding the quotes manually to the surrounding strings. That sounds much more reasonable to me than bumping the C++ version for the old support branch just for this. |
The previous implementation actually had undefined behavior when called with a double that can't be represented as time_t. With boost::numeric_cast, there's a convenient cast available that avoids this and throws an exceptions on overflow. It's undefined behavior ([0], where the implicit conversion rule comes into play because the C-style cast uses static_cast [1] which in turn uses the imlicit conversion as per rule 5 of [2]): > A prvalue of floating-point type can be converted to a prvalue of any integer > type. The fractional part is truncated, that is, the fractional part is > discarded. > > * If the truncated value cannot fit into the destination type, the behavior > is undefined (even when the destination type is unsigned, modulo arithmetic > does not apply). Note that on Linux amd64, the undefined behavior typically manifests itself in the result being the minimal value of time_t which then results in localtime_r failing with EOVERFLOW. [0]: https://en.cppreference.com/w/cpp/language/implicit_conversion#Floating.E2.80.93integral_conversions [1]: https://en.cppreference.com/w/cpp/language/explicit_cast [2]: https://en.cppreference.com/w/cpp/language/static_cast
localtime() is not thread-safe as it returns a pointer to a shared tm struct. Everywhere except on Windows, localtime_r() is used already which avoids the problem by using a struct allocated by the caller for the output. Windows actually has a similar function called localtime_s() which has the same properties, just with a different name and order of arguments.
So far, the return value of strftime() was simply ignored and the output buffer passed to the icinga::String constructor. However, there are error conditions where strftime() returns 0 to signal an error, like if the buffer was too small for the output. In that case, there's no guarantee on the buffer contents and reading it can result in undefined behavior. Unfortunately, returning 0 can also indicate success and strftime() doesn't set errno, so there's no reliable way to distinguish both situations. Thus, the implementation now returns the empty string in both cases. I attempted to use std::put_time() at first as that allows for better error handling, however, there were problems with the implementation on Windows (see inline comment), so I put that plan on hold at left strftime() there for the time being.
On Windows, the strftime() function family invokes an invalid parameter handler when the format string is invalid (see the "Remarks" section in their documentation). std::put_time() shows the same behavior as it uses _wcsftime_l() internally. The default invalid parameter handler may terminate the process, which can be a problem given that the format string can be specified by the user from the Icinga DSL. Thus, temporarily set a thread-local no-op handler to disable the default one allowing the program to continue. This then simply results in the function returning an error which then results in an exception as we ask the stream to throw one. See also: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/strftime-wcsftime-strftime-l-wcsftime-l?view=msvc-170 https://learn.microsoft.com/en-us/cpp/c-runtime-library/parameter-validation?view=msvc-170 https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/set-invalid-parameter-handler-set-thread-local-invalid-parameter-handler?view=msvc-170
This allows the function to be used both with a double timestamp or a pointer to a tm struct. With this, a similar implementation inside the tests can simply use our regular function.
Al2Klimov
force-pushed
the
formatdatetime-2.13
branch
from
771c2e7
to
396ce87
Compare
julianbrost
approved these changes
Sep 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.