improve abstraction level of stats cache and fix test

include/libtorrent/stat_cache.hpp

@@ -35,13 +35,15 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include "libtorrent/aux_/disable_warnings_push.hpp"
 
-#include <time.h>
 #include <vector>
+#include <string>
 #include <boost/cstdint.hpp>
 
 #include "libtorrent/aux_/disable_warnings_pop.hpp"
 
 #include "libtorrent/config.hpp"
+#include "libtorrent/error_code.hpp"
+#include "libtorrent/file_storage.hpp"
 
 namespace libtorrent
 {
@@ -50,36 +52,52 @@ namespace libtorrent
 		stat_cache();
 		~stat_cache();
 
-		void init(int num_files);
+		void reserve(int num_files);
 
-		enum
+		// returns the size of the file unless an error occurs, in which case ec
-		{
+		// is set to indicate the error
-			cache_error = -1,
+		boost::int64_t get_filesize(int i, file_storage const& fs
-			not_in_cache = -2,
+			, std::string const& save_path, error_code& ec);
-			no_exist = -3
-		};
 
-		// returns the size of the file or one
-		// of the enums, noent or not_in_cache
-		boost::int64_t get_filesize(int i) const;
-		time_t get_filetime(int i) const;
-
-		void set_cache(int i, boost::int64_t size, time_t time);
-		void set_noexist(int i);
-		void set_error(int i);
 		void set_dirty(int i);
 
 		void clear();
 
+		// internal
+		enum
+		{
+			not_in_cache = -1,
+			file_error = -2 // (first index in m_errors)
+		};
+
+		// internal
+		void set_cache(int i, boost::int64_t size);
+		void set_error(int i, error_code const& ec);
+
 	private:
 
+		// returns the index to the specified error. Either an existing one or a
+		// newly added entry
+		int add_error(error_code const& ec);
+
 		struct stat_cache_t
 		{
-			stat_cache_t(boost::int64_t s, time_t t = 0): file_size(s), file_time(t) {}
+			stat_cache_t(boost::int64_t s): file_size(s) {}
+
+			// the size of the file. Negative values have special meaning. -1 means
+			// not-in-cache (i.e. there's no data for this file in the cache).
+			// lower values (larger negative values) indicate that an error
+			// occurred while stat()ing the file. The positive value is an index
+			// into m_errors, that recorded the actual error.
 			boost::int64_t file_size;
-			time_t file_time;
 		};
+
+		// one entry per file
 		std::vector<stat_cache_t> m_stat_cache;
+
+		// These are the errors that have happened when stating files. Each entry
+		// that had an error, refers to an index into this vector.
+		std::vector<error_code> m_errors;
 	};
 }
 

src/stat_cache.cpp

@@ -32,22 +32,34 @@ POSSIBILITY OF SUCH DAMAGE.
 
 #include "libtorrent/stat_cache.hpp"
 #include "libtorrent/assert.hpp"
+#include "libtorrent/error_code.hpp"
+#include "libtorrent/file.hpp"
+
+#include <string>
 
 namespace libtorrent
 {
+	class file_storage;
+
 	stat_cache::stat_cache() {}
 	stat_cache::~stat_cache() {}
 
-	// TODO: 4 improve this interface by fall back to the actual stat() call internally. Don't
+	void stat_cache::set_cache(int i, boost::int64_t size)
-	// expose low-level functions to query the cache and set cache state. Also, we should
-	// probably cache the error_code too
-	void stat_cache::set_cache(int i, boost::int64_t size, time_t time)
 	{
 		TORRENT_ASSERT(i >= 0);
 		if (i >= int(m_stat_cache.size()))
 			m_stat_cache.resize(i + 1, not_in_cache);
 		m_stat_cache[i].file_size = size;
-		m_stat_cache[i].file_time = time;
+	}
+
+	void stat_cache::set_error(int i, error_code const& ec)
+	{
+		TORRENT_ASSERT(i >= 0);
+		if (i >= int(m_stat_cache.size()))
+			m_stat_cache.resize(i + 1, not_in_cache);
+
+		int error_index = add_error(ec);
+		m_stat_cache[i].file_size = file_error - error_index;
 	}
 
 	void stat_cache::set_dirty(int i)
@@ -57,37 +69,38 @@ namespace libtorrent
 		m_stat_cache[i].file_size = not_in_cache;
 	}
 
