Merge #715

715: Fix multiple issues with POSIX AIO r=asomers a=asomers

Fixed error handling in `AioCb::fsync`, `AioCb::read`, and `AioCb::write`
    
    Previously, the `AioCb`'s `in_progress` field would erroneously be set
    to `true`, even if the syscall had an error
    
    Fixes #714

AioCb::Drop will now panic for in-progress AioCb

    Printing a warning message to stderr isn't really appropriate, because
    there's no way to guarantee that stderr is even valid.  Nor is
    aio_suspend necessarily an appropriate action to take.

CHANGELOG.md

@@ -51,10 +51,14 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   ([#744](https://github.com/nix-rust/nix/pull/744))
 - Moved constants ptrace request, event and options to enums and updated ptrace functions and argument types accordingly. 
   ([#749](https://github.com/nix-rust/nix/pull/749))
+- `AioCb::Drop` will now panic if the `AioCb` is still in-progress ([#715](https://github.com/nix-rust/nix/pull/715))
 
 # Fixed
 - Fix compilation and tests for OpenBSD targets
   ([#688](https://github.com/nix-rust/nix/pull/688))
+- Fixed error handling in `AioCb::fsync`, `AioCb::read`, and `AioCb::write`.
+  It is no longer an error to drop an `AioCb` that failed to enqueue in the OS.
+  ([#715](https://github.com/nix-rust/nix/pull/715))
 
 # Removed
 - The syscall module has been removed. This only exposed enough functionality for

Cargo.toml

@@ -31,6 +31,10 @@ nix-test = { path = "nix-test", version = "0.0.1" }
 name = "test"
 path = "test/test.rs"
 
+[[test]]
+name = "test-aio-drop"
+path = "test/sys/test_aio_drop.rs"
+
 [[test]]
 name = "test-mount"
 path = "test/test_mount.rs"

src/sys/aio.rs

@@ -4,8 +4,6 @@ use libc::{c_void, off_t, size_t};
 use libc;
 use std::fmt;
 use std::fmt::Debug;
-use std::io::Write;
-use std::io::stderr;
 use std::marker::PhantomData;
 use std::mem;
 use std::rc::Rc;
@@ -234,16 +232,22 @@ impl<'a> AioCb<'a> {
     /// An asynchronous version of `fsync`.
     pub fn fsync(&mut self, mode: AioFsyncMode) -> Result<()> {
         let p: *mut libc::aiocb = &mut self.aiocb;
-        self.in_progress = true;
-        Errno::result(unsafe { libc::aio_fsync(mode as libc::c_int, p) }).map(drop)
+        Errno::result(unsafe {
+                libc::aio_fsync(mode as libc::c_int, p)
+        }).map(|_| {
+            self.in_progress = true;
+        })
     }
 
     /// Asynchronously reads from a file descriptor into a buffer
     pub fn read(&mut self) -> Result<()> {
         assert!(self.mutable, "Can't read into an immutable buffer");
         let p: *mut libc::aiocb = &mut self.aiocb;
-        self.in_progress = true;
-        Errno::result(unsafe { libc::aio_read(p) }).map(drop)
+        Errno::result(unsafe {
+            libc::aio_read(p)
+        }).map(|_| {
+            self.in_progress = true;
+        })
     }
 
     /// Retrieve return status of an asynchronous operation.  Should only be
@@ -259,8 +263,11 @@ impl<'a> AioCb<'a> {
     /// Asynchronously writes from a buffer to a file descriptor
     pub fn write(&mut self) -> Result<()> {
         let p: *mut libc::aiocb = &mut self.aiocb;
-        self.in_progress = true;
-        Errno::result(unsafe { libc::aio_write(p) }).map(drop)
+        Errno::result(unsafe {
+            libc::aio_write(p)
+        }).map(|_| {
+            self.in_progress = true;
+        })
     }
 
 }
@@ -332,24 +339,8 @@ impl<'a> Debug for AioCb<'a> {
 
 impl<'a> Drop for AioCb<'a> {
     /// If the `AioCb` has no remaining state in the kernel, just drop it.
-    /// Otherwise, collect its error and return values, so as not to leak
-    /// resources.
+    /// Otherwise, dropping constitutes a resource leak, which is an error
     fn drop(&mut self) {
-        if self.in_progress {
-            // Well-written programs should never get here.  They should always
-            // wait for an AioCb to complete before dropping it
-            let _ = write!(stderr(), "WARNING: dropped an in-progress AioCb");
-            loop {
-                let ret = aio_suspend(&[&self], None);
-                match ret {
-                    Ok(()) => break,
-                    Err(Error::Sys(Errno::EINVAL)) => panic!(
-                        "Inconsistent AioCb.in_progress value"),
-                    Err(Error::Sys(Errno::EINTR)) => (),    // Retry interrupted syscall
-                    _ => panic!("Unexpected aio_suspend return value {:?}", ret)
-                };
-            }
-            let _ = self.aio_return();
-        }
+        assert!(!self.in_progress, "Dropped an in-progress AioCb");
     }
 }

test/sys/test_aio.rs

@@ -88,6 +88,26 @@ fn test_fsync() {
     aiocb.aio_return().unwrap();
 }
 
+/// `AioCb::fsync` should not modify the `AioCb` object if libc::aio_fsync returns
+/// an error
+// Skip on Linux, because Linux's AIO implementation can't detect errors
+// synchronously
+#[test]
+#[cfg(any(target_os = "freebsd", target_os = "macos"))]
+fn test_fsync_error() {
+    use std::mem;
+
+    const INITIAL: &'static [u8] = b"abcdef123456";
+    // Create an invalid AioFsyncMode
+    let mode = unsafe { mem::transmute(666) };
+    let mut f = tempfile().unwrap();
+    f.write(INITIAL).unwrap();
+    let mut aiocb = AioCb::from_fd( f.as_raw_fd(),
+                            0,   //priority
+                            SigevNotify::SigevNone);
+    let err = aiocb.fsync(mode);
+    assert!(err.is_err());
+}
 
 #[test]
 #[cfg_attr(all(target_env = "musl", target_arch = "x86_64"), ignore)]
@@ -156,6 +176,26 @@ fn test_read() {
     assert!(EXPECT == rbuf.deref().deref());
 }
 
+/// `AioCb::read` should not modify the `AioCb` object if libc::aio_read returns
+/// an error
+// Skip on Linux, because Linux's AIO implementation can't detect errors
+// synchronously
+#[test]
+#[cfg(any(target_os = "freebsd", target_os = "macos"))]
+fn test_read_error() {
+    const INITIAL: &'static [u8] = b"abcdef123456";
+    let rbuf = Rc::new(vec![0; 4].into_boxed_slice());
+    let mut f = tempfile().unwrap();
+    f.write(INITIAL).unwrap();
+    let mut aiocb = AioCb::from_boxed_slice( f.as_raw_fd(),
+                           -1,   //an invalid offset
+                           rbuf.clone(),
+                           0,   //priority
+                           SigevNotify::SigevNone,
+                           LioOpcode::LIO_NOP);
+    assert!(aiocb.read().is_err());
+}
+
 // Tests from_mut_slice
 #[test]
 #[cfg_attr(all(target_env = "musl", target_arch = "x86_64"), ignore)]
@@ -230,6 +270,23 @@ fn test_write() {
     assert!(rbuf == EXPECT);
 }
 
+/// `AioCb::write` should not modify the `AioCb` object if libc::aio_write returns
+/// an error
+// Skip on Linux, because Linux's AIO implementation can't detect errors
+// synchronously
+#[test]
+#[cfg(any(target_os = "freebsd", target_os = "macos"))]
+fn test_write_error() {
+    let wbuf = "CDEF".to_string().into_bytes();
+    let mut aiocb = AioCb::from_slice( 666, // An invalid file descriptor
+                           0,   //offset
+                           &wbuf,
+                           0,   //priority
+                           SigevNotify::SigevNone,
+                           LioOpcode::LIO_NOP);
+    assert!(aiocb.write().is_err());
+}
+
 lazy_static! {
     pub static ref SIGNALED: AtomicBool = AtomicBool::new(false);
 }
@@ -442,31 +499,3 @@ fn test_lio_listio_read_immutable() {
                            LioOpcode::LIO_READ);
     let _ = lio_listio(LioMode::LIO_NOWAIT, &[&mut rcb], SigevNotify::SigevNone);
 }
-
-// Test dropping an AioCb that hasn't yet finished.  Behind the scenes, the
-// library should wait for the AioCb's completion.
-#[test]
-#[cfg_attr(all(target_env = "musl", target_arch = "x86_64"), ignore)]
-fn test_drop() {
-    const INITIAL: &'static [u8] = b"abcdef123456";
-    const WBUF: &'static [u8] = b"CDEF"; //"CDEF".to_string().into_bytes();
-    let mut rbuf = Vec::new();
-    const EXPECT: &'static [u8] = b"abCDEF123456";
-
-    let mut f = tempfile().unwrap();
-    f.write(INITIAL).unwrap();
-    {
-        let mut aiocb = AioCb::from_slice( f.as_raw_fd(),
-                               2,   //offset
-                               &WBUF,
-                               0,   //priority
-                               SigevNotify::SigevNone,
-                               LioOpcode::LIO_NOP);
-        aiocb.write().unwrap();
-    }
-
-    f.seek(SeekFrom::Start(0)).unwrap();
-    let len = f.read_to_end(&mut rbuf).unwrap();
-    assert!(len == EXPECT.len());
-    assert!(rbuf == EXPECT);
-}

test/sys/test_aio_drop.rs

@@ -0,0 +1,27 @@
+extern crate nix;
+extern crate tempfile;
+
+use nix::sys::aio::*;
+use nix::sys::signal::*;
+use std::os::unix::io::AsRawFd;
+use tempfile::tempfile;
+
+// Test dropping an AioCb that hasn't yet finished.
+// This must happen in its own process, because on OSX this test seems to hose
+// the AIO subsystem and causes subsequent tests to fail
+#[test]
+#[should_panic(expected = "Dropped an in-progress AioCb")]
+#[cfg(not(target_env = "musl"))]
+fn test_drop() {
+    const WBUF: &'static [u8] = b"CDEF";
+
+    let f = tempfile().unwrap();
+    f.set_len(6).unwrap();
+    let mut aiocb = AioCb::from_slice( f.as_raw_fd(),
+                           2,   //offset
+                           &WBUF,
+                           0,   //priority
+                           SigevNotify::SigevNone,
+                           LioOpcode::LIO_NOP);
+    aiocb.write().unwrap();
+}