diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 0877aa3f1e..db878724ef 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -737,10 +737,6 @@ std::string AppConfig::load() // Keep another instance from replacing or restoring the file mid-read. InstanceLock instance_lock(lock_path()); - if (instance_lock.locked()) { - const boost::filesystem::path conf(config_path()); - remove_stale_temp_files(conf.parent_path(), conf.filename().string()); - } // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; @@ -992,7 +988,6 @@ void AppConfig::save() // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); InstanceLock instance_lock(lock_path()); - std::string path_pid = (boost::format("%1%.%2%.tmp") % path % get_current_pid()).str(); json j; @@ -1122,37 +1117,25 @@ void AppConfig::save() j["local_machines"][local_machine.first] = m_json; } - boost::nowide::ofstream c; - c.open(path_pid, std::ios::out | std::ios::trunc); - c << j.dump(1, '\t') << std::endl; - + std::string config_str = j.dump(1, '\t'); #ifdef WIN32 - // WIN32 specific: The final "rename_file()" call is not safe in case of an application crash, there is no atomic "rename file" API + // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, // we save the config file into a backup first before moving it to the final destination. - c << appconfig_md5_hash_line(j.dump(1, '\t')); + // load() verifies the checksum over the text up to the closing brace, so it is taken before the newline. + const std::string md5_line = appconfig_md5_hash_line(config_str); + config_str += "\n"; + config_str += md5_line; + const std::string backup_path = (boost::format("%1%.bak") % path).str(); + if (const std::error_code ec = write_file_atomically(backup_path, config_str)) + BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message(); +#else + config_str += "\n"; #endif - - c.close(); - if (c.fail()) { - BOOST_LOG_TRIVIAL(error) << "Failed to write new configuration to " << path_pid << "; aborting attempt to overwrite original configuration"; - return; + if (const std::error_code ec = write_file_atomically(path, config_str)) { + BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message(); + return; } - -#ifdef WIN32 - // Make a backup of the configuration file before copying it to the final destination. - std::string error_message; - std::string backup_path = (boost::format("%1%.bak") % path).str(); - // Copy configuration file with PID suffix into the configuration file with "bak" suffix. - if (copy_file(path_pid, backup_path, error_message, false) != SUCCESS) - BOOST_LOG_TRIVIAL(error) << "Copying from " << path_pid << " to " << backup_path << " failed. Failed to create a backup configuration."; -#endif - - // Rename the config atomically. - // On Windows, the rename is likely NOT atomic, thus it may fail if PrusaSlicer crashes on another thread in the meanwhile. - // To cope with that, we already made a backup of the config on Windows. - if (const std::error_code ec = rename_file(path_pid, path)) - BOOST_LOG_TRIVIAL(error) << "Failed to replace " << path << " with the new configuration: " << ec.message(); m_dirty = false; } @@ -1162,10 +1145,6 @@ std::string AppConfig::load() { // Keep another instance from replacing or restoring the file mid-read. InstanceLock instance_lock(lock_path()); - if (instance_lock.locked()) { - const boost::filesystem::path conf(config_path()); - remove_stale_temp_files(conf.parent_path(), conf.filename().string()); - } // 1) Read the complete config file into a boost::property_tree. namespace pt = boost::property_tree; @@ -1305,7 +1284,6 @@ void AppConfig::save() // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); InstanceLock instance_lock(lock_path()); - std::string path_pid = (boost::format("%1%.%2%.tmp") % path % get_current_pid()).str(); std::stringstream config_ss; if (m_mode == EAppMode::Editor) @@ -1342,35 +1320,19 @@ void AppConfig::save() config_ss << std::endl; std::string config_str = config_ss.str(); - boost::nowide::ofstream c; - c.open(path_pid, std::ios::out | std::ios::trunc); - c << config_str; #ifdef WIN32 - // WIN32 specific: The final "rename_file()" call is not safe in case of an application crash, there is no atomic "rename file" API + // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, // we save the config file into a backup first before moving it to the final destination. - c << appconfig_md5_hash_line(config_str); + config_str += appconfig_md5_hash_line(config_str); + const std::string backup_path = (boost::format("%1%.bak") % path).str(); + if (const std::error_code ec = write_file_atomically(backup_path, config_str)) + BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message(); #endif - c.close(); - if (c.fail()) { - BOOST_LOG_TRIVIAL(error) << "Failed to write new configuration to " << path_pid << "; aborting attempt to overwrite original configuration"; - return; + if (const std::error_code ec = write_file_atomically(path, config_str)) { + BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message(); + return; } - -#ifdef WIN32 - // Make a backup of the configuration file before copying it to the final destination. - std::string error_message; - std::string backup_path = (boost::format("%1%.bak") % path).str(); - // Copy configuration file with PID suffix into the configuration file with "bak" suffix. - if (copy_file(path_pid, backup_path, error_message, false) != SUCCESS) - BOOST_LOG_TRIVIAL(error) << "Copying from " << path_pid << " to " << backup_path << " failed. Failed to create a backup configuration."; -#endif - - // Rename the config atomically. - // On Windows, the rename is likely NOT atomic, thus it may fail if PrusaSlicer crashes on another thread in the meanwhile. - // To cope with that, we already made a backup of the config on Windows. - if (const std::error_code ec = rename_file(path_pid, path)) - BOOST_LOG_TRIVIAL(error) << "Failed to replace " << path << " with the new configuration: " << ec.message(); m_dirty = false; } #endif diff --git a/src/libslic3r/InstanceLock.cpp b/src/libslic3r/InstanceLock.cpp index 450ef37b47..6a6ca93658 100644 --- a/src/libslic3r/InstanceLock.cpp +++ b/src/libslic3r/InstanceLock.cpp @@ -72,11 +72,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli m_slot = &slot_for(lock_file_path); m_slot->mutex.lock(); const auto now = std::chrono::steady_clock::now(); - if (m_slot->depth++ == 0 && now >= m_slot->retry_at) { - if (! m_slot->file_lock && ! open_lock_file(m_slot->file_lock, lock_file_path)) - m_slot->retry_at = now + cooldown; - } - if (m_slot->depth == 1 && m_slot->file_lock && now >= m_slot->retry_at) { + if (m_slot->depth++ == 0 && now >= m_slot->retry_at && + (m_slot->file_lock || open_lock_file(m_slot->file_lock, lock_file_path))) { const bool wait = now >= m_slot->skip_waiting_until; const auto deadline = now + timeout; for (;;) { @@ -102,6 +99,8 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli } std::this_thread::sleep_for(std::chrono::milliseconds(5)); } + } else if (m_slot->depth == 1 && ! m_slot->file_lock && now >= m_slot->retry_at) { + m_slot->retry_at = now + cooldown; } m_locked = m_slot->file_locked; } diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index ea0678dc9d..23084eecd6 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -107,9 +107,9 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const } } -std::string user_presets_lock_path() +std::string user_presets_lock_path(bool read_only) { - return data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string(); + return read_only || data_dir().empty() ? std::string() : (fs::path(data_dir()) / (PRESET_USER_DIR ".lock")).string(); } std::string get_preset_bare_name(const std::string &canonical_name) @@ -1723,9 +1723,7 @@ void PresetCollection::load_presets( Preset::get_extruder_names_and_keysets(m_type, extruder_id_name, extruder_variant_name, &key_set1, &key_set2); // Held across the scan so no instance replaces or removes a file mid-read. - // A read-only scan (the CLI) never rewrites or deletes, and many CLI jobs - // may share one data dir, so it does not queue behind the others. - InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); + InstanceLock instance_lock(user_presets_lock_path(read_only)); if (instance_lock.locked()) remove_stale_temp_files(dir); //BBS: change to json format diff --git a/src/libslic3r/Preset.hpp b/src/libslic3r/Preset.hpp index 202e676f66..7fbc3b77dc 100644 --- a/src/libslic3r/Preset.hpp +++ b/src/libslic3r/Preset.hpp @@ -485,8 +485,10 @@ std::string get_preset_canonical_name(const std::string &preset_bare_name, const std::string get_preset_bare_name(const std::string &canonical_name); // Lock file guarding every user preset file under data_dir() against other -// running instances and the preset sync thread; empty without a data dir. -std::string user_presets_lock_path(); +// running instances and the preset sync thread. Empty without a data dir, and +// for a read-only load (the CLI), which never rewrites or deletes and may run +// many jobs on one data dir. +std::string user_presets_lock_path(bool read_only = false); // Resolve an origin from a directory path when the caller passes Kind::Auto. PresetOrigin detect_origin_from_path(const boost::filesystem::path &path, const PresetOrigin &explicit_origin = PresetOrigin()); diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index 1bd2de3fc2..02144e6acf 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1232,8 +1232,6 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For fs::path local_dir(folder / PRESET_LOCAL_DIR); if (fs::exists(local_dir)) { dir_user_presets_local = local_dir; - // Held across the metadata reads; a read-only load (the CLI) takes no lock. - InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); for (auto& entry : fs::directory_iterator(local_dir)) { if (!fs::is_directory(entry.path())) continue; @@ -1243,7 +1241,11 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For if (!fs::exists(metadata_file)) continue; BundleMetadata metadata; - if (!metadata.load_from_json(metadata_file.string())) continue; + { + // Per file, so the lock is never held when bundles.WriteLock() is taken below. + InstanceLock instance_lock(user_presets_lock_path(read_only)); + if (!metadata.load_from_json(metadata_file.string())) continue; + } metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -1269,7 +1271,6 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For // Load bundle metadata from _subscribed directory fs::path subscribed_dir(folder / PRESET_SUBSCRIBED_DIR); if (fs::exists(subscribed_dir)) { - InstanceLock instance_lock(read_only ? std::string() : user_presets_lock_path()); for (auto& entry : fs::directory_iterator(subscribed_dir)) { if (!fs::is_directory(entry.path())) continue; @@ -1279,7 +1280,11 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For if (!fs::exists(metadata_file)) continue; BundleMetadata metadata; - if (!metadata.load_from_json(metadata_file.string())) continue; + { + // Per file, so the lock is never held when bundles.WriteLock() is taken below. + InstanceLock instance_lock(user_presets_lock_path(read_only)); + if (!metadata.load_from_json(metadata_file.string())) continue; + } metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); diff --git a/src/libslic3r/PresetCacheFormat.cpp b/src/libslic3r/PresetCacheFormat.cpp index 9b8f65f74e..67439de184 100644 --- a/src/libslic3r/PresetCacheFormat.cpp +++ b/src/libslic3r/PresetCacheFormat.cpp @@ -410,11 +410,8 @@ bool write_cache_blob(const std::string& path, const std::string& blob) fhdr.version = CACHE_VERSION; fhdr.data_size = static_cast(blob.size()); fhdr.crc32 = crc.checksum(); - std::string payload; - payload.reserve(sizeof(fhdr) + blob.size()); - payload.append(reinterpret_cast(&fhdr), sizeof(fhdr)); - payload += blob; - if (const std::error_code ec = write_file_atomically(path, payload, /*binary=*/true)) { + const std::string_view header(reinterpret_cast(&fhdr), sizeof(fhdr)); + if (const std::error_code ec = write_file_atomically(path, { header, std::string_view(blob) }, /*binary=*/true)) { BOOST_LOG_TRIVIAL(warning) << "VendorCacheFile: write failed (" << path << "): " << ec.message(); return false; } diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 0d8115ef02..31ffa8b5d6 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -224,15 +224,19 @@ extern std::vector split_string(const std::string &str, char delimi // On Windows, the file explorer (or anti-virus or whatever else) often locks the file // for a short while, so the file may not be movable. Retry while we see recoverable errors. extern std::error_code rename_file(const std::string &from, const std::string &to); -// Write `content` to `path` through a temporary file beside it that is then renamed -// over the target, so a concurrent reader sees the old or the new file, never a -// partial one. The temporary file is removed on failure and an existing target -// keeps its permissions. A target that is not a regular file (a symlink, device -// or pipe) is written in place, since replacing it would change what it is. -extern std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false); -// Remove the `..tmp` files a crashed write_file_atomically() left in -// `dir` at least ten minutes ago, restricted to names starting with `name_prefix` -// when given. Call it only while holding the lock that guards writes into `dir`. +// Write `chunks`, in order, to `path` through a temporary file beside it that is +// then renamed over the target, so a concurrent reader sees the old or the new +// file, never a partial one. The temporary is removed on failure and an existing +// target keeps its permissions. A target that is not a regular file (a symlink, +// device or pipe) is written in place, since replacing it would change what it +// is, and so is a target whose replace the filesystem refuses. A successful +// write also removes stale temporaries of the same target. +extern std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary = false); +inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false) + { return write_file_atomically(path, { std::string_view(content) }, binary); } +// Remove the `...tmp` files a crashed write_file_atomically() left in +// `dir` at least ten minutes ago. With `name_prefix`, only names starting with it +// go, and so do `.` leftovers of the older AppConfig writer. // Returns how many were removed. extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string()); diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index 32847c07b8..c712d48de1 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -710,44 +710,53 @@ std::error_code rename_file(const std::string &from, const std::string &to) if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) return {}; const int err = errno; - // Some mounts (sshfs without its rename workaround, for one) refuse to - // replace an existing target in one step; take the old two-step route there. - if ((err == EPERM || err == EEXIST || err == ENOTEMPTY) && boost::nowide::remove(to.c_str()) == 0 && + // Some mounts (sshfs, gvfs, MTP and a few SMB setups) refuse to replace an + // existing target in one step, with whichever error they see fit; take the + // old two-step route whenever the target is still there to be replaced. + boost::system::error_code ec; + if (err != ENOENT && err != EXDEV && boost::filesystem::exists(to, ec) && boost::nowide::remove(to.c_str()) == 0 && boost::nowide::rename(from.c_str(), to.c_str()) == 0) return {}; return std::make_error_code(static_cast(err)); #endif } -static std::error_code write_whole_file(const std::string &path, const std::string &content, bool binary) +static std::error_code write_whole_file(const std::string &path, std::initializer_list chunks, bool binary) { errno = 0; boost::nowide::ofstream out(path, std::ios::out | std::ios::trunc | (binary ? std::ios::binary : std::ios::openmode{})); - out << content; + for (const std::string_view chunk : chunks) + out.write(chunk.data(), static_cast(chunk.size())); out.close(); if (! out.fail()) return {}; return std::make_error_code(errno != 0 ? static_cast(errno) : std::errc::io_error); } -std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary) +std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary) { boost::system::error_code bec; const boost::filesystem::file_status target = boost::filesystem::symlink_status(path, bec); const bool target_exists = ! bec && boost::filesystem::exists(target); if (target_exists && ! boost::filesystem::is_regular_file(target)) - return write_whole_file(path, content, binary); + return write_whole_file(path, chunks, binary); - const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + ".tmp"; - if (std::error_code ec = write_whole_file(tmp_path, content, binary)) { + // Unique per process and per call, so two threads writing one target + // without a lock never share a temporary. + static std::atomic counter{0}; + const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + "." + std::to_string(counter++) + ".tmp"; + if (std::error_code ec = write_whole_file(tmp_path, chunks, binary)) { boost::nowide::remove(tmp_path.c_str()); // A directory that allows writing the file but not creating one beside it. if (target_exists && ec == std::errc::permission_denied) - return write_whole_file(path, content, binary); + return write_whole_file(path, chunks, binary); return ec; } +#ifndef _WIN32 + // On Windows a read-only bit on the temporary would stop the rename itself. if (target_exists) boost::filesystem::permissions(tmp_path, target.permissions(), bec); +#endif std::error_code ec = rename_file(tmp_path, path); if (ec) { boost::nowide::remove(tmp_path.c_str()); @@ -757,25 +766,35 @@ std::error_code write_file_atomically(const std::string &path, const std::string // save is worse than a reader seeing a partial file, so write in place // the way this used to work before the atomic path existed. BOOST_LOG_TRIVIAL(warning) << "Cannot replace " << path << " (" << ec.message() << "); writing in place"; - ec = write_whole_file(path, content, binary); + return write_whole_file(path, chunks, binary); } - return ec; +#ifdef _WIN32 + if (target_exists) + boost::filesystem::permissions(path, target.permissions(), bec); +#endif + const boost::filesystem::path target_path(path); + remove_stale_temp_files(target_path.parent_path(), target_path.filename().string()); + return {}; } size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) { - // ..tmp - auto is_temp_name = [&name_prefix](const std::string &name) { + auto all_digits = [](std::string::const_iterator begin, std::string::const_iterator end) { + return begin != end && std::all_of(begin, end, [](char c) { return c >= '0' && c <= '9'; }); + }; + // ....tmp, or the older . when a prefix is given. + auto is_temp_name = [&](const std::string &name) { if (name.compare(0, name_prefix.size(), name_prefix) != 0) return false; + if (! name_prefix.empty() && name.size() > name_prefix.size() + 1 && name[name_prefix.size()] == '.' && + all_digits(name.begin() + name_prefix.size() + 1, name.end())) + return true; static const std::string suffix = ".tmp"; if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0) return false; const size_t digits_end = name.size() - suffix.size(); const size_t dot = name.rfind('.', digits_end - 1); - if (dot == std::string::npos || dot == 0 || dot + 1 == digits_end) - return false; - return std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; }); + return dot != std::string::npos && dot != 0 && all_digits(name.begin() + dot + 1, name.begin() + digits_end); }; // An instance that gave up waiting for the lock writes unlocked by design, // so a temporary this young may still be in flight; a crash leftover is old. diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index f1ed2b2f59..0cc219f383 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #ifndef _WIN32 @@ -122,9 +123,34 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's } #endif +TEST_CASE("write_file_atomically survives two threads writing one target", "[utils]") { + ScopedTemporaryDir dir; + const boost::filesystem::path target = dir.path() / "shared.json"; + const std::string a(20000, 'a'), b(20000, 'b'); + + std::thread other([&] { + for (int i = 0; i < 50; ++i) + write_file_atomically(target.string(), a); + }); + for (int i = 0; i < 50; ++i) + write_file_atomically(target.string(), b); + other.join(); + + std::string content; + load_string_file(target, content); + const bool whole = content == a || content == b; + REQUIRE(whole); + size_t entries = 0; + for (auto &entry : boost::filesystem::directory_iterator(dir.path())) { + (void) entry; + ++entries; + } + REQUIRE(entries == 1); +} + TEST_CASE("remove_stale_temp_files removes only old ..tmp files", "[utils]") { ScopedTemporaryDir dir; - for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp" }) { + for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp", "a.json.99", "a.json.12.3.tmp" }) { REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x")); // An hour old: long past the age below which a temporary may still be in flight. boost::filesystem::last_write_time(dir.path() / name, std::time(nullptr) - 3600); @@ -132,19 +158,22 @@ TEST_CASE("remove_stale_temp_files removes only old ..tmp files", "[u // Just written: possibly another instance's in-flight save, so it stays. REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.tmp").string(), "x")); - SECTION("with a name prefix only matching names go") { - REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 1); + SECTION("with a name prefix only matching names go, including the older . form") { + REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 3); REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp")); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.99")); + REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.12.3.tmp")); REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp")); } SECTION("without a prefix every stale temporary goes and nothing else") { - REQUIRE(remove_stale_temp_files(dir.path()) == 2); + REQUIRE(remove_stale_temp_files(dir.path()) == 3); size_t entries = 0; for (auto &entry : boost::filesystem::directory_iterator(dir.path())) { (void) entry; ++entries; } - REQUIRE(entries == 5); + REQUIRE(entries == 6); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); REQUIRE(boost::filesystem::exists(dir.path() / "g.json.7.tmp")); } } diff --git a/tests/libslic3r/test_vendor_cache.cpp b/tests/libslic3r/test_vendor_cache.cpp index 85e100f4d4..184d4078ed 100644 --- a/tests/libslic3r/test_vendor_cache.cpp +++ b/tests/libslic3r/test_vendor_cache.cpp @@ -1,5 +1,9 @@ #include +#ifndef _WIN32 +#include +#endif + #include #include #include @@ -1356,23 +1360,31 @@ TEST_CASE("a header claiming more body than the file holds is rejected", "[Vendo REQUIRE_FALSE(bundle.load_vendor_cache(cache, "Bounded", Semver(1, 0, 0))); } +#ifndef _WIN32 +// Permissions are what makes the write fail here, which Windows does not +// express through chmod and root ignores; the helper's own tests cover the rest. TEST_CASE("a failed write leaves the previous cache in place", "[VendorCache]") { + if (::geteuid() == 0) + SKIP("permissions do not apply to root"); TempDir tmp; const std::string cache = (tmp.path / "Durable.opc").string(); REQUIRE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "1.0.0")); const std::string before = slurp(cache); - // A directory where the temp file wants to go: the write cannot complete, - // and must not have destroyed what was already there to find that out. - const fs::path blocker = fs::path(cache + "." + std::to_string(get_current_pid()) + ".tmp"); - fs::create_directories(blocker); + // No temp file can be created beside the cache and the cache itself cannot + // be opened for writing: the write cannot complete, and must not have + // destroyed what was already there to find that out. + fs::permissions(cache, fs::owner_read | fs::group_read | fs::others_read); + fs::permissions(tmp.path, fs::owner_read | fs::owner_exe | fs::group_read | fs::group_exe | fs::others_read | fs::others_exe); REQUIRE_FALSE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "2.0.0")); - CHECK(slurp(cache) == before); - fs::remove_all(blocker); + fs::permissions(tmp.path, fs::owner_all | fs::group_read | fs::group_exe | fs::others_read | fs::others_exe); + fs::permissions(cache, fs::owner_read | fs::owner_write | fs::group_read | fs::others_read); + CHECK(slurp(cache) == before); } +#endif TEST_CASE("a cache written by another build's option ordering still loads", "[VendorCache]") {