fix: Address PR sugestions

Co-authored-by: Antuan <antuan@leil.io> Co-authored-by: Guillex <guillex@leil.io>
leil-io · Oct 14, 2024 · bdeab93 · bdeab93
1 parent d6e0dbf
commit bdeab93
Show file tree

Hide file tree

Showing 7 changed files with 155 additions and 78 deletions.
diff --git a/src/chunkserver/chunkserver-common/chunk_trash_manager.cc b/src/chunkserver/chunkserver-common/chunk_trash_manager.cc
@@ -1,6 +1,20 @@
-// chunk_trash_manager.cc
+/*
+   Copyright 2023-2024  Leil Storage OÜ
 
-#include <iostream> // For debugging output
+   This file is part of SaunaFS.
+
+   SaunaFS is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, version 3.
+
+   SaunaFS is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with SaunaFS. If not, see <http://www.gnu.org/licenses/>.
+ */
 
 #include "chunk_trash_manager.h"
 #include "errors/saunafs_error_codes.h"
@@ -15,26 +29,26 @@ std::string ChunkTrashManager::getDeletionTimeString() {
 	return oss.str();
 }
 
-int ChunkTrashManager::moveToTrash(const std::filesystem::path& filePath, const std::filesystem::path& diskPath, const std::string& deletionTime) {
+int ChunkTrashManager::moveToTrash(const std::filesystem::path &filePath,
+                                   const std::filesystem::path &diskPath,
+                                   const std::string &deletionTime) {
 	if (!std::filesystem::exists(filePath)) {
-		// Log warning or error
 		return SAUNAFS_ERROR_ENOENT;
 	}
 
 	const std::filesystem::path trashDir = diskPath / kTrashDirname;
 	std::filesystem::create_directories(trashDir);
 
 	if (!filePath.string().starts_with(diskPath.string())) {
-		// Log warning or error
 		return SAUNAFS_ERROR_EINVAL;
 	}
 
-	const std::filesystem::path trashPath = trashDir / (filePath.filename().string() + "." + deletionTime);
+	const std::filesystem::path trashPath =
+			trashDir / (filePath.filename().string() + "." + deletionTime);
 
 	try {
 		std::filesystem::rename(filePath, trashPath);
-	} catch (const std::filesystem::filesystem_error& e) {
-		// Log error
+	} catch (const std::filesystem::filesystem_error &e) {
 		return SAUNAFS_ERROR_IO;
 	}
 

diff --git a/src/chunkserver/chunkserver-common/chunk_trash_manager.h b/src/chunkserver/chunkserver-common/chunk_trash_manager.h
@@ -1,6 +1,24 @@
-// chunk_trash_manager.h
+/*
+   Copyright 2023-2024  Leil Storage OÜ
+
+   This file is part of SaunaFS.
+
+   SaunaFS is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, version 3.
+
+   SaunaFS is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with SaunaFS. If not, see <http://www.gnu.org/licenses/>.
+ */
 #pragma once
 
+#include "common/platform.h"
+
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Warray-bounds"
 #pragma GCC diagnostic ignored "-Wstringop-overflow"
@@ -12,6 +30,7 @@ class ChunkTrashManager {
 public:
 	constexpr static const std::string_view kTrashDirname = ".trash.bin";
 	static std::string getDeletionTimeString();
-	static int moveToTrash(const std::filesystem::path& filePath,
-	                       const std::filesystem::path& diskPath, const std::string& deletionTime);
+	static int moveToTrash(const std::filesystem::path &filePath,
+	                       const std::filesystem::path &diskPath,
+	                       const std::string &deletionTime);
 };
diff --git a/src/chunkserver/chunkserver-common/chunk_trash_manager_unittest.cc b/src/chunkserver/chunkserver-common/chunk_trash_manager_unittest.cc
@@ -1,4 +1,20 @@
-// chunk_trash_manager_unittest.cc
+/*
+   Copyright 2023-2024  Leil Storage OÜ
+
+   This file is part of SaunaFS.
+
+   SaunaFS is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, version 3.
+
+   SaunaFS is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with SaunaFS. If not, see <http://www.gnu.org/licenses/>.
+ */
 // A fix for https://stackoverflow.com/q/77034039/10788155
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Warray-bounds"
@@ -22,58 +38,59 @@ class ChunkTrashManagerTest : public ::testing::Test {
 	}
 
 	void TearDown() override {
-		std::filesystem::remove_all(testDir);
+		if (std::filesystem::remove_all(testDir) == 0) {
+			std::cerr << "Failed to remove test directory: " << testDir << '\n';
+		}
 	}
 };
 
 std::filesystem::path ChunkTrashManagerTest::testDir;
 
 // Test getDeletionTimeString() to ensure it produces a UTC timestamp
 TEST_F(ChunkTrashManagerTest, GetDeletionTimeStringTest) {
-	ChunkTrashManager manager;
-	std::string timestamp = manager.getDeletionTimeString();
+	std::string timestamp = ChunkTrashManager::getDeletionTimeString();
 	EXPECT_EQ(timestamp.size(), 14);  // Check that the timestamp length is correct (YYYYMMDDHHMMSS)
 	EXPECT_TRUE(std::all_of(timestamp.begin(), timestamp.end(), ::isdigit));  // Check that all characters are digits
 }
 
 TEST_F(ChunkTrashManagerTest, MoveToTrashValidFile) {
-	// Test the moveToTrash() method directly using ChunkTrashManager
-	ChunkTrashManager manager;
-	std::filesystem::path filePath = testDir / "test_file.txt";
+	std::filesystem::path const filePath = testDir / "test_file.txt";
 	std::ofstream(filePath) << "dummy content";
 
-	std::string deletionTime = manager.getDeletionTimeString();
-	int result = manager.moveToTrash(filePath, testDir, deletionTime);
+	// Test the moveToTrash() method directly using ChunkTrashManager
+	std::string const deletionTime = ChunkTrashManager::getDeletionTimeString();
+	int const result = ChunkTrashManager::moveToTrash(filePath, testDir, deletionTime);
 
-	std::filesystem::path expectedTrashPath = testDir / ChunkTrashManager::kTrashDirname / ("test_file.txt." + deletionTime);
+	std::filesystem::path const expectedTrashPath = testDir / ChunkTrashManager::kTrashDirname /
+	                                          ("test_file.txt." + deletionTime);
 	EXPECT_TRUE(std::filesystem::exists(expectedTrashPath));
 	EXPECT_EQ(result, SAUNAFS_STATUS_OK);
 }
 
 TEST_F(ChunkTrashManagerTest, MoveToTrashNonExistentFile) {
-	// Test the moveToTrash() method directly using ChunkTrashManager
-	ChunkTrashManager manager;
 	// Define a non-existent file path
-	std::filesystem::path filePath = testDir / "non_existent_file.txt";
+	std::filesystem::path const filePath = testDir / "non_existent_file.txt";
 
-	std::string deletionTime = manager.getDeletionTimeString();
-	int result = manager.moveToTrash(filePath, testDir, deletionTime);
+	// Test the moveToTrash() method directly using ChunkTrashManager
+	std::string const deletionTime = ChunkTrashManager::getDeletionTimeString();
+	int const result = ChunkTrashManager::moveToTrash(filePath, testDir, deletionTime);
 
 	// Check that the function returned the correct error code for a non-existent file
 	EXPECT_EQ(result, SAUNAFS_ERROR_ENOENT);
 }
 
 TEST_F(ChunkTrashManagerTest, MoveToTrashFileInNestedDirectory) {
-	ChunkTrashManager manager;
-	std::filesystem::path nestedDir = testDir / "nested/dir/structure";
+	std::filesystem::path const nestedDir = testDir / "nested/dir/structure";
 	std::filesystem::create_directories(nestedDir);
-	std::filesystem::path filePath = nestedDir / "test_file_nested.txt";
+	std::filesystem::path const filePath = nestedDir / "test_file_nested.txt";
 	std::ofstream(filePath) << "nested content";
 
-	std::string deletionTime = manager.getDeletionTimeString();
-	int result = manager.moveToTrash(filePath, testDir, deletionTime);
+	std::string const deletionTime = ChunkTrashManager::getDeletionTimeString();
+	int const result = ChunkTrashManager::moveToTrash(filePath, testDir, deletionTime);
 
-	std::filesystem::path expectedTrashPath = testDir / ChunkTrashManager::kTrashDirname / ("test_file_nested.txt." + deletionTime);
+	std::filesystem::path const expectedTrashPath = testDir /
+	                                          ChunkTrashManager::kTrashDirname /
+	                                          ("test_file_nested.txt." + deletionTime);
 	EXPECT_TRUE(std::filesystem::exists(expectedTrashPath));
 	EXPECT_EQ(result, SAUNAFS_STATUS_OK);
 }
diff --git a/src/chunkserver/chunkserver-common/cmr_disk.cc b/src/chunkserver/chunkserver-common/cmr_disk.cc
@@ -1,7 +1,7 @@
 #include "cmr_disk.h"
 
 #include <sys/statvfs.h>
-#include <c++/13/iostream>
+#include <iostream>
 
 #include "chunkserver-common/chunk_interface.h"
 #include "chunkserver-common/cmr_chunk.h"
@@ -13,6 +13,8 @@
 #include "devtools/request_log.h"
 #include "errors/saunafs_error_codes.h"
 
+#include "chunk_trash_manager.h"
+
 CmrDisk::CmrDisk(const std::string &_metaPath, const std::string &_dataPath,
                  bool _isMarkedForRemoval, bool _isZonedDevice)
     : FDDisk(_metaPath, _dataPath, _isMarkedForRemoval, _isZonedDevice) {}
@@ -27,11 +29,14 @@ void CmrDisk::createPathsAndSubfolders() {
 
 	if (!isMarkedForDeletion()) {
 		ret &= (::mkdir(metaPath().c_str(), mode) == 0);
-		ret &= (::mkdir((std::filesystem::path(metaPath()) / ChunkTrashManager::kTrashDirname).c_str(), mode) == 0);
+		ret &= (::mkdir((std::filesystem::path(metaPath()) /
+		                 ChunkTrashManager::kTrashDirname).c_str(), mode) == 0);
 
 		if (dataPath() != metaPath()) {
 			ret &= (::mkdir(dataPath().c_str(), mode) == 0);
-			ret &= (::mkdir((std::filesystem::path(dataPath()) / ChunkTrashManager::kTrashDirname).c_str(), mode) == 0);
+			ret &= (::mkdir((std::filesystem::path(dataPath()) /
+			                 ChunkTrashManager::kTrashDirname).c_str(), mode)
+			        == 0);
 		}
 
 		for (uint32_t i = 0; i < Subfolder::kNumberOfSubfolders; ++i) {
@@ -181,26 +186,27 @@ int CmrDisk::unlinkChunk(IChunk *chunk) {
 	const std::filesystem::path dataFile = chunk->dataFilename();
 
 	// Use the metaPath() and dataPath() to get the disk paths
-	const std::string metaDiskPath = FDDisk::metaPath();
-	const std::string dataDiskPath = FDDisk::dataPath();
+	const std::string metaDiskPath = metaPath();
+	const std::string dataDiskPath = dataPath();
 
 	// Ensure we found a valid disk path
 	if (metaDiskPath.empty() || dataDiskPath.empty()) {
-		safs_pretty_syslog(LOG_WARNING, "Error finding disk path for chunk: %s", chunk->metaFilename().c_str());
+		safs_pretty_errlog(LOG_ERR, "Error finding disk path for chunk: %s",
+		                   chunk->metaFilename().c_str());
 		return SAUNAFS_ERROR_ENOENT;
 	}
 
 	// Create a deletion timestamp
-	const std::string deletionTime = trashManager.getDeletionTimeString();
+	const std::string deletionTime = ChunkTrashManager::getDeletionTimeString();
 
 	// Move meta file to trash
-	int result = trashManager.moveToTrash(metaFile, metaDiskPath, deletionTime);
+	int result = ChunkTrashManager::moveToTrash(metaFile, metaDiskPath, deletionTime);
 	if (result != SAUNAFS_STATUS_OK) {
 		return result;
 	}
 
 	// Move data file to trash
-	result = trashManager.moveToTrash(dataFile, dataDiskPath, deletionTime);
+	result = ChunkTrashManager::moveToTrash(dataFile, dataDiskPath, deletionTime);
 	if (result != SAUNAFS_STATUS_OK) {
 		return result;
 	}

diff --git a/src/chunkserver/chunkserver-common/cmr_disk.h b/src/chunkserver/chunkserver-common/cmr_disk.h
@@ -1,15 +1,7 @@
 #pragma once
 
-// A fix for https://stackoverflow.com/q/77034039/10788155
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Warray-bounds"
-#pragma GCC diagnostic ignored "-Wstringop-overflow"
-#include <filesystem>
-#pragma GCC diagnostic pop
-
 #include "common/platform.h"
 
-#include "chunk_trash_manager.h"
 #include "chunkserver-common/disk_with_fd.h"
 
 class CmrDisk : public FDDisk {
@@ -31,7 +23,7 @@ class CmrDisk : public FDDisk {
 	CmrDisk &operator=(CmrDisk &&) = delete;
 
 	/// Virtual destructor needed for correct polymorphism
-	~CmrDisk() = default;
+	~CmrDisk() override = default;
 
 	/// Tries to create the paths and subfolders for metaPath and dataPath
 	///
@@ -41,8 +33,8 @@ class CmrDisk : public FDDisk {
 
 	/// Creates the lock files for metadata and data directories
 	void createLockFiles(
-	    bool isLockNeeded,
-	    std::vector<std::unique_ptr<IDisk>> &allDisks) override;
+	        bool isLockNeeded,
+	        std::vector<std::unique_ptr<IDisk>> &allDisks) override;
 
 	/// Updates the disk usage information preserving the reserved space
 	///
@@ -57,7 +49,7 @@ class CmrDisk : public FDDisk {
 	/// Creates a new ChunkSignature for the given Chunk.
 	/// Used mostly to write the metadata file of an existing in-memory Chunk.
 	std::unique_ptr<ChunkSignature> createChunkSignature(
-	    IChunk *chunk) override;
+	        IChunk *chunk) override;
 
 	/// Creates a new empty ChunkSignature that later will be filled with the
 	/// information of the Chunk using readFromDescriptor.
@@ -137,7 +129,4 @@ class CmrDisk : public FDDisk {
 	/// Writes to device custom blockSize from blockBuffer
 	int writeChunkData(IChunk *chunk, uint8_t *blockBuffer, int32_t blockSize,
 	                   off64_t offset) override;
-
-private:
-	ChunkTrashManager trashManager;
 };