pass-dep/libc/libc-epoll.rs test does not pass when run against real system

[RalfJung]

Something seems wrong with that test. To make it pass against a real system, I need to apply the following diff:

diff --git a/tests/pass-dep/libc/libc-epoll.rs b/tests/pass-dep/libc/libc-epoll.rs
index 11a0257dc..57bec1c52 100644
--- a/tests/pass-dep/libc/libc-epoll.rs
+++ b/tests/pass-dep/libc/libc-epoll.rs
@@ -19,6 +19,7 @@ fn main() {
     test_socketpair_read();
 }
 
+#[track_caller]
 fn check_epoll_wait<const N: usize>(
     epfd: i32,
     mut expected_notifications: Vec<(u32, u64)>,
@@ -28,7 +29,10 @@ fn check_epoll_wait<const N: usize>(
     let maxsize = N;
     let array_ptr = array.as_mut_ptr();
     let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), 0) };
-    assert_eq!(res, expected_notifications.len().try_into().unwrap());
+    if res < 0 {
+        panic!("epoll_wait failed: {}", std::io::Error::last_os_error());
+    }
+    assert_eq!(res, expected_notifications.len().try_into().unwrap(), "didn't get the expected number of notifications");
     let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) };
     let mut return_events = slice.iter();
     while let Some(return_event) = return_events.next() {
@@ -84,7 +88,7 @@ fn test_epoll_socketpair() {
     assert_eq!(res, 0);
 
     // Check result from epoll_wait.
-    let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap();
+    let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap();
     let expected_value = u64::try_from(fds[1]).unwrap();
     assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)]));
 }
@@ -135,7 +139,7 @@ fn test_epoll_ctl_mod() {
     assert_eq!(res, 0);
 
     // Check result from epoll_wait.
-    let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap();
+    let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap();
     let expected_value = u64::try_from(fds[1]).unwrap();
     assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)]));
 }
@@ -168,7 +172,8 @@ fn test_epoll_ctl_del() {
     assert_ne!(res, -1);
 
     // Test EPOLL_CTL_DEL.
-    assert!(check_epoll_wait::<0>(epfd, vec![]));
+    //assert!(check_epoll_wait::<0>(epfd, vec![]));
+    // epoll_wait failed: Invalid argument (os error 22)
 }
 
 // This test is for one fd registered under two different epoll instance.
@@ -534,7 +539,7 @@ fn test_socketpair_read() {
     // But in real system, no notification will be provided here.
     let expected_event = u32::try_from(libc::EPOLLOUT).unwrap();
     let expected_value = fds[1] as u64;
-    assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)]));
+    assert!(check_epoll_wait::<8>(epfd, vec![]));
 
     // Read until the buffer is empty.
     let mut buf: [u8; 2] = [0; 2];

The last one is expected, it seems, given the comment -- that's the case we discussed. But the others are surprising to me, they should at least have comments explaining why we are diverging from the behavior of the real system.

There's also something else odd with the test: the test_epoll_ctl_del test doesn't ever use EPOLL_CTL_DEL!
Also, the test_epoll_ctl_mod test seems to be testing EPOLLRDHUP as well. I don't understand the organization of this test at all.

check_epoll_wait doesn't give very nice errors when something goes wrong, so this is also not easy to debug.

Cc @tiif

[tiif]

I will run through the test against real system again and apply necessary fix.

There's also something else odd with the test: the test_epoll_ctl_del test doesn't ever use EPOLL_CTL_DEL!

Oops that's definitely an error, sorry about that.

Also, the test_epoll_ctl_mod test seems to be testing EPOLLRDHUP as well. I don't understand the organization of this test at all.

I can explain this one here first. Basically what this test did is:

1. Write to fds[0].
2. Register fds[1] with epoll. So fds[1] is currently in the epoll interest list.
3. Then we check the current status of fds[1] using epoll_wait.
4. After that, we use EPOLL_CTL_MOD to add the EPOLLRHDHUP flags to fds[1] entry in the interest list, that basically means "for next epoll_wait on fds[1], return notification if it's peer is closed".
5. Close fds[0]
6. Check if fds[1] receive notification for EPOLLRDHUP since it's peer, fds[0] is closed.

[tiif]

@rustbot claim

[RalfJung]

I can explain this one here first. Basically what this test did is:

Seems better to test EPOLL_CTL_MOD with just epollin and epollout, as epollrdhup is generally a "more strange" flag. That lets us test the well-behaved basics independently of things like RDHUP.

[RalfJung]

Also for the MOD test, would be good to check that the relevant event does not lead to a notification at first, but then once MOD was used to extend the interest, then it does lead to a notification.

Or is there another test checking that events we are not interested in do not trigger notifications?

[tiif]

Also for the MOD test, would be good to check that the relevant event does not lead to a notification at first, but then once MOD was used to extend the interest, then it does lead to a notification.

This is achievable, I think we can register with EPOLLIN first, then epoll_ctl_mod to EPOLLOUT. Nothing needs to be done to the file description but EPOLLOUT notification should be returned.

Or is there another test checking that events we are not interested in do not trigger notifications?

Currently no, but what we can test is:

1. Open socketpair fds[0] and fds[1]
2. Register fds[0] with EPOLLOUT
3. Write to fds[1] (If we registered fds[0] with EPOLLIN, this event will trigger EPOLLIN notification by now)
4. Check if epoll_wait on fds[0] contains EPOLLIN, it is expected to not contain EPOLLIN.

[RalfJung]

Sounds good, thanks!

It is slightly concerning that in the test_epoll_ctl_del test, our behavior differs from that of the kernel -- we return an empty list, the kernel returns an error. Can you also figure out what happens there, and ideally make us return the same error?

I assume it is caused by maxevents being 0.

[tiif]

I assume it is caused by maxevents being 0.

Yup I tried this, this is indeed the problem, thanks!

[RalfJung]

In the PR you mentioned that EPOLLHUP is just not implemented yet. That seems fair, I assume you will complain in epoll_ctl if someone tries to register interest for this event we do not support? If yes, then it's entirely fine to not report it, please just have a comment in the test so that someone running it against real libc isn't too confused. :)

That makes the test_epoll_ctl_del test the only true difference.

[tiif]

Unfortunately, EPOLLHUP is a flag that will always be reported even though the user don't register them in epoll_ctl

EPOLLHUP
Hang up happened on the associated file descriptor.

epoll_wait will always wait for this event;
it is not necessary to set it in events when calling epoll_ctl().

Note that when reading from a channel such as a pipe or a
stream socket, this event merely indicates that the peer
closed its end of the channel. Subsequent reads from the
channel will return 0 (end of file) only after all
outstanding data in the channel has been consumed.

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

But from the description, this should be fairly easy to fix, we would only need to add one more flag to be triggered together with EPOLLRDHUP.