From e279b45aed5509efc537fc8c831f40733d7b1028 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 20 Jan 2021 21:11:01 +0100 Subject: [PATCH] Kernel: Make BlockBasedFS read/write functions return a KResult This way, if something goes wrong, we get to keep the actual error. Also, KResults are nodiscard, so we have to deal with that in Ext2FS instead of just silently ignoring I/O errors(!) --- Kernel/FileSystem/BlockBasedFileSystem.cpp | 49 ++++++++++++---------- Kernel/FileSystem/BlockBasedFileSystem.h | 8 ++-- Kernel/FileSystem/Ext2FileSystem.cpp | 42 +++++++++++++++---- 3 files changed, 66 insertions(+), 33 deletions(-) diff --git a/Kernel/FileSystem/BlockBasedFileSystem.cpp b/Kernel/FileSystem/BlockBasedFileSystem.cpp index fbdf11b75d..b4937d9294 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.cpp +++ b/Kernel/FileSystem/BlockBasedFileSystem.cpp @@ -142,7 +142,7 @@ BlockBasedFS::~BlockBasedFS() { } -int BlockBasedFS::write_block(unsigned index, const UserOrKernelBuffer& data, size_t count, size_t offset, bool allow_cache) +KResult BlockBasedFS::write_block(unsigned index, const UserOrKernelBuffer& data, size_t count, size_t offset, bool allow_cache) { ASSERT(m_logical_block_size); ASSERT(offset + count <= block_size()); @@ -156,22 +156,24 @@ int BlockBasedFS::write_block(unsigned index, const UserOrKernelBuffer& data, si file_description().seek(base_offset, SEEK_SET); auto nwritten = file_description().write(data, count); if (nwritten.is_error()) - return -EIO; // TODO: Return error code as-is, could be -EFAULT! + return nwritten.error(); ASSERT(nwritten.value() == count); - return 0; + return KSuccess; } auto& entry = cache().get(index); if (count < block_size()) { // Fill the cache first. - read_block(index, nullptr, block_size()); + auto result = read_block(index, nullptr, block_size()); + if (result.is_error()) + return result; } if (!data.read(entry.data + offset, count)) - return -EFAULT; + return KResult(-EFAULT); cache().mark_dirty(entry); entry.has_data = true; - return 0; + return KSuccess; } bool BlockBasedFS::raw_read(unsigned index, UserOrKernelBuffer& buffer) @@ -214,18 +216,21 @@ bool BlockBasedFS::raw_write_blocks(unsigned index, size_t count, const UserOrKe return true; } -int BlockBasedFS::write_blocks(unsigned index, unsigned count, const UserOrKernelBuffer& data, bool allow_cache) +KResult BlockBasedFS::write_blocks(unsigned index, unsigned count, const UserOrKernelBuffer& data, bool allow_cache) { ASSERT(m_logical_block_size); #ifdef BBFS_DEBUG klog() << "BlockBasedFileSystem::write_blocks " << index << " x" << count; #endif - for (unsigned i = 0; i < count; ++i) - write_block(index + i, data.offset(i * block_size()), block_size(), 0, allow_cache); - return 0; + for (unsigned i = 0; i < count; ++i) { + auto result = write_block(index + i, data.offset(i * block_size()), block_size(), 0, allow_cache); + if (result.is_error()) + return result; + } + return KSuccess; } -int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer* buffer, size_t count, size_t offset, bool allow_cache) const +KResult BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer* buffer, size_t count, size_t offset, bool allow_cache) const { ASSERT(m_logical_block_size); ASSERT(offset + count <= block_size()); @@ -239,9 +244,9 @@ int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer* buffer, size_t file_description().seek(base_offset, SEEK_SET); auto nread = file_description().read(*buffer, count); if (nread.is_error()) - return -EIO; + return nread.error(); ASSERT(nread.value() == count); - return 0; + return KSuccess; } auto& entry = cache().get(index); @@ -251,31 +256,31 @@ int BlockBasedFS::read_block(unsigned index, UserOrKernelBuffer* buffer, size_t auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry.data); auto nread = file_description().read(entry_data_buffer, block_size()); if (nread.is_error()) - return -EIO; + return nread.error(); ASSERT(nread.value() == block_size()); entry.has_data = true; } if (buffer && !buffer->write(entry.data + offset, count)) - return -EFAULT; - return 0; + return KResult(-EFAULT); + return KSuccess; } -int BlockBasedFS::read_blocks(unsigned index, unsigned count, UserOrKernelBuffer& buffer, bool allow_cache) const +KResult BlockBasedFS::read_blocks(unsigned index, unsigned count, UserOrKernelBuffer& buffer, bool allow_cache) const { ASSERT(m_logical_block_size); if (!count) - return false; + return KResult(-EINVAL); if (count == 1) return read_block(index, &buffer, block_size(), 0, allow_cache); auto out = buffer; for (unsigned i = 0; i < count; ++i) { - auto err = read_block(index + i, &out, block_size(), 0, allow_cache); - if (err < 0) - return err; + auto result = read_block(index + i, &out, block_size(), 0, allow_cache); + if (result.is_error()) + return result; out = out.offset(block_size()); } - return 0; + return KSuccess; } void BlockBasedFS::flush_specific_block_if_needed(unsigned index) diff --git a/Kernel/FileSystem/BlockBasedFileSystem.h b/Kernel/FileSystem/BlockBasedFileSystem.h index fcf096aec3..cbbc791594 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.h +++ b/Kernel/FileSystem/BlockBasedFileSystem.h @@ -42,8 +42,8 @@ public: protected: explicit BlockBasedFS(FileDescription&); - int read_block(unsigned index, UserOrKernelBuffer* buffer, size_t count, size_t offset = 0, bool allow_cache = true) const; - int read_blocks(unsigned index, unsigned count, UserOrKernelBuffer& buffer, bool allow_cache = true) const; + KResult read_block(unsigned index, UserOrKernelBuffer* buffer, size_t count, size_t offset = 0, bool allow_cache = true) const; + KResult read_blocks(unsigned index, unsigned count, UserOrKernelBuffer& buffer, bool allow_cache = true) const; bool raw_read(unsigned index, UserOrKernelBuffer& buffer); bool raw_write(unsigned index, const UserOrKernelBuffer& buffer); @@ -51,8 +51,8 @@ protected: bool raw_read_blocks(unsigned index, size_t count, UserOrKernelBuffer& buffer); bool raw_write_blocks(unsigned index, size_t count, const UserOrKernelBuffer& buffer); - int write_block(unsigned index, const UserOrKernelBuffer& buffer, size_t count, size_t offset = 0, bool allow_cache = true); - int write_blocks(unsigned index, unsigned count, const UserOrKernelBuffer&, bool allow_cache = true); + KResult write_block(unsigned index, const UserOrKernelBuffer& buffer, size_t count, size_t offset = 0, bool allow_cache = true); + KResult write_blocks(unsigned index, unsigned count, const UserOrKernelBuffer&, bool allow_cache = true); size_t m_logical_block_size { 512 }; diff --git a/Kernel/FileSystem/Ext2FileSystem.cpp b/Kernel/FileSystem/Ext2FileSystem.cpp index a097d05d9f..a9b349a8e2 100644 --- a/Kernel/FileSystem/Ext2FileSystem.cpp +++ b/Kernel/FileSystem/Ext2FileSystem.cpp @@ -147,7 +147,12 @@ bool Ext2FS::initialize() return false; } auto buffer = UserOrKernelBuffer::for_kernel_buffer(m_cached_group_descriptor_table->data()); - read_blocks(first_block_of_bgdt, blocks_to_read, buffer); + auto result = read_blocks(first_block_of_bgdt, blocks_to_read, buffer); + if (result.is_error()) { + // FIXME: Propagate the error + dbgln("Ext2FS: initialize had error: {}", result.error()); + return false; + } #ifdef EXT2_DEBUG for (unsigned i = 1; i <= m_block_group_count; ++i) { @@ -349,7 +354,12 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in dind_block_dirty = true; } else { auto buffer = UserOrKernelBuffer::for_kernel_buffer(dind_block_contents.data()); - read_block(e2inode.i_block[EXT2_DIND_BLOCK], &buffer, block_size()); + auto result = read_block(e2inode.i_block[EXT2_DIND_BLOCK], &buffer, block_size()); + if (result.is_error()) { + // FIXME: Propagate the error + dbgln("Ext2FS: write_block_list_for_inode had error: {}", result.error()); + return false; + } } auto* dind_block_as_pointers = (unsigned*)dind_block_contents.data(); @@ -372,7 +382,12 @@ bool Ext2FS::write_block_list_for_inode(InodeIndex inode_index, ext2_inode& e2in ind_block_dirty = true; } else { auto buffer = UserOrKernelBuffer::for_kernel_buffer(ind_block_contents.data()); - read_block(indirect_block_index, &buffer, block_size()); + auto result = read_block(indirect_block_index, &buffer, block_size()); + if (result.is_error()) { + // FIXME: Propagate the error + dbgln("Ext2FS: write_block_list_for_inode had error: {}", result.error()); + return false; + } } auto* ind_block_as_pointers = (unsigned*)ind_block_contents.data(); @@ -491,7 +506,11 @@ Vector Ext2FS::block_list_for_inode_impl(const ext2_inode& e auto count = min(blocks_remaining, entries_per_block); u32 array[count]; auto buffer = UserOrKernelBuffer::for_kernel_buffer((u8*)array); - read_block(array_block_index, &buffer, sizeof(array), 0); + auto result = read_block(array_block_index, &buffer, sizeof(array), 0); + if (result.is_error()) { + // FIXME: Stop here and propagate this error. + dbgln("Ext2FS: block_list_for_inode_impl had error: {}", result.error()); + } for (BlockIndex i = 0; i < count; ++i) callback(array[i]); }; @@ -562,7 +581,9 @@ void Ext2FS::flush_block_group_descriptor_table() unsigned blocks_to_write = ceil_div(m_block_group_count * sizeof(ext2_group_desc), block_size()); unsigned first_block_of_bgdt = block_size() == 1024 ? 2 : 1; auto buffer = UserOrKernelBuffer::for_kernel_buffer((u8*)block_group_descriptors()); - write_blocks(first_block_of_bgdt, blocks_to_write, buffer); + auto result = write_blocks(first_block_of_bgdt, blocks_to_write, buffer); + if (result.is_error()) + dbgln("Ext2FS: flush_block_group_descriptor_table had error: {}", result.error()); } void Ext2FS::flush_writes() @@ -579,7 +600,10 @@ void Ext2FS::flush_writes() for (auto& cached_bitmap : m_cached_bitmaps) { if (cached_bitmap->dirty) { auto buffer = UserOrKernelBuffer::for_kernel_buffer(cached_bitmap->buffer.data()); - write_block(cached_bitmap->bitmap_block_index, buffer, block_size()); + auto result = write_block(cached_bitmap->bitmap_block_index, buffer, block_size()); + if (result.is_error()) { + dbgln("Ext2FS: flush_writes() had error {}", result.error()); + } cached_bitmap->dirty = false; #ifdef EXT2_DEBUG dbgln("Flushed bitmap block {}", cached_bitmap->bitmap_block_index); @@ -685,7 +709,11 @@ RefPtr Ext2FS::get_inode(InodeIdentifier inode) const auto new_inode = adopt(*new Ext2FSInode(const_cast(*this), inode.index())); auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast(&new_inode->m_raw_inode)); - read_block(block_index, &buffer, sizeof(ext2_inode), offset); + auto result = read_block(block_index, &buffer, sizeof(ext2_inode), offset); + if (result.is_error()) { + // FIXME: Propagate the actual error. + return nullptr; + } m_inode_cache.set(inode.index(), new_inode); return new_inode; }