factor out move_storage function to storage_utils.cpp (#1571)

include/libtorrent/aux_/storage_utils.hpp

@@ -38,6 +38,7 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/config.hpp"
 #include "libtorrent/span.hpp"
 #include "libtorrent/units.hpp"
+#include "libtorrent/storage_defs.hpp" // for status_t
 
 #ifndef TORRENT_WINDOWS
 #include <sys/uio.h> // for iovec
@@ -46,6 +47,7 @@ POSSIBILITY OF SUCH DAMAGE.
 namespace libtorrent
 {
 	class file_storage;
+	struct part_file;
 	struct storage_error;
 
 #ifdef TORRENT_WINDOWS
@@ -72,7 +74,6 @@ namespace libtorrent
 		~fileop() {}
 	};
 
-
 	// this function is responsible for turning read and write operations in the
 	// torrent space (pieces) into read and write operations in the filesystem
 	// space (files on disk).
@@ -80,6 +81,15 @@ namespace libtorrent
 		, span<iovec_t const> bufs, piece_index_t piece, int offset
 		, fileop& op, storage_error& ec);
 
+	// moves the files in file_storage f from ``save_path`` to
+	// ``destination_save_path`` according to the rules defined by ``flags``.
+	// returns the status code and the new save_path.
+	TORRENT_EXTRA_EXPORT std::pair<status_t, std::string>
+	move_storage(file_storage const& f
+		, std::string const& save_path
+		, std::string const& destination_save_path
+		, part_file* pf
+		, int const flags, storage_error& ec);
 }
 
 #endif

include/libtorrent/disk_interface.hpp

@@ -58,15 +58,6 @@ namespace libtorrent
 	struct storage_params;
 	class file_storage;
 
-	enum class status_t : std::uint8_t
-	{
-		// return values from check_fastresume, and move_storage
-		no_error,
-		fatal_disk_error,
-		need_full_check,
-		file_exist
-	};
-
 	struct storage_holder;
 
 	struct TORRENT_EXTRA_EXPORT disk_interface

include/libtorrent/storage.hpp

@@ -231,27 +231,6 @@ namespace libtorrent
 		std::unordered_set<cached_piece_entry*> m_cached_pieces;
 	};
 
-	// flags for async_move_storage
-	enum move_flags_t
-	{
-		// replace any files in the destination when copying
-		// or moving the storage
-		always_replace_files,
-
-		// if any files that we want to copy exist in the destination
-		// exist, fail the whole operation and don't perform
-		// any copy or move. There is an inherent race condition
-		// in this mode. The files are checked for existence before
-		// the operation starts. In between the check and performing
-		// the copy, the destination files may be created, in which
-		// case they are replaced.
-		fail_if_exist,
-
-		// if any file exist in the target, take those files instead
-		// of the ones we may have in the source.
-		dont_replace
-	};
-
 	// The storage interface is a pure virtual class that can be implemented to
 	// customize how and where data for a torrent is stored. The default storage
 	// implementation uses regular files in the filesystem, mapping the files in

include/libtorrent/storage_defs.hpp

@@ -38,7 +38,6 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/aux_/vector.hpp"
 #include <functional>
 #include <string>
-#include <vector>
 
 namespace libtorrent
 {
@@ -63,6 +62,36 @@ namespace libtorrent
 		storage_mode_sparse
 	};
 
