From 69d190e48572f9af964eceb0504aab27a9c0a240 Mon Sep 17 00:00:00 2001 From: "Scott E. Graves" Date: Thu, 7 Nov 2024 12:32:07 -0600 Subject: [PATCH] winfsp unit tests and fixes --- .../include/file_manager/i_open_file.hpp | 38 ++--- .../include/file_manager/open_file.hpp | 2 +- .../include/file_manager/open_file_base.hpp | 2 +- .../src/file_manager/file_manager.cpp | 41 ++--- .../src/file_manager/open_file.cpp | 6 +- .../src/file_manager/open_file_base.cpp | 2 +- .../src/winfsp_drive_delete_test.cpp | 150 ++++++++++++++---- 7 files changed, 163 insertions(+), 78 deletions(-) diff --git a/repertory/librepertory/include/file_manager/i_open_file.hpp b/repertory/librepertory/include/file_manager/i_open_file.hpp index 28988d6c..4f180163 100644 --- a/repertory/librepertory/include/file_manager/i_open_file.hpp +++ b/repertory/librepertory/include/file_manager/i_open_file.hpp @@ -40,25 +40,25 @@ public: [[nodiscard]] virtual auto get_filesystem_item() const -> filesystem_item = 0; - [[nodiscard]] virtual auto - get_open_data() -> std::map & = 0; + [[nodiscard]] virtual auto get_open_data() + -> std::map & = 0; - [[nodiscard]] virtual auto - get_open_data() const -> const std::map & = 0; + [[nodiscard]] virtual auto get_open_data() const + -> const std::map & = 0; - [[nodiscard]] virtual auto - get_open_data(std::uint64_t handle) -> open_file_data & = 0; + [[nodiscard]] virtual auto get_open_data(std::uint64_t handle) + -> open_file_data & = 0; - [[nodiscard]] virtual auto - get_open_data(std::uint64_t handle) const -> const open_file_data & = 0; + [[nodiscard]] virtual auto get_open_data(std::uint64_t handle) const + -> const open_file_data & = 0; [[nodiscard]] virtual auto get_open_file_count() const -> std::size_t = 0; - [[nodiscard]] virtual auto - get_read_state() const -> boost::dynamic_bitset<> = 0; + [[nodiscard]] virtual auto get_read_state() const + -> boost::dynamic_bitset<> = 0; - [[nodiscard]] virtual auto - get_read_state(std::size_t chunk) const -> bool = 0; + [[nodiscard]] virtual auto get_read_state(std::size_t chunk) const + -> bool = 0; [[nodiscard]] virtual auto get_source_path() const -> std::string = 0; @@ -74,11 +74,11 @@ public: native_operation_callback callback) -> api_error = 0; [[nodiscard]] virtual auto read(std::size_t read_size, - std::uint64_t read_offset, - data_buffer &data) -> api_error = 0; + std::uint64_t read_offset, data_buffer &data) + -> api_error = 0; - [[nodiscard]] virtual auto - resize(std::uint64_t new_file_size) -> api_error = 0; + [[nodiscard]] virtual auto resize(std::uint64_t new_file_size) + -> api_error = 0; virtual void set_api_path(const std::string &api_path) = 0; @@ -97,8 +97,8 @@ public: virtual auto close() -> bool = 0; - [[nodiscard]] virtual auto - get_handles() const -> std::vector = 0; + [[nodiscard]] virtual auto get_handles() const + -> std::vector = 0; [[nodiscard]] virtual auto is_complete() const -> bool = 0; @@ -106,7 +106,7 @@ public: [[nodiscard]] virtual auto is_write_supported() const -> bool = 0; - virtual void remove(std::uint64_t handle) = 0; + virtual void remove(std::uint64_t handle, bool noQueue = false) = 0; }; } // namespace repertory diff --git a/repertory/librepertory/include/file_manager/open_file.hpp b/repertory/librepertory/include/file_manager/open_file.hpp index 3e031d01..5ede36d4 100644 --- a/repertory/librepertory/include/file_manager/open_file.hpp +++ b/repertory/librepertory/include/file_manager/open_file.hpp @@ -107,7 +107,7 @@ public: native_operation_callback callback) -> api_error override; - void remove(std::uint64_t handle) override; + void remove(std::uint64_t handle, bool noQueue = false) override; [[nodiscard]] auto read(std::size_t read_size, std::uint64_t read_offset, data_buffer &data) -> api_error override; diff --git a/repertory/librepertory/include/file_manager/open_file_base.hpp b/repertory/librepertory/include/file_manager/open_file_base.hpp index 707b9637..4da2862e 100644 --- a/repertory/librepertory/include/file_manager/open_file_base.hpp +++ b/repertory/librepertory/include/file_manager/open_file_base.hpp @@ -185,7 +185,7 @@ public: [[nodiscard]] auto is_modified() const -> bool override; - void remove(std::uint64_t handle) override; + void remove(std::uint64_t handle, bool noQueue = false) override; void set_api_path(const std::string &api_path) override; }; diff --git a/repertory/librepertory/src/file_manager/file_manager.cpp b/repertory/librepertory/src/file_manager/file_manager.cpp index 3522c8c7..857427ac 100644 --- a/repertory/librepertory/src/file_manager/file_manager.cpp +++ b/repertory/librepertory/src/file_manager/file_manager.cpp @@ -129,25 +129,39 @@ void file_manager::close(std::uint64_t handle) { recur_mutex_lock file_lock(open_file_mtx_); auto closeable_file = get_open_file_by_handle(handle); if (closeable_file) { - closeable_file->remove(handle); + closeable_file->remove(handle, false); } } void file_manager::close_all(const std::string &api_path) { + REPERTORY_USES_FUNCTION_NAME(); + recur_mutex_lock file_lock(open_file_mtx_); - std::vector handles; + auto file_iter = open_file_lookup_.find(api_path); if (file_iter == open_file_lookup_.end()) { return; } - auto closeable_file = file_iter->second; + remove_upload(api_path, true); - handles = closeable_file->get_handles(); - for (auto &&handle : handles) { - closeable_file->remove(handle); - } + auto closeable_file = file_iter->second; open_file_lookup_.erase(api_path); + + auto handles = closeable_file->get_handles(); + for (auto &&handle : handles) { + closeable_file->remove(handle, false); + } + + auto result = utils::db::sqlite::db_delete{*db_, resume_table} + .where("api_path") + .equals(api_path) + .go(); + if (not result.ok()) { + utils::error::raise_api_path_error(function_name, api_path, + api_error::error, + "failed to remove from resume table"); + } } void file_manager::close_timed_out_files() { @@ -505,6 +519,7 @@ void file_manager::queue_upload(const std::string &api_path, if (not no_lock) { lock = std::make_unique(upload_mtx_); } + remove_upload(api_path, true); auto result = @@ -542,18 +557,6 @@ auto file_manager::remove_file(const std::string &api_path) -> api_error { close_all(api_path); - remove_upload(api_path, true); - - auto result = utils::db::sqlite::db_delete{*db_, resume_table} - .where("api_path") - .equals(api_path) - .go(); - if (not result.ok()) { - utils::error::raise_api_path_error(function_name, api_path, - api_error::error, - "failed to remove from resume table"); - } - res = provider_.remove_file(api_path); if (res != api_error::success) { return res; diff --git a/repertory/librepertory/src/file_manager/open_file.cpp b/repertory/librepertory/src/file_manager/open_file.cpp index 3bfbc3ec..92446582 100644 --- a/repertory/librepertory/src/file_manager/open_file.cpp +++ b/repertory/librepertory/src/file_manager/open_file.cpp @@ -388,10 +388,10 @@ auto open_file::read(std::size_t read_size, std::uint64_t read_offset, : get_api_error(); } -void open_file::remove(std::uint64_t handle) { +void open_file::remove(std::uint64_t handle, bool noQueue) { recur_mutex_lock file_lock(file_mtx_); - open_file_base::remove(handle); - if (modified_ && read_state_.all() && + open_file_base::remove(handle, noQueue); + if (not noQueue && modified_ && read_state_.all() && (get_api_error() == api_error::success)) { mgr_.queue_upload(*this); modified_ = false; diff --git a/repertory/librepertory/src/file_manager/open_file_base.cpp b/repertory/librepertory/src/file_manager/open_file_base.cpp index 6ae7e30b..5f6e3e47 100644 --- a/repertory/librepertory/src/file_manager/open_file_base.cpp +++ b/repertory/librepertory/src/file_manager/open_file_base.cpp @@ -235,7 +235,7 @@ auto open_file_base::is_modified() const -> bool { return modified_; } -void open_file_base::remove(std::uint64_t handle) { +void open_file_base::remove(std::uint64_t handle, bool /* noQueue */) { recur_mutex_lock file_lock(file_mtx_); open_data_.erase(handle); event_system::instance().raise( diff --git a/repertory/repertory_test/src/winfsp_drive_delete_test.cpp b/repertory/repertory_test/src/winfsp_drive_delete_test.cpp index f5797fd9..f353b353 100644 --- a/repertory/repertory_test/src/winfsp_drive_delete_test.cpp +++ b/repertory/repertory_test/src/winfsp_drive_delete_test.cpp @@ -138,53 +138,60 @@ TYPED_TEST(winfsp_test, delete_can_delete_after_mapping) { auto seed = static_cast(std::time(nullptr)); srand(seed); - auto handle = ::CreateFileA(file_path.c_str(), GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, - CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); - ASSERT_NE(INVALID_HANDLE_VALUE, handle); - SYSTEM_INFO sys_info{}; ::GetSystemInfo(&sys_info); - auto *mapping = - ::CreateFileMappingA(handle, nullptr, PAGE_READWRITE, 0, - 16U * sys_info.dwAllocationGranularity, nullptr); - EXPECT_TRUE(::CloseHandle(handle)); - ASSERT_TRUE(mapping != nullptr); + { + auto handle = ::CreateFileA(file_path.c_str(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, + CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); + ASSERT_NE(INVALID_HANDLE_VALUE, handle); - auto *view = ::MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, 0); - ASSERT_TRUE(view != nullptr); + auto *mapping = + ::CreateFileMappingA(handle, nullptr, PAGE_READWRITE, 0, + 16U * sys_info.dwAllocationGranularity, nullptr); + EXPECT_TRUE(::CloseHandle(handle)); + ASSERT_TRUE(mapping != nullptr); - for (PUINT8 ptr = view, end = ptr + 16U * sys_info.dwAllocationGranularity; - end > ptr; ++ptr) { - *ptr = rand() & 0xFF; + auto *view = ::MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, 0); + ASSERT_TRUE(view != nullptr); + + for (PUINT8 ptr = reinterpret_cast(view), + end = ptr + 16U * sys_info.dwAllocationGranularity; + end > ptr; ++ptr) { + *ptr = rand() & 0xFF; + } + + EXPECT_TRUE(::UnmapViewOfFile(view)); + EXPECT_TRUE(::CloseHandle(mapping)); } - EXPECT_TRUE(::UnmapViewOfFile(view)); - EXPECT_TRUE(::CloseHandle(mapping)); + { + auto handle = + ::CreateFileA(file_path2.c_str(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, CREATE_NEW, + FILE_ATTRIBUTE_NORMAL, nullptr); + ASSERT_NE(INVALID_HANDLE_VALUE, handle); - auto handle = ::CreateFileA(file_path2.c_str(), GENERIC_READ | GENERIC_WRITE, - FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, - CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); - ASSERT_NE(INVALID_HANDLE_VALUE, handle); + auto mapping = + ::CreateFileMappingA(handle, nullptr, PAGE_READWRITE, 0, + 16U * sys_info.dwAllocationGranularity, nullptr); + EXPECT_TRUE(::CloseHandle(handle)); + ASSERT_TRUE(mapping != nullptr); - mapping = - ::CreateFileMappingA(handle, nullptr, PAGE_READWRITE, 0, - 16U * sys_info.dwAllocationGranularity, nullptr); - EXPECT_TRUE(::CloseHandle(handle)); - ASSERT_TRUE(mapping != nullptr); + auto view = ::MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, 0); + EXPECT_TRUE(view != nullptr); - view = ::MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, 0); - EXPECT_TRUE(view != nullptr); + for (PUINT8 ptr = reinterpret_cast(view), + end = ptr + 16U * sys_info.dwAllocationGranularity; + end > ptr; ++ptr) { + *ptr = rand() & 0xFF; + } - for (PUINT8 ptr = view, end = ptr + 16U * sys_info.dwAllocationGranularity; - end > ptr; ++ptr) { - *ptr = rand() & 0xff; + EXPECT_TRUE(::UnmapViewOfFile(view)); + EXPECT_TRUE(::CloseHandle(mapping)); } - EXPECT_TRUE(::UnmapViewOfFile(view)); - EXPECT_TRUE(::CloseHandle(mapping)); - EXPECT_TRUE(::DeleteFileA(file_path.c_str())); EXPECT_TRUE(::DeleteFileA(file_path2.c_str())); @@ -192,6 +199,81 @@ TYPED_TEST(winfsp_test, delete_can_delete_after_mapping) { EXPECT_FALSE(::RemoveDirectoryA(dir_path.c_str())); EXPECT_EQ(ERROR_FILE_NOT_FOUND, ::GetLastError()); } + +TYPED_TEST(winfsp_test, delete_can_delete_on_close_after_mapping) { + auto dir_path{ + utils::path::combine(this->mount_location, {"test_dir_3"}), + }; + auto file_path{ + utils::path::combine(dir_path, {"test_file_3"}), + }; + auto file_path2{ + utils::path::combine(dir_path, {"test_file2_3"}), + }; + + ASSERT_TRUE(::CreateDirectoryA(dir_path.c_str(), nullptr)); + + auto seed = static_cast(std::time(nullptr)); + srand(seed); + + SYSTEM_INFO sys_info{}; + ::GetSystemInfo(&sys_info); + + { + auto handle = ::CreateFileA( + file_path.c_str(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, CREATE_NEW, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, nullptr); + ASSERT_NE(INVALID_HANDLE_VALUE, handle); + + auto *mapping = + ::CreateFileMappingA(handle, nullptr, PAGE_READWRITE, 0, + 16U * sys_info.dwAllocationGranularity, nullptr); + EXPECT_TRUE(mapping != nullptr); + + auto *view = ::MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, 0); + EXPECT_TRUE(view != nullptr); + for (PUINT8 ptr = reinterpret_cast(view), + end = ptr + 16U * sys_info.dwAllocationGranularity; + end > ptr; ++ptr) { + *ptr = rand() & 0xFF; + } + + EXPECT_TRUE(::UnmapViewOfFile(view)); + EXPECT_TRUE(::CloseHandle(mapping)); + EXPECT_TRUE(::CloseHandle(handle)); + } + + { + auto handle = ::CreateFileA( + file_path2.c_str(), GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, nullptr, CREATE_NEW, + FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, nullptr); + ASSERT_NE(INVALID_HANDLE_VALUE, handle); + + auto *mapping = + ::CreateFileMappingA(handle, nullptr, PAGE_READWRITE, 0, + 16U * sys_info.dwAllocationGranularity, nullptr); + EXPECT_TRUE(mapping != nullptr); + + auto *view = MapViewOfFile(mapping, FILE_MAP_ALL_ACCESS, 0, 0, 0); + EXPECT_TRUE(view != nullptr); + + for (PUINT8 ptr = reinterpret_cast(view), + end = ptr + 16U * sys_info.dwAllocationGranularity; + end > ptr; ++ptr) { + *ptr = rand() & 0xFF; + } + + EXPECT_TRUE(::UnmapViewOfFile(view)); + EXPECT_TRUE(::CloseHandle(mapping)); + EXPECT_TRUE(::CloseHandle(handle)); + } + + EXPECT_TRUE(::RemoveDirectoryA(dir_path.c_str())); + EXPECT_FALSE(::RemoveDirectoryA(dir_path.c_str())); + EXPECT_EQ(ERROR_FILE_NOT_FOUND, ::GetLastError()); +} } // namespace repertory #endif // defined(_WIN32)