pull check resume data logic out of piece_manager into disk_io_thread and simplify it

include/libtorrent/disk_interface.hpp

@@ -49,6 +49,16 @@ namespace libtorrent
 
 	struct TORRENT_EXTRA_EXPORT disk_interface
 	{
+		enum return_t
+		{
+			// return values from check_fastresume, and move_storage
+			no_error = 0,
+			fatal_disk_error = -1,
+			need_full_check = -2,
+			disk_check_aborted = -3,
+			file_exist = -4
+		};
+
 		virtual void async_read(piece_manager* storage, peer_request const& r
 			, std::function<void(disk_io_job const*)> handler, void* requester
 			, int flags = 0) = 0;

include/libtorrent/disk_io_thread.hpp

@@ -367,10 +367,6 @@ namespace libtorrent
 
 		int prep_read_job_impl(disk_io_job* j, bool check_fence = true);
 
-#if TORRENT_USE_INVARIANT_CHECKS
-		void check_invariant() const;
-#endif
-
 		void maybe_issue_queued_read_jobs(cached_piece_entry* pe,
 			jobqueue_t& completed_jobs);
 		int do_read(disk_io_job* j, jobqueue_t& completed_jobs);

include/libtorrent/storage.hpp

@@ -548,16 +548,6 @@ namespace libtorrent
 
 		file_storage const* files() const { return &m_files; }
 
-		enum return_t
-		{
-			// return values from check_fastresume, and move_storage
-			no_error = 0,
-			fatal_disk_error = -1,
-			need_full_check = -2,
-			disk_check_aborted = -3,
-			file_exist = -4
-		};
-
 		storage_interface* get_storage_impl() { return m_storage.get(); }
 
 		bool set_need_tick()
@@ -575,21 +565,6 @@ namespace libtorrent
 
 	private:
 
-		// if error is set and return value is 'no_error' or 'need_full_check'
-		// the error message indicates that the fast resume data was rejected
-		// if 'fatal_disk_error' is returned, the error message indicates what
-		// when wrong in the disk access
-		int check_fastresume(add_torrent_params const& rd
-			, std::vector<std::string> const& links
-			, storage_error& error);
-
-		// helper functions for check_fastresume
-		int check_no_fastresume(storage_error& error);
-		int check_init_storage(storage_error& error);
-
-#if TORRENT_USE_INVARIANT_CHECKS
-		void check_invariant() const;
-#endif
 		file_storage const& m_files;
 
 		bool m_need_tick = false;

src/disk_io_thread.cpp

@@ -36,7 +36,6 @@ POSSIBILITY OF SUCH DAMAGE.
 #include "libtorrent/string_util.hpp" // for allocate_string_copy
 #include "libtorrent/disk_buffer_holder.hpp"
 #include "libtorrent/alloca.hpp"
-#include "libtorrent/invariant_check.hpp"
 #include "libtorrent/error_code.hpp"
 #include "libtorrent/error.hpp"
 #include "libtorrent/file_pool.hpp"
@@ -542,8 +541,6 @@ namespace libtorrent
 	int disk_io_thread::build_iovec(cached_piece_entry* pe, int start, int end
 		, span<file::iovec_t> iov, span<int> flushing, int block_base_index)
 	{
-		INVARIANT_CHECK;
-
 		DLOG("build_iovec: piece=%d [%d, %d)\n"
 			, int(pe->piece), start, end);
 		TORRENT_PIECE_ASSERT(start >= 0, pe);
@@ -727,7 +724,6 @@ namespace libtorrent
 		, jobqueue_t& completed_jobs, std::unique_lock<std::mutex>& l)
 	{
 		TORRENT_ASSERT(l.owns_lock());
-		INVARIANT_CHECK;
 
 		DLOG("flush_range: piece=%d [%d, %d)\n"
 			, int(pe->piece), start, end);
@@ -1065,7 +1061,6 @@ namespace libtorrent
 
 	void disk_io_thread::perform_job(disk_io_job* j, jobqueue_t& completed_jobs)
 	{
-		INVARIANT_CHECK;
 		TORRENT_ASSERT(j->next == nullptr);
 		TORRENT_ASSERT((j->flags & disk_io_job::in_progress) || !j->storage);
 
@@ -1455,7 +1450,6 @@ namespace libtorrent
 
 	int disk_io_thread::do_write(disk_io_job* j, jobqueue_t& completed_jobs)
 	{
-		INVARIANT_CHECK;
 		TORRENT_ASSERT(j->d.io.buffer_size <= m_disk_cache.block_size());
 
 		std::unique_lock<std::mutex> l(m_cache_mutex);
@@ -1517,8 +1511,6 @@ namespace libtorrent
 		, std::function<void(disk_io_job const*)> handler, void* requester
 		, int flags)
 	{
-		INVARIANT_CHECK;
-
 		TORRENT_ASSERT(r.length <= m_disk_cache.block_size());
 		TORRENT_ASSERT(r.length <= 16 * 1024);
 
@@ -1631,8 +1623,6 @@ namespace libtorrent
 		, std::function<void(disk_io_job const*)> handler
 		, int const flags)
 	{
-		INVARIANT_CHECK;
-
 		TORRENT_ASSERT(r.length <= m_disk_cache.block_size());
 		TORRENT_ASSERT(r.length <= 16 * 1024);
 
@@ -2141,8 +2131,6 @@ namespace libtorrent
 
 	int disk_io_thread::do_hash(disk_io_job* j, jobqueue_t& /* completed_jobs */ )
 	{
-		INVARIANT_CHECK;
-
 		int const piece_size = j->storage->files()->piece_size(j->piece);
 		int const file_flags = file_flags_for_job(j
 			, m_settings.get_bool(settings_pack::coalesce_reads));
@@ -2396,8 +2384,6 @@ namespace libtorrent
 
 	int disk_io_thread::do_release_files(disk_io_job* j, jobqueue_t& completed_jobs)
 	{
-		INVARIANT_CHECK;
-
 		// if this assert fails, something's wrong with the fence logic
 		TORRENT_ASSERT(j->storage->num_outstanding_jobs() == 1);
 
@@ -2412,7 +2398,6 @@ namespace libtorrent
 	int disk_io_thread::do_delete_files(disk_io_job* j, jobqueue_t& completed_jobs)
 	{
 		TORRENT_ASSERT(j->buffer.delete_options != 0);
-		INVARIANT_CHECK;
 
 		// if this assert fails, something's wrong with the fence logic
 		TORRENT_ASSERT(j->storage->num_outstanding_jobs() == 1);
@@ -2437,8 +2422,59 @@ namespace libtorrent
 		if (rd == nullptr) rd = &tmp;
 
 		std::unique_ptr<std::vector<std::string>> links(j->d.links);
-		return j->storage->check_fastresume(*rd
+		// check if the fastresume data is up to date
-			, links ? *links : std::vector<std::string>(), j->error);
+		// if it is, use it and return true. If it
+		// isn't return false and the full check
+		// will be run. If the links pointer is non-empty, it has the same number
+		// of elements as there are files. Each element is either empty or contains
+		// the absolute path to a file identical to the corresponding file in this
+		// torrent. The storage must create hard links (or copy) those files. If
+		// any file does not exist or is inaccessible, the disk job must fail.
+
+		TORRENT_ASSERT(j->storage->m_files.piece_length() > 0);
+
+		// if we don't have any resume data, return
+		// or if error is set and return value is 'no_error' or 'need_full_check'
+		// the error message indicates that the fast resume data was rejected
+		// if 'fatal_disk_error' is returned, the error message indicates what
+		// when wrong in the disk access
+		storage_error se;
+		if ((rd->have_pieces.empty()
+			|| !j->storage->get_storage_impl()->verify_resume_data(*rd
+				, links ? *links : std::vector<std::string>(), j->error))
+			&& !m_settings.get_bool(settings_pack::no_recheck_incomplete_resume))
+		{
+			// j->error may have been set at this point, by verify_resume_data()
+			// it's important to not have it cleared out subsequent calls, as long
+			// as they succeed.
+			bool const has_files = j->storage->get_storage_impl()->has_any_file(se);
+
+			if (se)
+			{
+				j->error = se;
+				return disk_interface::fatal_disk_error;
+			}
+
+			if (has_files)
+			{
+				// always initialize the storage
+				j->storage->get_storage_impl()->initialize(se);
+				if (se)
+				{
+					j->error = se;
+					return disk_interface::fatal_disk_error;
+				}
+				return disk_interface::need_full_check;
+			}
+		}
+
+		j->storage->get_storage_impl()->initialize(se);
+		if (se)
+		{
+			j->error = se;
+			return disk_interface::fatal_disk_error;
+		}
+		return disk_interface::no_error;
 	}
 
 	int disk_io_thread::do_rename_file(disk_io_job* j, jobqueue_t& /* completed_jobs */ )
@@ -3348,10 +3384,4 @@ namespace libtorrent
 
 		if (cnt > 0) free_jobs(to_delete.data(), cnt);
 	}
-
-#if TORRENT_USE_INVARIANT_CHECKS
-	void disk_io_thread::check_invariant() const
-	{
-	}
-#endif
 }

src/storage.cpp

@@ -927,7 +927,7 @@ namespace libtorrent
 	int default_storage::move_storage(std::string const& sp, int const flags
 		, storage_error& ec)
 	{
-		int ret = piece_manager::no_error;
+		int ret = disk_interface::no_error;
 		std::string const save_path = complete(sp);
 
 		// check to see if any of the files exist
@@ -952,7 +952,7 @@ namespace libtorrent
 						ec.ec = err;
 						ec.file = i;
 						ec.operation = storage_error::stat;
-						return piece_manager::file_exist;
+						return disk_interface::file_exist;
 					}
 				}
 			}
@@ -971,7 +971,7 @@ namespace libtorrent
 					ec.ec = err;
 					ec.file = -1;
 					ec.operation = storage_error::mkdir;
-					return piece_manager::fatal_disk_error;
+					return disk_interface::fatal_disk_error;
 				}
 			}
 			else if (err)
