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

Added direct support for fchown and dup3 #957

Closed
wants to merge 9 commits into from
Closed

Added direct support for fchown and dup3 #957

wants to merge 9 commits into from

Conversation

koutheir
Copy link

@koutheir koutheir commented Oct 19, 2018

Added direct support for fchown and dup3.

Fixes #939.

@koutheir
Copy link
Author

koutheir commented Oct 19, 2018

In order to be able to use dup3() on FreeBSD, I also added the pull request #1101 to the libc crate.

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.

Please add "Fixes #939" to your commit message.

src/unistd.rs 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
src/unistd.rs Outdated Show resolved Hide resolved
@koutheir koutheir changed the title Added direct support for fchown, lchown, chmod and dup3 Added direct support for fchown and dup3 Oct 20, 2018
@asomers
Copy link
Member

asomers commented Oct 20, 2018

Don't forget the CHANGELOG. Make it clear that dup3 has been removed from OSX.

@koutheir
Copy link
Author

Don't forget the CHANGELOG. Make it clear that dup3 has been removed from OSX.

The previous implementation of dup3 was buggy because it was vulnerable to a race condition. I would argue that it should have never been implemented for OSX (or at least, not via dup2 and fcntl). Furthermore, one of the principles in this project is to avoid providing APIs that are not actually implemented on the platform.

@Susurrus
Copy link
Contributor

The previous implementation of dup3 was buggy because it was vulnerable to a race condition. I would argue that it should have never been implemented for OSX (or at least, not via dup2 and fcntl). Furthermore, one of the principles in this project is to avoid providing APIs that are not actually implemented on the platform.

The CHANGELOG should also be viewed as everything someone needs to read to allow them to upgrade. So if a user was using it on OS X, and we broke their code during an upgrade, there should be a part under the Removed section of the CHANGELOG stating a) what we removed and b) why we did so. Could you add a line there for this?

@koutheir
Copy link
Author

Could you add a line there for this?

For dup3, I wrote two change log entries:

  • Under Changed, to mention the direct system call, instead of emulation, and
  • Under Removed, to inform OSX users of the removal.

Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

CHANGELOG entries look good, thanks for adding those!

src/unistd.rs Outdated Show resolved Hide resolved
@Susurrus
Copy link
Contributor

There are Travis failures that need to be addressed. Besides that this looks good, though it should be squashed into 2 commits:

  • Add fchown
  • Modify dup3

After those are addressed, this is ready for merging.

@koutheir koutheir closed this Nov 1, 2018
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.

3 participants