From 78c091f7f8b75db7ebc1968607a6f071a6f732b3 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Fri, 7 Jul 2023 23:11:37 +0300 Subject: [PATCH] Kernel: Move open file descriptors to their own class This simplifies code a lot :) --- kernel/CMakeLists.txt | 1 + kernel/include/kernel/OpenFileDescriptorSet.h | 72 +++++ kernel/include/kernel/Process.h | 19 +- kernel/kernel/OpenFileDescriptorSet.cpp | 245 ++++++++++++++++ kernel/kernel/Process.cpp | 267 ++---------------- 5 files changed, 343 insertions(+), 261 deletions(-) create mode 100644 kernel/include/kernel/OpenFileDescriptorSet.h create mode 100644 kernel/kernel/OpenFileDescriptorSet.cpp diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index b76892c4..475d3f41 100644 --- a/kernel/CMakeLists.txt +++ b/kernel/CMakeLists.txt @@ -33,6 +33,7 @@ set(KERNEL_SOURCES kernel/Memory/kmalloc.cpp kernel/Memory/PhysicalRange.cpp kernel/Memory/VirtualRange.cpp + kernel/OpenFileDescriptorSet.cpp kernel/Panic.cpp kernel/PCI.cpp kernel/PIC.cpp diff --git a/kernel/include/kernel/OpenFileDescriptorSet.h b/kernel/include/kernel/OpenFileDescriptorSet.h new file mode 100644 index 00000000..a5fa5067 --- /dev/null +++ b/kernel/include/kernel/OpenFileDescriptorSet.h @@ -0,0 +1,72 @@ +#pragma once + +#include +#include + +#include +#include + +namespace Kernel +{ + + class OpenFileDescriptorSet + { + BAN_NON_COPYABLE(OpenFileDescriptorSet); + + public: + OpenFileDescriptorSet(const Credentials&); + ~OpenFileDescriptorSet(); + + BAN::ErrorOr clone_from(const OpenFileDescriptorSet&); + + BAN::ErrorOr open(BAN::StringView absolute_path, int flags); + + BAN::ErrorOr pipe(int fds[2]); + + BAN::ErrorOr dup2(int, int); + + BAN::ErrorOr seek(int fd, off_t offset, int whence); + BAN::ErrorOr tell(int) const; + + BAN::ErrorOr fstat(int fd, struct stat*) const; + + BAN::ErrorOr close(int); + void close_all(); + void close_cloexec(); + + BAN::ErrorOr read(int fd, void* buffer, size_t count); + BAN::ErrorOr write(int fd, const void* buffer, size_t count); + + BAN::ErrorOr read_dir_entries(int fd, DirectoryEntryList* list, size_t list_size); + + BAN::ErrorOr path_of(int) const; + + private: + struct OpenFileDescription : public BAN::RefCounted + { + OpenFileDescription(BAN::RefPtr inode, BAN::String path, off_t offset, uint8_t flags) + : inode(inode) + , path(BAN::move(path)) + , offset(offset) + , flags(flags) + { } + + BAN::RefPtr inode; + BAN::String path; + off_t offset { 0 }; + uint8_t flags { 0 }; + + friend class BAN::RefPtr; + }; + + BAN::ErrorOr validate_fd(int) const; + BAN::ErrorOr get_free_fd() const; + BAN::ErrorOr get_free_fd_pair(int fds[2]) const; + + private: + const Credentials& m_credentials; + + BAN::Array, OPEN_MAX> m_open_files; + }; + +} \ No newline at end of file diff --git a/kernel/include/kernel/Process.h b/kernel/include/kernel/Process.h index 738d78de..32ffa67e 100644 --- a/kernel/include/kernel/Process.h +++ b/kernel/include/kernel/Process.h @@ -9,11 +9,11 @@ #include #include #include +#include #include #include #include -#include #include namespace LibELF { class ELF; } @@ -128,20 +128,6 @@ namespace Kernel BAN::ErrorOr absolute_path_of(BAN::StringView) const; private: - struct OpenFileDescription - { - BAN::RefPtr inode; - BAN::String path; - off_t offset { 0 }; - uint8_t flags { 0 }; - }; - - BAN::ErrorOr validate_fd(int); - OpenFileDescription& open_file_description(int); - BAN::ErrorOr get_free_fd(); - BAN::ErrorOr get_free_fd_pair(int fds[2]); - - struct ExitStatus { Semaphore semaphore; @@ -152,7 +138,8 @@ namespace Kernel Credentials m_credentials; - BAN::Vector m_open_files; + OpenFileDescriptorSet m_open_file_descriptors; + BAN::Vector m_mapped_ranges; mutable RecursiveSpinLock m_lock; diff --git a/kernel/kernel/OpenFileDescriptorSet.cpp b/kernel/kernel/OpenFileDescriptorSet.cpp new file mode 100644 index 00000000..e99bd366 --- /dev/null +++ b/kernel/kernel/OpenFileDescriptorSet.cpp @@ -0,0 +1,245 @@ +#include +#include +#include + +#include + +namespace Kernel +{ + + + OpenFileDescriptorSet::OpenFileDescriptorSet(const Credentials& credentials) + : m_credentials(credentials) + { + + } + + OpenFileDescriptorSet::~OpenFileDescriptorSet() + { + close_all(); + } + + BAN::ErrorOr OpenFileDescriptorSet::clone_from(const OpenFileDescriptorSet& other) + { + close_all(); + + for (int fd = 0; fd < (int)other.m_open_files.size(); fd++) + { + if (other.validate_fd(fd).is_error()) + continue; + + auto& open_file = other.m_open_files[fd]; + + auto result = BAN::RefPtr::create(open_file->inode, open_file->path, open_file->offset, open_file->flags); + + if (result.is_error()) + { + close_all(); + return result.error(); + } + + m_open_files[fd] = result.release_value(); + + if (m_open_files[fd]->flags & O_WRONLY && m_open_files[fd]->inode->is_pipe()) + ((Pipe*)m_open_files[fd]->inode.ptr())->clone_writing(); + } + + return {}; + } + + BAN::ErrorOr OpenFileDescriptorSet::open(BAN::StringView absolute_path, int flags) + { + if (flags & ~(O_RDONLY | O_WRONLY | O_NOFOLLOW | O_SEARCH | O_CLOEXEC)) + return BAN::Error::from_errno(ENOTSUP); + + auto file = TRY(VirtualFileSystem::get().file_from_absolute_path(m_credentials, absolute_path, flags)); + + int fd = TRY(get_free_fd()); + m_open_files[fd] = TRY(BAN::RefPtr::create(file.inode, BAN::move(file.canonical_path), 0, flags)); + + return fd; + } + + BAN::ErrorOr OpenFileDescriptorSet::pipe(int fds[2]) + { + TRY(get_free_fd_pair(fds)); + + auto pipe = TRY(Pipe::create(m_credentials)); + m_open_files[fds[0]] = TRY(BAN::RefPtr::create(pipe, ""sv, 0, O_RDONLY)); + m_open_files[fds[1]] = TRY(BAN::RefPtr::create(pipe, ""sv, 0, O_WRONLY)); + + return {}; + } + + BAN::ErrorOr OpenFileDescriptorSet::dup2(int fildes, int fildes2) + { + if (fildes2 < 0 || fildes2 >= (int)m_open_files.size()) + return BAN::Error::from_errno(EBADF); + + TRY(validate_fd(fildes)); + if (fildes == fildes2) + return fildes; + + (void)close(fildes2); + + m_open_files[fildes2] = m_open_files[fildes]; + m_open_files[fildes2]->flags &= ~O_CLOEXEC; + + if (m_open_files[fildes]->flags & O_WRONLY && m_open_files[fildes]->inode->is_pipe()) + ((Pipe*)m_open_files[fildes]->inode.ptr())->clone_writing(); + + return fildes; + } + + BAN::ErrorOr OpenFileDescriptorSet::seek(int fd, off_t offset, int whence) + { + TRY(validate_fd(fd)); + + off_t new_offset = 0; + + switch (whence) + { + case SEEK_CUR: + new_offset = m_open_files[fd]->offset + offset; + break; + case SEEK_END: + new_offset = m_open_files[fd]->inode->size() - offset; + break; + case SEEK_SET: + new_offset = offset; + break; + default: + return BAN::Error::from_errno(EINVAL); + } + + if (new_offset < 0) + return BAN::Error::from_errno(EINVAL); + + m_open_files[fd]->offset = new_offset; + + return {}; + } + + BAN::ErrorOr OpenFileDescriptorSet::tell(int fd) const + { + TRY(validate_fd(fd)); + return m_open_files[fd]->offset; + } + + BAN::ErrorOr OpenFileDescriptorSet::fstat(int fd, struct stat* out) const + { + TRY(validate_fd(fd)); + + auto inode = m_open_files[fd]->inode; + out->st_dev = inode->dev(); + out->st_ino = inode->ino(); + out->st_mode = inode->mode().mode; + out->st_nlink = inode->nlink(); + out->st_uid = inode->uid(); + out->st_gid = inode->gid(); + out->st_rdev = inode->rdev(); + out->st_size = inode->size(); + out->st_atim = inode->atime(); + out->st_mtim = inode->mtime(); + out->st_ctim = inode->ctime(); + out->st_blksize = inode->blksize(); + out->st_blocks = inode->blocks(); + + return {}; + } + + BAN::ErrorOr OpenFileDescriptorSet::close(int fd) + { + TRY(validate_fd(fd)); + + if (m_open_files[fd]->flags & O_WRONLY && m_open_files[fd]->inode->is_pipe()) + ((Pipe*)m_open_files[fd]->inode.ptr())->close_writing(); + + m_open_files[fd].clear(); + + return {}; + } + + void OpenFileDescriptorSet::close_all() + { + for (int fd = 0; fd < (int)m_open_files.size(); fd++) + (void)close(fd); + } + + void OpenFileDescriptorSet::close_cloexec() + { + for (int fd = 0; fd < (int)m_open_files.size(); fd++) + { + if (validate_fd(fd).is_error()) + continue; + if (m_open_files[fd]->flags & O_CLOEXEC) + (void)close(fd); + } + } + + BAN::ErrorOr OpenFileDescriptorSet::read(int fd, void* buffer, size_t count) + { + TRY(validate_fd(fd)); + auto& open_file = m_open_files[fd]; + size_t nread = TRY(open_file->inode->read(open_file->offset, buffer, count)); + open_file->offset += nread; + return nread; + } + + BAN::ErrorOr OpenFileDescriptorSet::write(int fd, const void* buffer, size_t count) + { + TRY(validate_fd(fd)); + auto& open_file = m_open_files[fd]; + size_t nwrite = TRY(open_file->inode->write(open_file->offset, buffer, count)); + open_file->offset += nwrite; + return nwrite; + } + + BAN::ErrorOr OpenFileDescriptorSet::read_dir_entries(int fd, DirectoryEntryList* list, size_t list_size) + { + TRY(validate_fd(fd)); + auto& open_file = m_open_files[fd]; + TRY(open_file->inode->read_next_directory_entries(open_file->offset, list, list_size)); + open_file->offset++; + return {}; + } + + + BAN::ErrorOr OpenFileDescriptorSet::path_of(int fd) const + { + TRY(validate_fd(fd)); + return m_open_files[fd]->path.sv(); + } + + + BAN::ErrorOr OpenFileDescriptorSet::validate_fd(int fd) const + { + if (fd < 0 || fd >= (int)m_open_files.size()) + return BAN::Error::from_errno(EBADF); + if (!m_open_files[fd]) + return BAN::Error::from_errno(EBADF); + return {}; + } + + BAN::ErrorOr OpenFileDescriptorSet::get_free_fd() const + { + for (int fd = 0; fd < (int)m_open_files.size(); fd++) + if (!m_open_files[fd]) + return fd; + return BAN::Error::from_errno(EMFILE); + } + + BAN::ErrorOr OpenFileDescriptorSet::get_free_fd_pair(int fds[2]) const + { + size_t found = 0; + for (int fd = 0; fd < (int)m_open_files.size(); fd++) + { + if (!m_open_files[fd]) + fds[found++] = fd; + if (found == 2) + return {}; + } + return BAN::Error::from_errno(EMFILE); + } + +} \ No newline at end of file diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index cca4e63b..7b1033b9 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -1,6 +1,5 @@ #include #include -#include #include #include #include @@ -92,6 +91,7 @@ namespace Kernel Process::Process(const Credentials& credentials, pid_t pid) : m_credentials(credentials) + , m_open_file_descriptors(m_credentials) , m_pid(pid) , m_tty(TTY::current()) { } @@ -135,13 +135,7 @@ namespace Kernel } m_threads.clear(); - - for (int fd = 0; fd < (int)m_open_files.size(); fd++) - { - if (validate_fd(fd).is_error()) - continue; - (void)sys_close(fd); - } + m_open_file_descriptors.close_all(); // NOTE: We must unmap ranges while the page table is still alive for (auto* range : m_mapped_ranges) @@ -282,15 +276,7 @@ namespace Kernel forked->m_tty = m_tty; forked->m_working_directory = m_working_directory; - forked->m_open_files = m_open_files; - for (int fd = 0; fd < (int)m_open_files.size(); fd++) - { - if (validate_fd(fd).is_error()) - continue; - auto& openfd = open_file_description(fd); - if (openfd.flags & O_WRONLY && openfd.inode->is_pipe()) - ((Pipe*)openfd.inode.ptr())->clone_writing(); - } + MUST(forked->m_open_file_descriptors.clone_from(m_open_file_descriptors)); forked->m_userspace_info = m_userspace_info; @@ -339,14 +325,7 @@ namespace Kernel LockGuard lock_guard(m_lock); - for (int fd = 0; fd < (int)m_open_files.size(); fd++) - { - if (validate_fd(fd).is_error()) - continue; - auto& file = open_file_description(fd); - if (file.flags & O_CLOEXEC) - MUST(sys_close(fd)); - } + m_open_file_descriptors.close_cloexec(); m_fixed_width_allocators.clear(); m_general_allocator.clear(); @@ -513,34 +492,17 @@ namespace Kernel BAN::ErrorOr Process::sys_open(BAN::StringView path, int flags) { - if (flags & ~(O_RDONLY | O_WRONLY | O_NOFOLLOW | O_SEARCH | O_CLOEXEC)) - return BAN::Error::from_errno(ENOTSUP); - - BAN::String absolute_path = TRY(absolute_path_of(path)); - - auto file = TRY(VirtualFileSystem::get().file_from_absolute_path(m_credentials, absolute_path, flags)); - LockGuard _(m_lock); - int fd = TRY(get_free_fd()); - auto& open_file_description = m_open_files[fd]; - open_file_description.inode = file.inode; - open_file_description.path = BAN::move(file.canonical_path); - open_file_description.offset = 0; - open_file_description.flags = flags; - - return fd; + BAN::String absolute_path = TRY(absolute_path_of(path)); + return TRY(m_open_file_descriptors.open(absolute_path, flags)); } BAN::ErrorOr Process::sys_openat(int fd, BAN::StringView path, int flags) { + LockGuard _(m_lock); + BAN::String absolute_path; - - { - LockGuard _(m_lock); - TRY(validate_fd(fd)); - TRY(absolute_path.append(open_file_description(fd).path)); - } - + TRY(absolute_path.append(TRY(m_open_file_descriptors.path_of(fd)))); TRY(absolute_path.push_back('/')); TRY(absolute_path.append(path)); @@ -550,149 +512,46 @@ namespace Kernel BAN::ErrorOr Process::sys_close(int fd) { LockGuard _(m_lock); - TRY(validate_fd(fd)); - auto& open_file_description = this->open_file_description(fd); - if (open_file_description.flags & O_WRONLY && open_file_description.inode->is_pipe()) - ((Pipe*)open_file_description.inode.ptr())->close_writing(); - open_file_description.inode = nullptr; + TRY(m_open_file_descriptors.close(fd)); return 0; } BAN::ErrorOr Process::sys_read(int fd, void* buffer, size_t count) { - OpenFileDescription open_fd_copy; - - { - LockGuard _(m_lock); - TRY(validate_fd(fd)); - open_fd_copy = open_file_description(fd); - } - - if (!(open_fd_copy.flags & O_RDONLY)) - return BAN::Error::from_errno(EBADF); - - size_t nread = TRY(open_fd_copy.inode->read(open_fd_copy.offset, buffer, count)); - - { - LockGuard _(m_lock); - MUST(validate_fd(fd)); - open_file_description(fd).offset += nread; - } - - return nread; + LockGuard _(m_lock); + return TRY(m_open_file_descriptors.read(fd, buffer, count)); } BAN::ErrorOr Process::sys_write(int fd, const void* buffer, size_t count) { - OpenFileDescription open_fd_copy; - - { - LockGuard _(m_lock); - TRY(validate_fd(fd)); - open_fd_copy = open_file_description(fd); - } - - if (!(open_fd_copy.flags & O_WRONLY)) - return BAN::Error::from_errno(EBADF); - - size_t nwrite = TRY(open_fd_copy.inode->write(open_fd_copy.offset, buffer, count)); - - { - LockGuard _(m_lock); - MUST(validate_fd(fd)); - open_file_description(fd).offset += nwrite; - } - - return nwrite; + LockGuard _(m_lock); + return TRY(m_open_file_descriptors.write(fd, buffer, count)); } BAN::ErrorOr Process::sys_pipe(int fildes[2]) { LockGuard _(m_lock); - - auto pipe = TRY(Pipe::create(m_credentials)); - - TRY(get_free_fd_pair(fildes)); - - auto& openfd_read = m_open_files[fildes[0]]; - openfd_read.inode = pipe; - openfd_read.flags = O_RDONLY; - openfd_read.offset = 0; - openfd_read.path.clear(); - - auto& openfd_write = m_open_files[fildes[1]]; - openfd_write.inode = pipe; - openfd_write.flags = O_WRONLY; - openfd_write.offset = 0; - openfd_write.path.clear(); - + TRY(m_open_file_descriptors.pipe(fildes)); return 0; } BAN::ErrorOr Process::sys_dup2(int fildes, int fildes2) { - // FIXME: m_open_files should be BAN::Array, OPEN_MAX> for accurate dup2 - - if (fildes2 < 0 || fildes2 >= 100) - return BAN::Error::from_errno(EBADF); - LockGuard _(m_lock); - - TRY(validate_fd(fildes)); - if (fildes == fildes2) - return fildes; - - if (m_open_files.size() <= (size_t)fildes2) - TRY(m_open_files.resize(fildes2 + 1)); - - if (!validate_fd(fildes2).is_error()) - TRY(sys_close(fildes2)); - - m_open_files[fildes2] = m_open_files[fildes]; - m_open_files[fildes2].flags &= ~O_CLOEXEC; - - if (m_open_files[fildes].flags & O_WRONLY && m_open_files[fildes].inode->is_pipe()) - ((Pipe*)m_open_files[fildes].inode.ptr())->clone_writing(); - - return fildes; + return TRY(m_open_file_descriptors.dup2(fildes, fildes2)); } BAN::ErrorOr Process::sys_seek(int fd, off_t offset, int whence) { LockGuard _(m_lock); - TRY(validate_fd(fd)); - - auto& open_fd = open_file_description(fd); - - off_t new_offset = 0; - - switch (whence) - { - case SEEK_CUR: - new_offset = open_fd.offset + offset; - break; - case SEEK_END: - new_offset = open_fd.inode->size() - offset; - break; - case SEEK_SET: - new_offset = offset; - break; - default: - return BAN::Error::from_errno(EINVAL); - } - - if (new_offset < 0) - return BAN::Error::from_errno(EINVAL); - open_fd.offset = new_offset; - + TRY(m_open_file_descriptors.seek(fd, offset, whence)); return 0; } BAN::ErrorOr Process::sys_tell(int fd) { LockGuard _(m_lock); - TRY(validate_fd(fd)); - return open_file_description(fd).offset; + return TRY(m_open_file_descriptors.tell(fd)); } BAN::ErrorOr Process::sys_creat(BAN::StringView path, mode_t mode) @@ -727,28 +586,8 @@ namespace Kernel BAN::ErrorOr Process::sys_fstat(int fd, struct stat* out) { - OpenFileDescription open_fd_copy; - - { - LockGuard _(m_lock); - TRY(validate_fd(fd)); - open_fd_copy = open_file_description(fd); - } - - out->st_dev = open_fd_copy.inode->dev(); - out->st_ino = open_fd_copy.inode->ino(); - out->st_mode = open_fd_copy.inode->mode().mode; - out->st_nlink = open_fd_copy.inode->nlink(); - out->st_uid = open_fd_copy.inode->uid(); - out->st_gid = open_fd_copy.inode->gid(); - out->st_rdev = open_fd_copy.inode->rdev(); - out->st_size = open_fd_copy.inode->size(); - out->st_atim = open_fd_copy.inode->atime(); - out->st_mtim = open_fd_copy.inode->mtime(); - out->st_ctim = open_fd_copy.inode->ctime(); - out->st_blksize = open_fd_copy.inode->blksize(); - out->st_blocks = open_fd_copy.inode->blocks(); - + LockGuard _(m_lock); + TRY(m_open_file_descriptors.fstat(fd, out)); return 0; } @@ -762,22 +601,8 @@ namespace Kernel BAN::ErrorOr Process::sys_read_dir_entries(int fd, DirectoryEntryList* list, size_t list_size) { - OpenFileDescription open_fd_copy; - - { - LockGuard _(m_lock); - TRY(validate_fd(fd)); - open_fd_copy = open_file_description(fd); - } - - TRY(open_fd_copy.inode->read_next_directory_entries(open_fd_copy.offset, list, list_size)); - - { - LockGuard _(m_lock); - MUST(validate_fd(fd)); - open_file_description(fd).offset = open_fd_copy.offset + 1; - } - + LockGuard _(m_lock); + TRY(m_open_file_descriptors.read_dir_entries(fd, list, list_size)); return 0; } @@ -1131,52 +956,4 @@ namespace Kernel return absolute_path; } - BAN::ErrorOr Process::validate_fd(int fd) - { - ASSERT(m_lock.is_locked()); - if (fd < 0 || m_open_files.size() <= (size_t)fd || !m_open_files[fd].inode) - return BAN::Error::from_errno(EBADF); - return {}; - } - - Process::OpenFileDescription& Process::open_file_description(int fd) - { - ASSERT(m_lock.is_locked()); - MUST(validate_fd(fd)); - return m_open_files[fd]; - } - - BAN::ErrorOr Process::get_free_fd() - { - ASSERT(m_lock.is_locked()); - for (size_t fd = 0; fd < m_open_files.size(); fd++) - if (!m_open_files[fd].inode) - return fd; - TRY(m_open_files.push_back({})); - return m_open_files.size() - 1; - } - - BAN::ErrorOr Process::get_free_fd_pair(int fds[2]) - { - ASSERT(m_lock.is_locked()); - int found = 0; - for (size_t fd = 0; fd < m_open_files.size(); fd++) - { - if (!m_open_files[fd].inode) - { - fds[found++] = fd; - if (found >= 2) - return {}; - } - } - - while (found < 2) - { - TRY(m_open_files.push_back({})); - fds[found++] = m_open_files.size() - 1; - } - - return {}; - } - } \ No newline at end of file