From d004869149185df362cc533c799f8a857da92ef1 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Sat, 15 Feb 2020 22:12:56 +0100 Subject: [PATCH] Refactor and combine all FileType structs in yanix This commit does a bit of everything: refactors bits here and there, fixes a bug discovered in another #701, and combines all structs that we used in `yanix` and `wasi-common` crates to represent file types on *nix into one struct, `yanix::file::FileType`. Up until now, in `yanix`, we've had two separate structs used to represent file types on the host: `yanix::dir::FileType` and `yanix::file::SFlags` (well, not quite, but that was its main use). They both were used in different context (the former when parsing `dirent` struct, and the latter when parsing `stat` struct), they were C-compatible (as far as their representation goes), and as it turns out, they shared possible enumeration values. This commit combines them both into an idiomatic Rust enum with the caveat that it is now *not* C-compatible, however, I couldn't find a single use where that would actually matter, and even if it does in the future, we can simply add appropriate impl methods. The combine `yanix::file::FileType` struct can be constructed in two ways: 1) either from `stat.st_mode` value (and while we're here, now it's done correctly according to POSIX which fixes the bug mentioned in VFS impl PR #701), or 2) from `dirent.d_type` value. Also, since we now have one struct for representing both contexts, this cleans up nicely a lot of duplicated code in `host` module. --- .../snapshot_0/sys/unix/bsd/hostcalls_impl.rs | 4 +- .../src/old/snapshot_0/sys/unix/host_impl.rs | 64 ++++--------------- .../snapshot_0/sys/unix/hostcalls_impl/fs.rs | 6 +- .../src/sys/unix/bsd/hostcalls_impl.rs | 4 +- crates/wasi-common/src/sys/unix/host_impl.rs | 64 ++++--------------- .../src/sys/unix/hostcalls_impl/fs.rs | 6 +- crates/wasi-common/yanix/src/dir.rs | 44 +------------ crates/wasi-common/yanix/src/file.rs | 47 +++++++++++--- 8 files changed, 75 insertions(+), 164 deletions(-) diff --git a/crates/wasi-common/src/old/snapshot_0/sys/unix/bsd/hostcalls_impl.rs b/crates/wasi-common/src/old/snapshot_0/sys/unix/bsd/hostcalls_impl.rs index 5afab3305bad..48736ac7dfef 100644 --- a/crates/wasi-common/src/old/snapshot_0/sys/unix/bsd/hostcalls_impl.rs +++ b/crates/wasi-common/src/old/snapshot_0/sys/unix/bsd/hostcalls_impl.rs @@ -23,7 +23,7 @@ pub(crate) fn path_unlink_file(resolved: PathGet) -> Result<()> { // is created before fstatat sees it, we're racing with that change anyway // and unlinkat could have legitimately seen the directory if the race had // turned out differently. - use yanix::file::{fstatat, SFlag}; + use yanix::file::{fstatat, FileType}; if errno == Errno::EPERM { if let Ok(stat) = unsafe { @@ -33,7 +33,7 @@ pub(crate) fn path_unlink_file(resolved: PathGet) -> Result<()> { AtFlag::SYMLINK_NOFOLLOW, ) } { - if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::IFDIR) { + if FileType::from_stat_st_mode(stat.st_mode) == FileType::Directory { errno = Errno::EISDIR; } } else { diff --git a/crates/wasi-common/src/old/snapshot_0/sys/unix/host_impl.rs b/crates/wasi-common/src/old/snapshot_0/sys/unix/host_impl.rs index 1233478ab539..63f92d5ce315 100644 --- a/crates/wasi-common/src/old/snapshot_0/sys/unix/host_impl.rs +++ b/crates/wasi-common/src/old/snapshot_0/sys/unix/host_impl.rs @@ -8,10 +8,7 @@ use crate::old::snapshot_0::{ }; use std::ffi::OsStr; use std::os::unix::prelude::OsStrExt; -use yanix::{ - file::{OFlag, SFlag}, - Errno, -}; +use yanix::{file::OFlag, Errno}; pub(crate) use sys_impl::host_impl::*; @@ -159,24 +156,6 @@ pub(crate) fn nix_from_oflags(oflags: wasi::__wasi_oflags_t) -> OFlag { nix_flags } -pub(crate) fn filetype_from_nix(sflags: SFlag) -> FileType { - if sflags.contains(SFlag::IFCHR) { - FileType::CharacterDevice - } else if sflags.contains(SFlag::IFBLK) { - FileType::BlockDevice - } else if sflags.contains(SFlag::IFSOCK) { - FileType::SocketStream - } else if sflags.contains(SFlag::IFDIR) { - FileType::Directory - } else if sflags.contains(SFlag::IFREG) { - FileType::RegularFile - } else if sflags.contains(SFlag::IFLNK) { - FileType::Symlink - } else { - FileType::Unknown - } -} - pub(crate) fn filestat_from_nix(filestat: libc::stat) -> Result { use std::convert::TryInto; @@ -186,7 +165,7 @@ pub(crate) fn filestat_from_nix(filestat: libc::stat) -> Result Result Result { - match host_entry.d_type { - libc::DT_FIFO => Ok(wasi::__WASI_FILETYPE_UNKNOWN), - libc::DT_CHR => Ok(wasi::__WASI_FILETYPE_CHARACTER_DEVICE), - libc::DT_DIR => Ok(wasi::__WASI_FILETYPE_DIRECTORY), - libc::DT_BLK => Ok(wasi::__WASI_FILETYPE_BLOCK_DEVICE), - libc::DT_REG => Ok(wasi::__WASI_FILETYPE_REGULAR_FILE), - libc::DT_LNK => Ok(wasi::__WASI_FILETYPE_SYMBOLIC_LINK), - libc::DT_SOCK => { - // TODO how to discriminate between STREAM and DGRAM? - // Perhaps, we should create a more general WASI filetype - // such as __WASI_FILETYPE_SOCKET, and then it would be - // up to the client to check whether it's actually - // STREAM or DGRAM? - Ok(wasi::__WASI_FILETYPE_UNKNOWN) - } - libc::DT_UNKNOWN => Ok(wasi::__WASI_FILETYPE_UNKNOWN), - _ => Err(Error::EINVAL), - } -} - /// Creates owned WASI path from OS string. /// /// NB WASI spec requires OS string to be valid UTF-8. Otherwise, @@ -245,16 +201,22 @@ pub(crate) fn path_from_host>(s: S) -> Result { helpers::path_from_slice(s.as_ref().as_bytes()).map(String::from) } -impl From for FileType { - fn from(ft: yanix::dir::FileType) -> Self { - use yanix::dir::FileType::*; +impl From for FileType { + fn from(ft: yanix::file::FileType) -> Self { + use yanix::file::FileType::*; match ft { RegularFile => Self::RegularFile, Symlink => Self::Symlink, Directory => Self::Directory, BlockDevice => Self::BlockDevice, CharacterDevice => Self::CharacterDevice, - /* Unknown | Socket | Fifo */ _ => Self::Unknown, + /* Unknown | Socket | Fifo */ + _ => Self::Unknown, + // TODO how to discriminate between STREAM and DGRAM? + // Perhaps, we should create a more general WASI filetype + // such as __WASI_FILETYPE_SOCKET, and then it would be + // up to the client to check whether it's actually + // STREAM or DGRAM? } } } diff --git a/crates/wasi-common/src/old/snapshot_0/sys/unix/hostcalls_impl/fs.rs b/crates/wasi-common/src/old/snapshot_0/sys/unix/hostcalls_impl/fs.rs index f67320d88c9d..c8a3d5973448 100644 --- a/crates/wasi-common/src/old/snapshot_0/sys/unix/hostcalls_impl/fs.rs +++ b/crates/wasi-common/src/old/snapshot_0/sys/unix/hostcalls_impl/fs.rs @@ -89,7 +89,7 @@ pub(crate) fn path_open( fs_flags: wasi::__wasi_fdflags_t, ) -> Result { use yanix::{ - file::{fstatat, openat, AtFlag, Mode, OFlag, SFlag}, + file::{fstatat, openat, AtFlag, FileType, Mode, OFlag}, Errno, }; @@ -138,7 +138,7 @@ pub(crate) fn path_open( AtFlag::SYMLINK_NOFOLLOW, ) } { - if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::IFSOCK) { + if FileType::from_stat_st_mode(stat.st_mode) == FileType::Socket { return Err(Error::ENOTSUP); } else { return Err(Error::ENXIO); @@ -159,7 +159,7 @@ pub(crate) fn path_open( AtFlag::SYMLINK_NOFOLLOW, ) } { - if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::IFLNK) { + if FileType::from_stat_st_mode(stat.st_mode) == FileType::Symlink { return Err(Error::ELOOP); } } diff --git a/crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs b/crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs index e03cbb8a86cf..b939e8257ac5 100644 --- a/crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs +++ b/crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs @@ -23,7 +23,7 @@ pub(crate) fn path_unlink_file(resolved: PathGet) -> Result<()> { // is created before fstatat sees it, we're racing with that change anyway // and unlinkat could have legitimately seen the directory if the race had // turned out differently. - use yanix::file::{fstatat, SFlag}; + use yanix::file::{fstatat, FileType}; if errno == Errno::EPERM { if let Ok(stat) = unsafe { @@ -33,7 +33,7 @@ pub(crate) fn path_unlink_file(resolved: PathGet) -> Result<()> { AtFlag::SYMLINK_NOFOLLOW, ) } { - if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::IFDIR) { + if FileType::from_stat_st_mode(stat.st_mode) == FileType::Directory { errno = Errno::EISDIR; } } else { diff --git a/crates/wasi-common/src/sys/unix/host_impl.rs b/crates/wasi-common/src/sys/unix/host_impl.rs index 8aecef3083a5..f06727fc5f4e 100644 --- a/crates/wasi-common/src/sys/unix/host_impl.rs +++ b/crates/wasi-common/src/sys/unix/host_impl.rs @@ -6,10 +6,7 @@ use crate::host::FileType; use crate::{error::FromRawOsError, helpers, sys::unix::sys_impl, wasi, Error, Result}; use std::ffi::OsStr; use std::os::unix::prelude::OsStrExt; -use yanix::{ - file::{OFlag, SFlag}, - Errno, -}; +use yanix::{file::OFlag, Errno}; pub(crate) use sys_impl::host_impl::*; @@ -157,24 +154,6 @@ pub(crate) fn nix_from_oflags(oflags: wasi::__wasi_oflags_t) -> OFlag { nix_flags } -pub(crate) fn filetype_from_nix(sflags: SFlag) -> FileType { - if sflags.contains(SFlag::IFCHR) { - FileType::CharacterDevice - } else if sflags.contains(SFlag::IFBLK) { - FileType::BlockDevice - } else if sflags.contains(SFlag::IFSOCK) { - FileType::SocketStream - } else if sflags.contains(SFlag::IFDIR) { - FileType::Directory - } else if sflags.contains(SFlag::IFREG) { - FileType::RegularFile - } else if sflags.contains(SFlag::IFLNK) { - FileType::Symlink - } else { - FileType::Unknown - } -} - pub(crate) fn filestat_from_nix(filestat: libc::stat) -> Result { use std::convert::TryInto; @@ -184,7 +163,7 @@ pub(crate) fn filestat_from_nix(filestat: libc::stat) -> Result Result Result { - match host_entry.d_type { - libc::DT_FIFO => Ok(wasi::__WASI_FILETYPE_UNKNOWN), - libc::DT_CHR => Ok(wasi::__WASI_FILETYPE_CHARACTER_DEVICE), - libc::DT_DIR => Ok(wasi::__WASI_FILETYPE_DIRECTORY), - libc::DT_BLK => Ok(wasi::__WASI_FILETYPE_BLOCK_DEVICE), - libc::DT_REG => Ok(wasi::__WASI_FILETYPE_REGULAR_FILE), - libc::DT_LNK => Ok(wasi::__WASI_FILETYPE_SYMBOLIC_LINK), - libc::DT_SOCK => { - // TODO how to discriminate between STREAM and DGRAM? - // Perhaps, we should create a more general WASI filetype - // such as __WASI_FILETYPE_SOCKET, and then it would be - // up to the client to check whether it's actually - // STREAM or DGRAM? - Ok(wasi::__WASI_FILETYPE_UNKNOWN) - } - libc::DT_UNKNOWN => Ok(wasi::__WASI_FILETYPE_UNKNOWN), - _ => Err(Error::EINVAL), - } -} - /// Creates owned WASI path from OS string. /// /// NB WASI spec requires OS string to be valid UTF-8. Otherwise, @@ -243,16 +199,22 @@ pub(crate) fn path_from_host>(s: S) -> Result { helpers::path_from_slice(s.as_ref().as_bytes()).map(String::from) } -impl From for FileType { - fn from(ft: yanix::dir::FileType) -> Self { - use yanix::dir::FileType::*; +impl From for FileType { + fn from(ft: yanix::file::FileType) -> Self { + use yanix::file::FileType::*; match ft { RegularFile => Self::RegularFile, Symlink => Self::Symlink, Directory => Self::Directory, BlockDevice => Self::BlockDevice, CharacterDevice => Self::CharacterDevice, - /* Unknown | Socket | Fifo */ _ => Self::Unknown, + /* Unknown | Socket | Fifo */ + _ => Self::Unknown, + // TODO how to discriminate between STREAM and DGRAM? + // Perhaps, we should create a more general WASI filetype + // such as __WASI_FILETYPE_SOCKET, and then it would be + // up to the client to check whether it's actually + // STREAM or DGRAM? } } } diff --git a/crates/wasi-common/src/sys/unix/hostcalls_impl/fs.rs b/crates/wasi-common/src/sys/unix/hostcalls_impl/fs.rs index 1ac926cb1f63..a4f1fe9dd088 100644 --- a/crates/wasi-common/src/sys/unix/hostcalls_impl/fs.rs +++ b/crates/wasi-common/src/sys/unix/hostcalls_impl/fs.rs @@ -94,7 +94,7 @@ pub(crate) fn path_open( fs_flags: wasi::__wasi_fdflags_t, ) -> Result { use yanix::{ - file::{fstatat, openat, AtFlag, Mode, OFlag, SFlag}, + file::{fstatat, openat, AtFlag, FileType, Mode, OFlag}, Errno, }; @@ -143,7 +143,7 @@ pub(crate) fn path_open( AtFlag::SYMLINK_NOFOLLOW, ) } { - if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::IFSOCK) { + if FileType::from_stat_st_mode(stat.st_mode) == FileType::Socket { return Err(Error::ENOTSUP); } else { return Err(Error::ENXIO); @@ -164,7 +164,7 @@ pub(crate) fn path_open( AtFlag::SYMLINK_NOFOLLOW, ) } { - if SFlag::from_bits_truncate(stat.st_mode).contains(SFlag::IFLNK) { + if FileType::from_stat_st_mode(stat.st_mode) == FileType::Symlink { return Err(Error::ELOOP); } } diff --git a/crates/wasi-common/yanix/src/dir.rs b/crates/wasi-common/yanix/src/dir.rs index 00f41ab9081c..fc13afc02957 100644 --- a/crates/wasi-common/yanix/src/dir.rs +++ b/crates/wasi-common/yanix/src/dir.rs @@ -1,4 +1,5 @@ use crate::{ + file::FileType, sys::dir::{iter_impl, EntryImpl}, Errno, Result, }; @@ -84,7 +85,7 @@ impl Entry { /// Returns the type of this directory entry. pub fn file_type(&self) -> FileType { - FileType::from_raw(self.0.d_type) + FileType::from_dirent_d_type(self.0.d_type) } } @@ -99,47 +100,6 @@ impl SeekLoc { } } -#[derive(Clone, Copy, Debug)] -#[repr(u8)] -pub enum FileType { - CharacterDevice = libc::DT_CHR, - Directory = libc::DT_DIR, - BlockDevice = libc::DT_BLK, - RegularFile = libc::DT_REG, - Symlink = libc::DT_LNK, - Fifo = libc::DT_FIFO, - Socket = libc::DT_SOCK, - Unknown = libc::DT_UNKNOWN, -} - -impl FileType { - pub fn from_raw(file_type: u8) -> Self { - match file_type { - libc::DT_CHR => Self::CharacterDevice, - libc::DT_DIR => Self::Directory, - libc::DT_BLK => Self::BlockDevice, - libc::DT_REG => Self::RegularFile, - libc::DT_LNK => Self::Symlink, - libc::DT_SOCK => Self::Socket, - libc::DT_FIFO => Self::Fifo, - /* libc::DT_UNKNOWN */ _ => Self::Unknown, - } - } - - pub fn to_raw(&self) -> u8 { - match self { - Self::CharacterDevice => libc::DT_CHR, - Self::Directory => libc::DT_DIR, - Self::BlockDevice => libc::DT_BLK, - Self::RegularFile => libc::DT_REG, - Self::Symlink => libc::DT_LNK, - Self::Socket => libc::DT_SOCK, - Self::Fifo => libc::DT_FIFO, - Self::Unknown => libc::DT_UNKNOWN, - } - } -} - #[derive(Debug)] pub struct DirIter>(T); diff --git a/crates/wasi-common/yanix/src/file.rs b/crates/wasi-common/yanix/src/file.rs index ee24d0070994..54524bc23452 100644 --- a/crates/wasi-common/yanix/src/file.rs +++ b/crates/wasi-common/yanix/src/file.rs @@ -90,16 +90,43 @@ bitflags! { } } -bitflags! { - pub struct SFlag: libc::mode_t { - const IFIFO = libc::S_IFIFO; - const IFCHR = libc::S_IFCHR; - const IFDIR = libc::S_IFDIR; - const IFBLK = libc::S_IFBLK; - const IFREG = libc::S_IFREG; - const IFLNK = libc::S_IFLNK; - const IFSOCK = libc::S_IFSOCK; - const IFMT = libc::S_IFMT; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum FileType { + CharacterDevice, + Directory, + BlockDevice, + RegularFile, + Symlink, + Fifo, + Socket, + Unknown, +} + +impl FileType { + pub fn from_stat_st_mode(st_mode: libc::mode_t) -> Self { + match st_mode & libc::S_IFMT { + libc::S_IFIFO => Self::Fifo, + libc::S_IFCHR => Self::CharacterDevice, + libc::S_IFDIR => Self::Directory, + libc::S_IFBLK => Self::BlockDevice, + libc::S_IFREG => Self::RegularFile, + libc::S_IFLNK => Self::Symlink, + libc::S_IFSOCK => Self::Socket, + _ => Self::Unknown, // Should we actually panic here since this one *should* never happen? + } + } + + pub fn from_dirent_d_type(d_type: u8) -> Self { + match d_type { + libc::DT_CHR => Self::CharacterDevice, + libc::DT_DIR => Self::Directory, + libc::DT_BLK => Self::BlockDevice, + libc::DT_REG => Self::RegularFile, + libc::DT_LNK => Self::Symlink, + libc::DT_SOCK => Self::Socket, + libc::DT_FIFO => Self::Fifo, + /* libc::DT_UNKNOWN */ _ => Self::Unknown, + } } }