diff options
author | Andreas Kling <awesomekling@gmail.com> | 2019-02-25 20:47:56 +0100 |
---|---|---|
committer | Andreas Kling <awesomekling@gmail.com> | 2019-02-25 20:50:22 +0100 |
commit | 5af4e622b94a194e3feaea1b3b09229d7a93578b (patch) | |
tree | 1d814ef3b92989e464e4409dfabfed9324c00773 | |
parent | 901b7d5d91b3eacda12dad01211b84a28e1a39a6 (diff) | |
download | serenity-5af4e622b94a194e3feaea1b3b09229d7a93578b.zip |
Kernel: Add KResult and KResultOr<T> classes.
The idea here is to combine a potential syscall error code with an arbitrary
type in the case of success. I feel like this will end up much less error
prone than returning some arbitrary type that kinda sorta has bool semantics
(but sometimes not really) and passing the error through an out-param.
This patch only converts a few syscalls to using it. More to come.
-rw-r--r-- | Kernel/Ext2FileSystem.cpp | 7 | ||||
-rw-r--r-- | Kernel/Ext2FileSystem.h | 2 | ||||
-rw-r--r-- | Kernel/FileSystem.h | 3 | ||||
-rw-r--r-- | Kernel/KResult.h | 51 | ||||
-rw-r--r-- | Kernel/ProcFS.cpp | 9 | ||||
-rw-r--r-- | Kernel/ProcFS.h | 2 | ||||
-rw-r--r-- | Kernel/Process.cpp | 15 | ||||
-rw-r--r-- | Kernel/SyntheticFileSystem.cpp | 5 | ||||
-rw-r--r-- | Kernel/SyntheticFileSystem.h | 2 | ||||
-rw-r--r-- | Kernel/VirtualFileSystem.cpp | 169 | ||||
-rw-r--r-- | Kernel/VirtualFileSystem.h | 13 | ||||
-rw-r--r-- | LibC/errno_numbers.h | 2 |
12 files changed, 162 insertions, 118 deletions
diff --git a/Kernel/Ext2FileSystem.cpp b/Kernel/Ext2FileSystem.cpp index 0c41ce4f21..866974a9b8 100644 --- a/Kernel/Ext2FileSystem.cpp +++ b/Kernel/Ext2FileSystem.cpp @@ -1350,15 +1350,14 @@ size_t Ext2FSInode::directory_entry_count() const return m_lookup_cache.size(); } -bool Ext2FSInode::chmod(mode_t mode, int& error) +KResult Ext2FSInode::chmod(mode_t mode) { LOCKER(m_lock); - error = 0; if (m_raw_inode.i_mode == mode) - return true; + return KSuccess; m_raw_inode.i_mode = mode; set_metadata_dirty(true); - return true; + return KSuccess; } unsigned Ext2FS::total_block_count() const diff --git a/Kernel/Ext2FileSystem.h b/Kernel/Ext2FileSystem.h index 6f1d8421d1..1b0c8aecd9 100644 --- a/Kernel/Ext2FileSystem.h +++ b/Kernel/Ext2FileSystem.h @@ -41,7 +41,7 @@ private: virtual int increment_link_count() override; virtual int decrement_link_count() override; virtual size_t directory_entry_count() const override; - virtual bool chmod(mode_t, int& error) override; + virtual KResult chmod(mode_t) override; void populate_lookup_cache() const; diff --git a/Kernel/FileSystem.h b/Kernel/FileSystem.h index 77169cd751..b3081e3269 100644 --- a/Kernel/FileSystem.h +++ b/Kernel/FileSystem.h @@ -15,6 +15,7 @@ #include <AK/kstdio.h> #include <AK/Lock.h> #include <AK/WeakPtr.h> +#include <Kernel/KResult.h> static const dword mepoch = 476763780; @@ -96,7 +97,7 @@ public: virtual bool remove_child(const String& name, int& error) = 0; virtual RetainPtr<Inode> parent() const = 0; virtual size_t directory_entry_count() const = 0; - virtual bool chmod(mode_t, int& error) = 0; + virtual KResult chmod(mode_t) = 0; LocalSocket* socket() { return m_socket.ptr(); } const LocalSocket* socket() const { return m_socket.ptr(); } diff --git a/Kernel/KResult.h b/Kernel/KResult.h new file mode 100644 index 0000000000..01d3a38bbf --- /dev/null +++ b/Kernel/KResult.h @@ -0,0 +1,51 @@ +#pragma once + +#include <AK/Assertions.h> +#include <LibC/errno_numbers.h> + +enum KSuccessTag { KSuccess }; + +class KResult { +public: + explicit KResult(__errno_value e) : m_error(-e) { } + explicit KResult(int negative_e) : m_error(negative_e) { ASSERT(negative_e <= 0); } + KResult(KSuccessTag) : m_error(0) { } + operator int() const { return m_error; } + +private: + template<typename T> friend class KResultOr; + KResult() { } + + int m_error { 0 }; +}; + +template<typename T> +class alignas(T) KResultOr { +public: + KResultOr(KResult error) + : m_error(error) + , m_is_error(true) + { } + + KResultOr(T&& value) + { + new (&m_storage) T(move(value)); + } + + ~KResultOr() + { + if (!m_is_error) + value().~T(); + } + + bool is_error() const { return m_is_error; } + KResult error() const { ASSERT(m_is_error); return m_error; } + T& value() { ASSERT(!m_is_error); return *reinterpret_cast<T*>(&m_storage); } + const T& value() const { ASSERT(!m_is_error); return *reinterpret_cast<T*>(&m_storage); } + +private: + char m_storage[sizeof(T)] __attribute__((aligned(sizeof(T)))); + KResult m_error; + bool m_is_error { false }; +}; + diff --git a/Kernel/ProcFS.cpp b/Kernel/ProcFS.cpp index 680c46b187..0b26b30102 100644 --- a/Kernel/ProcFS.cpp +++ b/Kernel/ProcFS.cpp @@ -1049,12 +1049,16 @@ ssize_t ProcFSInode::write_bytes(off_t offset, size_t size, const byte* buffer, bool ProcFSInode::add_child(InodeIdentifier child_id, const String& name, byte file_type, int& error) { + (void)child_id; + (void)name; + (void)file_type; error = -EPERM; return false; } bool ProcFSInode::remove_child(const String& name, int& error) { + (void)name; error = -EPERM; return false; } @@ -1074,10 +1078,9 @@ size_t ProcFSInode::directory_entry_count() const return count; } -bool ProcFSInode::chmod(mode_t, int& error) +KResult ProcFSInode::chmod(mode_t) { - error = -EPERM; - return false; + return KResult(-EPERM); } ProcFS::ProcFS() diff --git a/Kernel/ProcFS.h b/Kernel/ProcFS.h index ad64feae41..f2b0e0a0e0 100644 --- a/Kernel/ProcFS.h +++ b/Kernel/ProcFS.h @@ -87,7 +87,7 @@ private: virtual bool remove_child(const String& name, int& error) override; virtual RetainPtr<Inode> parent() const override; virtual size_t directory_entry_count() const override; - virtual bool chmod(mode_t, int& error) override; + virtual KResult chmod(mode_t) override; ProcFS& fs() { return static_cast<ProcFS&>(Inode::fs()); } const ProcFS& fs() const { return static_cast<const ProcFS&>(Inode::fs()); } diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index bc0c69d977..11d1acd279 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -1136,10 +1136,7 @@ int Process::sys$utime(const char* pathname, const utimbuf* buf) mtime = now; atime = now; } - int error; - if (!VFS::the().utime(String(pathname), error, cwd_inode(), atime, mtime)) - return error; - return 0; + return VFS::the().utime(String(pathname), cwd_inode(), atime, mtime); } int Process::sys$access(const char* pathname, int mode) @@ -1931,10 +1928,7 @@ int Process::sys$mkdir(const char* pathname, mode_t mode) return -EINVAL; if (pathname_length >= 255) return -ENAMETOOLONG; - int error; - if (!VFS::the().mkdir(String(pathname, pathname_length), mode & ~umask(), cwd_inode(), error)) - return error; - return 0; + return VFS::the().mkdir(String(pathname, pathname_length), mode & ~umask(), cwd_inode()); } clock_t Process::sys$times(tms* times) @@ -2143,10 +2137,7 @@ int Process::sys$chmod(const char* pathname, mode_t mode) { if (!validate_read_str(pathname)) return -EFAULT; - int error; - if (!VFS::the().chmod(String(pathname), mode, cwd_inode(), error)) - return error; - return 0; + return VFS::the().chmod(String(pathname), mode, cwd_inode()); } void Process::finalize() diff --git a/Kernel/SyntheticFileSystem.cpp b/Kernel/SyntheticFileSystem.cpp index 1ed6e6eb50..bfdf6a8c8e 100644 --- a/Kernel/SyntheticFileSystem.cpp +++ b/Kernel/SyntheticFileSystem.cpp @@ -311,8 +311,7 @@ size_t SynthFSInode::directory_entry_count() const return m_children.size() + 2; } -bool SynthFSInode::chmod(mode_t, int& error) +KResult SynthFSInode::chmod(mode_t) { - error = -EPERM; - return false; + return KResult(-EPERM); } diff --git a/Kernel/SyntheticFileSystem.h b/Kernel/SyntheticFileSystem.h index cfe04180d7..fe4b312941 100644 --- a/Kernel/SyntheticFileSystem.h +++ b/Kernel/SyntheticFileSystem.h @@ -67,7 +67,7 @@ private: virtual bool remove_child(const String& name, int& error) override; virtual RetainPtr<Inode> parent() const override; virtual size_t directory_entry_count() const override; - virtual bool chmod(mode_t, int& error) override; + virtual KResult chmod(mode_t) override; SynthFS& fs(); const SynthFS& fs() const; diff --git a/Kernel/VirtualFileSystem.cpp b/Kernel/VirtualFileSystem.cpp index 2f24779ccc..a144640bb6 100644 --- a/Kernel/VirtualFileSystem.cpp +++ b/Kernel/VirtualFileSystem.cpp @@ -42,7 +42,7 @@ bool VFS::mount(RetainPtr<FS>&& file_system, const String& path) { ASSERT(file_system); int error; - auto inode = resolve_path(path, root_inode_id(), error); + auto inode = old_resolve_path(path, root_inode_id(), error); if (!inode.is_valid()) { kprintf("VFS: mount can't resolve mount point '%s'\n", path.characters()); return false; @@ -131,32 +131,29 @@ RetainPtr<FileDescriptor> VFS::open(RetainPtr<Device>&& device, int& error, int return FileDescriptor::create(move(device)); } -bool VFS::utime(const String& path, int& error, Inode& base, time_t atime, time_t mtime) +KResult VFS::utime(const String& path, Inode& base, time_t atime, time_t mtime) { + int error; auto descriptor = VFS::the().open(move(path), error, 0, 0, base); if (!descriptor) - return false; + return KResult(error); auto& inode = *descriptor->inode(); - if (inode.fs().is_readonly()) { - error = -EROFS; - return false; - } - if (inode.metadata().uid != current->euid()) { - error = -EACCES; - return false; - } + if (inode.fs().is_readonly()) + return KResult(-EROFS); + if (inode.metadata().uid != current->euid()) + return KResult(-EACCES); error = inode.set_atime(atime); if (error) - return false; + return KResult(error); error = inode.set_mtime(mtime); if (error) - return false; - return true; + return KResult(error); + return KSuccess; } bool VFS::stat(const String& path, int& error, int options, Inode& base, struct stat& statbuf) { - auto inode_id = resolve_path(path, base.identifier(), error, options); + auto inode_id = old_resolve_path(path, base.identifier(), error, options); if (!inode_id.is_valid()) return false; error = FileDescriptor::create(get_inode(inode_id))->fstat(&statbuf); @@ -167,7 +164,7 @@ bool VFS::stat(const String& path, int& error, int options, Inode& base, struct RetainPtr<FileDescriptor> VFS::open(const String& path, int& error, int options, mode_t mode, Inode& base) { - auto inode_id = resolve_path(path, base.identifier(), error, options); + auto inode_id = old_resolve_path(path, base.identifier(), error, options); auto inode = get_inode(inode_id); if (options & O_CREAT) { if (!inode) @@ -248,63 +245,62 @@ RetainPtr<FileDescriptor> VFS::create(const String& path, int& error, int option return FileDescriptor::create(move(new_file)); } -bool VFS::mkdir(const String& path, mode_t mode, Inode& base, int& error) +KResult VFS::mkdir(const String& path, mode_t mode, Inode& base) { - error = -EWHYTHO; - RetainPtr<Inode> parent_inode; - auto existing_dir = resolve_path_to_inode(path, base, error, &parent_inode); - if (existing_dir) { - error = -EEXIST; - return false; - } - if (!parent_inode) { - error = -ENOENT; - return false; - } - if (error != -ENOENT) { - return false; - } - if (!parent_inode->metadata().may_write(*current)) { - error = -EACCES; - return false; - } + auto result = resolve_path_to_inode(path, base, &parent_inode); + if (!result.is_error()) + return KResult(-EEXIST); + if (!parent_inode) + return KResult(-ENOENT); + if (result.error() != -ENOENT) + return result.error(); + + if (!parent_inode->metadata().may_write(*current)) + return KResult(-EACCES); FileSystemPath p(path); dbgprintf("VFS::mkdir: '%s' in %u:%u\n", p.basename().characters(), parent_inode->fsid(), parent_inode->index()); + int error; auto new_dir = parent_inode->fs().create_directory(parent_inode->identifier(), p.basename(), mode, error); - if (new_dir) { - error = 0; - return true; - } - return false; + if (new_dir) + return KSuccess; + return KResult(error); } -bool VFS::chmod(const String& path, mode_t mode, Inode& base, int& error) +KResult VFS::chmod(const String& path, mode_t mode, Inode& base) { - error = -EWHYTHO; - auto inode = resolve_path_to_inode(path, base, error); - if (!inode) - return false; + auto inode_or_error = resolve_path_to_inode(path, base); + if (inode_or_error.is_error()) + return inode_or_error.error(); + auto inode = inode_or_error.value(); - if (inode->fs().is_readonly()) { - error = -EROFS; - return false; - } + if (inode->fs().is_readonly()) + return KResult(-EROFS); - if (current->euid() != inode->metadata().uid) { - error = -EPERM; - return false; - } + if (current->euid() != inode->metadata().uid) + return KResult(-EPERM); // Only change the permission bits. mode = (inode->mode() & ~04777) | (mode & 04777); kprintf("VFS::chmod(): %u:%u mode %o\n", inode->fsid(), inode->index(), mode); - if (!inode->chmod(mode, error)) - return false; - error = 0; - return true; + return inode->chmod(mode); +} + +KResultOr<RetainPtr<Inode>> VFS::resolve_path_to_inode(const String& path, Inode& base, RetainPtr<Inode>* parent_inode) +{ + // FIXME: This won't work nicely across mount boundaries. + FileSystemPath p(path); + if (!p.is_valid()) + return KResult(-EINVAL); + InodeIdentifier parent_id; + auto result = resolve_path(path, base.identifier(), 0, &parent_id); + if (parent_inode && parent_id.is_valid()) + *parent_inode = get_inode(parent_id); + if (result.is_error()) + return result.error(); + return get_inode(result.value()); } RetainPtr<Inode> VFS::resolve_path_to_inode(const String& path, Inode& base, int& error, RetainPtr<Inode>* parent_inode) @@ -316,7 +312,7 @@ RetainPtr<Inode> VFS::resolve_path_to_inode(const String& path, Inode& base, int return nullptr; } InodeIdentifier parent_id; - auto inode_id = resolve_path(path, base.identifier(), error, 0, &parent_id); + auto inode_id = old_resolve_path(path, base.identifier(), error, 0, &parent_id); if (parent_inode && parent_id.is_valid()) *parent_inode = get_inode(parent_id); if (!inode_id.is_valid()) { @@ -437,16 +433,16 @@ bool VFS::rmdir(const String& path, Inode& base, int& error) return true; } -InodeIdentifier VFS::resolve_symbolic_link(InodeIdentifier base, Inode& symlink_inode, int& error) +KResultOr<InodeIdentifier> VFS::resolve_symbolic_link(InodeIdentifier base, Inode& symlink_inode) { auto symlink_contents = symlink_inode.read_entire(); if (!symlink_contents) - return { }; + return KResult(-ENOENT); auto linkee = String((const char*)symlink_contents.pointer(), symlink_contents.size()); #ifdef VFS_DEBUG kprintf("linkee (%s)(%u) from %u:%u\n", linkee.characters(), linkee.length(), base.fsid(), base.index()); #endif - return resolve_path(linkee, base, error); + return resolve_path(linkee, base); } RetainPtr<Inode> VFS::get_inode(InodeIdentifier inode_id) @@ -477,7 +473,7 @@ String VFS::absolute_path(Inode& core_inode) InodeIdentifier parent_id; if (inode->is_directory()) { - parent_id = resolve_path("..", inode->identifier(), error); + parent_id = old_resolve_path("..", inode->identifier(), error); } else { parent_id = inode->parent()->identifier(); } @@ -500,12 +496,10 @@ String VFS::absolute_path(Inode& core_inode) return builder.to_string(); } -InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int& error, int options, InodeIdentifier* parent_id) +KResultOr<InodeIdentifier> VFS::resolve_path(const String& path, InodeIdentifier base, int options, InodeIdentifier* parent_id) { - if (path.is_empty()) { - error = -EINVAL; - return { }; - } + if (path.is_empty()) + return KResult(-EINVAL); auto parts = path.split('/'); InodeIdentifier crumb_id; @@ -528,29 +522,24 @@ InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int& #ifdef VFS_DEBUG kprintf("invalid metadata\n"); #endif - error = -EIO; - return { }; + return KResult(-EIO); } auto metadata = crumb_inode->metadata(); if (!metadata.is_directory()) { #ifdef VFS_DEBUG kprintf("parent of <%s> not directory, it's inode %u:%u / %u:%u, mode: %u, size: %u\n", part.characters(), crumb_id.fsid(), crumb_id.index(), metadata.inode.fsid(), metadata.inode.index(), metadata.mode, metadata.size); #endif - error = -ENOTDIR; - return { }; - } - if (!metadata.may_execute(*current)) { - error = -EACCES; - return { }; + return KResult(-ENOTDIR); } + if (!metadata.may_execute(*current)) + return KResult(-EACCES); auto parent = crumb_id; crumb_id = crumb_inode->lookup(part); if (!crumb_id.is_valid()) { #ifdef VFS_DEBUG kprintf("child <%s>(%u) not found in directory, %02u:%08u\n", part.characters(), part.length(), parent.fsid(), parent.index()); #endif - error = -ENOENT; - return { }; + return KResult(-ENOENT); } #ifdef VFS_DEBUG kprintf("<%s> %u:%u\n", part.characters(), crumb_id.fsid(), crumb_id.index()); @@ -581,24 +570,32 @@ InodeIdentifier VFS::resolve_path(const String& path, InodeIdentifier base, int& } if (metadata.is_symlink()) { if (i == parts.size() - 1) { - if (options & O_NOFOLLOW) { - error = -ELOOP; - return { }; - } + if (options & O_NOFOLLOW) + return KResult(-ELOOP); if (options & O_NOFOLLOW_NOERROR) return crumb_id; } - crumb_id = resolve_symbolic_link(parent, *crumb_inode, error); - if (!crumb_id.is_valid()) { - kprintf("Symbolic link resolution failed :(\n"); - return { }; - } + auto result = resolve_symbolic_link(parent, *crumb_inode); + if (result.is_error()) + return KResult(-ENOENT); + crumb_id = result.value(); + ASSERT(crumb_id.is_valid()); } } return crumb_id; } +InodeIdentifier VFS::old_resolve_path(const String& path, InodeIdentifier base, int& error, int options, InodeIdentifier* parent_id) +{ + auto result = resolve_path(path, base, options, parent_id); + if (result.is_error()) { + error = result.error(); + return { }; + } + return result.value(); +} + VFS::Mount::Mount(InodeIdentifier host, RetainPtr<FS>&& guest_fs) : m_host(host) , m_guest(guest_fs->root_inode()) diff --git a/Kernel/VirtualFileSystem.h b/Kernel/VirtualFileSystem.h index b7e9529443..dc41b2dacf 100644 --- a/Kernel/VirtualFileSystem.h +++ b/Kernel/VirtualFileSystem.h @@ -10,6 +10,7 @@ #include "InodeMetadata.h" #include "Limits.h" #include "FileSystem.h" +#include <Kernel/KResult.h> #define O_RDONLY 0 #define O_WRONLY 1 @@ -64,13 +65,13 @@ public: RetainPtr<FileDescriptor> open(RetainPtr<Device>&&, int& error, int options); RetainPtr<FileDescriptor> open(const String& path, int& error, int options, mode_t mode, Inode& base); RetainPtr<FileDescriptor> create(const String& path, int& error, int options, mode_t mode, Inode& base); - bool mkdir(const String& path, mode_t mode, Inode& base, int& error); + KResult mkdir(const String& path, mode_t mode, Inode& base); bool link(const String& old_path, const String& new_path, Inode& base, int& error); bool unlink(const String& path, Inode& base, int& error); bool rmdir(const String& path, Inode& base, int& error); - bool chmod(const String& path, mode_t, Inode& base, int& error); + KResult chmod(const String& path, mode_t, Inode& base); bool stat(const String& path, int& error, int options, Inode& base, struct stat&); - bool utime(const String& path, int& error, Inode& base, time_t atime, time_t mtime); + KResult utime(const String& path, Inode& base, time_t atime, time_t mtime); void register_device(Device&); void unregister_device(Device&); @@ -97,9 +98,11 @@ private: bool is_vfs_root(InodeIdentifier) const; void traverse_directory_inode(Inode&, Function<bool(const FS::DirectoryEntry&)>); - InodeIdentifier resolve_path(const String& path, InodeIdentifier base, int& error, int options = 0, InodeIdentifier* parent_id = nullptr); + InodeIdentifier old_resolve_path(const String& path, InodeIdentifier base, int& error, int options = 0, InodeIdentifier* parent_id = nullptr); + KResultOr<InodeIdentifier> resolve_path(const String& path, InodeIdentifier base, int options = 0, InodeIdentifier* parent_id = nullptr); RetainPtr<Inode> resolve_path_to_inode(const String& path, Inode& base, int& error, RetainPtr<Inode>* parent_id = nullptr); - InodeIdentifier resolve_symbolic_link(InodeIdentifier base, Inode& symlink_inode, int& error); + KResultOr<RetainPtr<Inode>> resolve_path_to_inode(const String& path, Inode& base, RetainPtr<Inode>* parent_id = nullptr); + KResultOr<InodeIdentifier> resolve_symbolic_link(InodeIdentifier base, Inode& symlink_inode); Mount* find_mount_for_host(InodeIdentifier); Mount* find_mount_for_guest(InodeIdentifier); diff --git a/LibC/errno_numbers.h b/LibC/errno_numbers.h index b441460a57..03cd9ce433 100644 --- a/LibC/errno_numbers.h +++ b/LibC/errno_numbers.h @@ -74,7 +74,7 @@ __ERROR(EMAXERRNO, "The highest errno +1 :^)") -enum __errno_values { +enum __errno_value { #undef __ENUMERATE_ERROR #define __ERROR(a, b) a, __ENUMERATE_ALL_ERRORS |