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

Replace RawFd with IntoRawFd or AsRawFd as appropriate #678

Open
Susurrus opened this issue Jul 15, 2017 · 11 comments
Open

Replace RawFd with IntoRawFd or AsRawFd as appropriate #678

Susurrus opened this issue Jul 15, 2017 · 11 comments

Comments

@Susurrus
Copy link
Contributor

This is blocking on the resolution of rust-lang/rust#43254. Right now you can't have both RawFd and T: AsRawFd supported as inputs to the same function, so implementing this doesn't actually improve ergonomics. We may be able to create a newtype wrapper in nix, but I want to see if there might be a solution from the Rust devs here before we do our own newtype-wrapper based implementation.

@Susurrus
Copy link
Contributor Author

I think unistd::close is the only function that should use IntoRawFd, the others can just use AsRawFd, but this should be checked.

@roblabla
Copy link
Contributor

I am very much in favor of this. Wouldn't newtype-ing RawFd be a good idea anyway ? The fact that it's an int should be, IMO, hidden from the user's view, as a lot of operations on int don't yield interesting results on fds (what does RawFd + int do ?).

We could have our own newtype'd RawFd, and implement AsRawFd/IntoRawFd/FromRawFd ourselves. Is there a good reason not to do this (other than it being a breaking change) ?

@lucab
Copy link
Contributor

lucab commented Jul 22, 2017

a lot of operations on int don't yield interesting results on fds (what does RawFd + int do?)

Integer arithmetic on fd is quite common with IPC and socket passing. If you want a specific concrete usecase, see https://www.freedesktop.org/software/systemd/man/sd_listen_fds.html.

In general, I'm in favor of having a more sound RawFd replacement (see #594) but I still need to do integer arithmetic around it.

@roblabla
Copy link
Contributor

roblabla commented Jul 22, 2017

Well, you could still use AsRawFd to get the raw integer, and FromRawFd (which is unsafe) to create the new Fd.

Then we'd still have our safe Fd abstraction, while having the full power of RawFd at hand.

@Kixunil
Copy link
Contributor

Kixunil commented Jul 25, 2017

@lucab such operation is basically unsafe.

@kahing
Copy link

kahing commented Jul 26, 2017

What about operations that return RawFd? For example, dup2() returns RawFd, how would one pass that to another function if everything takes AsRawFd? (for example, statvfs::fstatvfs takes AsRawFd and so is unusable without writing my own wrapper type)

@Kixunil
Copy link
Contributor

Kixunil commented Jul 26, 2017

@kahing The interface should be changed to return Fd, just as I did it in case of Pid, Uid, and Gid.

@asomers
Copy link
Member

asomers commented Jul 26, 2017

I agree that we should use a Newtype. But we should name it FileDesc, because that's what libstd uses. The libstd type isn't public now. But if it ever becomes public in the future, it would be easy for us to delete our own implementation and for our users to change nix::FileDesc into FileDesc.

@Susurrus
Copy link
Contributor Author

@asomers Why don't we just copy directly the private type from std and use that? We can amend it as it evolves in std, but that way we can make sure we're nicely integrated.

@roblabla
Copy link
Contributor

roblabla commented Jul 26, 2017

Would it be worth making an issue on the rust-lang repo asking if there is any plan to make FileDesc public, and stabilize it ? It seems silly to copy the std private type, it'd make a lot more sense for std to expose it IMO.

@Susurrus
Copy link
Contributor Author

@roblabla Indeed, having this discussion with them would be the smart play before we commit any code to anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants