Refactor error handling (#29)

Use std::expected instead of throwing an exception , allows much better error handling in the tools. Still throw an exception when no other option (like during iterator) Also made the API for retrieving files case-insensitive. And upgrade the project to C++23 for std::expected
koolkdev · Feb 14, 2024 · c0e97e6 · c0e97e6
1 parent 923c002
commit c0e97e6
Show file tree

Hide file tree

Showing 25 changed files with 374 additions and 226 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -6,7 +6,7 @@ jobs:
   build:
     strategy:
       matrix:
-        platform: [ubuntu-latest, windows-latest, macos-latest]
+        platform: [ubuntu-latest, windows-latest, macos-13]
         compiler: [msvc, gcc, llvm]
         exclude:
           - platform: windows-latest
@@ -15,51 +15,62 @@ jobs:
             compiler: llvm
           - platform: ubuntu-latest
             compiler: msvc
-          - platform: macos-latest
+          # llvm doesn't work with libstd++ until P2493R0 is done
+          - platform: ubuntu-latest
+            compiler: llvm
+          - platform: macos-13
             compiler: msvc
-          - platform: macos-latest
+          - platform: macos-13
             compiler: gcc
     runs-on: ${{matrix.platform}}
 
     steps:
-    - uses: actions/checkout@v2
-      with:
-        submodules: 'true'
+      - uses: actions/checkout@v2
+        with:
+          submodules: "true"
+
+      - uses: lukka/get-cmake@latest
 
-    - uses: lukka/get-cmake@latest
+      - name: Run vcpkg
+        uses: lukka/run-vcpkg@v10
 
-    - name: Run vcpkg
-      uses: lukka/run-vcpkg@v10
+      - name: Set gcc
+        run: |
+          echo "CC=gcc-13" >> $GITHUB_ENV
+          echo "CXX=g++-13" >> $GITHUB_ENV
+        shell: bash
+        if: matrix.platform == 'ubuntu-latest' && matrix.compiler == 'gcc'
 
-    - name: Set llvm (Ubuntu)
-      run: |
+      - name: Set llvm (Ubuntu)
+        run: |
           wget https://apt.llvm.org/llvm.sh
           chmod +x llvm.sh
-          sudo ./llvm.sh 16
-          echo "CC=clang-16" >> $GITHUB_ENV
-          echo "CXX=clang++-16" >> $GITHUB_ENV
-      shell: bash
-      if: matrix.platform == 'ubuntu-latest' && matrix.compiler == 'llvm'
+          sudo ./llvm.sh 17
+          sudo apt install libc++-17-dev
+          echo "CC=clang-17" >> $GITHUB_ENV
+          echo "CXX=clang++-17" >> $GITHUB_ENV
+        shell: bash
+        if: matrix.platform == 'ubuntu-latest' && matrix.compiler == 'llvm'
 
-    - name: Set llvm (MacOS)
-      run: |
-           brew update
-           # Temporary fix, see https://github.com/actions/setup-python/issues/577
-           rm /usr/local/bin/2to3 || true
-           rm /usr/local/bin/idle3 || true
-           rm /usr/local/bin/pydoc3 || true
-           rm /usr/local/bin/python3 || true
-           rm /usr/local/bin/python3-config || true
-           brew install llvm
-           echo "CC=/usr/local/opt/llvm/bin/clang" >> $GITHUB_ENV
-           echo "CXX=/usr/local/opt/llvm/bin/clang++" >> $GITHUB_ENV
-           echo "LDFLAGS=-L/usr/local/opt/llvm/lib" >> $GITHUB_ENV
-           echo "CXXFLAGS=-isystem /usr/local/opt/llvm/include" >> $GITHUB_ENV
-      shell: bash
-      if: matrix.platform == 'macos-latest'
+      - name: Set llvm (MacOS)
+        run: |
+          brew update
+          # Temporary fix, see https://github.com/actions/setup-python/issues/577
+          rm /usr/local/bin/2to3 || true
+          rm /usr/local/bin/idle3 || true
+          rm /usr/local/bin/pydoc3 || true
+          rm /usr/local/bin/python3 || true
+          rm /usr/local/bin/python3-config || true
+          brew install llvm
+          echo "CC=/usr/local/opt/llvm/bin/clang" >> $GITHUB_ENV
+          echo "CXX=/usr/local/opt/llvm/bin/clang++" >> $GITHUB_ENV
+          echo "LDFLAGS=-L/usr/local/opt/llvm/lib" >> $GITHUB_ENV
+          echo "CXXFLAGS=-isystem /usr/local/opt/llvm/include" >> $GITHUB_ENV
+        shell: bash
+        if: matrix.platform == 'macos-13'
 
-    - name: Run cmake
-      uses: lukka/run-cmake@v10
-      with:
-        configurePreset: 'default'
-        buildPreset: 'release'
+      - name: Run cmake
+        uses: lukka/run-cmake@v10
+        with:
+          configurePreset: "default"
+          buildPreset: "release"
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -28,6 +28,7 @@ add_library(${PROJECT_NAME}
     src/device_encryption.cpp
     src/directory.cpp
     src/directory_items_iterator.cpp
+    src/errors.cpp
     src/file.cpp
     src/file_device.cpp
     src/free_blocks_allocator.cpp
@@ -44,7 +45,7 @@ if(BUILD_STATIC AND MSVC)
         MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
 endif()
 
-target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_20)
+target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_23)
 target_compile_options(${PROJECT_NAME} PRIVATE
   $<$<CXX_COMPILER_ID:MSVC>:/W4 /WX>
   $<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Wpedantic -Werror>

diff --git a/include/wfslib/directory.h b/include/wfslib/directory.h
@@ -7,9 +7,12 @@
 
 #pragma once
 
+#include <expected>
 #include <memory>
 #include <string>
+
 #include "directory_items_iterator.h"
+#include "errors.h"
 #include "wfs_item.h"
 
 class Area;
@@ -26,9 +29,9 @@ class Directory : public WfsItem, public std::enable_shared_from_this<Directory>
             const std::shared_ptr<MetadataBlock>& block)
       : WfsItem(name, attributes), area_(area), block_(block) {}
 
