diff --git a/src/libslic3r/Format/bbs_3mf.cpp b/src/libslic3r/Format/bbs_3mf.cpp index 2914693c42..86e306d4ac 100644 --- a/src/libslic3r/Format/bbs_3mf.cpp +++ b/src/libslic3r/Format/bbs_3mf.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -9776,13 +9777,13 @@ bool has_restore_data(std::string & path, std::string& origin) origin = ""; return false; } - // ORCA: non-throwing probes; ec is deliberately unread, a backup path that cannot be - // probed is treated as having no restore data rather than aborting the application. + // ORCA: the caller removes this directory unless origin is "", so every way of + // failing to establish the backup's state has to claim the lock instead. boost::system::error_code ec; if (boost::filesystem::exists(path + "/lock.txt", ec)) { std::string pid; - load_string_file(path + "/lock.txt", pid); try { + load_string_file(path + "/lock.txt", pid); if (get_process_name(boost::lexical_cast(pid)) == get_process_name(0)) { origin = ""; @@ -9790,12 +9791,20 @@ bool has_restore_data(std::string & path, std::string& origin) } } catch (...) { + origin = ""; return false; } } std::string file3mf = path + "/.3mf"; - if (!boost::filesystem::exists(file3mf, ec)) + if (!boost::filesystem::exists(file3mf, ec)) { + // ORCA: a .3mf that is simply absent is a spent backup and stays collectable. A probe + // that could not look must not be: ec reports ENOENT/ENOTDIR for "not there" as + // well, so only a different error means the answer is unknown. + if (ec && ec != boost::system::errc::no_such_file_or_directory && + ec != boost::system::errc::not_a_directory) + origin = ""; return false; + } try { if (boost::filesystem::exists(path + "/origin.txt", ec)) load_string_file(path + "/origin.txt", origin); diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index e3cf4b92c5..2d5656b733 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -2361,5 +2361,96 @@ TEST_CASE("has_restore_data reports no restore data for a path it cannot probe", REQUIRE_NOTHROW(has = has_restore_data(unprobeable, origin2)); CHECK_FALSE(has); CHECK(unprobeable == backup.string()); // not rewritten to the .3mf it could not confirm + CHECK(origin2 == ""); // and the caller is told not to remove it } #endif // _WIN32 + +// Every "return false" that leaves origin empty is an instruction to the caller to +// remove_all() the backup directory (Plater.cpp's EVT_RESTORE_PROJECT handler), so the +// distinction these cases draw is between a backup that is spent and one whose state could +// not be established. Only the first may be collected. +TEST_CASE("has_restore_data claims the lock when lock.txt cannot be interpreted", "[3mf]") { + ScopedTemporaryDir outer("orca_lock"); + boost::filesystem::path backup = outer.path() / "backup"; + REQUIRE(boost::filesystem::create_directory(backup)); + std::ofstream((backup / ".3mf").string()) << "stand-in for the backed-up project"; + + SECTION("an empty lock.txt holds no pid") { + std::ofstream lock((backup / "lock.txt").string()); + lock.close(); // created, left zero length + REQUIRE(boost::filesystem::file_size(backup / "lock.txt") == 0); + + std::string path = backup.string(), origin; + bool has = true; + REQUIRE_NOTHROW(has = has_restore_data(path, origin)); + CHECK_FALSE(has); + CHECK(origin == ""); + } + + SECTION("lock.txt holds something that is not a pid") { + std::ofstream((backup / "lock.txt").string()) << "not-a-pid"; + + std::string path = backup.string(), origin; + bool has = true; + REQUIRE_NOTHROW(has = has_restore_data(path, origin)); + CHECK_FALSE(has); + CHECK(origin == ""); + } +} + +#ifndef _WIN32 +TEST_CASE("has_restore_data claims the lock when lock.txt cannot be read", "[3mf]") { + if (::geteuid() == 0) + SKIP("root ignores the file permissions this test depends on"); + + ScopedTemporaryDir outer("orca_lock"); + boost::filesystem::path backup = outer.path() / "backup"; + REQUIRE(boost::filesystem::create_directory(backup)); + std::ofstream((backup / ".3mf").string()) << "stand-in for the backed-up project"; + std::ofstream((backup / "lock.txt").string()) << "1"; + boost::system::error_code mode_ec; // a filesystem that refuses the mode change must skip below + boost::filesystem::permissions(backup / "lock.txt", boost::filesystem::no_perms, mode_ec); + + boost::nowide::ifstream probe((backup / "lock.txt").string()); + if (probe.good()) + SKIP("this filesystem does not enforce file read permissions"); + + // load_string_file() throws here. + std::string path = backup.string(), origin; + bool has = true; + REQUIRE_NOTHROW(has = has_restore_data(path, origin)); + CHECK_FALSE(has); + CHECK(origin == ""); +} +#endif // _WIN32 + +// The negative control for the three cases above: claiming the lock whenever anything is +// amiss would strand every spent backup on disk forever, so a readable directory that +// simply has nothing to restore must still come back collectable. +TEST_CASE("has_restore_data leaves a spent backup collectable", "[3mf]") { + ScopedTemporaryDir outer("orca_spent"); + boost::filesystem::path backup = outer.path() / "backup"; + REQUIRE(boost::filesystem::create_directory(backup)); + + SECTION("no lock and no project") { + std::string path = backup.string(), origin; + CHECK_FALSE(has_restore_data(path, origin)); + CHECK(origin.empty()); + } + + SECTION("a lock from a process that is not us, and no project") { + // get_process_name() compares executable names and answers empty for a pid it cannot + // read, so pid 1 never matches this process and the lock must not be claimed. + std::ofstream((backup / "lock.txt").string()) << "1"; + + std::string path = backup.string(), origin; + CHECK_FALSE(has_restore_data(path, origin)); + CHECK(origin.empty()); + } + + SECTION("the directory does not exist at all") { + std::string path = (outer.path() / "gone").string(), origin; + CHECK_FALSE(has_restore_data(path, origin)); + CHECK(origin.empty()); + } +}