-	void stat_cache::set_noexist(int i)
+	boost::int64_t stat_cache::get_filesize(int i, file_storage const& fs
+		, std::string const& save_path, error_code& ec)
 	{
-		TORRENT_ASSERT(i >= 0);
+		TORRENT_ASSERT(i < int(fs.num_files()));
-		if (i >= int(m_stat_cache.size()))
+		if (i >= int(m_stat_cache.size())) m_stat_cache.resize(i + 1, not_in_cache);
-			m_stat_cache.resize(i + 1, not_in_cache);
+		boost::int64_t sz = m_stat_cache[i].file_size;
-		m_stat_cache[i].file_size = no_exist;
+		if (sz < not_in_cache)
+		{
+			ec = m_errors[-sz + file_error];
+			return file_error;
+		}
+		else if (sz == not_in_cache)
+		{
+			// query the filesystem
+			file_status s;
+			std::string file_path = fs.file_path(i, save_path);
+			stat_file(file_path, &s, ec);
+			if (ec)
+			{
+				set_error(i, ec);
+				sz = file_error;
+			}
+			else
+			{
+				set_cache(i, s.file_size);
+				sz = s.file_size;
+			}
+		}
+		return sz;
 	}
 
-	void stat_cache::set_error(int i)
+	void stat_cache::reserve(int num_files)
-	{
-		TORRENT_ASSERT(i >= 0);
-		if (i >= int(m_stat_cache.size()))
-			m_stat_cache.resize(i + 1, not_in_cache);
-		m_stat_cache[i].file_size = cache_error;
-	}
-
-	boost::int64_t stat_cache::get_filesize(int i) const
-	{
-		if (i >= int(m_stat_cache.size())) return not_in_cache;
-		return m_stat_cache[i].file_size;
-	}
-
-	// TODO: 4 file_time can probably be removed from the cache now
-	time_t stat_cache::get_filetime(int i) const
-	{
-		if (i >= int(m_stat_cache.size())) return not_in_cache;
-		if (m_stat_cache[i].file_size < 0) return m_stat_cache[i].file_size;
-		return m_stat_cache[i].file_time;
-	}
-
-	void stat_cache::init(int num_files)
 	{
 		m_stat_cache.resize(num_files, not_in_cache);
 	}
@@ -95,7 +108,15 @@ namespace libtorrent
 	void stat_cache::clear()
 	{
 		std::vector<stat_cache_t>().swap(m_stat_cache);
+		std::vector<error_code>().swap(m_errors);
 	}
 
+	int stat_cache::add_error(error_code const& ec)
+	{
+		std::vector<error_code>::iterator i = std::find(m_errors.begin(), m_errors.end(), ec);
+		if (i != m_errors.end()) return i - m_errors.begin();
+		m_errors.push_back(ec);
+		return m_errors.size() - 1;
+	}
 }
 

src/storage.cpp

