OpenFlags: switch to bitflags

It makes far more sense to expose O_* flags as bitflags. We could
re-export nix::OFlags (when we switch to it) but since we want to have
special handling for O_ACCMODE, it makes more sense to have our own type
anyway.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

src/capi/core.rs

@@ -98,13 +98,13 @@ pub extern "C" fn pathrs_root_open(path: *const c_char) -> RawFd {
 /// pathrs_errorinfo().
 #[no_mangle]
 pub extern "C" fn pathrs_reopen(fd: RawFd, flags: c_int) -> RawFd {
-    let flags = OpenFlags(flags);
+    let flags = OpenFlags::from_bits_retain(flags);
 
     fd.reopen(flags)
         .and_then(|file| {
             // Rust sets O_CLOEXEC by default, without an opt-out. We need to
             // disable it if we weren't asked to do O_CLOEXEC.
-            if flags.0 & libc::O_CLOEXEC == 0 {
+            if !flags.contains(OpenFlags::O_CLOEXEC) {
                 syscalls::fcntl_unset_cloexec(file.as_raw_fd()).context(error::RawOsSnafu {
                     operation: "clear O_CLOEXEC on fd",
                 })?;
@@ -198,7 +198,7 @@ pub extern "C" fn pathrs_creat(
     ret::with_fd(root_fd, |root: &mut Root| {
         let mode = mode & !libc::S_IFMT;
         let perm = Permissions::from_mode(mode);
-        root.create_file(parse_path(path)?, OpenFlags(flags), &perm)
+        root.create_file(parse_path(path)?, OpenFlags::from_bits_retain(flags), &perm)
     })
 }
 

src/flags.rs

@@ -0,0 +1,84 @@
+/*
+ * libpathrs: safe path resolution on Linux
+ * Copyright (C) 2019-2024 Aleksa Sarai <cyphar@cyphar.com>
+ * Copyright (C) 2019-2024 SUSE LLC
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License as published by the Free
+ * Software Foundation, either version 3 of the License, or (at your option) any
+ * later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT ANY
+ * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
+ * PARTICULAR PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License along
+ * with this program. If not, see <https://www.gnu.org/licenses/>.
+ */
+
+bitflags! {
+    /// Wrapper for the underlying `libc`'s `O_*` flags.
+    ///
+    /// The flag values and their meaning is identical to the description in the
+    /// `open(2)` man page.
+    ///
+    /// # Caveats
+    ///
+    /// For historical reasons, the first three bits of `open(2)`'s flags are for
+    /// the access mode and are actually treated as a 2-bit number. So, it is
+    /// incorrect to attempt to do any checks on the access mode without masking it
+    /// correctly. So some helpers were added to make usage more ergonomic.
+    #[derive(Default, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Clone, Copy)]
+    pub struct OpenFlags: libc::c_int {
+        const O_EXCL = libc::O_EXCL;
+        const O_PATH = libc::O_PATH;
+        const O_RDWR = libc::O_RDWR;
+        const O_SYNC = libc::O_SYNC;
+        const O_ASYNC = libc::O_ASYNC;
+        const O_CREAT = libc::O_CREAT;
+        const O_DSYNC = libc::O_DSYNC;
+        const O_FSYNC = libc::O_FSYNC;
+        const O_RSYNC = libc::O_RSYNC;
+        const O_TRUNC = libc::O_TRUNC;
+        const O_APPEND = libc::O_APPEND;
+        const O_DIRECT = libc::O_DIRECT;
+        const O_NDELAY = libc::O_NDELAY;
+        const O_NOCTTY = libc::O_NOCTTY;
+        const O_RDONLY = libc::O_RDONLY;
+        const O_WRONLY = libc::O_WRONLY;
+        const O_ACCMODE = libc::O_ACCMODE;
+        const O_CLOEXEC = libc::O_CLOEXEC;
+        const O_NOATIME = libc::O_NOATIME;
+        const O_TMPFILE = libc::O_TMPFILE;
+        const O_NOFOLLOW = libc::O_NOFOLLOW;
+        const O_NONBLOCK = libc::O_NONBLOCK;
+        const O_DIRECTORY = libc::O_DIRECTORY;
+        //const O_LARGEFILE = libc::O_LARGEFILE;
+        const _ = !0;
+    }
+}
+
+impl OpenFlags {
+    /// Grab the access mode bits from the flags.
+    #[inline]
+    pub fn access_mode(self) -> libc::c_int {
+        self.bits() & libc::O_ACCMODE
+    }
+
+    /// Does the access mode imply read access?
+    #[inline]
+    pub fn wants_read(self) -> bool {
+        let acc = self.access_mode();
+        acc == libc::O_RDONLY || acc == libc::O_RDWR
+    }
+
+    /// Does the access mode imply write access? Note that there are several
+    /// other bits (such as `O_TRUNC`) which imply write access but are not part
+    /// of the access mode, and thus a `false` value from `.wants_write()` does
+    /// not guarantee that the kernel will not do a `MAY_WRITE` check.
+    #[inline]
+    pub fn wants_write(self) -> bool {
+        let acc = self.access_mode();
+        acc == libc::O_WRONLY || acc == libc::O_RDWR
+    }
+}

src/handle.rs

@@ -18,12 +18,10 @@
 
 #![forbid(unsafe_code)]
 
-use crate::{error::Error, utils::RawFdExt};
+use crate::{error::Error, flags::OpenFlags, utils::RawFdExt};
 
 use std::fs::File;
 
-use libc::c_int;
-
 /// A handle to an existing inode within a [`Root`].
 ///
 /// This handle references an already-resolved path which can be used for the
@@ -47,52 +45,6 @@ pub struct Handle {
     inner: File,
 }
 
-/// Wrapper for the underlying `libc`'s `O_*` flags.
-///
-/// The flag values and their meaning is identical to the description in the
-/// `open(2)` man page.
-///
-/// # Caveats
-///
-/// For historical reasons, the first three bits of `open(2)`'s flags are for
-/// the access mode and are actually treated as a 2-bit number. So, it is
-/// incorrect to attempt to do any checks on the access mode without masking it
-/// correctly. So some helpers were added to make usage more ergonomic.
-// TODO: Should probably be a u64, and use a constructor...
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
-pub struct OpenFlags(pub c_int);
-
-impl From<c_int> for OpenFlags {
-    fn from(flags: c_int) -> Self {
-        Self(flags)
-    }
-}
-
-impl OpenFlags {
-    /// Grab the access mode bits from the flags.
-    #[inline]
-    pub fn access_mode(self) -> c_int {
-        self.0 & libc::O_ACCMODE
-    }
-
-    /// Does the access mode imply read access?
-    #[inline]
-    pub fn wants_read(self) -> bool {
-        let acc = self.access_mode();
-        acc == libc::O_RDONLY || acc == libc::O_RDWR
-    }
-
-    /// Does the access mode imply write access? Note that there are several
-    /// other bits (such as `O_TRUNC`) which imply write access but are not part
-    /// of the access mode, and thus a `false` value from `.wants_write()` does
-    /// not guarantee that the kernel will not do a `MAY_WRITE` check.
-    #[inline]
-    pub fn wants_write(self) -> bool {
-        let acc = self.access_mode();
-        acc == libc::O_WRONLY || acc == libc::O_RDWR
-    }
-}
-
 impl Handle {
     /// "Upgrade" the handle to a usable [`File`] handle.
     ///

src/lib.rs

@@ -57,11 +57,11 @@
 //! // Resolve the path.
 //! let handle = root.resolve(unsafe_path)?;
 //! // Upgrade the handle to a full std::fs::File.
-//! let file = handle.reopen(libc::O_RDONLY)?;
+//! let file = handle.reopen(OpenFlags::O_RDONLY)?;
 //!
 //! // Or, in one line:
 //! let file = root.resolve(unsafe_path)?
-//!                .reopen(libc::O_RDONLY)?;
+//!                .reopen(OpenFlags::O_RDONLY)?;
 //! # Ok(())
 //! # }
 //! ```
@@ -127,6 +127,11 @@ extern crate libc;
 #[macro_use]
 extern crate snafu;
 
+// `OpenFlags`
+pub mod flags;
+#[doc(inline)]
+pub use flags::OpenFlags;
+
 // `Handle` implementation.
 mod handle;
 #[doc(inline)]

src/root.rs

@@ -390,7 +390,7 @@ impl Root {
     pub fn create_file<P: AsRef<Path>>(
         &self,
         path: P,
-        flags: OpenFlags,
+        mut flags: OpenFlags,
         perm: &Permissions,
     ) -> Result<Handle, Error> {
         // Get a handle for the lexical parent of the target path. It must
@@ -407,11 +407,12 @@ impl Root {
         // XXX: openat2(2) supports doing O_CREAT on trailing symlinks without
         // O_NOFOLLOW. We might want to expose that here, though because it
         // can't be done with the emulated backend that might be a bad idea.
-        let flags = flags.0 | libc::O_CREAT;
-        let file =
-            syscalls::openat(dirfd, name, flags, perm.mode()).context(error::RawOsSnafu {
+        flags.insert(OpenFlags::O_CREAT);
+        let file = syscalls::openat(dirfd, name, flags.bits(), perm.mode()).context(
+            error::RawOsSnafu {
                 operation: "pathrs create_file",
-            })?;
+            },
+        )?;
 
         Ok(Handle::from_file_unchecked(file))
     }

src/tests/test_resolve.rs

@@ -281,9 +281,9 @@ mod utils {
         format!("{:?} ({})", err, Errno(err))
     }
 
-    pub(super) fn check_reopen<F: Into<OpenFlags>>(
+    pub(super) fn check_reopen(
         handle: &Handle,
-        flags: F,
+        flags: OpenFlags,
         expected_error: Option<RawOsError>,
     ) -> Result<(), Error> {
         let file = match (handle.reopen(flags), expected_error) {
@@ -390,18 +390,18 @@ mod utils {
 
         match real_file_type {
             libc::S_IFDIR => {
-                check_reopen(&handle, libc::O_RDONLY, None)?;
-                check_reopen(&handle, libc::O_DIRECTORY, None)?;
+                check_reopen(&handle, OpenFlags::O_RDONLY, None)?;
+                check_reopen(&handle, OpenFlags::O_DIRECTORY, None)?;
             }
             libc::S_IFREG => {
-                check_reopen(&handle, libc::O_RDWR, None)?;
-                check_reopen(&handle, libc::O_DIRECTORY, Some(libc::ENOTDIR))?;
+                check_reopen(&handle, OpenFlags::O_RDWR, None)?;
+                check_reopen(&handle, OpenFlags::O_DIRECTORY, Some(libc::ENOTDIR))?;
             }
             _ => {
-                check_reopen(&handle, libc::O_PATH, None)?;
+                check_reopen(&handle, OpenFlags::O_PATH, None)?;
                 check_reopen(
                     &handle,
-                    libc::O_PATH | libc::O_DIRECTORY,
+                    OpenFlags::O_PATH | OpenFlags::O_DIRECTORY,
                     Some(libc::ENOTDIR),
                 )?;
             }

src/utils.rs

@@ -181,10 +181,15 @@ impl RawFdExt for RawFd {
         // TODO: We should look into using O_EMPTYPATH if it's available to
         //       avoid the /proc dependency -- though then again, as_unsafe_path
         //       necessarily requires /proc.
-        syscalls::openat_follow(PROCFS_HANDLE.as_raw_fd(), proc_subpath(*self)?, flags.0, 0)
-            .context(error::RawOsSnafu {
-                operation: "reopen fd through procfs",
-            })
+        syscalls::openat_follow(
+            PROCFS_HANDLE.as_raw_fd(),
+            proc_subpath(*self)?,
+            flags.bits(),
+            0,
+        )
+        .context(error::RawOsSnafu {
+            operation: "reopen fd through procfs",
+        })
     }
 
     fn as_unsafe_path(&self) -> Result<PathBuf, Error> {