Add a type-safe wrapper for open flags (#1054)

* Introduce OpenFlags

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Tidy up

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Fix linux build

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Support attributes

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Ignore example code

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

* Address access mode flags

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

---------

Signed-off-by: Alessandro Passaro <alexpax@amazon.co.uk>

mountpoint-s3/Cargo.toml

@@ -16,6 +16,7 @@ async-channel = "2.1.1"
 async-lock = "3.3.0"
 async-trait = "0.1.57"
 bincode = "1.3.3"
+bitflags = "2.5.0"
 bytes = { version = "1.2.1", features = ["serde"] }
 clap = { version = "4.1.9", features = ["derive"] }
 const_format = "0.2.30"

mountpoint-s3/src/fs.rs

@@ -33,6 +33,9 @@ pub use error::{Error, ToErrno};
 pub mod error_metadata;
 use error_metadata::{ErrorMetadata, MOUNTPOINT_ERROR_LOOKUP_NONEXISTENT};
 
+mod flags;
+pub use flags::OpenFlags;
+
 mod handles;
 use handles::{DirHandle, FileHandle, FileHandleState, UploadState};
 
@@ -281,17 +284,17 @@ where
         self.superblock.forget(ino, n);
     }
 
-    pub async fn open(&self, ino: InodeNo, flags: i32, pid: u32) -> Result<Opened, Error> {
-        trace!("fs:open with ino {:?} flags {:#b} pid {:?}", ino, flags, pid);
+    pub async fn open(&self, ino: InodeNo, flags: OpenFlags, pid: u32) -> Result<Opened, Error> {
+        trace!("fs:open with ino {:?} flags {} pid {:?}", ino, flags, pid);
 
         #[cfg(not(target_os = "linux"))]
         let direct_io = false;
         #[cfg(target_os = "linux")]
-        let direct_io = flags & libc::O_DIRECT != 0;
+        let direct_io = flags.contains(OpenFlags::O_DIRECT);
 
         // We can't support O_SYNC writes because they require the data to go to stable storage
         // at `write` time, but we only commit a PUT at `close` time.
-        if flags & (libc::O_SYNC | libc::O_DSYNC) != 0 {
+        if flags.intersects(OpenFlags::O_SYNC | OpenFlags::O_DSYNC) {
             return Err(err!(libc::EINVAL, "O_SYNC and O_DSYNC are not supported"));
         }
 
@@ -309,12 +312,12 @@ where
 
         // Open with O_APPEND is ok for new files because it's same as creating a new one.
         // but we can't support it on existing files and we should explicitly say we don't allow that.
-        if remote_file && (flags & libc::O_APPEND != 0) {
+        if remote_file && flags.contains(OpenFlags::O_APPEND) {
             return Err(err!(libc::EINVAL, "O_APPEND is not supported on existing files"));
         }
 
-        let state = if flags & libc::O_RDWR != 0 {
-            let is_truncate = flags & libc::O_TRUNC != 0;
+        let state = if flags.contains(OpenFlags::O_RDWR) {
+            let is_truncate = flags.contains(OpenFlags::O_TRUNC);
             if !remote_file || (self.config.allow_overwrite && is_truncate) {
                 // If the file is new or opened in truncate mode, we know it must be a write handle.
                 debug!("fs:open choosing write handle for O_RDWR");
@@ -324,7 +327,7 @@ where
                 debug!("fs:open choosing read handle for O_RDWR");
                 FileHandleState::new_read_handle(&lookup, self).await?
             }
-        } else if flags & libc::O_WRONLY != 0 {
+        } else if flags.contains(OpenFlags::O_WRONLY) {
             FileHandleState::new_write_handle(&lookup, lookup.inode.ino(), flags, pid, self).await?
         } else {
             FileHandleState::new_read_handle(&lookup, self).await?
@@ -832,7 +835,7 @@ mod tests {
             .await
             .unwrap();
         assert_eq!(dentry.attr.size, 0);
-        fs.open(dentry.attr.ino, libc::S_IFREG as i32 | libc::O_WRONLY, 0)
+        fs.open(dentry.attr.ino, OpenFlags::O_WRONLY, 0)
             .await
             .expect("open before the corruption should succeed");
 
@@ -845,7 +848,7 @@ mod tests {
             .unwrap();
         assert_eq!(dentry.attr.size, 0);
         let err = fs
-            .open(dentry.attr.ino, libc::S_IFREG as i32 | libc::O_WRONLY, 0)
+            .open(dentry.attr.ino, OpenFlags::O_WRONLY, 0)
             .await
             .expect_err("open after the corruption should fail");
         assert_eq!(err.errno, libc::EIO);

mountpoint-s3/src/fs/flags.rs

@@ -0,0 +1,115 @@
+/// Helper to define type-safe flags from `libc` constants.
+/// Usage:
+///
+/// ```ignore
+/// struct MyFlags(u32);
+///
+/// libc_flags! {
+///     MyFlags : u32 {
+///         A,
+///         B,
+///         C,
+///     }
+/// }    
+/// ```
+///
+/// See [`OpenFlags`] for an example.
+macro_rules! libc_flags {
+    (
+        $struct_name:ident : $raw_type:ty {
+        $(
+            $(#[$flag_attr:ident $($flag_meta_args:tt)*])*
+            $flag_name:ident
+        ),*$(,)+
+    }
+    ) => {
+        bitflags::bitflags! {
+            impl $struct_name : $raw_type {
+                $(
+                    $(#[$flag_attr $($flag_meta_args)*])*
+                    const $flag_name = libc::$flag_name;
+                )*
+
+                const _ = !0;
+            }
+        }
+
+        impl std::fmt::Display for $struct_name {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                bitflags::parser::to_writer(self, f)
+            }
+        }
+
+        impl std::fmt::Debug for $struct_name {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                write!(f, "{}({:#x} = {})", stringify!($struct_name), self.0, self.to_string())
+            }
+        }
+
+        impl From<$raw_type> for $struct_name {
+            fn from(value: $raw_type) -> Self {
+                Self::from_bits_retain(value)
+            }
+        }
+    }
+}
+
+/// Flags used in [`open`](super::S3Filesystem::open).
+///
+/// Note that the "access mode" constants are not mapped as values because they do not behave
+/// like normal bit flags (and will not be shown in `Debug`/`Display`):
+/// * `libc::O_ACCMODE = 0b11` is a mask for the access mode,
+/// * `libc::O_RDONLY = 0b00` is also the default value.
+///
+/// The other access mode constants, `libc::O_WRONLY = 0b01` and `libc::O_RDWR = 0b10`, are
+/// valid bit flags and mapped respectively to `OpenFlags::O_WRONLY` and `OpenFlags::O_RDWR`.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct OpenFlags(i32);
+
+libc_flags! {
+    OpenFlags : i32 {
+        O_WRONLY,
+        O_RDWR,
+        O_APPEND,
+        O_SYNC,
+        O_TRUNC,
+        O_DSYNC,
+        O_NONBLOCK,
+        O_NOCTTY,
+        O_CREAT,
+
+        #[cfg(target_os = "linux")]
+        O_DIRECT,
+
+        // Incomplete list. To be integrated if/when required.
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn conversion_test() {
+        let raw = libc::O_WRONLY | libc::O_APPEND;
+        let flags: OpenFlags = raw.into();
+        assert_eq!(flags, OpenFlags::O_WRONLY | OpenFlags::O_APPEND);
+    }
+
+    #[test]
+    fn unknown_test() {
+        let raw = libc::O_ACCMODE; // Not defined as a value in `OpenFlags`.
+        let flags: OpenFlags = raw.into();
+        assert_eq!(flags.bits(), raw, "Unknown value should be preserved");
+    }
+
+    #[test]
+    fn debug_test() {
+        let flags = OpenFlags::O_WRONLY | OpenFlags::O_APPEND;
+        let expected = format!(
+            "OpenFlags({:#x} = O_WRONLY | O_APPEND)",
+            libc::O_WRONLY | libc::O_APPEND
+        );
+        assert_eq!(format!("{:?}", flags), expected);
+    }
+}

mountpoint-s3/src/fs/handles.rs

@@ -11,7 +11,7 @@ use crate::sync::atomic::{AtomicI64, Ordering};
 use crate::sync::AsyncMutex;
 use crate::upload::UploadRequest;
 
-use super::{DirectoryEntry, Error, InodeNo, S3Filesystem, ToErrno};
+use super::{DirectoryEntry, Error, InodeNo, OpenFlags, S3Filesystem, ToErrno};
 
 #[derive(Debug)]
 pub struct DirHandle {
@@ -90,11 +90,11 @@ where
     pub async fn new_write_handle(
         lookup: &LookedUp,
         ino: InodeNo,
-        flags: i32,
+        flags: OpenFlags,
         pid: u32,
         fs: &S3Filesystem<Client, Prefetcher>,
     ) -> Result<FileHandleState<Client, Prefetcher>, Error> {
-        let is_truncate = flags & libc::O_TRUNC != 0;
+        let is_truncate = flags.contains(OpenFlags::O_TRUNC);
         let handle = fs
             .superblock
             .write(&fs.client, ino, fs.config.allow_overwrite, is_truncate)

mountpoint-s3/src/fuse.rs

@@ -122,7 +122,7 @@ where
 
     #[instrument(level="warn", skip_all, fields(req=req.unique(), ino=ino, pid=req.pid(), name=field::Empty))]
     fn open(&self, req: &Request<'_>, ino: InodeNo, flags: i32, reply: ReplyOpen) {
-        match block_on(self.fs.open(ino, flags, req.pid()).in_current_span()) {
+        match block_on(self.fs.open(ino, flags.into(), req.pid()).in_current_span()) {
             Ok(opened) => reply.opened(opened.fh, opened.flags),
             Err(e) => fuse_error!("open", reply, e),
         }