Implement AsFd and conversion to OwnedFd

[joshtriplett]

In particular, this makes it possible to get a BorrowedFd from a
TimerFd, which allows integration with polling mechanisms that don't
use RawFd. This will be needed to work with the next version of
async-io.

[main--]

What surprises me about this impl is that it provides seemingly-innocuous from/.into() for converting any file descriptor into a TimerFd. The implementation of TimerFd quite strictly assumes that the underlying fd is in fact a timerfd. While there would (AFAICT) not be any memory safety concerns, it still seems like a potential footgun to me to provide this (it will trigger otherwise unreachable panics).

Unless there is some sort of established consensus/pattern in the rustix ecosystem/community that this kind of conversion is expected to be available (in spite of the risks), I think I would prefer a TryFrom conversion which ensures that the fd is actually compatible (e.g. by simply calling timerfd_gettime). (And then maybe/optionally a try_from_unchecked() function that offers the same conversion without the overhead of that check, where the risks are clearly documented.)

[sunfishcode]

There is something of a convention here. In the standard library, File and TcpStream have conversions from OwnedFd, even though most of their functions will fail if the fd isn't actually a file or a socket, respectively. OwnedFd isn't what most users should be using most of the time; that's what File, TcpStream, and also things like TimerFd are for. OwnedFd is for when you want to reach down to the lower level of abstraction and route something through that the higher-level types don't handle. And as you say, there's no memory safety risk here, or I/O safety.

[main--]

most of their functions will fail

But that is exactly my point: the functions in this crate don't fail gracefully when operating on the wrong kind of file descriptor - they panic. The difference here is that (in my eyes) the designers of std made the conscious choice to treat all possible operations on a File as fallible (motivated by cross-platform compatibility concerns I suppose), which then opens up the possibility of just plugging any random fd in there and trying to do random things with it.

A design goal I had with timerfd on the other hand is that I wanted the functions (set_state, get_state, read) to reflect in their type signature that these operations are infallible (perhaps this goal turns out to be futile?). The "error boundary" is TimerFd::new() (no way to statically prevent e.g. EMFILE). Once that succeeds, you are good to go. The TimerFd struct that you're holding represents a promise that this timerfd works and supports all basic operations. That's why I brought up the idea of having a TryFrom conversion which implements the same kind of error boundary. Once try_from succeeds, we know that the timerfd is valid and supports all basic operations.

Obviously, an alternative would be to redesign an std-style interface where all methods on TimerFd are fallible.

I have been pretty hand-off with the development of this crate, so I'd be willing to follow your advice/request here in spite of my concerns, but I do want to make sure that you clearly understand why I'm having these concerns.

[joshtriplett]

@main-- For my part, I really don't feel strongly that this conversion has to exist as an infallible operation, and it isn't an essential part of this PR; the most important part of this PR was the AsFd implementation.

I'll remove the conversion from OwnedFd to TimerFd from this PR, and then you can decide separately about whether you'd prefer a From or TryFrom.

[sunfishcode]

@main-- Ah, that's a great point. I agree then that TimerFd shouldn't implement From<OwnedFd>.

[joshtriplett]

@main-- FWIW, there are already ways today to trigger that panic via entirely safe code: if you set the TimerCancelOnSet flag (which TimerFd::set_state supports), then the read operation can return ECANCELED, which AFAICT will lead to a panic in TimerFd::read. (And that's leaving aside the possibility that the kernel has other error paths, or may in the future.) So, you may want to consider having ways of handling and returning errors without panic.

That said, given that it's trivially possible to verify via timer_gettime that you have a timerfd, the idea of having a TryFrom seems reasonable, as a way to catch an invalid conversion at the point of the conversion rather than at the later point of usage.

[main--]

there are already ways today to trigger that panic via entirely safe code: if you set the TimerCancelOnSet flag (which TimerFd::set_state supports), then the read operation can return ECANCELED, which AFAICT will lead to a panic in TimerFd::read.

Yes, and I consider this a bug (#6).

And that's leaving aside the possibility that the kernel has other error paths, or may in the future.) So, you may want to consider having ways of handling and returning errors without panic.

I’m aware, though given the relatively small kernel API surface of timerfd, the hope is that introducing such a case in a backwards-compatible way (i.e. without passing some new flag ok creation) should be near-impossible - and so far this bet seems to be working out 🤷‍♂️

[main--]

Thanks!

[joshtriplett]

@main-- Thank you! Would you mind publishing a version of the timerfd crate with this change included?

[main--]

Will do tomorrow!

[main--]

@joshtriplett https://crates.io/crates/timerfd/1.5.0