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

@@ -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)

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 };
 

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::BlockIndex> 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<Inode> Ext2FS::get_inode(InodeIdentifier inode) const
 
     auto new_inode = adopt(*new Ext2FSInode(const_cast<Ext2FS&>(*this), inode.index()));
     auto buffer = UserOrKernelBuffer::for_kernel_buffer(reinterpret_cast<u8*>(&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;
 }