From 45988aea98d6c4da3917afe0d064d46f9860096b Mon Sep 17 00:00:00 2001
From: Patrick Kappl <kappl.pa@gmail.com>
Date: Sat, 5 Oct 2024 13:55:52 +0000
Subject: [PATCH] Refactor `LfsWrapper.[ch]pp`

- Always pass paths as `Path` aka `etl::string` and use `c_str()` to
  ensure that the littlefs functions get null-terminated strings
- Move member variables which are only used in `CreateLockFile()` to
  that function
---
 Sts1CobcSw/FileSystem/LfsWrapper.cpp | 33 ++++++++++++++--------------
 Sts1CobcSw/FileSystem/LfsWrapper.hpp |  9 +++-----
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/Sts1CobcSw/FileSystem/LfsWrapper.cpp b/Sts1CobcSw/FileSystem/LfsWrapper.cpp
index c3f09b1b..fbf545bc 100644
--- a/Sts1CobcSw/FileSystem/LfsWrapper.cpp
+++ b/Sts1CobcSw/FileSystem/LfsWrapper.cpp
@@ -43,22 +43,23 @@ auto Unmount() -> Result<void>
 }
 
 
-auto Open(std::string_view path, unsigned int flags) -> Result<File>
+auto Open(Path const & path, unsigned int flags) -> Result<File>
 {
     auto file = File();
-    file.lockFilePath_ = Path(path.data(), path.size()).append(".lock");
-
-    auto errorLockFile = file.CreateLockFile();
-    if(errorLockFile.has_error())
+    // We need to create a temporary with Path(path) here since append() modifies the object and we
+    // don't want to change the original path
+    file.lockFilePath_ = Path(path).append(".lock");
+    auto createLockFileResult = file.CreateLockFile();
+    if(createLockFileResult.has_error())
     {
-        return errorLockFile.error();
+        return createLockFileResult.error();
     }
 
     auto error = lfs_file_opencfg(
-        &lfs, &file.lfsFile_, path.data(), static_cast<int>(flags), &file.lfsFileConfig_);
+        &lfs, &file.lfsFile_, path.c_str(), static_cast<int>(flags), &file.lfsFileConfig_);
     if(error == 0)
     {
-        file.path_ = Path(path.data(), path.size());
+        file.path_ = path;
         file.openFlags_ = flags;
         file.isOpen_ = true;
         return file;
@@ -115,7 +116,7 @@ auto File::Close() -> Result<void>
         return outcome_v2::success();
     }
 
-    auto error = lfs_remove(&lfs, lockFilePath_.data());
+    auto error = lfs_remove(&lfs, lockFilePath_.c_str());
     if(error != 0)
     {
         return static_cast<ErrorCode>(error);
@@ -161,16 +162,16 @@ auto File::MoveConstructFrom(File * other) noexcept -> void
 }
 
 
-auto File::CreateLockFile() noexcept -> Result<int>
+auto File::CreateLockFile() noexcept -> Result<void>
 {
-    lfs_file_t lfsLockFile;
-
+    auto lfsLockFile = lfs_file_t{};
+    auto lockFileBuffer = std::array<Byte, lfsCacheSize>{};
+    auto lfsLockFileConfig = lfs_file_config{.buffer = lockFileBuffer.data()};
     auto error = lfs_file_opencfg(&lfs,
                                   &lfsLockFile,
-                                  lockFilePath_.data(),
-                                  static_cast<unsigned int>(LFS_O_RDWR | LFS_O_CREAT) | LFS_O_EXCL,
-                                  &lfsLockFileConfig_);
-
+                                  lockFilePath_.c_str(),
+                                  static_cast<unsigned int>(LFS_O_RDWR) | LFS_O_CREAT | LFS_O_EXCL,
+                                  &lfsLockFileConfig);
     if(error == 0)
     {
         error = lfs_file_close(&lfs, &lfsLockFile);
diff --git a/Sts1CobcSw/FileSystem/LfsWrapper.hpp b/Sts1CobcSw/FileSystem/LfsWrapper.hpp
index f25ea73e..e20918b6 100644
--- a/Sts1CobcSw/FileSystem/LfsWrapper.hpp
+++ b/Sts1CobcSw/FileSystem/LfsWrapper.hpp
@@ -24,10 +24,9 @@ class File;
 
 [[nodiscard]] auto Mount() -> Result<void>;
 [[nodiscard]] auto Unmount() -> Result<void>;
-[[nodiscard]] auto Open(std::string_view path, unsigned int flags) -> Result<File>;
+[[nodiscard]] auto Open(Path const & path, unsigned int flags) -> Result<File>;
 
 
-// FIXME: Make File const-correct (only Write() should be non-const)
 // TODO: Consider moving this class to a separate file
 class File
 {
@@ -38,7 +37,7 @@ class File
     auto operator=(File && other) noexcept -> File &;
     ~File();
 
-    friend auto Open(std::string_view path, unsigned int flags) -> Result<File>;
+    friend auto Open(Path const & path, unsigned int flags) -> Result<File>;
 
     template<std::size_t extent>
     [[nodiscard]] auto Read(std::span<Byte, extent> data) const -> Result<int>;
@@ -52,7 +51,7 @@ class File
     // Only allow creation of File class through friend function Open()
     File() = default;
     auto MoveConstructFrom(File * other) noexcept -> void;
-    [[nodiscard]] auto CreateLockFile() noexcept -> Result<int>;
+    [[nodiscard]] auto CreateLockFile() noexcept -> Result<void>;
     [[nodiscard]] auto Read(void * buffer, std::size_t size) const -> Result<int>;
     [[nodiscard]] auto Write(void const * buffer, std::size_t size) -> Result<int>;
 
@@ -62,9 +61,7 @@ class File
     mutable lfs_file_t lfsFile_ = {};
     bool isOpen_ = false;
     std::array<Byte, lfsCacheSize> buffer_ = {};
-    std::array<Byte, lfsCacheSize> bufferLockFile_ = {};
     lfs_file_config lfsFileConfig_ = {.buffer = buffer_.data()};
-    lfs_file_config lfsLockFileConfig_ = {.buffer = bufferLockFile_.data()};
 };
 }