Move storage "file-by-file" (#632)

When moving storage for a torrent, only move files belonging to the torrent, one by one.

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

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.

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)
 	{

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<std::string, int> 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<std::string, int>::const_iterator i = to_move.begin()
+		int i;
-			, end(to_move.end()); i != end; ++i)
+		for (i = 0; i < f.num_files(); ++i)
 		{
-			std::string old_path = combine_path(m_save_path, i->first);
+			// files moved out to absolute paths are not moved
-			std::string new_path = combine_path(save_path, i->first);
+			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)
+				ec.ec = e;
-				{
+				ec.file = i;
-					if (ret == piece_manager::no_error) ret = piece_manager::need_full_check;
+				ec.operation = storage_error::rename;
-					continue;
+				break;
-				}
-
-				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;
-				}
 			}
 		}
 
-		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
+				ec.ec = e;
-				// we currently won't update the save path, which breaks things.
+				ec.file = -1;
-				// it would probably make more sense to give up on the partfile
+				ec.operation = storage_error::partfile_move;
-				m_part_file->move_partfile(save_path, ec.ec);
+			}
-				if (ec)
+		}
+
+		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;
+					// ignore errors when rolling back
-					ec.operation = storage_error::partfile_move;
+					error_code ignore;
-					return piece_manager::fatal_disk_error;
+					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<std::string> 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<std::string>::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;
 	}
 

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<char> buf;
 	file_pool fp;
 	io_service ios;
 	disk_buffer_pool dp(16 * 1024, ios, boost::bind(&nop));
-	boost::shared_ptr<default_storage> s = setup_torrent(fs, fp, buf
+	boost::shared_ptr<default_storage> s = setup_torrent(fs, fp, buf, save_path, set);
-		, current_working_directory()
-		, 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());
+	TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage"
-#ifdef _WIN32
+		, combine_path("folder1", "test2.tmp")))));
-	TEST_EQUAL(se.ec, boost::system::errc::permission_denied);
+
-#else
+	// these directories and files are created up-front because they are empty files
-	TEST_EQUAL(se.ec, boost::system::errc::invalid_argument);
+	TEST_CHECK(exists(combine_path(test_path, combine_path("temp_storage"
-#endif
+		, 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<char> buf;
+	file_pool fp;
+	io_service ios;
+	disk_buffer_pool dp(16 * 1024, ios, boost::bind(&nop));
+	boost::shared_ptr<default_storage> 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")))));
 }