Page MenuHomeFreeBSD

Reimplement dirname(3) to be thread-safe.
ClosedPublic

Authored by ed on Jul 28 2016, 8:36 PM.

Details

Summary

Now that we've updated the prototypes of the basename(3) and dirname(3)
functions to conform to POSIX, let's go ahead and reimplement dirname(3)
in such a way that it's thread-safe, but also guaranteed to succeed.
C libraries like the one that's part of Solaris and musl already follow
such an approach.

Move the existing implementation to another source file,
freebsd11_dirname.c to keep existing users of the API that pass in a
constant string happy, using symbol versioning.

Put a new version of the function in dirname.c, obtained from CloudABI's
C library. This version scans through the pathname string from left to
right, normalizing it, while discarding the last pathname component.

Note: this change will not be committed immediately. I'm only planning
on doing this in a couple of weeks from now, to see if the previous
change (to switch over to the POSIX prototypes) causes any unexpected
regressions.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ed retitled this revision from to Reimplement dirname(3) to be thread-safe..
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added reviewers: emaste, jilles.
ed set the repository for this revision to rS FreeBSD src repository - subversion.

In general I like this idea. One question, what does glibc do?

For the next upload would you please include context (e.g. git diff -U9999)?

lib/libc/Versions.def
30–32 ↗(On Diff #18859)

I've never understood why our symbol version names are decoupled from FreeBSD versions.

lib/libc/gen/Makefile.inc
48 ↗(On Diff #18859)

It seems we have a common convention of foo_compat.c, should we not use that here?

lib/libc/gen/dirname.3
53–55 ↗(On Diff #18859)

What do you think about committing a man page update now, intended for 11.0, that mentions the current (internal storage) will change in the future and should not be relied on?

ed marked 2 inline comments as done.Jul 29 2016, 4:30 PM

In general I like this idea. One question, what does glibc do?

For dirname(), glibc works similar to what we're trying to accomplish here: have a function with the POSIX prototype that is also thread-safe. For basename() things are a bit more complicated, where there are two different copies, depending on _GNU_SOURCE, _POSIX_SOURCE, etc.

For the next upload would you please include context (e.g. git diff -U9999)?

Sure! The latest SVN release broke Arcanist for me, which is why I'm falling back to uploading diffs by hand now.

lib/libc/Versions.def
30–32 ↗(On Diff #18859)

Yes. Me neither.

ed edited edge metadata.

Use dirname_compat.c as a source file name. Also sync in the latest man page.

After the discussion I started on arch@ regarding symbol versioning naming, I think we'd better just stick to the current approach and go ahead with this specific change as proposed. Ed, Jilles, any more remarks on this change? Let's land this change on August 12th.

ed edited edge metadata.
ed removed rS FreeBSD src repository - subversion as the repository for this revision.

Sync with HEAD.

In D7355#154344, @ed wrote:

After the discussion I started on arch@ regarding symbol versioning naming, I think we'd better just stick to the current approach and go ahead with this specific change as proposed. Ed, Jilles, any more remarks on this change? Let's land this change on August 12th.

It looks good to me, but how much confidence do you have that this does not break important ports at runtime?

It looks good to me, but how much confidence do you have that this does not break important ports at runtime?

Thanks for the review! I'm fairly certain. glibc also has an implementation similar to the one we're adding here, so stuff that runs properly on Linux should also work for us.

This revision was automatically updated to reflect the committed changes.