made readdir stateful
#1738
made readdir stateful
#1738
Conversation
jounathaen
force-pushed
the
readdir
branch
2 times, most recently
from
0d0a2d1 to
691484e
Compare
|
|
||
| #[hermit_macro::system] | ||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn sys_getdents64( | ||
| fd: FileDescriptor, | ||
| dirp: *mut Dirent64, | ||
| // rename: bytes/size/length |
There was a problem hiding this comment.
I'd argue for keeping the name consistent with Linux:
ssize_t getdents64(int fd, void dirp[.count], size_t count);| /// Filename (null-terminated) | ||
| pub d_name: PhantomData<c_char>, | ||
| } | ||
| impl core::fmt::Debug for Dirent64 { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| let cstr = unsafe { CStr::from_ptr((&raw const self.d_name).cast::<c_char>()) }; |
There was a problem hiding this comment.
This is not sound, since the API allows creating the struct without a filename. This would then read out of bounds.
| #[repr(u8)] | ||
| #[derive(Debug, Clone, Copy)] | ||
| // Same numbers as a normal Linux (no guarantee) | ||
| pub enum FileType { | ||
| DtUnknown = 0, | ||
| DtFifo = 1, | ||
| DtChr = 2, | ||
| DtDir = 4, | ||
| DtBlk = 6, | ||
| DtReg = 8, | ||
| DtLnk = 10, | ||
| DtSock = 12, | ||
| DtWht = 14, | ||
| } | ||
| impl TryFrom<u8> for FileType { | ||
| type Error = &'static str; | ||
| fn try_from(value: u8) -> Result<Self, Self::Error> { | ||
| match value { | ||
| 0 => Ok(Self::DtUnknown), | ||
| 1 => Ok(Self::DtFifo), | ||
| 2 => Ok(Self::DtChr), | ||
| 4 => Ok(Self::DtDir), | ||
| 6 => Ok(Self::DtBlk), | ||
| 8 => Ok(Self::DtReg), | ||
| 10 => Ok(Self::DtLnk), | ||
| 12 => Ok(Self::DtSock), | ||
| 14 => Ok(Self::DtWht), | ||
| _ => Err("inv. Discriminant"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We already have these constants in fs::FileType, which is also what we use in sys/dirent.h.
| @@ -324,6 +323,7 @@ pub unsafe extern "C" fn sys_opendir(name: *const c_char) -> FileDescriptor { | |||
| if let Ok(name) = unsafe { CStr::from_ptr(name) }.to_str() { | |||
| crate::fs::opendir(name).unwrap_or_else(|e| -num::ToPrimitive::to_i32(&e).unwrap()) | |||
| } else { | |||
| error!("sys opendir invalid name"); | |||
There was a problem hiding this comment.
I am not sure if kernel logging is useful here, since this would be an application error, not a kernel error and the application should log and report errors.
Regardless, this is probably a separate commit or even PR, right?
| pub struct Dirent64 { | ||
| /// 64-bit inode number | ||
| pub d_ino: u64, | ||
| /// 64-bit offset to next structure | ||
| /// Field without meaning. Kept for BW compatibility. |
There was a problem hiding this comment.
d_off is a filesystem-specific value with no specific meaning to user space, though on older filesystems it used to be the distance from the start of the directory to the start of the next linux_dirent
I am fine with making this field meaningless, I just wanted to be conscious about this. Could there be a benefit of using absolute offsets in user space instead of iterating incrementally using d_reclen?
| if dirp.is_null() || count == 0 { | ||
| error!("dirp is null"); |
There was a problem hiding this comment.
I am not sure if this makes sense as kernel error log. It is an application error, not a kernel error.
| let _ = self.0.clone(); // Dummy instruction to avoid warning for the moment | ||
| unimplemented!("") |
There was a problem hiding this comment.
| let _ = self.0.clone(); // Dummy instruction to avoid warning for the moment | |
| unimplemented!("") | |
| let _ = &self.0; // Dummy statement to avoid warning for the moment | |
| unimplemented!() |
There was a problem hiding this comment.
@stlankes, I think we can also remove DirectoryReader completely and panic on sys_opendir, which is deprecated, right?
| nameptr, | ||
| namelen, | ||
| ); | ||
| nameptr.add(namelen).write_bytes(0, 1); // zero termination |
There was a problem hiding this comment.
| nameptr.add(namelen).write_bytes(0, 1); // zero termination | |
| nameptr.add(namelen).write(0); // zero termination |
| /// Any other offset than 0 is not supported. (Mostly because it doesn't make any sense, as | ||
| /// userspace applications have no way of knowing valid offsets) | ||
| async fn lseek(&self, offset: isize, _whence: SeekWhence) -> io::Result<isize> { |
There was a problem hiding this comment.
User space could know offsets through seeking without moving the head, which returns the current position. Not sure if it makes sense to expose that now, though. I don't really understand newlib's implementation of telldir, seekdir and rewinddir yet.
Can we check that whence is SeekWhence::Set?
| nameptr, | ||
| dirent.namelen as usize, | ||
| ); | ||
| nameptr.add(dirent.namelen as usize).write_bytes(0, 1); // zero termination |
There was a problem hiding this comment.
| nameptr.add(dirent.namelen as usize).write_bytes(0, 1); // zero termination | |
| nameptr.add(dirent.namelen as usize).write(0); // zero termination |
This fixes #1589 by saving the read state in the backends of the
sys_getdents64syscall.Currently, implemented only for RAMFS and FUSE - mostly because I couldn't find an example on how to use the ROMFS.
In general, I'm not a huge fan of this, as we basically reimplement the old POSIX interface instead of designing a better interface ourselves, but this avoids complications and breaking changes in the standard libraries (Rust and Newlib).
Notable Quirks/Issues:
1. This is unlikely to be a problem because I can't imagine an application utilizing Inodes being seriously executed in an unikernel.