EventFd type #1945

JonathanWoollett-Light · 2022-12-18T18:11:55Z

Added an EventFd type that makes usage a little more convenient and explicit.

CHANGELOG.md

asomers · 2023-01-14T18:25:14Z

src/sys/eventfd.rs

+        Errno::result(res).map(|r| Self(unsafe { OwnedFd::from_raw_fd(r) }))
+    }
+    /// [`EventFd::write`] with `1`.
+    pub fn arm(&self) -> Result<usize> {


What is the meaning of the return value, here and for defuse?

It passes through the return value of unistd::write.

Yeah, but what does that mean? What does it even mean to write to an eventfd ?

I think this is answered by #1945 (comment)

asomers · 2023-01-14T18:25:39Z

src/sys/eventfd.rs

+    /// Constructs [`EventFd`] with the given `init_val` and `flags`.
+    /// 
+    /// Wrapper around [`libc::eventfd`].
+    pub fn value_and_flags(init_val: u32, flags: EfdFlags) -> Result<Self> {


It would be more idiomatic to name these constructors from_XXX

Renamed with from_ prefix.

asomers · 2023-01-14T18:26:25Z

src/sys/eventfd.rs

+        unistd::write(self.0.as_raw_fd(),&0u64.to_ne_bytes())
+    }
+    /// Writes a given `value` to the file descriptor.
+    pub fn write(&self, value: u64) -> Result<usize> {


What is the meaning of writing a value to the file descriptor?

This is literally what it is doing, although I would agree its not the clearest documentation although I'm not sure what to replace it with.

https://man7.org/linux/man-pages/man2/write.2.html

write() writes up to count bytes from the buffer starting at buf to the file referred to by the file descriptor fd.

But what is even the purpose of writing to an eventFD? Is there any reason why anybody would ever want to do that?

When you poll on an EventFd you can consider that the event fires when a non-zero value can be read from the EventFd. By writing a value to the EventFd you are setting it up to trigger value events when polled.

If you write 2 here then poll 3 times on the EventFd the 1st 2 polls will return and the 3rd will wait.

There are many reasons to manually rearm events, one is testing.

Ok. Could you please rewrite the docs then, in terms of "arming" instead of "writing". We should strive to be as high-level as possible while remaining zero-cost. Also, is there any point in passing through the return value of unistd::write?

Could you please rewrite the docs then, in terms of "arming" instead of "writing". We should strive to be as high-level as possible while remaining zero-cost.

I've updated the docs.

Also, is there any point in passing through the return value of unistd::write?

I'm not aware of a specific use-case but I would tend to favour returning a value rather than dropping it. I don't see harm in returning it.

SteveLauC

Left some comments

src/sys/eventfd.rs

SteveLauC · 2023-12-09T09:50:29Z

src/sys/eventfd.rs

 pub fn eventfd(initval: libc::c_uint, flags: EfdFlags) -> Result<OwnedFd> {
    let res = unsafe { libc::eventfd(initval, flags.bits()) };

    Errno::result(res).map(|r| unsafe { OwnedFd::from_raw_fd(r) })
 }
+
+#[derive(Debug)]
+pub struct EventFd(pub OwnedFd);


I generally prefer to not expose the inner OwnedFd type, instead, we should impl AsFd for EventFd

But if the OwnedFd way is easier to use than impl AsFd in some cases, I am totally fine to leave it as-it.

And, maybe we can make it #[repr(transparent)]

By making the field public it allows a user to deconstruct it which AsFd doesn't allow, so for whatever reason they want they can extract the OwnedFd into another type.

I've added #[repr(transparent)].

What about impl From<EventFd> for OwnedFd, this is how the std handles them I think

What is the downside of inner item being public? I don't see any.

Still, functionally, no difference, but I would like to be consistent with the std:

impl From<ChildStderr> for OwnedFd impl From<ChildStdin> for OwnedFd impl From<ChildStdout> for OwnedFd impl From<File> for OwnedFd impl From<PidFd> for OwnedFd impl From<TcpListener> for OwnedFd

Okay, I've made the change.

Could you please make that OwnedFd field private given that we have impl From<EventFd for OwnedFd now

Updated to remove pub.

src/sys/eventfd.rs

SteveLauC · 2023-12-09T10:00:30Z

src/sys/eventfd.rs

+        self.0.as_fd()
+    }
+}
+impl AsRawFd for EventFd {


Do you need to impl AsRawFd for it?

I think so, is there any alternative approach to get the raw file descriptor you would suggest?

I was thinking that we have AsFd, then we can get a BorrowedFd, then we can extract the raw fd by calling .as_raw_fd() on the BorrowedFd, functionally, they have no difference, but this approach may discourage our user from using the raw fd

I think its generally more ergonomic for users to have AsRawFd rather than not have it. I could see someone complaining about not having it, but I can't see someone complaining about having it. I don't see any harm in having it.

Yeah, it has no harm, let's keep it

SteveLauC

Thanks!

JonathanWoollett-Light force-pushed the eventfd branch from f217227 to 8ce915d Compare December 18, 2022 18:18

rtzoeller reviewed Dec 18, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

JonathanWoollett-Light force-pushed the eventfd branch from 8ce915d to 943d9ab Compare December 18, 2022 21:40

JonathanWoollett-Light force-pushed the eventfd branch from 943d9ab to 2cd69e9 Compare December 31, 2022 22:37

JonathanWoollett-Light force-pushed the eventfd branch 2 times, most recently from c396c04 to 7a855bf Compare January 13, 2023 21:54

asomers reviewed Jan 14, 2023

View reviewed changes

JonathanWoollett-Light force-pushed the eventfd branch 4 times, most recently from e7794ec to 35dee10 Compare January 21, 2023 16:59

JonathanWoollett-Light force-pushed the eventfd branch from 35dee10 to b5dde38 Compare February 14, 2023 21:24

JonathanWoollett-Light force-pushed the eventfd branch 2 times, most recently from 5c5cb22 to 24e484c Compare November 16, 2023 17:34

JonathanWoollett-Light force-pushed the eventfd branch 2 times, most recently from 02545da to 6f9495d Compare December 5, 2023 21:34

SteveLauC requested changes Dec 9, 2023

View reviewed changes

JonathanWoollett-Light force-pushed the eventfd branch 3 times, most recently from 951c4ff to d13dbbb Compare December 10, 2023 02:21

feat: EventFd type

b2255c7

JonathanWoollett-Light force-pushed the eventfd branch from d13dbbb to b2255c7 Compare December 10, 2023 02:33

SteveLauC approved these changes Dec 10, 2023

View reviewed changes

SteveLauC added this pull request to the merge queue Dec 10, 2023

Merged via the queue into nix-rust:master with commit 9113d60 Dec 10, 2023
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventFd type #1945

EventFd type #1945

JonathanWoollett-Light commented Dec 18, 2022 •

edited

Loading

asomers Jan 14, 2023

JonathanWoollett-Light Jan 14, 2023

asomers Jan 15, 2023

JonathanWoollett-Light Jan 15, 2023

asomers Jan 14, 2023

JonathanWoollett-Light Jan 14, 2023

asomers Jan 14, 2023

JonathanWoollett-Light Jan 14, 2023

asomers Jan 15, 2023

JonathanWoollett-Light Jan 15, 2023 •

edited

Loading

asomers Jan 15, 2023

JonathanWoollett-Light Jan 21, 2023 •

edited

Loading

SteveLauC left a comment

SteveLauC Dec 9, 2023

JonathanWoollett-Light Dec 9, 2023

SteveLauC Dec 10, 2023

JonathanWoollett-Light Dec 10, 2023 •

edited

Loading

SteveLauC Dec 10, 2023

JonathanWoollett-Light Dec 10, 2023

SteveLauC Dec 10, 2023

JonathanWoollett-Light Dec 10, 2023

SteveLauC Dec 9, 2023

JonathanWoollett-Light Dec 9, 2023

SteveLauC Dec 10, 2023

JonathanWoollett-Light Dec 10, 2023 •

edited

Loading

SteveLauC Dec 10, 2023

SteveLauC left a comment

EventFd type #1945

EventFd type #1945

Conversation

JonathanWoollett-Light commented Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanWoollett-Light Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanWoollett-Light Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

SteveLauC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanWoollett-Light Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanWoollett-Light Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveLauC left a comment

Choose a reason for hiding this comment

JonathanWoollett-Light commented Dec 18, 2022 •

edited

Loading

JonathanWoollett-Light Jan 15, 2023 •

edited

Loading

JonathanWoollett-Light Jan 21, 2023 •

edited

Loading

JonathanWoollett-Light Dec 10, 2023 •

edited

Loading

JonathanWoollett-Light Dec 10, 2023 •

edited

Loading