@@ -979,7 +979,7 @@ namespace libtorrent
 				ec.ec = err;
 				ec.file = -1;
 				ec.operation = storage_error::stat;
-				return piece_manager::fatal_disk_error;
+				return disk_interface::fatal_disk_error;
 			}
 		}
 
@@ -1001,7 +1001,7 @@ namespace libtorrent
 
 			if (flags == dont_replace && exists(new_path))
 			{
-				if (ret == piece_manager::no_error) ret = piece_manager::need_full_check;
+				if (ret == disk_interface::no_error) ret = disk_interface::need_full_check;
 				continue;
 			}
 
@@ -1053,7 +1053,7 @@ namespace libtorrent
 				}
 			}
 
-			return piece_manager::fatal_disk_error;
+			return disk_interface::fatal_disk_error;
 		}
 
 		std::string const old_save_path = m_save_path;
@@ -1481,68 +1481,6 @@ namespace libtorrent
 
 	piece_manager::~piece_manager() = default;
 
-	int piece_manager::check_no_fastresume(storage_error& ec)
-	{
-		if (!m_storage->settings().get_bool(settings_pack::no_recheck_incomplete_resume))
-		{
-			storage_error se;
-			bool const has_files = m_storage->has_any_file(se);
-
-			if (se)
-			{
-				ec = se;
-				return fatal_disk_error;
-			}
-
-			if (has_files)
-			{
-				// always initialize the storage
-				int ret = check_init_storage(ec);
-				return ret != no_error ? ret : need_full_check;
-			}
-		}
-
-		return check_init_storage(ec);
-	}
-
-	int piece_manager::check_init_storage(storage_error& ec)
-	{
-		storage_error se;
-		// initialize may clear the error we pass in and it's important to
-		// preserve the error code in ec, even when initialize() is successful
-		m_storage->initialize(se);
-		if (se)
-		{
-			ec = se;
-			return fatal_disk_error;
-		}
-		return no_error;
-	}
-
-	// check if the fastresume data is up to date
-	// if it is, use it and return true. If it
-	// isn't return false and the full check
-	// will be run. If the links pointer is non-empty, it has the same number
-	// of elements as there are files. Each element is either empty or contains
-	// the absolute path to a file identical to the corresponding file in this
-	// torrent. The storage must create hard links (or copy) those files. If
-	// any file does not exist or is inaccessible, the disk job must fail.
-	int piece_manager::check_fastresume(
-		add_torrent_params const& rd
-		, std::vector<std::string> const& links
-		, storage_error& ec)
-	{
-		TORRENT_ASSERT(m_files.piece_length() > 0);
-
-		// if we don't have any resume data, return
-		if (rd.have_pieces.empty()) return check_no_fastresume(ec);
-
-		if (!m_storage->verify_resume_data(rd, links, ec))
-			return check_no_fastresume(ec);
-
-		return check_init_storage(ec);
-	}
-
 	// ====== disk_job_fence implementation ========
 
 	disk_job_fence::disk_job_fence()