+	enum class status_t : std::uint8_t
+	{
+		// return values from check_fastresume, and move_storage
+		no_error,
+		fatal_disk_error,
+		need_full_check,
+		file_exist
+	};
+
+	// flags for async_move_storage
+	enum move_flags_t
+	{
+		// replace any files in the destination when copying
+		// or moving the storage
+		always_replace_files,
+
+		// if any files that we want to copy exist in the destination
+		// exist, fail the whole operation and don't perform
+		// any copy or move. There is an inherent race condition
+		// in this mode. The files are checked for existence before
+		// the operation starts. In between the check and performing
+		// the copy, the destination files may be created, in which
+		// case they are replaced.
+		fail_if_exist,
+
+		// if any file exist in the target, take those files instead
+		// of the ones we may have in the source.
+		dont_replace
+	};
+
 	// see default_storage::default_storage()
 	struct TORRENT_EXPORT storage_params
 	{

src/storage.cpp

@@ -804,185 +804,11 @@ namespace libtorrent
 	status_t default_storage::move_storage(std::string const& sp, int const flags
 		, storage_error& ec)
 	{
-		status_t ret = status_t::no_error;
-		std::string const save_path = complete(sp);
-
-		// check to see if any of the files exist
-		file_storage const& f = files();
-
-		if (flags == fail_if_exist)
-		{
-			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 (file_index_t i(0); i < f.end_file(); ++i)
-				{
-					// files moved out to absolute paths are ignored
-					if (f.file_absolute_path(i)) continue;
-
-					stat_file(f.file_path(i, save_path), &s, err);
-					if (err != boost::system::errc::no_such_file_or_directory)
-					{
-						ec.ec = err;
-						ec.file(i);
-						ec.operation = storage_error::stat;
-						return status_t::file_exist;
-					}
-				}
-			}
-		}
-
-		{
-			file_status s;
-			error_code err;
-			stat_file(save_path, &s, err);
-			if (err == boost::system::errc::no_such_file_or_directory)
-			{
-				err.clear();
-				create_directories(save_path, err);
-				if (err)
-				{
-					ec.ec = err;
-					ec.file(file_index_t(-1));
-					ec.operation = storage_error::mkdir;
-					return status_t::fatal_disk_error;
-				}
-			}
-			else if (err)
-			{
-				ec.ec = err;
-				ec.file(file_index_t(-1));
-				ec.operation = storage_error::stat;
-				return status_t::fatal_disk_error;
-			}
-		}
-
 		m_pool.release(storage_index());
 
-		// indices of all files we ended up copying. These need to be deleted
+		status_t ret;
-		// later
+		std::tie(ret, m_save_path) = libtorrent::move_storage(files(), m_save_path, sp
-		aux::vector<bool, file_index_t> copied_files(std::size_t(f.num_files()), false);
+			, m_part_file.get(), flags, ec);
-
-		file_index_t i;
-		error_code e;
-		for (i = file_index_t(0); i < f.end_file(); ++i)
-		{
-			// files moved out to absolute paths are not moved
-			if (f.file_absolute_path(i)) continue;
-
-			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))
-			{
-				if (ret == status_t::no_error) ret = status_t::need_full_check;
-				continue;
-			}
-
-			// 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
-			if (e == boost::system::errc::no_such_file_or_directory)
-				e.clear();
-			else if (e
-				&& e != boost::system::errc::invalid_argument
-				&& e != boost::system::errc::permission_denied)
-			{
-				// moving the file failed
-				// on OSX, the error when trying to rename a file across different
-				// volumes is EXDEV, which will make it fall back to copying.
-				e.clear();
-				copy_file(old_path, new_path, e);
-				if (!e) copied_files[i] = true;
-			}
-
-			if (e)
-			{
-				ec.ec = e;
-				ec.file(i);
-				ec.operation = storage_error::rename;
-				break;
-			}
-		}
-
-		if (!e && m_part_file)
-		{
-			m_part_file->move_partfile(save_path, e);
-			if (e)
-			{
-				ec.ec = e;
-				ec.file(file_index_t(-1));
-				ec.operation = storage_error::partfile_move;
-			}
-		}
-
-		if (e)
-		{
-			// rollback
-			while (--i >= file_index_t(0))
-			{
-				// files moved out to absolute paths are not moved
-				if (f.file_absolute_path(i)) continue;
-
-				// if we ended up copying the file, don't do anything during
-				// roll-back
-				if (copied_files[i]) continue;
-
-				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));
-
-				// ignore errors when rolling back
-				error_code ignore;
-				move_file(new_path, old_path, ignore);
-			}
-
-			return status_t::fatal_disk_error;
-		}
-
-		std::string const old_save_path = m_save_path;
-		m_save_path = save_path;
-
-		std::set<std::string> subdirs;
-		for (i = file_index_t(0); i < f.end_file(); ++i)
-		{
-			// files moved out to absolute paths are not moved
-			if (f.file_absolute_path(i)) continue;
-
-			if (has_parent_path(f.file_path(i)))
-				subdirs.insert(parent_path(f.file_path(i)));
-
-			// if we ended up renaming the file instead of moving it, there's no
-			// need to delete the source.
-			if (copied_files[i] == false) continue;
-
-			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 const& s : subdirs)
-		{
-			error_code err;
-			std::string subdir = combine_path(old_save_path, s);
-
-			while (subdir != old_save_path && !err)
-			{
-				remove(subdir, err);
-				subdir = parent_path(subdir);
-			}
-		}
-
 		return ret;
 	}
 

src/storage_utils.cpp

@@ -34,6 +34,10 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/file_storage.hpp"
 #include "libtorrent/alloca.hpp"
 #include "libtorrent/file.hpp" // for count_bufs
+#include "libtorrent/part_file.hpp"
+
+#include <set>
+#include <string>
 
 namespace libtorrent
 {
@@ -183,5 +187,192 @@ namespace libtorrent
 		return size;
 	}
 
