Added test and comments for poll_syscall (#296)

* added tests and comment for poll_syscall

* minor adjustment

* resolved comments

* resolved some comments

* resolved comments

---------

Co-authored-by: lind <[email protected]>

src/safeposix/syscalls/net_calls.rs

@@ -3215,12 +3215,77 @@ impl Cage {
         return 0;
     }
 
+    /// ## ------------------POLL SYSCALL------------------
+    /// ### Description
+    /// poll_syscall performs a similar task to select_syscall: it waits for
+    /// one of a set of file descriptors to become ready to perform I/O.
+
+    /// ### Function Arguments
+    /// The `poll_syscall()` receives two arguments:
+    /// * `fds` - The set of file descriptors to be monitored is specified in
+    ///   the fds argument, which is an array of PollStruct structures
+    ///   containing three fields: fd, events and revents. events and revents
+    ///   are requested events and returned events, respectively. The field fd
+    ///   contains a file descriptor for an open file. If this field is
+    ///   negative, then the corresponding events field is ignored and the
+    ///   revents field returns zero. The field events is an input parameter, a
+    ///   bit mask specifying the events the application is interested in for
+    ///   the file descriptor fd. The bits returned in revents can include any
+    ///   of those specified in events, or POLLNVAL. The bits that may be
+    ///   set/returned in events and revents are: 1. POLLIN: There is data to
+    ///   read. 2. POLLPRI: There is some exceptional condition on the file
+    ///   descriptor, currently not supported 3. POLLOUT: Writing is now
+    ///   possible, though a write larger than the available space in a socket
+    ///   or pipe will still block
+    ///   4. POLLNVAL: Invalid request: fd not open (only returned in revents;
+    ///   ignored in events).
+    /// * `timeout` - The timeout argument is a RustDuration structure that
+    ///   specifies the interval that poll() should block waiting for a file
+    ///   descriptor to become ready. The call will block until either: 1.  a
+    ///   file descriptor becomes ready; 2. the call is interrupted by a signal
+    ///   handler; 3. the timeout expires.
+
+    /// ### Returns
+    /// On success, poll_syscall returns a nonnegative value which is the
+    /// number of elements in the pollfds whose revents fields have been
+    /// set to a nonzero value (indicating an event or an error). A
+    /// return value of zero indicates that the system call timed out
+    /// before any file descriptors became ready.
+    ///
+    /// ### Errors
+    /// * EINTR - A signal was caught.
+    /// * EINVAL - fd exceeds the FD_SET_MAX_FD.
+    ///
+    /// ### Panics
+    /// No panic is expected from this syscall
     pub fn poll_syscall(
         &self,
         fds: &mut [PollStruct],
         timeout: Option<interface::RustDuration>,
     ) -> i32 {
-        //timeout is supposed to be in milliseconds
+        // timeout is supposed to be in milliseconds
+
+        // current implementation of poll_syscall is based on select_syscall
+        // which gives several issues:
+        // 1. according to standards, select_syscall should only support file descriptor
+        //    that is smaller than 1024, while poll_syscall should not have such
+        //    limitation but our implementation of poll_syscall is actually calling
+        //    select_syscall directly which would mean poll_syscall would also have the
+        //    1024 maximum size limitation However, rustposix itself only support file
+        //    descriptor that is smaller than 1024 which solves this issue automatically
+        //    in an interesting way
+        // 2. current implementation of poll_syscall is very inefficient, that it passes
+        //    each of the file descriptor into select_syscall one by one. A better
+        //    solution might be transforming pollstruct into fdsets and pass into
+        //    select_syscall once (TODO). A even more efficienct way would be completely
+        //    rewriting poll_syscall so it does not depend on select_syscall anymore.
+        //    This is also how Linux does for poll_syscall since Linux claims that poll
+        //    have a better performance than select.
+        // 3. several revent value such as POLLERR (which should be set when pipe is
+        //    broken), or POLLHUP (when peer closed its channel) are not possible to
+        //    monitor. Since select_syscall does not have these features, so our
+        //    poll_syscall, which derived from select_syscall, would subsequently not be
+        //    able to support these features.
 
         let mut return_code: i32 = 0;
         let start_time = interface::starttimer();
@@ -3230,34 +3295,52 @@ impl Cage {
             None => interface::RustDuration::MAX,
         };
 
+        // according to standard, we should clear all revents
+        for structpoll in &mut *fds {
+            structpoll.revents = 0;
+        }
+
+        // we loop until either timeout
+        // or any of the file descriptor is ready
         loop {
+            // iterate through each file descriptor
             for structpoll in &mut *fds {
+                // get the file descriptor
                 let fd = structpoll.fd;
+
+                // according to standard, we should ignore all file descriptor
+                // that is smaller than 0
+                if fd < 0 {
+                    continue;
+                }
+
+                // get the associated events to monitor
                 let events = structpoll.events;
 
                 // init FdSet structures
                 let reads = &mut interface::FdSet::new();
                 let writes = &mut interface::FdSet::new();
                 let errors = &mut interface::FdSet::new();
 
-                //read
+                // POLLIN for readable fd
                 if events & POLLIN > 0 {
                     reads.set(fd)
                 }
-                //write
+                // POLLOUT for writable fd
                 if events & POLLOUT > 0 {
                     writes.set(fd)
                 }
-                //err
-                if events & POLLERR > 0 {
+                // POLLPRI for except fd
+                if events & POLLPRI > 0 {
                     errors.set(fd)
                 }
 
+                // this mask is used for storing final revent result
                 let mut mask: i16 = 0;
 
-                //0 essentially sets the timeout to the max value allowed (which is almost
-                // always more than enough time) NOTE that the nfds argument is
-                // highest fd + 1
+                // here we just call select_syscall with timeout of zero,
+                // which essentially just check each fd set once then return
+                // NOTE that the nfds argument is highest fd + 1
                 let selectret = Self::select_syscall(
                     &self,
                     fd + 1,
@@ -3266,23 +3349,44 @@ impl Cage {
                     Some(errors),
                     Some(interface::RustDuration::ZERO),
                 );
+                // if there is any file descriptor ready
                 if selectret > 0 {
-                    mask += if !reads.is_empty() { POLLIN } else { 0 };
-                    mask += if !writes.is_empty() { POLLOUT } else { 0 };
-                    mask += if !errors.is_empty() { POLLERR } else { 0 };
+                    // is the file descriptor ready to read?
+                    mask |= if !reads.is_empty() { POLLIN } else { 0 };
+                    // is the file descriptor ready to write?
+                    mask |= if !writes.is_empty() { POLLOUT } else { 0 };
+                    // is there any exception conditions on the file descriptor?
+                    mask |= if !errors.is_empty() { POLLPRI } else { 0 };
+                    // this file descriptor is ready for something,
+                    // increment the return value
                     return_code += 1;
                 } else if selectret < 0 {
-                    return selectret;
+                    // if there is any error, first check if the error
+                    // is EBADF, which refers to invalid file descriptor error
+                    // in this case, we should set POLLNVAL to revent
+                    if selectret == -(Errno::EBADF as i32) {
+                        mask |= POLLNVAL;
+                        // according to standard, return value is the number of fds
+                        // with non-zero revent, which may indicate an error as well
+                        return_code += 1;
+                    } else {
+                        return selectret;
+                    }
                 }
+                // set the revents
                 structpoll.revents = mask;
             }
 
+            // we break if there is any file descriptor ready
+            // or timeout is reached
             if return_code != 0 || interface::readtimer(start_time) > end_time {
                 break;
             } else {
+                // otherwise, check for signal and loop again
                 if interface::sigcheck() {
                     return syscall_error(Errno::EINTR, "poll", "interrupted function call");
                 }
+                // We yield to let other threads continue if we've found no ready descriptors
                 interface::lind_yield();
             }
         }