src/torrent.cpp

@@ -1994,7 +1994,7 @@ namespace libtorrent
 
 		TORRENT_ASSERT(is_single_thread());
 
-		if (j->ret == piece_manager::fatal_disk_error)
+		if (j->ret == disk_interface::fatal_disk_error)
 		{
 			TORRENT_ASSERT(m_outstanding_check_files == false);
 			m_add_torrent_params.reset();
@@ -2248,7 +2248,7 @@ namespace libtorrent
 
 		if (m_abort) return;
 
-		if (j->ret == piece_manager::fatal_disk_error)
+		if (j->ret == disk_interface::fatal_disk_error)
 		{
 			handle_disk_error(j);
 			return;
@@ -2320,7 +2320,7 @@ namespace libtorrent
 
 		if (m_abort) return;
 
-		if (j->ret == piece_manager::disk_check_aborted)
+		if (j->ret == disk_interface::disk_check_aborted)
 		{
 			m_checking_piece = 0;
 			m_num_checked_pieces = 0;
@@ -7825,13 +7825,14 @@ namespace libtorrent
 		TORRENT_ASSERT(is_single_thread());
 
 		m_moving_storage = false;
-		if (j->ret == piece_manager::no_error || j->ret == piece_manager::need_full_check)
+		if (j->ret == disk_interface::no_error
+			|| j->ret == disk_interface::need_full_check)
 		{
 			if (alerts().should_post<storage_moved_alert>())
 				alerts().emplace_alert<storage_moved_alert>(get_handle(), j->buffer.string);
 			m_save_path = j->buffer.string;
 			set_need_save_resume();
-			if (j->ret == piece_manager::need_full_check)
+			if (j->ret == disk_interface::need_full_check)
 				force_recheck();
 		}
 		else

test/test_storage.cpp

@@ -73,17 +73,17 @@ void on_check_resume_data(disk_io_job const* j, bool* done)
 	std::cerr << time_now_string() << " on_check_resume_data ret: " << j->ret;
 	switch (j->ret)
 	{
-		case piece_manager::no_error:
+		case disk_interface::no_error:
 			std::cerr << time_now_string() << " success" << std::endl;
 			break;
-		case piece_manager::fatal_disk_error:
+		case disk_interface::fatal_disk_error:
 			std::cerr << time_now_string() << " disk error: " << j->error.ec.message()
 				<< " file: " << j->error.file << std::endl;
 			break;
-		case piece_manager::need_full_check:
+		case disk_interface::need_full_check:
 			std::cerr << time_now_string() << " need full check" << std::endl;
 			break;
-		case piece_manager::disk_check_aborted:
+		case disk_interface::disk_check_aborted:
 			std::cerr << time_now_string() << " aborted" << std::endl;
 			break;
 	}
@@ -717,6 +717,7 @@ void test_fastresume(bool const test_deprecated)
 		p.storage_mode = storage_mode_sparse;
 		torrent_handle h = ses.add_torrent(p, ec);
 
+		std::printf("expecting fastresume to be rejected becase the files were removed");
 		alert const* a = wait_for_alert(ses, fastresume_rejected_alert::alert_type
 			, "ses");
 		// we expect the fast resume to be rejected because the files were removed