+	std::pair<status_t, std::string> move_storage(file_storage const& f
+		, std::string const& save_path
+		, std::string const& destination_save_path
+		, part_file* pf
+		, int const flags, storage_error& ec)
+	{
+		status_t ret = status_t::no_error;
+		std::string const new_save_path = complete(destination_save_path);
+
+		// check to see if any of the files exist
+		if (flags == fail_if_exist)
+		{
+			file_status s;
+			error_code err;
+			stat_file(new_save_path, &s, err);
+			if (err != boost::system::errc::no_such_file_or_directory)
+			{
+				// the directory exists, check all the files
+				for (file_index_t i(0); i < f.end_file(); ++i)
+				{
+					// files moved out to absolute paths are ignored
+					if (f.file_absolute_path(i)) continue;
+
+					stat_file(f.file_path(i, new_save_path), &s, err);
+					if (err != boost::system::errc::no_such_file_or_directory)
+					{
+						ec.ec = err;
+						ec.file(i);
+						ec.operation = storage_error::stat;
+						return { status_t::file_exist, save_path };
+					}
+				}
+			}
+		}
+
+		{
+			file_status s;
+			error_code err;
+			stat_file(new_save_path, &s, err);
+			if (err == boost::system::errc::no_such_file_or_directory)
+			{
+				err.clear();
+				create_directories(new_save_path, err);
+				if (err)
+				{
+					ec.ec = err;
+					ec.file(file_index_t(-1));
+					ec.operation = storage_error::mkdir;
+					return { status_t::fatal_disk_error, save_path };
+				}
+			}
+			else if (err)
+			{
+				ec.ec = err;
+				ec.file(file_index_t(-1));
+				ec.operation = storage_error::stat;
+				return { status_t::fatal_disk_error, save_path };
+			}
+		}
+
+		// indices of all files we ended up copying. These need to be deleted
+		// later
+		aux::vector<bool, file_index_t> copied_files(std::size_t(f.num_files()), false);
+
+		file_index_t i;
+		error_code e;
+		for (i = file_index_t(0); i < f.end_file(); ++i)
+		{
+			// files moved out to absolute paths are not moved
+			if (f.file_absolute_path(i)) continue;
+
+			std::string const old_path = combine_path(save_path, f.file_path(i));
+			std::string const new_path = combine_path(new_save_path, f.file_path(i));
+
+			if (flags == dont_replace && exists(new_path))
+			{
+				if (ret == status_t::no_error) ret = status_t::need_full_check;
+				continue;
+			}
+
+			// 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
+			if (e == boost::system::errc::no_such_file_or_directory)
+				e.clear();
+			else if (e
+				&& e != boost::system::errc::invalid_argument
+				&& e != boost::system::errc::permission_denied)
+			{
+				// moving the file failed
+				// on OSX, the error when trying to rename a file across different
+				// volumes is EXDEV, which will make it fall back to copying.
+				e.clear();
+				copy_file(old_path, new_path, e);
+				if (!e) copied_files[i] = true;
+			}
+
+			if (e)
+			{
+				ec.ec = e;
+				ec.file(i);
+				ec.operation = storage_error::rename;
+				break;
+			}
+		}
+
+		if (!e && pf)
+		{
+			pf->move_partfile(new_save_path, e);
+			if (e)
+			{
+				ec.ec = e;
+				ec.file(file_index_t(-1));
+				ec.operation = storage_error::partfile_move;
+			}
+		}
+
+		if (e)
+		{
+			// rollback
+			while (--i >= file_index_t(0))
+			{
+				// files moved out to absolute paths are not moved
+				if (f.file_absolute_path(i)) continue;
+
+				// if we ended up copying the file, don't do anything during
+				// roll-back
+				if (copied_files[i]) continue;
+
+				std::string const old_path = combine_path(save_path, f.file_path(i));
+				std::string const new_path = combine_path(new_save_path, f.file_path(i));
+
+				// ignore errors when rolling back
+				error_code ignore;
+				move_file(new_path, old_path, ignore);
+			}
+
+			return { status_t::fatal_disk_error, save_path };
+		}
+
+		// TODO: 2 technically, this is where the transaction of moving the files
+		// is completed. This is where the new save_path should be committed. If
+		// there is an error in the code below, that should not prevent the new
+		// save path to be set. Maybe it would make sense to make the save_path
+		// an in-out parameter
+
+		std::set<std::string> subdirs;
+		for (i = file_index_t(0); i < f.end_file(); ++i)
+		{
+			// files moved out to absolute paths are not moved
+			if (f.file_absolute_path(i)) continue;
+
+			if (has_parent_path(f.file_path(i)))
+				subdirs.insert(parent_path(f.file_path(i)));
+
+			// if we ended up renaming the file instead of moving it, there's no
+			// need to delete the source.
+			if (copied_files[i] == false) continue;
+
+			std::string const 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::string const& s : subdirs)
+		{
+			error_code err;
+			std::string subdir = combine_path(save_path, s);
+
+			while (subdir != save_path && !err)
+			{
+				remove(subdir, err);
+				subdir = parent_path(subdir);
+			}
+		}
+
+		return { ret, new_save_path };
+	}
+
 }