Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a fchownat(2) wrapper #955

Merged
merged 2 commits into from
Oct 20, 2018
Merged

Add a fchownat(2) wrapper #955

merged 2 commits into from
Oct 20, 2018

Conversation

jmmv
Copy link
Contributor

@jmmv jmmv commented Oct 17, 2018

No description provided.

@jmmv
Copy link
Contributor Author

jmmv commented Oct 17, 2018

Per the TODO, do you think chown should be removed?

The way I'd validate that is: add a trivial test for fchownat to ensure we reference this function somewhere and then ask CI to validate all platforms. If that succeeds, I can later try to see if this is sufficient for older macOS versions, which still exist in Travis, to prevent the issue we saw with utimes.

I can do this before the merge; just let me know if there are other considerations.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A successful Travis run is insufficient for removing a binding. We need to make sure that the newer alternative is supported on all Nix platforms, including those that aren't tested in CI, like NetBSD and OpenBSD. Though in this case, I don't think there's much to be gained by removing chown. Since it's required by POSIX, OSes won't ever drop it from their C libraries. And it doesn't cost anything to keep around. Do you agree?

src/fcntl.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
@jmmv jmmv force-pushed the fchownat branch 2 times, most recently from 9740612 to aec00bd Compare October 17, 2018 20:03
@jmmv
Copy link
Contributor Author

jmmv commented Oct 17, 2018

I don't mind keeping chown, but then would the same rationale apply for lchown and we should add it? Why doesn't the same apply for other traditional functions like utimes?

In fact, I think that if the OS exposes such functions, they should be exposed in nix as well. nix shouldn't be in the business of calling "traditional but ugly" functions obsolete... because all the new-ish counterparts are not always available :(

@asomers
Copy link
Member

asomers commented Oct 17, 2018

I don't mind keeping chown, but then would the same rationale apply for lchown and we should add it? Why doesn't the same apply for other traditional functions like utimes?

In fact, I think that if the OS exposes such functions, they should be exposed in nix as well. nix shouldn't be in the business of calling "traditional but ugly" functions obsolete... because all the new-ish counterparts are not always available :(

The same rationale would apply to lchown only if we already provide it. I don't want to withdraw chown because consumers may already be using it. lchown, OTOH, is unnecessary unless somebody ports Nix to a weird platform that doesn't have fchownat.

@jmmv
Copy link
Contributor Author

jmmv commented Oct 17, 2018

I've added tests to ensure these functions exists in all CI platforms, and also confirmed in my project that fchownat exists for the older macOS that I need. So no need to add lchown from my point of view, which is what I wanted to confirm.

src/fcntl.rs Outdated Show resolved Hide resolved
test/test_unistd.rs Outdated Show resolved Hide resolved
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM now. But I think you should remove the mention of deprecating chown from your commit message. I don't think we should ever remove chown.

jmmv and others added 2 commits October 19, 2018 22:09
These are mostly to ensure that all the platforms we care about in our
CI can reference these primitives.
@jmmv
Copy link
Contributor Author

jmmv commented Oct 20, 2018

Updated the commit message and removed the original contents of the PR message, which I think would be added to the history as well.

Also rebased on top of the truncate merge to avoid CHANGELOG conflicts.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot added a commit that referenced this pull request Oct 20, 2018
955: Add a fchownat(2) wrapper r=asomers a=jmmv



Co-authored-by: Julio Merino <[email protected]>
Co-authored-by: Julio Merino <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 20, 2018

@bors bors bot merged commit 9bf4ab3 into nix-rust:master Oct 20, 2018
@jmmv jmmv deleted the fchownat branch October 20, 2018 10:37
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.

2 participants