@@ -509,7 +509,7 @@ namespace libtorrent
 
 	void default_storage::initialize(storage_error& ec)
 	{
-		m_stat_cache.init(files().num_files());
+		m_stat_cache.reserve(files().num_files());
 
 #ifdef TORRENT_WINDOWS
 		// don't do full file allocations on network drives
@@ -540,25 +540,22 @@ namespace libtorrent
 			// ignore pad files
 			if (files().pad_file_at(file_index)) continue;
 
-			if (m_stat_cache.get_filesize(file_index) == stat_cache::not_in_cache)
+			error_code err;
+			boost::int64_t size = m_stat_cache.get_filesize(file_index, files()
+				, m_save_path, err);
+
+			if (err && err != boost::system::errc::no_such_file_or_directory)
 			{
-				file_status s;
+				ec.file = file_index;
-				std::string file_path = files().file_path(file_index, m_save_path);
+				ec.operation = storage_error::stat;
-				stat_file(file_path, &s, ec.ec);
+				ec.ec = err;
-				if (ec && ec.ec != boost::system::errc::no_such_file_or_directory)
+				break;
-				{
-					m_stat_cache.set_error(file_index);
-					ec.file = file_index;
-					ec.operation = storage_error::stat;
-					break;
-				}
-				m_stat_cache.set_cache(file_index, s.file_size, s.mtime);
 			}
 
 			// if the file already exists, but is larger than what
 			// it's supposed to be, truncate it
 			// if the file is empty, just create it either way.
-			if ((!ec && m_stat_cache.get_filesize(file_index) > files().file_size(file_index))
+			if ((!err && size > files().file_size(file_index))
 				|| files().file_size(file_index) == 0)
 			{
 				std::string file_path = files().file_path(file_index, m_save_path);
@@ -579,9 +576,14 @@ namespace libtorrent
 				ec.ec.clear();
 				file_handle f = open_file(file_index, file::read_write
 					| file::random_access, ec);
-				if (ec) return;
+				if (ec)
+				{
+					ec.file = file_index;
+					ec.operation = storage_error::fallocate;
+					return;
+				}
 
-				boost::int64_t size = files().file_size(file_index);
+				size = files().file_size(file_index);
 				f->set_size(size, ec.ec);
 				if (ec)
 				{
@@ -589,8 +591,6 @@ namespace libtorrent
 					ec.operation = storage_error::fallocate;
 					break;
 				}
-				size_t mtime = m_stat_cache.get_filetime(file_index);
-				m_stat_cache.set_cache(file_index, size, mtime);
 			}
 			ec.ec.clear();
 		}
@@ -609,48 +609,37 @@ namespace libtorrent
 
 	bool default_storage::has_any_file(storage_error& ec)
 	{
-		m_stat_cache.init(files().num_files());
+		m_stat_cache.reserve(files().num_files());
 
 		std::string file_path;
 		for (int i = 0; i < files().num_files(); ++i)
 		{
-			file_status s;
+			boost::int64_t sz = m_stat_cache.get_filesize(
-			boost::int64_t cache_status = m_stat_cache.get_filesize(i);
+				i, files(), m_save_path, ec.ec);
-			if (cache_status < 0 && cache_status != stat_cache::no_exist)
+
+			if (sz < 0)
 			{
-				file_path = files().file_path(i, m_save_path);
+				if (ec && ec.ec != boost::system::errc::no_such_file_or_directory)
-				stat_file(file_path, &s, ec.ec);
-				boost::int64_t r = s.file_size;
-				if (ec.ec || !(s.mode & file_status::regular_file)) r = -1;
-
-				if (ec && ec.ec == boost::system::errc::no_such_file_or_directory)
-				{
-					ec.ec.clear();
-					r = -3;
-				}
-				m_stat_cache.set_cache(i, r, s.mtime);
-
-				if (ec)
 				{
 					ec.file = i;
 					ec.operation = storage_error::stat;
 					m_stat_cache.clear();
 					return false;
 				}
+				// some files not existing is expected and not an error
+				ec.ec.clear();
 			}
 
-			// if we didn't find the file, check the next one
+			if (sz > 0) return true;
-			if (m_stat_cache.get_filesize(i) == stat_cache::no_exist) continue;
-
-			if (m_stat_cache.get_filesize(i) > 0)
-				return true;
 		}
 		file_status s;
 		stat_file(combine_path(m_save_path, m_part_file_name), &s, ec.ec);
 		if (!ec) return true;
 
+		// the part file not existing is expected
 		if (ec && ec.ec == boost::system::errc::no_such_file_or_directory)
 			ec.ec.clear();
+
 		if (ec)
 		{
 			ec.file = -1;
@@ -889,39 +878,27 @@ namespace libtorrent
 					TORRENT_ASSERT(!f.empty());
 
 					const int file_index = f[0].file_index;
-					boost::int64_t size = m_stat_cache.get_filesize(f[0].file_index);
+					error_code error;
+					boost::int64_t size = m_stat_cache.get_filesize(f[0].file_index
+						, fs, m_save_path, error);
 
-					if (size == stat_cache::not_in_cache)
+					if (size < 0)
 					{
-						file_status s;
+						if (error != boost::system::errc::no_such_file_or_directory)
-						error_code error;
+						{
-						std::string file_path = fs.file_path(file_index, m_save_path);
+							ec.ec = error;
-						stat_file(file_path, &s, error);
+							ec.file = i;
-						size = s.file_size;
+							ec.operation = storage_error::stat;
-						if (error)
+							return false;
+						}
+						else
 						{
-							if (error != boost::system::errc::no_such_file_or_directory)
-							{
-								m_stat_cache.set_error(i);
-								ec.ec = error;
-								ec.file = i;
-								ec.operation = storage_error::stat;
-								return false;
-							}
-							m_stat_cache.set_noexist(i);
 							ec.ec = errors::mismatching_file_size;
 							ec.file = i;
 							ec.operation = storage_error::stat;
 							return false;
 						}
 					}
-					if (size < 0)
-					{
-						ec.ec = errors::mismatching_file_size;
-						ec.file = i;
-						ec.operation = storage_error::check_resume;
-						return false;
-					}
 
 					// OK, this file existed, good. Now, skip all remaining pieces in
 					// this file. We're just sanity-checking whether the files exist

test/test_stat_cache.cpp

@@ -42,39 +42,46 @@ TORRENT_TEST(stat_cache)
 
 	stat_cache sc;
 
-	sc.init(10);
+	file_storage fs;
-
+	for (int i = 0; i < 20; ++i)
-	for (int i = 0; i < 10; ++i)
 	{
-		TEST_CHECK(sc.get_filesize(i) == stat_cache::not_in_cache);
+		char buf[50];
-		TEST_CHECK(sc.get_filetime(i) == stat_cache::not_in_cache);
+		snprintf(buf, sizeof(buf), "test_torrent/test-%d", i);
+		fs.add_file(buf, (i + 1) * 10);
 	}
 
-	// out of bound accesses count as not-in-cache
+	std::string save_path = ".";
-	TEST_CHECK(sc.get_filesize(10) == stat_cache::not_in_cache);
-	TEST_CHECK(sc.get_filesize(11) == stat_cache::not_in_cache);
 
-	sc.set_error(3);
+	sc.reserve(10);
-	TEST_CHECK(sc.get_filesize(3) == stat_cache::cache_error);
 
-	sc.set_noexist(3);
+	sc.set_error(3, error_code(boost::system::errc::permission_denied, generic_category()));
-	TEST_CHECK(sc.get_filesize(3) == stat_cache::no_exist);
+	ec.clear();
+	TEST_EQUAL(sc.get_filesize(3, fs, save_path, ec), stat_cache::file_error);
+	TEST_EQUAL(ec, error_code(boost::system::errc::permission_denied, generic_category()));
 
-	sc.set_cache(3, 101, 5555);
+	sc.set_error(3, error_code(boost::system::errc::no_such_file_or_directory, generic_category()));
-	TEST_CHECK(sc.get_filesize(3) == 101);
+	ec.clear();
-	TEST_CHECK(sc.get_filetime(3) == 5555);
+	TEST_EQUAL(sc.get_filesize(3, fs, save_path, ec), stat_cache::file_error);
+	TEST_EQUAL(ec, error_code(boost::system::errc::no_such_file_or_directory, generic_category()));
 
-	sc.set_error(11);
+	ec.clear();
-	TEST_CHECK(sc.get_filesize(10) == stat_cache::not_in_cache);
+	sc.set_cache(3, 101);
-	TEST_CHECK(sc.get_filesize(11) == stat_cache::cache_error);
+	TEST_EQUAL(sc.get_filesize(3, fs, save_path, ec), 101);
+	TEST_CHECK(!ec);
 
-	sc.set_noexist(13);
+	sc.set_error(11, error_code(boost::system::errc::broken_pipe, generic_category()));
-	TEST_CHECK(sc.get_filesize(12) == stat_cache::not_in_cache);
+	ec.clear();
-	TEST_CHECK(sc.get_filesize(13) == stat_cache::no_exist);
+	TEST_EQUAL(sc.get_filesize(11, fs, save_path, ec), stat_cache::file_error);
+	TEST_EQUAL(ec, error_code(boost::system::errc::broken_pipe, generic_category()));
 
-	sc.set_cache(15, 1000, 3000);
+	ec.clear();
-	TEST_CHECK(sc.get_filesize(14) == stat_cache::not_in_cache);
+	sc.set_error(13, error_code(boost::system::errc::no_such_file_or_directory, generic_category()));
-	TEST_CHECK(sc.get_filesize(15) == 1000);
+	TEST_EQUAL(sc.get_filesize(13, fs, save_path, ec), stat_cache::file_error);
-	TEST_CHECK(sc.get_filetime(15) == 3000);
+	TEST_EQUAL(ec, error_code(boost::system::errc::no_such_file_or_directory, generic_category()));
+
+	ec.clear();
+	sc.set_cache(15, 1000);
+	TEST_CHECK(sc.get_filesize(15, fs, save_path, ec) == 1000);
+	TEST_CHECK(!ec);
 }