From faef995cdc11b2eea2584688c3cedebf60ced364 Mon Sep 17 00:00:00 2001 From: Vladimir Golovnev Date: Sun, 1 May 2016 05:18:38 +0300 Subject: [PATCH 1/2] Move storage "file-by-file" (#632) When moving storage for a torrent, only move files belonging to the torrent, one by one. --- ChangeLog | 1 + include/libtorrent/file.hpp | 2 + src/file.cpp | 42 +++++++++- src/storage.cpp | 148 ++++++++++++++++++------------------ test/test_storage.cpp | 100 +++++++++++++++++++++--- 5 files changed, 209 insertions(+), 84 deletions(-) diff --git a/ChangeLog b/ChangeLog index 7d7dbbd36..8008b89a8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ 1.1.1 release + * move files one-by-one when moving storage for a torrent * fix bug in enum_net() for BSD and Mac * fix bug in python binding of announce_entry * fixed bug related to flag_merge_resume_http_seeds flag in add_torrent_params diff --git a/include/libtorrent/file.hpp b/include/libtorrent/file.hpp index 736e94752..ff48618e8 100644 --- a/include/libtorrent/file.hpp +++ b/include/libtorrent/file.hpp @@ -135,6 +135,8 @@ namespace libtorrent , std::string const& new_path, error_code& ec); TORRENT_EXTRA_EXPORT void copy_file(std::string const& f , std::string const& newf, error_code& ec); + TORRENT_EXTRA_EXPORT void move_file(std::string const& f + , std::string const& newf, error_code& ec); // file is expected to exist, link will be created to point to it. If hard // links are not supported by the filesystem or OS, the file will be copied. diff --git a/src/file.cpp b/src/file.cpp index e49b453c8..6557de4b0 100644 --- a/src/file.cpp +++ b/src/file.cpp @@ -684,6 +684,44 @@ namespace libtorrent #endif // TORRENT_WINDOWS } + void move_file(std::string const& inf, std::string const& newf, error_code& ec) + { + ec.clear(); + + file_status s; + stat_file(inf, &s, ec); + if (ec) return; + + if (has_parent_path(newf)) + { + create_directories(parent_path(newf), ec); + if (ec) return; + } + + rename(inf, newf, ec); + + // on OSX, the error when trying to rename a file across different + // volumes is EXDEV, which will make it fall back to copying. + + if (ec) + { + if (ec != boost::system::errc::no_such_file_or_directory + && ec != boost::system::errc::invalid_argument + && ec != boost::system::errc::permission_denied) + { + ec.clear(); + copy_file(inf, newf, ec); + + if (!ec) + { + // ignore errors when removing + error_code ignore; + remove(inf, ignore); + } + } + } + } + std::string split_path(std::string const& f) { if (f.empty()) return f; @@ -879,7 +917,7 @@ namespace libtorrent ++len; } return std::string(first, len); - + } return std::string(sep + 1); } @@ -1017,7 +1055,7 @@ namespace libtorrent ret.resize(write_cur - &ret[0]); return ret; } -#endif +#endif boost::int64_t file_size(std::string const& f) { diff --git a/src/storage.cpp b/src/storage.cpp index 97b00b3a1..2abea7490 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -1158,19 +1158,6 @@ namespace libtorrent } } - // collect all directories in to_move. This is because we - // try to move entire directories by default (instead of - // files independently). - std::map to_move; - for (int i = 0; i < f.num_files(); ++i) - { - // files moved out to absolute paths are not moved - if (is_complete(f.file_path(i))) continue; - - std::string split = split_path(f.file_path(i)); - to_move.insert(to_move.begin(), std::make_pair(split, i)); - } - e.clear(); stat_file(save_path, &s, e); if (e == boost::system::errc::no_such_file_or_directory) @@ -1198,85 +1185,102 @@ namespace libtorrent print_open_files("release files", m_files.name().c_str()); #endif - for (std::map::const_iterator i = to_move.begin() - , end(to_move.end()); i != end; ++i) + int i; + for (i = 0; i < f.num_files(); ++i) { - std::string old_path = combine_path(m_save_path, i->first); - std::string new_path = combine_path(save_path, i->first); + // files moved out to absolute paths are not moved + if (is_complete(f.file_path(i))) continue; + + std::string old_path = combine_path(m_save_path, f.file_path(i)); + std::string new_path = combine_path(save_path, f.file_path(i)); + + if (flags == dont_replace && exists(new_path)) + { + if (ret == piece_manager::no_error) ret = piece_manager::need_full_check; + continue; + } e.clear(); - rename(old_path, new_path, e); + move_file(old_path, new_path, e); // if the source file doesn't exist. That's not a problem // we just ignore that file if (e == boost::system::errc::no_such_file_or_directory) e.clear(); - // on OSX, the error when trying to rename a file across different - // volumes is EXDEV, which will make it fall back to copying. - if (e) { - if (flags == dont_replace && e == boost::system::errc::file_exists) - { - if (ret == piece_manager::no_error) ret = piece_manager::need_full_check; - continue; - } - - if (e == boost::system::errc::invalid_argument - || e == boost::system::errc::permission_denied) - { - ec.ec = e; - ec.file = i->second; - ec.operation = storage_error::rename; - break; - } - - if (e != boost::system::errc::no_such_file_or_directory) - { - e.clear(); - recursive_copy(old_path, new_path, ec.ec); - if (ec.ec == boost::system::errc::no_such_file_or_directory) - { - // it's a bit weird that rename() would not return - // ENOENT, but the file still wouldn't exist. But, - // in case it does, we're done. - ec.ec.clear(); - break; - } - if (ec) - { - ec.file = i->second; - ec.operation = storage_error::copy; - } - else - { - // ignore errors when removing - error_code ignore; - remove_all(old_path, ignore); - } - break; - } + ec.ec = e; + ec.file = i; + ec.operation = storage_error::rename; + break; } } - if (!ec) + if (!e && m_part_file) { - if (m_part_file) + m_part_file->move_partfile(save_path, e); + if (e) { - // TODO: if everything moves OK, except for the partfile - // we currently won't update the save path, which breaks things. - // it would probably make more sense to give up on the partfile - m_part_file->move_partfile(save_path, ec.ec); - if (ec) + ec.ec = e; + ec.file = -1; + ec.operation = storage_error::partfile_move; + } + } + + if (e) + { + // rollback + while (--i >= 0) + { + // files moved out to absolute paths are not moved + if (is_complete(f.file_path(i))) continue; + + std::string old_path = combine_path(m_save_path, f.file_path(i)); + std::string new_path = combine_path(save_path, f.file_path(i)); + + if (!exists(old_path)) { - ec.file = -1; - ec.operation = storage_error::partfile_move; - return piece_manager::fatal_disk_error; + // ignore errors when rolling back + error_code ignore; + move_file(new_path, old_path, ignore); } } - m_save_path = save_path; + return piece_manager::fatal_disk_error; } + + m_save_path.swap(save_path); + // now we have old save path in save_path + + std::set subdirs; + for (i = 0; i < f.num_files(); ++i) + { + // files moved out to absolute paths are not moved + if (is_complete(f.file_path(i))) continue; + + if (has_parent_path(f.file_path(i))) + subdirs.insert(parent_path(f.file_path(i))); + std::string old_path = combine_path(save_path, f.file_path(i)); + + // we may still have some files in old save_path + // eg. if (flags == dont_replace && exists(new_path)) + // ignore errors when removing + error_code ignore; + remove(old_path, ignore); + } + + for (std::set::iterator it(subdirs.begin()) + , end(subdirs.end()); it != end; ++it) + { + e.clear(); + std::string subdir(*it); + while (!subdir.empty() && !e) + { + remove(combine_path(save_path, subdir), e); + subdir = parent_path(subdir); + } + } + return ret; } diff --git a/test/test_storage.cpp b/test/test_storage.cpp index 3c975b0e4..519996b9a 100644 --- a/test/test_storage.cpp +++ b/test/test_storage.cpp @@ -1253,27 +1253,107 @@ TORRENT_TEST(readwritev_zero_size_files) TORRENT_TEST(move_storage_into_self) { + error_code ec; + std::string save_path = current_working_directory(); + remove_all(combine_path(save_path, "temp_storage"), ec); + if (ec && ec != boost::system::errc::no_such_file_or_directory) + std::cerr << "remove_all '" << combine_path(save_path, "temp_storage") + << "': " << ec.message() << std::endl; + TEST_CHECK(!exists(combine_path(save_path, "temp_storage"))); + aux::session_settings set; file_storage fs; std::vector buf; file_pool fp; io_service ios; disk_buffer_pool dp(16 * 1024, ios, boost::bind(&nop)); - boost::shared_ptr s = setup_torrent(fs, fp, buf - , current_working_directory() - , set); + boost::shared_ptr s = setup_torrent(fs, fp, buf, save_path, set); file::iovec_t b = {&buf[0], 4}; storage_error se; s->writev(&b, 1, 2, 0, 0, se); - s->move_storage(combine_path("temp_storage", "folder1"), 0, se); + std::string test_path = combine_path(save_path, combine_path("temp_storage", "folder1")); + s->move_storage(test_path, 0, se); + TEST_EQUAL(se.ec, boost::system::errc::success); - printf("move error: %s\n", se.ec.message().c_str()); -#ifdef _WIN32 - TEST_EQUAL(se.ec, boost::system::errc::permission_denied); -#else - TEST_EQUAL(se.ec, boost::system::errc::invalid_argument); -#endif + TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("folder1", "test2.tmp"))))); + + // these directories and files are created up-front because they are empty files + TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("folder2", "test3.tmp"))))); + TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("_folder3", "test4.tmp"))))); +} + +TORRENT_TEST(dont_move_intermingled_files) +{ + error_code ec; + + std::string save_path = combine_path(current_working_directory(), "save_path_1"); + remove_all(combine_path(save_path, "temp_storage"), ec); + if (ec && ec != boost::system::errc::no_such_file_or_directory) + std::cerr << "remove_all '" << combine_path(save_path, "temp_storage") + << "': " << ec.message() << std::endl; + TEST_CHECK(!exists(combine_path(save_path, "temp_storage"))); + + std::string test_path = combine_path(current_working_directory(), "save_path_2"); + remove_all(combine_path(test_path, "temp_storage"), ec); + if (ec && ec != boost::system::errc::no_such_file_or_directory) + std::cerr << "remove_all '" << combine_path(test_path, "temp_storage") + << "': " << ec.message() << std::endl; + TEST_CHECK(!exists(combine_path(test_path, "temp_storage"))); + + aux::session_settings set; + file_storage fs; + std::vector buf; + file_pool fp; + io_service ios; + disk_buffer_pool dp(16 * 1024, ios, boost::bind(&nop)); + boost::shared_ptr s = setup_torrent(fs, fp, buf, save_path, set); + + file::iovec_t b = {&buf[0], 4}; + storage_error se; + s->writev(&b, 1, 2, 0, 0, se); + + create_directory(combine_path(save_path, combine_path("temp_storage" + , combine_path("_folder3", "alien_folder1"))), ec); + TEST_EQUAL(ec, boost::system::errc::success); + file f; + f.open(combine_path(save_path, combine_path("temp_storage", "alien1.tmp")) + , file::write_only, ec); + f.close(); + TEST_EQUAL(ec, boost::system::errc::success); + f.open(combine_path(save_path, combine_path("temp_storage" + , combine_path("folder1", "alien2.tmp"))), file::write_only, ec); + f.close(); + TEST_EQUAL(ec, boost::system::errc::success); + + s->move_storage(test_path, 0, se); + TEST_EQUAL(se.ec, boost::system::errc::success); + + // torrent files moved to new place + TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("folder1", "test2.tmp"))))); + // these directories and files are created up-front because they are empty files + TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("folder2", "test3.tmp"))))); + TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("_folder3", "test4.tmp"))))); + + // intermingled files and directories are still in old place + TEST_CHECK(exists(combine_path(save_path, combine_path("temp_storage" + , "alien1.tmp")))); + TEST_CHECK(!exists(combine_path(test_path, combine_path("temp_storage" + , "alien1.tmp")))); + TEST_CHECK(exists(combine_path(save_path, combine_path("temp_storage" + , combine_path("folder1", "alien2.tmp"))))); + TEST_CHECK(!exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("folder1", "alien2.tmp"))))); + TEST_CHECK(exists(combine_path(save_path, combine_path("temp_storage" + , combine_path("_folder3", "alien_folder1"))))); + TEST_CHECK(!exists(combine_path(test_path, combine_path("temp_storage" + , combine_path("_folder3", "alien_folder1"))))); } From 97ad7510af2298a6e13ad19ea8ca024369df9ad3 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sun, 1 May 2016 12:23:02 -0400 Subject: [PATCH 2/2] minor refactoring to move-storage patch (#680) minor refactoring to move-storage patch --- include/libtorrent/file_storage.hpp | 5 ++ src/file_storage.cpp | 6 ++ src/storage.cpp | 94 ++++++++++++++++------------- test/test_storage.cpp | 40 ++++++------ 4 files changed, 81 insertions(+), 64 deletions(-) diff --git a/include/libtorrent/file_storage.hpp b/include/libtorrent/file_storage.hpp index 86c860a0a..069397c26 100644 --- a/include/libtorrent/file_storage.hpp +++ b/include/libtorrent/file_storage.hpp @@ -530,6 +530,11 @@ namespace libtorrent // to file at ``index``. int file_flags(int index) const; + // returns true if the file at the specified index has been renamed to + // have an absolute path, i.e. is not anchored in the save path of the + // torrent. + bool file_absolute_path(int index) const; + // returns the index of the file at the given offset in the torrent int file_index_at_offset(boost::int64_t offset) const; diff --git a/src/file_storage.cpp b/src/file_storage.cpp index db11be4f0..41fde9ac6 100644 --- a/src/file_storage.cpp +++ b/src/file_storage.cpp @@ -816,6 +816,12 @@ namespace libtorrent | (fe.symlink_attribute ? flag_symlink : 0); } + bool file_storage::file_absolute_path(int index) const + { + internal_file_entry const& fe = m_files[index]; + return fe.path_index == -2; + } + #ifndef TORRENT_NO_DEPRECATE void file_storage::set_file_base(int index, boost::int64_t off) { diff --git a/src/storage.cpp b/src/storage.cpp index 2abea7490..18cd4f641 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -795,8 +795,8 @@ namespace libtorrent typedef std::set::iterator iter_t; for (int i = 0; i < files().num_files(); ++i) { - std::string fp = files().file_path(i); - bool complete = is_complete(fp); + std::string const fp = files().file_path(i); + bool const complete = files().file_absolute_path(i); std::string p = complete ? fp : combine_path(m_save_path, fp); if (!complete) { @@ -1124,32 +1124,32 @@ namespace libtorrent return true; } - int default_storage::move_storage(std::string const& sp, int flags, storage_error& ec) + int default_storage::move_storage(std::string const& sp, int const flags + , storage_error& ec) { int ret = piece_manager::no_error; - std::string save_path = complete(sp); + std::string const save_path = complete(sp); // check to see if any of the files exist - error_code e; file_storage const& f = files(); - file_status s; if (flags == fail_if_exist) { - stat_file(save_path, &s, e); - if (e != boost::system::errc::no_such_file_or_directory) + file_status s; + error_code err; + stat_file(save_path, &s, err); + if (err != boost::system::errc::no_such_file_or_directory) { // the directory exists, check all the files for (int i = 0; i < f.num_files(); ++i) { // files moved out to absolute paths are ignored - if (is_complete(f.file_path(i))) continue; + if (f.file_absolute_path(i)) continue; - std::string new_path = f.file_path(i, save_path); - stat_file(new_path, &s, e); - if (e != boost::system::errc::no_such_file_or_directory) + stat_file(f.file_path(i, save_path), &s, err); + if (err != boost::system::errc::no_such_file_or_directory) { - ec.ec = e; + ec.ec = err; ec.file = i; ec.operation = storage_error::stat; return piece_manager::file_exist; @@ -1158,26 +1158,30 @@ namespace libtorrent } } - e.clear(); - stat_file(save_path, &s, e); - if (e == boost::system::errc::no_such_file_or_directory) { - create_directories(save_path, e); - if (e) + file_status s; + error_code err; + stat_file(save_path, &s, err); + if (err == boost::system::errc::no_such_file_or_directory) { - ec.ec = e; + err.clear(); + create_directories(save_path, err); + if (err) + { + ec.ec = err; + ec.file = -1; + ec.operation = storage_error::mkdir; + return piece_manager::fatal_disk_error; + } + } + else if (err) + { + ec.ec = err; ec.file = -1; - ec.operation = storage_error::mkdir; + ec.operation = storage_error::stat; return piece_manager::fatal_disk_error; } } - else if (e) - { - ec.ec = e; - ec.file = -1; - ec.operation = storage_error::mkdir; - return piece_manager::fatal_disk_error; - } m_pool.release(this); @@ -1186,13 +1190,14 @@ namespace libtorrent #endif int i; + error_code e; for (i = 0; i < f.num_files(); ++i) { // files moved out to absolute paths are not moved - if (is_complete(f.file_path(i))) continue; + if (f.file_absolute_path(i)) continue; - std::string old_path = combine_path(m_save_path, f.file_path(i)); - std::string new_path = combine_path(save_path, f.file_path(i)); + std::string const old_path = combine_path(m_save_path, f.file_path(i)); + std::string const new_path = combine_path(save_path, f.file_path(i)); if (flags == dont_replace && exists(new_path)) { @@ -1200,7 +1205,9 @@ namespace libtorrent continue; } - e.clear(); + // TODO: ideally, if we end up copying files because of a move across + // volumes, the source should not be deleted until they've all been + // copied. That would let us rollback with higher confidence. move_file(old_path, new_path, e); // if the source file doesn't exist. That's not a problem // we just ignore that file @@ -1233,10 +1240,10 @@ namespace libtorrent while (--i >= 0) { // files moved out to absolute paths are not moved - if (is_complete(f.file_path(i))) continue; + if (f.file_absolute_path(i)) continue; - std::string old_path = combine_path(m_save_path, f.file_path(i)); - std::string new_path = combine_path(save_path, f.file_path(i)); + std::string const old_path = combine_path(m_save_path, f.file_path(i)); + std::string const new_path = combine_path(save_path, f.file_path(i)); if (!exists(old_path)) { @@ -1249,20 +1256,21 @@ namespace libtorrent return piece_manager::fatal_disk_error; } - m_save_path.swap(save_path); - // now we have old save path in save_path + std::string const old_save_path = m_save_path; + m_save_path = save_path; std::set subdirs; for (i = 0; i < f.num_files(); ++i) { // files moved out to absolute paths are not moved - if (is_complete(f.file_path(i))) continue; + if (f.file_absolute_path(i)) continue; if (has_parent_path(f.file_path(i))) subdirs.insert(parent_path(f.file_path(i))); - std::string old_path = combine_path(save_path, f.file_path(i)); - // we may still have some files in old save_path + std::string const old_path = combine_path(old_save_path, f.file_path(i)); + + // we may still have some files in old old_save_path // eg. if (flags == dont_replace && exists(new_path)) // ignore errors when removing error_code ignore; @@ -1272,11 +1280,11 @@ namespace libtorrent for (std::set::iterator it(subdirs.begin()) , end(subdirs.end()); it != end; ++it) { - e.clear(); - std::string subdir(*it); - while (!subdir.empty() && !e) + error_code err; + std::string subdir = combine_path(old_save_path, *it); + while (subdir != old_save_path && !err) { - remove(combine_path(save_path, subdir), e); + remove(subdir, err); subdir = parent_path(subdir); } } diff --git a/test/test_storage.cpp b/test/test_storage.cpp index 519996b9a..fef4a36e4 100644 --- a/test/test_storage.cpp +++ b/test/test_storage.cpp @@ -1251,15 +1251,22 @@ TORRENT_TEST(readwritev_zero_size_files) TEST_CHECK(check_pattern(buf, 0)); } -TORRENT_TEST(move_storage_into_self) +void delete_dirs(std::string path) { error_code ec; - std::string save_path = current_working_directory(); - remove_all(combine_path(save_path, "temp_storage"), ec); + remove_all(path, ec); if (ec && ec != boost::system::errc::no_such_file_or_directory) - std::cerr << "remove_all '" << combine_path(save_path, "temp_storage") - << "': " << ec.message() << std::endl; - TEST_CHECK(!exists(combine_path(save_path, "temp_storage"))); + { + fprintf(stderr, "remove_all \"%s\": %s\n" + , path.c_str(), ec.message().c_str()); + } + TEST_CHECK(!exists(path)); +} + +TORRENT_TEST(move_storage_into_self) +{ + std::string const save_path = current_working_directory(); + delete_dirs(combine_path(save_path, "temp_storage")); aux::session_settings set; file_storage fs; @@ -1269,11 +1276,11 @@ TORRENT_TEST(move_storage_into_self) disk_buffer_pool dp(16 * 1024, ios, boost::bind(&nop)); boost::shared_ptr s = setup_torrent(fs, fp, buf, save_path, set); - file::iovec_t b = {&buf[0], 4}; + file::iovec_t const b = {&buf[0], 4}; storage_error se; s->writev(&b, 1, 2, 0, 0, se); - std::string test_path = combine_path(save_path, combine_path("temp_storage", "folder1")); + std::string const test_path = combine_path(save_path, combine_path("temp_storage", "folder1")); s->move_storage(test_path, 0, se); TEST_EQUAL(se.ec, boost::system::errc::success); @@ -1289,21 +1296,11 @@ TORRENT_TEST(move_storage_into_self) TORRENT_TEST(dont_move_intermingled_files) { - error_code ec; - - std::string save_path = combine_path(current_working_directory(), "save_path_1"); - remove_all(combine_path(save_path, "temp_storage"), ec); - if (ec && ec != boost::system::errc::no_such_file_or_directory) - std::cerr << "remove_all '" << combine_path(save_path, "temp_storage") - << "': " << ec.message() << std::endl; - TEST_CHECK(!exists(combine_path(save_path, "temp_storage"))); + std::string const save_path = combine_path(current_working_directory(), "save_path_1"); + delete_dirs(combine_path(save_path, "temp_storage")); std::string test_path = combine_path(current_working_directory(), "save_path_2"); - remove_all(combine_path(test_path, "temp_storage"), ec); - if (ec && ec != boost::system::errc::no_such_file_or_directory) - std::cerr << "remove_all '" << combine_path(test_path, "temp_storage") - << "': " << ec.message() << std::endl; - TEST_CHECK(!exists(combine_path(test_path, "temp_storage"))); + delete_dirs(combine_path(test_path, "temp_storage")); aux::session_settings set; file_storage fs; @@ -1317,6 +1314,7 @@ TORRENT_TEST(dont_move_intermingled_files) storage_error se; s->writev(&b, 1, 2, 0, 0, se); + error_code ec; create_directory(combine_path(save_path, combine_path("temp_storage" , combine_path("_folder3", "alien_folder1"))), ec); TEST_EQUAL(ec, boost::system::errc::success);