-  std::shared_ptr<WfsItem> GetObject(const std::string& name) const;
-  std::shared_ptr<Directory> GetDirectory(const std::string& name) const;
-  std::shared_ptr<File> GetFile(const std::string& name) const;
+  std::expected<std::shared_ptr<WfsItem>, WfsError> GetObject(const std::string& name) const;
+  std::expected<std::shared_ptr<Directory>, WfsError> GetDirectory(const std::string& name) const;
+  std::expected<std::shared_ptr<File>, WfsError> GetFile(const std::string& name) const;
 
   size_t Size() const;
   DirectoryItemsIterator begin() const;
@@ -44,6 +47,8 @@ class Directory : public WfsItem, public std::enable_shared_from_this<Directory>
 
   std::shared_ptr<MetadataBlock> block_;
 
-  std::shared_ptr<WfsItem> Create(const std::string& name, const AttributesBlock& attributes) const;
-  AttributesBlock GetObjectAttributes(const std::shared_ptr<MetadataBlock>& block, const std::string& name) const;
+  std::expected<std::shared_ptr<WfsItem>, WfsError> GetObjectInternal(const std::string& name,
+                                                                      const AttributesBlock& attributes) const;
+  std::expected<AttributesBlock, WfsError> GetObjectAttributes(const std::shared_ptr<MetadataBlock>& block,
+                                                               const std::string& name) const;
 };
diff --git a/include/wfslib/directory_items_iterator.h b/include/wfslib/directory_items_iterator.h
@@ -7,10 +7,13 @@
 
 #pragma once
 
+#include <expected>
 #include <iterator>
 #include <memory>
 #include <string>
 
+#include "errors.h"
+
 class Directory;
 class WfsItem;
 class MetadataBlock;
