Page MenuHomeFreeBSD

ctime.3: Add a note about a possible return value to the description of localtime(3)
ClosedPublic

Authored by gbe on Sep 10 2022, 11:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jan 3, 11:29 PM
Unknown Object (File)
Tue, Jan 3, 11:29 PM
Unknown Object (File)
Tue, Jan 3, 11:28 PM
Unknown Object (File)
Tue, Jan 3, 11:28 PM
Unknown Object (File)
Tue, Jan 3, 11:27 PM
Unknown Object (File)
Dec 15 2022, 7:50 PM
Unknown Object (File)
Dec 14 2022, 10:09 PM
Unknown Object (File)
Dec 4 2022, 6:54 PM
Subscribers

Details

Summary

The localtime(3) function returns a NULL pointer, if the passed in time
translates to a year that will not fit in an integer type. It is stricly
recommended to check the return value to avoid garage output.

Reported by: mckusick
MFC after: 1 week

Test Plan

mandoc output review and 'mandoc -Tlint' checks

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

gbe requested review of this revision.Sep 10 2022, 11:37 AM
gbe created this revision.

I think that it would be better to just add a sentence in the description of localtime that notes that it may return NULL since that is where it is most likely to be seen by readers of the manual page.

diff --git a/contrib/tzcode/stdtime/ctime.3 b/contrib/tzcode/stdtime/ctime.3
index 771027fa62ec..c577073a9a2a 100644
--- a/contrib/tzcode/stdtime/ctime.3
+++ b/contrib/tzcode/stdtime/ctime.3
@@ -94,6 +94,10 @@ and returns a pointer to a
 (described below) which contains
 the broken-out time information for the value after adjusting for the current
 time zone (and any other factors such as Daylight Saving Time).
+When the passed in time translates to a year that will not fit
+in an integer type,
+.Fn localtime
+returns NULL.
 Time zone adjustments are performed as specified by the
 .Ev TZ
 environment variable (see

Otherwise add it as a standard section as noted in my inline comment.

I also recommend that you do the formatting changes as a separate commit so that people looking at this commit do not have to sift through it to find the very important addition.

contrib/tzcode/stdtime/ctime.3
370–375

If you add a new section, I propose that you use a more standard section header:

.Sh RETURN VALUES
When successful, the
.Fn localtime
function return a pointer to a
.Dq Fa struct tm .
When the passed in time translates to a year that will not fit in an integer type,
.Fn localtime
returns NULL.

contrib/tzcode/stdtime/ctime.3
370–375

Is it a 'integral type' or is it specifically time_t?

I'd be tempted to add some vague statement about 'the standard allows any error to cause a NULL return. FreeBSD will return an error when...'

384

Should these reference the specific POSIX standard?
I know that's likely for another commit since this is just cleaning up .Tn, but seeing the change makes me wonder as we've often moved that way in other man pages.

gbe marked an inline comment as done.
  • Don't add a CAVEATS section, add a note the description of localtime(3)
gbe retitled this revision from ctime.3: Add a CAVEATS section for localtime(3) to ctime.3: Add a note about a possible return value to the description of localtime(3).Sep 14 2022, 7:55 AM
gbe edited the summary of this revision. (Show Details)
imp requested changes to this revision.Sep 14 2022, 1:27 PM
imp added inline comments.
contrib/tzcode/stdtime/ctime.3
98

Please specify what kind of integer. Is it an int, time_t or ????
This is too vague because nearly all times can fit into an integral type if it's a long long, but only a limited subset if it is a int32_t.

This revision now requires changes to proceed.Sep 14 2022, 1:27 PM

Proposed new wording.

contrib/tzcode/stdtime/ctime.3
98

The field for year in the tm structure is defined as int'. So it depends on how the architecture defines int'. Perhaps the wording should be:

When the passed in time translates to a year that will not fit in in an int' for architectures that define int' to be int32_t,

contrib/tzcode/stdtime/ctime.3
98

When the passed in time translates to a year that overflows the hosts'
.Dv int
type.

is likely fine. And it makes perfect sense now where we go awry and what the impact will be (in practical terms, not much).

  • Refine wording about the possible return value of locatime(3)
gbe marked 4 inline comments as done.Sep 15 2022, 5:45 AM

Update wording to address comments.

contrib/tzcode/stdtime/ctime.3
101

"for architectures where
.Dv int
is 32-bits,"

instead, because int32_t is defined in terms of things like int, long, etc so this may confuse people.

  • Use a more generic term for int32_t
gbe marked an inline comment as done.Sep 15 2022, 3:49 PM

Is the problem when the year won't fit into an int, or specifically when it won't fit into a 32-bit int?

I would think that the problem is when it won't fit into an int regardless of size, but that we would only run into that on architectures that use 32-bit ints.

contrib/tzcode/stdtime/ctime.3
97

"passed-in"

Is the problem when the year won't fit into an int, or specifically when it won't fit into a 32-bit int?

I would think that the problem is when it won't fit into an int regardless of size, but that we would only run into that on architectures that use 32-bit ints.

I just realized all our architectures an int is 32-bits, so I've suggested a simpler wording above. But that's only possible on !i386 archtectures with the 8 byte time_t since i386 can't specify a time that's 2 billion years in the future/past.

contrib/tzcode/stdtime/ctime.3
99–100

I hate to keep picking nits, please forgive me for not realizing this comment sooner.

I think we can just remove the 'for architectures where int is 32-bits' entirely and have it read OK.

When the specified time translates to a year that will not fit in an
.Dv int ,
.Fn localtime
returns NULL.
gbe marked an inline comment as done.Sep 16 2022, 11:34 AM

I'm happy now. Thanks for putting up with what must have been frustrating comments as I circled in on this wording.

This revision is now accepted and ready to land.Sep 16 2022, 5:31 PM
In D36515#831121, @imp wrote:

I'm happy now. Thanks for putting up with what must have been frustrating comments as I circled in on this wording.

No problem, that are what differentials are for.

It took us a long time to figure out one sentence, but I am happy with the result. Thanks Gordon for your patience.

I fixed ls(1) and find(1). There are only about a hundred more applications and libraries to go (see https://reviews.freebsd.org/D36474).