Project Tags

None

Referenced Files

	F177422941: D60799.1794346306.diff
	Mon, Nov 9, 9:31 PM

Subscribers

imp

ziaee

Details

Reviewers

dteske
fuz
des
ziaee

Group Reviewers

manpages

Commits

rG3ac9299ee90c: libc: Export timespec_getres(3) and add tests

Summary

timespec_getres() has been declared in <time.h> and built into libc,
but never listed in a symbol map. It is therefore local to libc.so,
and dynamically linked callers fail to link.

Fixes: 9b5d724cad10 ("libc: Add timespec_getres(3) as per C23.")
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 78394
Build 75277: arc lint + arc unit

Event Timeline

kfv created this revision.Sat, Oct 17, 9:17 AM

Herald added a subscriber: imp. · View Herald TranscriptSat, Oct 17, 9:17 AM

kfv requested review of this revision.Sat, Oct 17, 9:17 AM

Harbormaster completed remote builds in B78290: Diff 190098.Sat, Oct 17, 9:17 AM

Good catch!

Might be a good opportunity to add a companion unit test.

lib/libc/gen/Symbol.map
443	This needs to go in `FBSD_1.9` in my opinion, as the symbol was only added to the interface at that point. That we had it as an unexported interface before is an accident, but does not affect the versioning.

Please add an appropriate Fixes: line

kfv edited the summary of this revision. (Show Details)Tue, Oct 20, 7:02 AM

In D60799#1395747, @des wrote:

Please add an appropriate Fixes: line

Done.

kfv added inline comments.Tue, Oct 20, 7:09 AM

lib/libc/gen/Symbol.map
443	I was thinking of doing the same thing, but I wasn’t sure to be honest. Do you think we should update the man page as well? It currently says that the interface was first introduced in FreeBSD 14. We could leave that as is, or add a brief note clarifying that it wasn’t exposed until FreeBSD 16, or update the version to 16 altogether, ... . I’d appreciate @des ’s view on this as well.

In D60799#1395151, @fuz wrote:

Might be a good opportunity to add a companion unit test.

Yep, I’ve drafted something in the meantime. I’ll give it a bit of polish and share it shortly. Do you recommend doing it here or opening a separate DR?

In D60799#1396222, @kfv wrote:

In D60799#1395151, @fuz wrote:

Might be a good opportunity to add a companion unit test.

Yep, I’ve drafted something in the meantime. I’ll give it a bit of polish and share it shortly. Do you recommend doing it here or opening a separate DR?

Sure, just add it here.

lib/libc/gen/Symbol.map
443	Yes, adding a note to the man page is a good idea. As you should of course MFC this change to stable/15 and stable/14, it will be closer to “Due to an oversight, timespec_getres() was only exposed in FreeBSD 14.x and 15.y” I think.

kfv retitled this revision from libc: Export timespec_getres(3) to libc: Export timespec_getres(3) and add tests.Tue, Oct 20, 9:45 AM

Add tests

Harbormaster completed remote builds in B78394: Diff 190366.Tue, Oct 20, 9:45 AM

Move the symbol to FBSD_1.9

Harbormaster completed remote builds in B78395: Diff 190368.Tue, Oct 20, 9:54 AM

Update manpage

Herald added a subscriber: ziaee. · View Herald TranscriptTue, Oct 20, 10:10 AM

Harbormaster completed remote builds in B78399: Diff 190372.Tue, Oct 20, 10:10 AM

kfv marked an inline comment as done.Tue, Oct 20, 10:10 AM

kfv added inline comments.Tue, Oct 20, 10:22 AM

lib/libc/gen/Symbol.map
443	I wrote as the following to avoid going into why it was not exposed when it appeared first. Let me know if we'd rather explain it explicitly and I’ll switch to your wording. This interface first appeared in FreeBSD 14.0, but it was not exported from the shared C library until FreeBSD 14.6, FreeBSD 15.2 and FreeBSD 16.0.

@kfv Note that you can add multiple commits at once. I recommend only pushing an update once things are ready to be reviewed again (or marking intermediate states as WIP) so that reviewers don't waste time reviewing a WIP patch set.

You can leave out the 16.0 from your note as it came after the other two. When in doubt, ask @ziaee for his opinion on man pages.

Good unit tests (except for my nit pick).

lib/libc/tests/gen/timespec_getres_test.c
27	I'm not sure if TIME_MONOTONIC + 1 is guaranteed to stay bad in the future, so maybe not a good test case.

In D60799#1397160, @fuz wrote:

@kfv Note that you can add multiple commits at once. I recommend only pushing an update once things are ready to be reviewed again (or marking intermediate states as WIP) so that reviewers don't waste time reviewing a WIP patch set.

Yes, you're right. I only realised this after my last update, and by then it was too late. Sorry for the noise.

You can leave out the 16.0 from your note as it came after the other two.

Makes sense, alright.

When in doubt, ask @ziaee for his opinion on man pages.

Sure.

Good unit tests (except for my nit pick).

I'll take care of it shortly.

lib/libc/tests/gen/timespec_getres_test.c
27	Agreed, I'll remove it. Thanks for the catch.

Update man page
Remove TIME_MONOTONIC + 1 from bad bases

Harbormaster completed remote builds in B78471: Diff 190521.Wed, Oct 21, 8:01 PM

kfv marked 2 inline comments as done.Wed, Oct 21, 8:01 PM

Added man pages to reviewers (this means @ziaee). Not sure if the phrasing you used is what we would usually write. Code looks fine from what I can say.
Could you add this DR to the C23 wiki page?

This revision is now accepted and ready to land.Wed, Oct 21, 10:13 PM

In D60799#1397658, @fuz wrote:

Added man pages to reviewers (this means @ziaee). Not sure if the phrasing you used is what we would usually write. Code looks fine from what I can say.

Thanks. I'll be waiting for their review then.

Could you add this DR to the C23 wiki page?

Sure thing.

@ziaee: I would very much appreciate your review.

Looks good to me, thanks for inviting me! Sorry I've been a bit behind lately!

No worries, mate. Cheers!

One question: code slush has been in effect since 23 October, and the releng/15.2 branch is due to be created tomorrow. Should I MFC after one day so that I can get it into stable/15 before the branch is created, or is it fine to proceed as proposed and MFC after a week?

des accepted this revision.Thu, Nov 5, 1:25 PM

In D60799#1403632, @kfv wrote:

One question: code slush has been in effect since 23 October, and the releng/15.2 branch is due to be created tomorrow. Should I MFC after one day so that I can get it into stable/15 before the branch is created, or is it fine to proceed as proposed and MFC after a week?

I would proceed as proposed, then after you MFC, send re@ a mail with the stable commit hash and a short layman's explanation of why you think this should go in the release and why you think it won't break anything.

Makes sense. Alright, I'll go ahead, then. Thanks!

Closed by commit rG3ac9299ee90c: libc: Export timespec_getres(3) and add tests. · Explain WhyThu, Nov 5, 8:12 PM

This revision was automatically updated to reflect the committed changes.

kfv added a commit: rG3ac9299ee90c: libc: Export timespec_getres(3) and add tests.

libc: Export timespec_getres(3) and add tests
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 190366

lib/libc/gen/Symbol.map

lib/libc/tests/gen/Makefile

lib/libc/tests/gen/timespec_getres_test.c

libc: Export timespec_getres(3) and add testsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 190366

lib/libc/gen/Symbol.map

lib/libc/tests/gen/Makefile

lib/libc/tests/gen/timespec_getres_test.c

libc: Export timespec_getres(3) and add tests
ClosedPublic
Actions

Revision Contents
Changeset List