ptfs: change return value of FileHandle::from_name_at()

Change return value of FileHandle::from_name_at() from io::Result<Self>
to io::Result<Option<Self>>.

Signed-off-by: Jiang Liu <[email protected]>

Author: jiangliu

src/passthrough/file_handle.rs

@@ -14,6 +14,7 @@ use std::sync::Arc;
 use vmm_sys_util::fam::{FamStruct, FamStructWrapper};
 
 use super::mount_fd::{MountFd, MountFds, MountId};
+use super::util::enosys;
 
 /// An arbitrary maximum size for CFileHandle::f_handle.
 ///
@@ -174,7 +175,15 @@ extern "C" {
 
 impl FileHandle {
     /// Create a file handle for the given file.
-    pub fn from_name_at(dir_fd: &impl AsRawFd, path: &CStr) -> io::Result<Self> {
+    ///
+    /// Return `Ok(None)` if no file handle can be generated for this file: Either because the
+    /// filesystem does not support it, or because it would require a larger file handle than we
+    /// can store.  These are not intermittent failures, i.e. if this function returns `Ok(None)`
+    /// for a specific file, it will always return `Ok(None)` for it.  Conversely, if this function
+    /// returns `Ok(Some)` at some point, it will never return `Ok(None)` later.
+    ///
+    /// Return an `io::Error` for all other errors.
+    pub fn from_name_at(dir_fd: &impl AsRawFd, path: &CStr) -> io::Result<Option<Self>> {
         let mut mount_id: libc::c_int = 0;
         let mut c_fh = CFileHandle::new(0);
 
@@ -194,10 +203,14 @@ impl FileHandle {
             )
         };
         if ret == -1 {
-            let e = io::Error::last_os_error();
-            // unwrap is safe as e is obtained from last_os_error().
-            if e.raw_os_error().unwrap() != libc::EOVERFLOW {
-                return Err(e);
+            let err = io::Error::last_os_error();
+            match err.raw_os_error() {
+                // Got the needed buffer size.
+                Some(libc::EOVERFLOW) => {}
+                // Filesystem does not support file handles
+                Some(libc::EOPNOTSUPP) => return Ok(None),
+                // Other error
+                _ => return Err(err),
             }
         } else {
             return Err(io::Error::from(io::ErrorKind::InvalidData));
@@ -221,14 +234,14 @@ impl FileHandle {
                 libc::AT_EMPTY_PATH,
             )
         };
-        if ret == 0 {
-            Ok(FileHandle {
-                mnt_id: mount_id as u64,
-                handle: c_fh,
-            })
-        } else {
-            Err(io::Error::last_os_error())
+        if ret == -1 {
+            return Err(io::Error::last_os_error());
         }
+
+        Ok(Some(FileHandle {
+            mnt_id: mount_id as MountId,
+            handle: c_fh,
+        }))
     }
 }
 
@@ -243,7 +256,7 @@ impl Debug for OpenableFileHandle {
         write!(
             f,
             "Openable file handle: mountfd {}, type {}, len {}",
-            self.mount_fd.file().as_raw_fd(),
+            self.mount_fd.as_raw_fd(),
             fh.handle_type,
             fh.handle_bytes
         )
@@ -267,10 +280,12 @@ impl OpenableFileHandle {
     where
         F: FnOnce(RawFd, libc::c_int, u32) -> io::Result<File>,
     {
-        let handle = FileHandle::from_name_at(dir_fd, path).map_err(|e| {
-            error!("from_name_at failed error {:?}", e);
-            e
-        })?;
+        let handle = FileHandle::from_name_at(dir_fd, path)
+            .map_err(|e| {
+                error!("from_name_at failed error {:?}", e);
+                e
+            })?
+            .ok_or_else(enosys)?;
 
         let mount_fd = mount_fds
             .get(handle.mnt_id, reopen_dir)
@@ -286,7 +301,7 @@ impl OpenableFileHandle {
     pub fn open(&self, flags: libc::c_int) -> io::Result<File> {
         let ret = unsafe {
             open_by_handle_at(
-                self.mount_fd.file().as_raw_fd(),
+                self.mount_fd.as_raw_fd(),
                 self.handle.handle.wrapper.as_fam_struct_ptr(),
                 flags,
             )
@@ -314,6 +329,7 @@ impl OpenableFileHandle {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use nix::unistd::getuid;
     use std::ffi::CString;
     use std::io::Read;
 
@@ -412,8 +428,10 @@ mod tests {
         let dir = File::open(topdir).unwrap();
         let filename = CString::new("build.rs").unwrap();
 
-        let dir_handle = FileHandle::from_name_at(&dir, &CString::new("").unwrap()).unwrap();
-        let file_handle = FileHandle::from_name_at(&dir, &filename).unwrap();
+        let dir_handle = FileHandle::from_name_at(&dir, &CString::new("").unwrap())
+            .unwrap()
+            .unwrap();
+        let file_handle = FileHandle::from_name_at(&dir, &filename).unwrap().unwrap();
 
         assert_eq!(dir_handle.mnt_id, file_handle.mnt_id);
         assert_ne!(
@@ -428,6 +446,11 @@ mod tests {
 
     #[test]
     fn test_openable_file_handle() {
+        let uid = getuid();
+        if !uid.is_root() {
+            return;
+        }
+
         let topdir = env!("CARGO_MANIFEST_DIR");
         let dir = File::open(topdir).unwrap();
         let filename = CString::new("build.rs").unwrap();

src/passthrough/mod.rs

@@ -526,7 +526,7 @@ impl<S: BitmapSlice + Send + Sync> PassthroughFs<S> {
         // Be careful:
         // - readlink() does not append a terminating null byte to buf
         // - OsString instances are not NUL terminated
-        return Ok(PathBuf::from(OsString::from_vec(buf)));
+        Ok(PathBuf::from(OsString::from_vec(buf)))
     }
 
     /// Get the file pathname corresponding to the Inode
@@ -621,8 +621,8 @@ impl<S: BitmapSlice + Send + Sync> PassthroughFs<S> {
                 // TODO: use statx(2) to query mntid when 5.8 kernel or later are widely used.
                 let mnt_id = if use_mntid {
                     match FileHandle::from_name_at(dir_fd, name) {
-                        Ok(h) => h.mnt_id,
-                        Err(_) => 0,
+                        Ok(Some(h)) => h.mnt_id,
+                        _ => 0,
                     }
                 } else {
                     0

src/passthrough/mount_fd.rs

@@ -5,25 +5,26 @@ use std::collections::{HashMap, HashSet};
 use std::ffi::CString;
 use std::fs::File;
 use std::io::{self, Read, Seek};
-use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
+use std::os::unix::io::{AsRawFd, RawFd};
 use std::sync::{Arc, Mutex, RwLock, Weak};
 
 use super::statx::statx;
-use super::util::einval;
+use super::util::{einval, is_safe_inode};
 
 const MOUNT_INFO_FILE: &str = "/proc/self/mountinfo";
 
+/// Type alias for mount id.
 pub type MountId = u64;
 
 pub struct MountFd {
-    map: Weak<RwLock<HashMap<MountId, Weak<MountFd>>>>,
-    mount_id: MountId,
     file: File,
+    mount_id: MountId,
+    map: Weak<RwLock<HashMap<MountId, Weak<MountFd>>>>,
 }
 
-impl MountFd {
-    pub fn file(&self) -> &File {
-        &self.file
+impl AsRawFd for MountFd {
+    fn as_raw_fd(&self) -> RawFd {
+        self.file.as_raw_fd()
     }
 }
 
@@ -132,15 +133,12 @@ impl MountFds {
                     .prefix(format!("Failed to open mount point \"{mount_point}\"")));
             }
 
-            // Safe because we have just opened this FD
-            let mount_point_file = unsafe { File::from_raw_fd(mount_point_fd) };
-
             // Check the mount point has the expected `mount_id`.
-            let st_mode = self.validate_mount_id(mount_id, &mount_point_file, &mount_point)?;
+            let st_mode = self.validate_mount_id(mount_id, &mount_point_fd, &mount_point)?;
 
             // Ensure that we can safely reopen `mount_point_path` with `O_RDONLY`
             let file_type = st_mode & libc::S_IFMT;
-            if file_type != libc::S_IFREG && file_type != libc::S_IFDIR {
+            if !is_safe_inode(file_type) {
                 return Err(self
                     .error_for(mount_id, io::Error::from_raw_os_error(libc::EIO))
                     .set_desc(format!(
@@ -150,7 +148,7 @@ impl MountFds {
 
             // Now that we know that this is a regular file or directory, really open it
             let file = reopen_fd(
-                mount_point_file.as_raw_fd(),
+                mount_point_fd.as_raw_fd(),
                 libc::O_RDONLY | libc::O_NOFOLLOW | libc::O_CLOEXEC,
                 st_mode,
             )
@@ -177,9 +175,9 @@ impl MountFds {
                     file.as_raw_fd(),
                 );
                 let mount_fd = Arc::new(MountFd {
-                    map: Arc::downgrade(&self.map),
-                    mount_id,
                     file,
+                    mount_id,
+                    map: Arc::downgrade(&self.map),
                 });
                 mount_fds_locked.insert(mount_id, Arc::downgrade(&mount_fd));
                 mount_fd
@@ -193,10 +191,10 @@ impl MountFds {
     fn validate_mount_id(
         &self,
         mount_id: MountId,
-        mount_point_file: &File,
+        mount_point_fd: &impl AsRawFd,
         mount_point: &str,
     ) -> MPRResult<libc::mode_t> {
-        let stx = statx(mount_point_file, None).map_err(|e| {
+        let stx = statx(mount_point_fd, None).map_err(|e| {
             self.error_for(mount_id, e)
                 .prefix(format!("Failed to stat mount point \"{mount_point}\""))
         })?;
@@ -449,7 +447,7 @@ mod tests {
         let dir = File::open(topdir).unwrap();
         let filename = CString::new("build.rs").unwrap();
         let mount_fds = MountFds::new(None).unwrap();
-        let handle = FileHandle::from_name_at(&dir, &filename).unwrap();
+        let handle = FileHandle::from_name_at(&dir, &filename).unwrap().unwrap();
 
         // Ensure that `MountFds::get()` works for new entry.
         let fd1 = mount_fds
@@ -466,7 +464,7 @@ mod tests {
         assert_eq!(mount_fds.map.read().unwrap().len(), 1);
 
         // Ensure fd1 and fd2 are the same object.
-        assert_eq!(fd1.file().as_raw_fd(), fd2.file().as_raw_fd());
+        assert_eq!(fd1.as_raw_fd(), fd2.as_raw_fd());
 
         drop(fd1);
         assert_eq!(Arc::strong_count(&fd2), 1);

src/passthrough/statx.rs

@@ -82,7 +82,10 @@ impl SafeStatXAccess for statx_st {
 }
 
 fn get_mount_id(dir: &impl AsRawFd, path: &CStr) -> Option<MountId> {
-    FileHandle::from_name_at(dir, path).map(|v| v.mnt_id).ok()
+    match FileHandle::from_name_at(dir, path) {
+        Ok(Some(v)) => Some(v.mnt_id),
+        _ => None,
+    }
 }
 
 // Only works on Linux, and libc::SYS_statx is only defined for these

src/passthrough/util.rs

@@ -216,6 +216,10 @@ pub fn einval() -> io::Error {
     io::Error::from_raw_os_error(libc::EINVAL)
 }
 
+pub fn enosys() -> io::Error {
+    io::Error::from_raw_os_error(libc::ENOSYS)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;