diff --git a/include/metall/detail/properly_closed_mark.hpp b/include/metall/detail/properly_closed_mark.hpp index 270050cb..520716c5 100644 --- a/include/metall/detail/properly_closed_mark.hpp +++ b/include/metall/detail/properly_closed_mark.hpp @@ -22,6 +22,8 @@ namespace metall::mtlldetail { */ struct properly_closed_mark { private: + // if mark_path is empty, this mark is in an invalid state + // and should not create the mark on close std::filesystem::path m_mark_path{}; int m_mark_fd = -1; bool m_read_only = false; @@ -64,6 +66,18 @@ struct properly_closed_mark { return ret == 0; } + bool open_impl() { + if (m_read_only) { + return lock_shared(); + } else { + if (!lock_exclusive()) { + return false; + } + + return remove_file(m_mark_path); + } + } + public: properly_closed_mark() noexcept = default; @@ -71,22 +85,26 @@ struct properly_closed_mark { m_mark_path = path; m_read_only = false; - return remove_file(m_mark_path); + if (!remove_file(m_mark_path)) { + // could not create, prevent mark creation on close + m_mark_path.clear(); + return false; + } + + return true; } bool open(const std::filesystem::path &path, const bool read_only) { m_mark_path = path; m_read_only = read_only; - if (read_only) { - return lock_shared(); - } else { - if (!lock_exclusive()) { - return false; - } - - return remove_file(m_mark_path); + if (!open_impl()) { + // could not open, prevent mark creation on close + m_mark_path.clear(); + return false; } + + return true; } properly_closed_mark(const properly_closed_mark &other) = delete; @@ -111,7 +129,7 @@ struct properly_closed_mark { } void close() { - if (!m_read_only) { + if (!m_read_only && !m_mark_path.empty()) { create_file(m_mark_path); } unlock(); diff --git a/test/kernel/manager_test.cpp b/test/kernel/manager_test.cpp index ff37e047..26ae3d2e 100644 --- a/test/kernel/manager_test.cpp +++ b/test/kernel/manager_test.cpp @@ -150,6 +150,49 @@ TEST(ManagerTest, CreateAndOpenModes) { } } +TEST(ManagerTest, ProperlyClosedMarkBug) { + manager_type::remove(dir_path()); + + { + manager_type mg1{metall::create_only, dir_path()}; + ASSERT_FALSE(manager_type::consistent(dir_path())); // properly closed mark should not exist + + { + manager_type mg2{metall::open_only, dir_path()}; + } + + ASSERT_FALSE(manager_type::consistent(dir_path())); // properly closed mark should still not exist (mg2 should not create it) + } + + ASSERT_TRUE(manager_type::consistent(dir_path())); // now mg1 should have created it +} + +TEST(ManagerTest, MoveManager) { + auto const p1 = dir_path() / "1"; + auto const p2 = dir_path() / "2"; + + manager_type::remove(p1); + manager_type::remove(p2); + + { + manager_type mg1{metall::create_only, p1}; + ASSERT_FALSE(manager_type::consistent(p1)); // properly closed mark should not exist + + { + manager_type mg2{metall::create_only, p2}; + auto tmp = std::move(mg2); + mg2 = std::move(mg1); + mg1 = std::move(tmp); + } + + ASSERT_TRUE(manager_type::consistent(p1)); + ASSERT_FALSE(manager_type::consistent(p2)); + } + + ASSERT_TRUE(manager_type::consistent(p1)); + ASSERT_TRUE(manager_type::consistent(p2)); +} + TEST(ManagerTest, ConstructArray) { { {