@@ -21,7 +24,7 @@ class DirectoryItemsIterator {
  public:
   using iterator_category = std::input_iterator_tag;
   using difference_type = std::ptrdiff_t;
-  using value_type = std::shared_ptr<WfsItem>;
+  using value_type = std::tuple<std::string, std::expected<std::shared_ptr<WfsItem>, WfsError>>;
 
   struct NodeState {
     std::shared_ptr<MetadataBlock> block;
@@ -39,7 +42,7 @@ class DirectoryItemsIterator {
   DirectoryItemsIterator operator++(int);
   bool operator==(const DirectoryItemsIterator& rhs) const;
   bool operator!=(const DirectoryItemsIterator& rhs) const;
-  std::shared_ptr<WfsItem> operator*();
+  value_type operator*();
 
  private:
   std::shared_ptr<const Directory> directory_;

diff --git a/include/wfslib/errors.h b/include/wfslib/errors.h
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2024 koolkdev
+ *
+ * This software may be modified and distributed under the terms
+ * of the MIT license.  See the LICENSE file for details.
+ */
+
+#pragma once
+
+#include <exception>
+#include <expected>
+
+enum WfsError {
+  kItemNotFound,
+  kNotDirectory,
+  kNotFile,
+  kBlockBadHash,
+  kAreaHeaderCorrupted,
+  kDirectoryCorrupted,
+  kFreeBlocksAllocatorCorrupted,
+  kFileDataCorrupted,
+  kFileMetadataCorrupted,
+};
+
+class WfsException : public std::exception {
+ public:
+  WfsException(WfsError error) : error_(error) {}
+  virtual char const* what() const noexcept override;
+
+ private:
+  WfsError error_;
+};
+
+template <typename T>
+T throw_if_error(std::expected<T, WfsError> res) {
+  if (!res.has_value())
+    throw WfsException(res.error());
+  return *res;
+}
diff --git a/include/wfslib/wfs_item.h b/include/wfslib/wfs_item.h
@@ -18,8 +18,6 @@ struct AttributesBlock {
   std::shared_ptr<MetadataBlock> block;
   size_t attributes_offset;
 
-  operator bool() const { return !!block; }
-
   ::Attributes* Attributes();
   const ::Attributes* Attributes() const;
 };
@@ -29,7 +27,6 @@ class WfsItem {
   WfsItem(const std::string& name, const AttributesBlock& block);
   virtual ~WfsItem() {}
   const std::string& GetName() const { return name_; }
-  std::string GetRealName() const;
   virtual bool IsDirectory() const;
   virtual bool IsFile() const;
   virtual bool IsLink() const;

diff --git a/src/area.cpp b/src/area.cpp
@@ -27,39 +27,44 @@ Area::Area(const std::shared_ptr<DeviceEncryption>& device,
       root_directory_name_(root_directory_name),
       root_directory_attributes_(root_directory_attributes) {}
 
-std::shared_ptr<Area> Area::LoadRootArea(const std::shared_ptr<DeviceEncryption>& device) {
-  std::shared_ptr<MetadataBlock> block;
-  try {
-    block = MetadataBlock::LoadBlock(device, 0, Block::BlockSize::Basic, 0);
-  } catch (const Block::BadHash&) {
+std::expected<std::shared_ptr<Area>, WfsError> Area::LoadRootArea(const std::shared_ptr<DeviceEncryption>& device) {
+  auto block = MetadataBlock::LoadBlock(device, 0, Block::BlockSize::Basic, 0);
+  if (!block.has_value()) {
     block = MetadataBlock::LoadBlock(device, 0, Block::BlockSize::Regular, 0);
+    if (!block.has_value())
+      return std::unexpected(WfsError::kAreaHeaderCorrupted);
   }
   return std::make_shared<Area>(
-      device, nullptr, block, "",
-      AttributesBlock{block, sizeof(MetadataBlockHeader) + offsetof(WfsHeader, root_area_attributes)});
+      device, nullptr, *block, "",
+      AttributesBlock{*block, sizeof(MetadataBlockHeader) + offsetof(WfsHeader, root_area_attributes)});
 }
 
-std::shared_ptr<Directory> Area::GetDirectory(uint32_t block_number,
-                                              const std::string& name,
-                                              const AttributesBlock& attributes) {
-  // TODO: Cache
-  return std::make_shared<Directory>(name, attributes, shared_from_this(), GetMetadataBlock(block_number));
+std::expected<std::shared_ptr<Directory>, WfsError> Area::GetDirectory(uint32_t block_number,
+                                                                       const std::string& name,
+                                                                       const AttributesBlock& attributes) {
+  auto block = GetMetadataBlock(block_number);
+  if (!block.has_value())
+    return std::unexpected(WfsError::kDirectoryCorrupted);
+  return std::make_shared<Directory>(name, attributes, shared_from_this(), std::move(*block));
 }
 
-std::shared_ptr<Directory> Area::GetRootDirectory() {
+std::expected<std::shared_ptr<Directory>, WfsError> Area::GetRootDirectory() {
   return GetDirectory(as_const(this)->header()->root_directory_block_number.value(), root_directory_name_,
                       root_directory_attributes_);
 }
 
-std::shared_ptr<Area> Area::GetArea(uint32_t block_number,
-                                    const std::string& root_directory_name,
-                                    const AttributesBlock& root_directory_attributes,
-                                    Block::BlockSize size) {
-  return std::make_shared<Area>(device_, root_area_ ? root_area_ : shared_from_this(),
-                                GetMetadataBlock(block_number, size), root_directory_name, root_directory_attributes);
+std::expected<std::shared_ptr<Area>, WfsError> Area::GetArea(uint32_t block_number,
+                                                             const std::string& root_directory_name,
+                                                             const AttributesBlock& root_directory_attributes,
+                                                             Block::BlockSize size) {
+  auto area_metadata_block = GetMetadataBlock(block_number, size);
+  if (!area_metadata_block.has_value())
+    return std::unexpected(WfsError::kAreaHeaderCorrupted);
+  return std::make_shared<Area>(device_, root_area_ ? root_area_ : shared_from_this(), std::move(*area_metadata_block),
+                                root_directory_name, root_directory_attributes);
 }
 
-std::shared_ptr<MetadataBlock> Area::GetMetadataBlock(uint32_t block_number) const {
+std::expected<std::shared_ptr<MetadataBlock>, WfsError> Area::GetMetadataBlock(uint32_t block_number) const {
   return GetMetadataBlock(block_number, static_cast<Block::BlockSize>(header()->log2_block_size.value()));
 }
 
@@ -68,16 +73,17 @@ uint32_t Area::IV(uint32_t block_number) const {
          (ToBasicBlockNumber(block_number) << (Block::BlockSize::Basic - device_->device()->Log2SectorSize()));
 }
 
-std::shared_ptr<MetadataBlock> Area::GetMetadataBlock(uint32_t block_number, Block::BlockSize size) const {
+std::expected<std::shared_ptr<MetadataBlock>, WfsError> Area::GetMetadataBlock(uint32_t block_number,
+                                                                               Block::BlockSize size) const {
   return MetadataBlock::LoadBlock(device_, header_block_->BlockNumber() + ToBasicBlockNumber(block_number), size,
                                   IV(block_number));
 }
 
-std::shared_ptr<DataBlock> Area::GetDataBlock(uint32_t block_number,
-                                              Block::BlockSize size,
-                                              uint32_t data_size,
-                                              const DataBlock::DataBlockHash& data_hash,
-                                              bool encrypted) const {
+std::expected<std::shared_ptr<DataBlock>, WfsError> Area::GetDataBlock(uint32_t block_number,
+                                                                       Block::BlockSize size,
+                                                                       uint32_t data_size,
+                                                                       const DataBlock::DataBlockHash& data_hash,
+                                                                       bool encrypted) const {
   return DataBlock::LoadBlock(device_, header_block_->BlockNumber() + ToBasicBlockNumber(block_number), size, data_size,
                               IV(block_number), data_hash, encrypted);
 }
@@ -106,10 +112,16 @@ uint32_t Area::BlocksCount() const {
   return root_directory_attributes_.Attributes()->blocks_count.value();
 }
 
-std::shared_ptr<const FreeBlocksAllocator> Area::GetFreeBlocksAllocator() const {
-  return std::make_unique<FreeBlocksAllocator>(shared_from_this(), FreeBlocksAllocatorBlockNumber);
+std::expected<std::shared_ptr<const FreeBlocksAllocator>, WfsError> Area::GetFreeBlocksAllocator() const {
+  auto metadata_block = GetMetadataBlock(FreeBlocksAllocatorBlockNumber);
+  if (!metadata_block.has_value())
+    return std::unexpected(WfsError::kFreeBlocksAllocatorCorrupted);
+  return std::make_unique<FreeBlocksAllocator>(shared_from_this(), std::move(*metadata_block));
 }
 
-std::shared_ptr<FreeBlocksAllocator> Area::GetFreeBlocksAllocator() {
-  return std::make_unique<FreeBlocksAllocator>(shared_from_this(), FreeBlocksAllocatorBlockNumber);
+std::expected<std::shared_ptr<FreeBlocksAllocator>, WfsError> Area::GetFreeBlocksAllocator() {
+  auto res = as_const(this)->GetFreeBlocksAllocator();
+  if (!res.has_value())
+    return std::unexpected(res.error());
+  return std::const_pointer_cast<FreeBlocksAllocator>(*res);
 }