From cf45370c4784edd8f0dadc2a1728902629681410 Mon Sep 17 00:00:00 2001 From: David Briggs Date: Sat, 22 Jan 2022 19:53:02 -0500 Subject: [PATCH] Kernel: Use ErrorOr in BlockBased and Ext2 filesystem raw read and write These functions used to return booleans which withheld useful error information for callers. Internally they would suppress and convert Error objects. We now log or propagate these errors up the stack. --- Kernel/FileSystem/BlockBasedFileSystem.cpp | 32 ++++++++++------------ Kernel/FileSystem/BlockBasedFileSystem.h | 8 +++--- Kernel/FileSystem/Ext2FileSystem.cpp | 16 ++++++----- Kernel/FileSystem/Ext2FileSystem.h | 2 +- Kernel/FileSystem/ISO9660FileSystem.cpp | 19 ++++--------- 5 files changed, 34 insertions(+), 43 deletions(-) diff --git a/Kernel/FileSystem/BlockBasedFileSystem.cpp b/Kernel/FileSystem/BlockBasedFileSystem.cpp index 92b25609d5..e02b3211a0 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.cpp +++ b/Kernel/FileSystem/BlockBasedFileSystem.cpp @@ -169,44 +169,40 @@ ErrorOr BlockBasedFileSystem::write_block(BlockIndex index, const UserOrKe }); } -bool BlockBasedFileSystem::raw_read(BlockIndex index, UserOrKernelBuffer& buffer) +ErrorOr BlockBasedFileSystem::raw_read(BlockIndex index, UserOrKernelBuffer& buffer) { auto base_offset = index.value() * m_logical_block_size; - auto nread = file_description().read(buffer, base_offset, m_logical_block_size); - VERIFY(!nread.is_error()); - VERIFY(nread.value() == m_logical_block_size); - return true; + auto nread = TRY(file_description().read(buffer, base_offset, m_logical_block_size)); + VERIFY(nread == m_logical_block_size); + return {}; } -bool BlockBasedFileSystem::raw_write(BlockIndex index, const UserOrKernelBuffer& buffer) +ErrorOr BlockBasedFileSystem::raw_write(BlockIndex index, const UserOrKernelBuffer& buffer) { auto base_offset = index.value() * m_logical_block_size; - auto nwritten = file_description().write(base_offset, buffer, m_logical_block_size); - VERIFY(!nwritten.is_error()); - VERIFY(nwritten.value() == m_logical_block_size); - return true; + auto nwritten = TRY(file_description().write(base_offset, buffer, m_logical_block_size)); + VERIFY(nwritten == m_logical_block_size); + return {}; } -bool BlockBasedFileSystem::raw_read_blocks(BlockIndex index, size_t count, UserOrKernelBuffer& buffer) +ErrorOr BlockBasedFileSystem::raw_read_blocks(BlockIndex index, size_t count, UserOrKernelBuffer& buffer) { auto current = buffer; for (auto block = index.value(); block < (index.value() + count); block++) { - if (!raw_read(BlockIndex { block }, current)) - return false; + TRY(raw_read(BlockIndex { block }, current)); current = current.offset(logical_block_size()); } - return true; + return {}; } -bool BlockBasedFileSystem::raw_write_blocks(BlockIndex index, size_t count, const UserOrKernelBuffer& buffer) +ErrorOr BlockBasedFileSystem::raw_write_blocks(BlockIndex index, size_t count, const UserOrKernelBuffer& buffer) { auto current = buffer; for (auto block = index.value(); block < (index.value() + count); block++) { - if (!raw_write(block, current)) - return false; + TRY(raw_write(block, current)); current = current.offset(logical_block_size()); } - return true; + return {}; } ErrorOr BlockBasedFileSystem::write_blocks(BlockIndex index, unsigned count, const UserOrKernelBuffer& data, bool allow_cache) diff --git a/Kernel/FileSystem/BlockBasedFileSystem.h b/Kernel/FileSystem/BlockBasedFileSystem.h index cc0008dd64..f1cfa29818 100644 --- a/Kernel/FileSystem/BlockBasedFileSystem.h +++ b/Kernel/FileSystem/BlockBasedFileSystem.h @@ -29,11 +29,11 @@ protected: ErrorOr read_block(BlockIndex, UserOrKernelBuffer*, size_t count, size_t offset = 0, bool allow_cache = true) const; ErrorOr read_blocks(BlockIndex, unsigned count, UserOrKernelBuffer&, bool allow_cache = true) const; - bool raw_read(BlockIndex, UserOrKernelBuffer&); - bool raw_write(BlockIndex, const UserOrKernelBuffer&); + ErrorOr raw_read(BlockIndex, UserOrKernelBuffer&); + ErrorOr raw_write(BlockIndex, const UserOrKernelBuffer&); - bool raw_read_blocks(BlockIndex index, size_t count, UserOrKernelBuffer&); - bool raw_write_blocks(BlockIndex index, size_t count, const UserOrKernelBuffer&); + ErrorOr raw_read_blocks(BlockIndex index, size_t count, UserOrKernelBuffer&); + ErrorOr raw_write_blocks(BlockIndex index, size_t count, const UserOrKernelBuffer&); ErrorOr write_block(BlockIndex, const UserOrKernelBuffer&, size_t count, size_t offset = 0, bool allow_cache = true); ErrorOr write_blocks(BlockIndex, unsigned count, const UserOrKernelBuffer&, bool allow_cache = true); diff --git a/Kernel/FileSystem/Ext2FileSystem.cpp b/Kernel/FileSystem/Ext2FileSystem.cpp index ef9612f2aa..a1e50ed1ff 100644 --- a/Kernel/FileSystem/Ext2FileSystem.cpp +++ b/Kernel/FileSystem/Ext2FileSystem.cpp @@ -63,14 +63,12 @@ Ext2FS::~Ext2FS() { } -bool Ext2FS::flush_super_block() +ErrorOr Ext2FS::flush_super_block() { MutexLocker locker(m_lock); VERIFY((sizeof(ext2_super_block) % logical_block_size()) == 0); auto super_block_buffer = UserOrKernelBuffer::for_kernel_buffer((u8*)&m_super_block); - bool success = raw_write_blocks(2, (sizeof(ext2_super_block) / logical_block_size()), super_block_buffer); - VERIFY(success); - return true; + return raw_write_blocks(2, (sizeof(ext2_super_block) / logical_block_size()), super_block_buffer); } const ext2_group_desc& Ext2FS::group_descriptor(GroupIndex group_index) const @@ -87,8 +85,7 @@ ErrorOr Ext2FS::initialize() VERIFY((sizeof(ext2_super_block) % logical_block_size()) == 0); auto super_block_buffer = UserOrKernelBuffer::for_kernel_buffer((u8*)&m_super_block); - bool success = raw_read_blocks(2, (sizeof(ext2_super_block) / logical_block_size()), super_block_buffer); - VERIFY(success); + TRY(raw_read_blocks(2, (sizeof(ext2_super_block) / logical_block_size()), super_block_buffer)); auto const& super_block = this->super_block(); if constexpr (EXT2_DEBUG) { @@ -679,7 +676,12 @@ void Ext2FS::flush_writes() { MutexLocker locker(m_lock); if (m_super_block_dirty) { - flush_super_block(); + auto result = flush_super_block(); + if (result.is_error()) { + dbgln("Ext2FS[{}]::flush_writes(): Failed to write superblock: {}", fsid(), result.error()); + // FIXME: We should handle this error. + VERIFY_NOT_REACHED(); + } m_super_block_dirty = false; } if (m_block_group_descriptors_dirty) { diff --git a/Kernel/FileSystem/Ext2FileSystem.h b/Kernel/FileSystem/Ext2FileSystem.h index 7f13e9658f..4dd6ad904b 100644 --- a/Kernel/FileSystem/Ext2FileSystem.h +++ b/Kernel/FileSystem/Ext2FileSystem.h @@ -122,7 +122,7 @@ private: ErrorOr write_ext2_inode(InodeIndex, ext2_inode const&); bool find_block_containing_inode(InodeIndex, BlockIndex& block_index, unsigned& offset) const; - bool flush_super_block(); + ErrorOr flush_super_block(); virtual StringView class_name() const override { return "Ext2FS"sv; } virtual Ext2FSInode& root_inode() override; diff --git a/Kernel/FileSystem/ISO9660FileSystem.cpp b/Kernel/FileSystem/ISO9660FileSystem.cpp index ee208e9f3d..316209e6de 100644 --- a/Kernel/FileSystem/ISO9660FileSystem.cpp +++ b/Kernel/FileSystem/ISO9660FileSystem.cpp @@ -234,10 +234,10 @@ ErrorOr ISO9660FS::parse_volume_set() auto current_block_index = first_data_area_block; while (true) { - bool result = raw_read(BlockIndex { current_block_index }, block_buffer); - if (!result) { - dbgln_if(ISO9660_DEBUG, "Failed to read volume descriptor from ISO file"); - return EIO; + auto result = raw_read(BlockIndex { current_block_index }, block_buffer); + if (result.is_error()) { + dbgln_if(ISO9660_DEBUG, "Failed to read volume descriptor from ISO file: {}", result.error()); + return result; } auto const* header = reinterpret_cast(block->data()); @@ -387,11 +387,7 @@ ErrorOr> ISO9660FS::directory_entry_for auto blocks = TRY(KBuffer::try_create_with_size(data_length, Memory::Region::Access::Read | Memory::Region::Access::Write, "ISO9660FS: Directory traversal buffer")); auto blocks_buffer = UserOrKernelBuffer::for_kernel_buffer(blocks->data()); - auto did_read = raw_read_blocks(BlockBasedFileSystem::BlockIndex { extent_location }, data_length / logical_block_size(), blocks_buffer); - if (!did_read) { - return EIO; - } - + TRY(raw_read_blocks(BlockBasedFileSystem::BlockIndex { extent_location }, data_length / logical_block_size(), blocks_buffer)); auto entry = TRY(DirectoryEntry::try_create(extent_location, data_length, move(blocks))); m_directory_entry_cache.set(key, entry); @@ -428,10 +424,7 @@ ErrorOr ISO9660Inode::read_bytes(off_t offset, size_t size, UserOrKernel auto buffer_offset = buffer.offset(nread); dbgln_if(ISO9660_VERY_DEBUG, "ISO9660Inode::read_bytes: Reading {} bytes into buffer offset {}/{}, logical block index: {}", bytes_to_read, nread, total_bytes, current_block_index.value()); - if (bool result = const_cast(fs()).raw_read(current_block_index, block_buffer); !result) { - return EIO; - } - + TRY(const_cast(fs()).raw_read(current_block_index, block_buffer)); TRY(buffer_offset.write(block->data() + initial_offset, bytes_to_read)); nread += bytes_to_read;