minor refactoring to move-storage patch (#680)

minor refactoring to move-storage patch

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;
 

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

src/storage.cpp

@@ -795,8 +795,8 @@ namespace libtorrent
 			typedef std::set<std::string>::iterator iter_t;
 			for (int i = 0; i < files().num_files(); ++i)
 			{
-				std::string fp = files().file_path(i);
+				std::string const fp = files().file_path(i);
-				bool complete = is_complete(fp);
+				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);
+			file_status s;
-			if (e != boost::system::errc::no_such_file_or_directory)
+			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(f.file_path(i, save_path), &s, err);
-					stat_file(new_path, &s, e);
+					if (err != boost::system::errc::no_such_file_or_directory)
-					if (e != 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);
+			file_status s;
-			if (e)
+			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 const 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 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 const 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 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);
+		std::string const old_save_path = m_save_path;
-		// now we have old save path in save_path
+		m_save_path = 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 (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<std::string>::iterator it(subdirs.begin())
 			 , end(subdirs.end()); it != end; ++it)
 		{
-			e.clear();
+			error_code err;
-			std::string subdir(*it);
+			std::string subdir = combine_path(old_save_path, *it);
-			while (!subdir.empty() && !e)
+			while (subdir != old_save_path && !err)
 			{
-				remove(combine_path(save_path, subdir), e);
+				remove(subdir, err);
 				subdir = parent_path(subdir);
 			}
 		}

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(path, ec);
-	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;
+		fprintf(stderr, "remove_all \"%s\": %s\n"
-	TEST_CHECK(!exists(combine_path(save_path, "temp_storage")));
+			, 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<default_storage> 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 const save_path = combine_path(current_working_directory(), "save_path_1");
-
+	delete_dirs(combine_path(save_path, "temp_storage"));
-	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);
+	delete_dirs(combine_path(test_path, "temp_storage"));
-	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;
@@ -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);