BorrowedFd::to_owned() gives you another BorrowedFd

Firstly, I should say that I really like the basic ideas in RFC3128 (#87074) . I'm reporting a wrinkle which may or may not be soluble.

This program:

#![feature(io_safety)]
use std::os::unix::prelude::*;

fn main() -> std::io::Result<()> {
    let file = std::fs::File::open("/dev/null")?;
    let borrowed = file.as_fd();
    let owned = borrowed.to_owned();
    eprintln!("file={:?}\nborrd={:?}\nowned={:?}", &file, &borrowed, &owned);
    Ok(())
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d51ddfa5a390e4d1b8f8bf81af8b0d73

Prints:

file=File { fd: 3, path: "/dev/null", read: true, write: false }
borrd=BorrowedFd { fd: 3 }
owned=BorrowedFd { fd: 3 }

A naive reader might have thought it would print:

file=File { fd: 3, path: "/dev/null", read: true, write: false }
borrd=BorrowedFd { fd: 3 }
owned=OwnedFd { fd: 4 }

But of course converting a borrowed to an owned fd is fallible. And there is no TryToOwned. This no-op to_owned() exists because BorrowedFd is Copy and therefore Clone and eveyrhing Clone has a no-op ToOwned.

The ideal fix from a type system and traits point of view (other than going back in time and abolishing the blanket ToOwned for Clone) would be for BorrowedFd to somehow be a reference type. But it would have to actually be represented as an integer. pub struct BorrowableFdValue { x:() } and transmuting c_int via usize to &BorrowableFdValue and back would solve this but that is really quite stomach-churning (and the result can't be used in ffi the way the existing BorrowedFd can).

Maybe the answer is to simply document this quirk. We should probably provide impl TryFrom<OwnedFd> from BorrowedFd at the very least, and of course OwnedFd::try_clone().

Thanks for the report!

Another option would be to remove the Copy and Clone impl for BorrowedFd. Somewhat surprisingly, they turn out to be infrequently used from the use cases I've seen so far, because the common pattern is to do as_fd() or FFI to obtain a BorrowedFd, use the BorrowedFd without copying it because it has a limited scope and there usually isn't a need to have multiple copies of the same handle within that scope, and then drop it. And one could still call as_fd() on a BorrowedFd to obtain a copy of it if desired. Thoughts?

Is there a need for impl TryFrom<OwnedFd> for BorrowedFd, given that we can just use the infallible as_fd() to obtain a BorrowedFd from an OwnedFd?

The reason I didn't add OwnedFd::try_clone was that the semantics of what's shared and what's independent after a dup depend on the dynamic type of the resource they refer to, so the use cases for a generic try_clone aren't clear to me. Could you say more about how you'd use this?

Another option would be to remove the Copy and Clone impl for BorrowedFd. ...Thoughts?

Hrm. Interesting. I haven't worked with this API much, so I don't feel I qualified to have an opinion. It does feel a bit odd, but I think it's going to be odd either way.

Is there a need for impl TryFrom<OwnedFd> for BorrowedFd, given that we can just use the infallible as_fd() to obtain a BorrowedFd from an OwnedFd?

Err. I miswrote. I meant impl TryFrom<BorrowedFd> for OwnedFd. Ie, the try_clone(), only starting from a BorrowedFd.

The reason I didn't add OwnedFd::try_clone was that the semantics of what's shared and what's independent after a dup depend on the dynamic type of the resource they refer to, so the use cases for a generic try_clone aren't clear to me. Could you say more about how you'd use this?

I think anyone who is working with fds knows (ought to know) what the implications are. Obviously we should document that try_clone is just dup, but there isn't anything else it could be.

As for the "how you'd use this", I was just doing some work in std::process::Command: #88561. The internals there could make good use of BorrowedFd. I felt dirty open-coding a call to libc::dup :-).

Also, I think there are a number of places where APIs consumes an fd that could very helpfully take F: TryInto<OwnedFd> or F: AsFd. For example, at some point perhaps impl<F> Into<process::Stdio> for F where F: AsFd, whose implementation needs TryInto.

Add a `try_clone()` function to `OwnedFd`.

#88794 adds try_clone() to OwnedFd.

On removing BorrowFd's Copy+Clone: This went smoothly in a few codebases I tried, which suggests the change is feasible. Before I submit a PR though, I want to figure out BorrowedFd::try_clone first.

Should we add BorrowedFd::try_clone? It is safe. However, it's somewhat surprising; a typical "borrow" means access to the resource is statically scoped, but a BorrowedFd::try_clone would mean that access to the resource could outlive the static scope. Most uses of BorrowedFd don't need to dup it. I have two ideas so far:

Don't add BorrowedFd::try_clone, and tell users to use &OwnedFd instead of &BorrowedFd<'_> in situations where they know they want the borrower to be able to try_clone().
Do add it, but give it a more verbose name, like BorrowedFd::try_clone_into_owned_fd() or so. Document that users should use .as_fd() instead if they just want to copy the existing fd, and that users should be careful when using resources outside the static scope of the borrow.

Rollup merge of rust-lang#88794 - sunfishcode:sunfishcode/try-clone, …

#88794 adding try_clone() to OwnedFd has now landed.

Thinking about Copy and Clone on BorrowedFd, even though most uses in practice don't need Copy and Clone, it does come up occasionally. For example, in rustix, while most public functions take Fd: AsFd, internally it uses BorrowedFd to reduce the amount of generic code, and whenever it needs to pass a BorrowedFd to more than one system call, it uses Copy to make a copy. It could call as_fd() instead, but that feels unnecessary; BorrowedFd really is safely copyable.

So my inclination for this issue is to say that BorrowedFd::to_owned returning a BorrowedFd is fine. It is surprising, but it doesn't do anything dangerous. And removing it would mean making BorrowedFd non-Copy, which would also be surprising, since BorrowedFd is trivially and safely copyable. #93354 is a PR to address the surprising part by documenting it.

However, this is very much a judgement call, and I'm open to input here.

Rollup merge of rust-lang#93354 - sunfishcode:sunfishcode/document-bo…

As mentioned in #93354, we looked at several options for what to do about to_owned here, but just documenting it seems to be the least bad option. That's now implemented.

#88794 added try_clone() to OwnedFd.

There isn't a BorrowedFd::try_clone or TryFrom<BorrowedFd> for OwnedFd or equivalent yet. There's nothing fundamentally wrong with it, however I think we'll need to look at use cases to shape the API. For now, one can construct a temporary ManuallyDrop<OwnedFd> and try_clone that, and if you find yourself doing that, please also file an issue about it.

Auto merge of rust-lang#97178 - sunfishcode:ownedfd-and-dup, r=joshtr…

Auto merge of #97178 - sunfishcode:ownedfd-and-dup, r=joshtriplett

Navigation Menu

Repository navigation

BorrowedFd::to_owned() gives you another BorrowedFd #88564

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

BorrowedFd::to_owned() gives you another BorrowedFd #88564

Description

Activity

sunfishcode commented on Sep 1, 2021

ijackson commented on Sep 1, 2021

sunfishcode commented on Sep 9, 2021

sunfishcode commented on Jan 28, 2022

sunfishcode commented on Mar 3, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions