From 0d5428a6649450abfc8d9c0d7785dc2a6bd7a8c6 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sun, 1 May 2016 12:23:02 -0400 Subject: [PATCH] minor refactoring to move-storage patch (#680) minor refactoring to move-storage patch --- ChangeLog | 1 + include/libtorrent/file_storage.hpp | 5 ++ src/file_storage.cpp | 6 ++ src/storage.cpp | 97 ++++++++++++++++------------- test/test_storage.cpp | 40 ++++++------ 5 files changed, 84 insertions(+), 65 deletions(-) diff --git a/ChangeLog b/ChangeLog index 66c147400..c6710b383 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,4 @@ + * move files one-by-one when moving storage for a torrent * removed RSS support * removed feature to resolve country for peers * added support for BEP 32, "IPv6 extension for DHT" diff --git a/include/libtorrent/file_storage.hpp b/include/libtorrent/file_storage.hpp index b4a929a23..e50feaeaf 100644 --- a/include/libtorrent/file_storage.hpp +++ b/include/libtorrent/file_storage.hpp @@ -531,6 +531,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 81d234ae7..e46ee9cc6 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 98e2f3de0..341cf612c 100644 --- a/src/storage.cpp +++ b/src/storage.cpp @@ -785,11 +785,11 @@ namespace libtorrent #endif // delete the files from disk std::set directories; - typedef std::set::iterator iter_t; + using iter_t = std::set::iterator; 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) { @@ -936,32 +936,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; @@ -970,26 +970,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); @@ -998,13 +1002,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)) { @@ -1012,7 +1017,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 @@ -1045,10 +1052,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)) { @@ -1061,32 +1068,34 @@ 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; remove(old_path, ignore); } - for (std::string subdir: subdirs) + for (std::string const& s : subdirs) { - e.clear(); - while (!subdir.empty() && !e) + error_code err; + std::string subdir = combine_path(old_save_path, s); + 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 9b0ead052..2e271c626 100644 --- a/test/test_storage.cpp +++ b/test/test_storage.cpp @@ -1263,15 +1263,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; @@ -1281,11 +1288,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); @@ -1301,21 +1308,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; @@ -1329,6 +1326,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);