From dad961e672ee3f5e8b99292ba31491dc8c93b1ab Mon Sep 17 00:00:00 2001 From: arvidn Date: Sun, 10 Mar 2024 14:35:14 +0100 Subject: [PATCH] improve const correctness of disk I/O code by making readwrite() a template that can work with both span and span. --- CMakeLists.txt | 1 + Makefile | 1 + include/libtorrent/aux_/mmap_storage.hpp | 2 +- include/libtorrent/aux_/part_file.hpp | 3 +- include/libtorrent/aux_/posix_part_file.hpp | 3 +- include/libtorrent/aux_/posix_storage.hpp | 3 +- include/libtorrent/aux_/readwrite.hpp | 137 ++++++++++++++++++++ include/libtorrent/aux_/storage_utils.hpp | 22 +--- include/libtorrent/aux_/utp_stream.hpp | 3 +- src/mmap_storage.cpp | 9 +- src/part_file.cpp | 3 +- src/posix_part_file.cpp | 4 +- src/posix_storage.cpp | 6 +- src/storage_utils.cpp | 85 ------------ src/utp_stream.cpp | 1 - test/test_storage.cpp | 14 +- 16 files changed, 166 insertions(+), 131 deletions(-) create mode 100644 include/libtorrent/aux_/readwrite.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index e8237c6858c..31768b82e43 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -231,6 +231,7 @@ set(libtorrent_aux_include_files puff.hpp random.hpp range.hpp + readwrite.hpp receive_buffer.hpp request_blocks.hpp resolve_links.hpp diff --git a/Makefile b/Makefile index 3b65d9fa338..624eafefaa1 100644 --- a/Makefile +++ b/Makefile @@ -631,6 +631,7 @@ HEADERS = \ aux_/puff.hpp \ aux_/random.hpp \ aux_/range.hpp \ + aux_/readwrite.hpp \ aux_/receive_buffer.hpp \ aux_/request_blocks.hpp \ aux_/resolve_links.hpp \ diff --git a/include/libtorrent/aux_/mmap_storage.hpp b/include/libtorrent/aux_/mmap_storage.hpp index 491f2b1eedb..eacbabd9de1 100644 --- a/include/libtorrent/aux_/mmap_storage.hpp +++ b/include/libtorrent/aux_/mmap_storage.hpp @@ -83,7 +83,7 @@ namespace libtorrent::aux { , piece_index_t piece, int offset, aux::open_mode_t mode , disk_job_flags_t flags , storage_error&); - int write(settings_interface const&, span buffer + int write(settings_interface const&, span buffer , piece_index_t piece, int offset, aux::open_mode_t mode , disk_job_flags_t flags , storage_error&); diff --git a/include/libtorrent/aux_/part_file.hpp b/include/libtorrent/aux_/part_file.hpp index 90a24e9a66b..2d4603dc30b 100644 --- a/include/libtorrent/aux_/part_file.hpp +++ b/include/libtorrent/aux_/part_file.hpp @@ -26,7 +26,6 @@ see LICENSE file. #include "libtorrent/units.hpp" #include "libtorrent/hasher.hpp" #include "libtorrent/aux_/open_mode.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t namespace libtorrent::aux { @@ -39,7 +38,7 @@ namespace libtorrent::aux { part_file(std::string path, std::string name, int num_pieces, int piece_size); ~part_file(); - int write(span buf, piece_index_t piece, int offset, error_code& ec); + int write(span buf, piece_index_t piece, int offset, error_code& ec); int read(span buf, piece_index_t piece, int offset, error_code& ec); int hash(hasher& ph, std::ptrdiff_t len, piece_index_t piece, int offset, error_code& ec); int hash2(hasher256& ph, std::ptrdiff_t len, piece_index_t piece, int offset, error_code& ec); diff --git a/include/libtorrent/aux_/posix_part_file.hpp b/include/libtorrent/aux_/posix_part_file.hpp index ed6115dadc0..89c3224425a 100644 --- a/include/libtorrent/aux_/posix_part_file.hpp +++ b/include/libtorrent/aux_/posix_part_file.hpp @@ -23,7 +23,6 @@ see LICENSE file. #include "libtorrent/units.hpp" #include "libtorrent/hasher.hpp" #include "libtorrent/aux_/open_mode.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t #include "libtorrent/aux_/file_pointer.hpp" namespace libtorrent { @@ -38,7 +37,7 @@ namespace aux { posix_part_file(std::string path, std::string name, int num_pieces, int piece_size); ~posix_part_file(); - int write(span bufs, piece_index_t piece, int offset, error_code& ec); + int write(span bufs, piece_index_t piece, int offset, error_code& ec); int read(span buf, piece_index_t piece, int offset, error_code& ec); int hash(hasher& ph, std::ptrdiff_t len, piece_index_t piece, int offset, error_code& ec); int hash2(hasher256& ph, std::ptrdiff_t len, piece_index_t piece, int offset, error_code& ec); diff --git a/include/libtorrent/aux_/posix_storage.hpp b/include/libtorrent/aux_/posix_storage.hpp index cd29c1562f1..8295c6c09cc 100644 --- a/include/libtorrent/aux_/posix_storage.hpp +++ b/include/libtorrent/aux_/posix_storage.hpp @@ -16,7 +16,6 @@ see LICENSE file. #include "libtorrent/aux_/stat_cache.hpp" #include "libtorrent/file_storage.hpp" #include "libtorrent/storage_defs.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t #include "libtorrent/hex.hpp" // to_hex #include "libtorrent/aux_/open_mode.hpp" // for aux::open_mode_t #include "libtorrent/aux_/file_pointer.hpp" @@ -41,7 +40,7 @@ namespace aux { , storage_error& error); int write(settings_interface const& sett - , span buffer + , span buffer , piece_index_t const piece, int const offset , storage_error& error); diff --git a/include/libtorrent/aux_/readwrite.hpp b/include/libtorrent/aux_/readwrite.hpp new file mode 100644 index 00000000000..0832ecf13f0 --- /dev/null +++ b/include/libtorrent/aux_/readwrite.hpp @@ -0,0 +1,137 @@ +/* + +Copyright (c) 2017-2020, 2022, Arvid Norberg +Copyright (c) 2018, 2020, Alden Torres +All rights reserved. + +You may use, distribute and modify this code under the terms of the BSD license, +see LICENSE file. +*/ + +#ifndef TORRENT_READWRITE_HPP_INCLUDE +#define TORRENT_READWRITE_HPP_INCLUDE + +#include +#include + +#include "libtorrent/units.hpp" +#include "libtorrent/file_storage.hpp" +#include "libtorrent/span.hpp" +#include "libtorrent/assert.hpp" +#include "libtorrent/socket.hpp" + +namespace libtorrent::aux { + +// 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). +// +// Much of what needs to be done when reading and writing is buffer +// management and piece to file mapping. Most of that is the same for reading +// and writing. This function is a template, and the fileop decides what to +// do with the file and the buffers. +// +// op is a read or write operation so that readwrite() knows +// what to do when it's actually touching the file + +template +int readwrite(file_storage const& files, span buf + , piece_index_t const piece, const int offset + , storage_error& ec, Fun op) +{ + return readwrite_impl(files, buf, piece, offset, ec, op); +} + +template +int readwrite(file_storage const& files, span buf + , piece_index_t const piece, const int offset + , storage_error& ec, Fun op) +{ + return readwrite_impl(files, buf, piece, offset, ec, op); +} + +template +int readwrite_impl(file_storage const& files, span buf + , piece_index_t const piece, const int offset + , storage_error& ec, Fun op) +{ + TORRENT_ASSERT(piece >= piece_index_t(0)); + TORRENT_ASSERT(piece < files.end_piece()); + TORRENT_ASSERT(offset >= 0); + TORRENT_ASSERT(buf.size() > 0); + + TORRENT_ASSERT(buf.size() > 0); + TORRENT_ASSERT(static_cast(piece) * static_cast(files.piece_length()) + + offset + buf.size() <= files.total_size()); + + // find the file iterator and file offset + std::int64_t const torrent_offset = static_cast(piece) * std::int64_t(files.piece_length()) + offset; + file_index_t file_index = files.file_index_at_offset(torrent_offset); + TORRENT_ASSERT(torrent_offset >= files.file_offset(file_index)); + TORRENT_ASSERT(torrent_offset < files.file_offset(file_index) + files.file_size(file_index)); + std::int64_t file_offset = torrent_offset - files.file_offset(file_index); + + int ret = 0; + + while (buf.size() > 0) + { + // the number of bytes left to read in the current file (specified by + // file_index). This is the minimum of (file_size - file_offset) and + // buf.size(). + int file_bytes_left = int(buf.size()); + if (file_offset + file_bytes_left > files.file_size(file_index)) + file_bytes_left = std::max(static_cast(files.file_size(file_index) - file_offset), 0); + + // there are no bytes left in this file, move to the next one + // this loop skips over empty files + if (file_bytes_left == 0) + { + do + { + ++file_index; + file_offset = 0; + TORRENT_ASSERT(file_index < files.end_file()); + + // this should not happen. buf.size() should be clamped by the total + // size of the torrent, so we should never run off the end of it + if (file_index >= files.end_file()) return ret; + + // skip empty files + } + while (files.file_size(file_index) == 0); + + file_bytes_left = int(buf.size()); + if (file_offset + file_bytes_left > files.file_size(file_index)) + file_bytes_left = std::max(static_cast(files.file_size(file_index) - file_offset), 0); + TORRENT_ASSERT(file_bytes_left > 0); + } + + int const bytes_transferred = op(file_index, file_offset + , buf.first(file_bytes_left), ec); + TORRENT_ASSERT(bytes_transferred <= file_bytes_left); + if (ec) + { + ec.file(file_index); + return ret; + } + + buf = buf.subspan(bytes_transferred); + file_offset += bytes_transferred; + ret += bytes_transferred; + + // if the file operation returned 0, we've hit end-of-file. We're done + if (bytes_transferred == 0 && file_bytes_left > 0 ) + { + // fill in this information in case the caller wants to treat + // a short-read as an error + ec.operation = operation_t::file_read; + ec.ec = boost::asio::error::eof; + ec.file(file_index); + } + } + return ret; +} + +} + +#endif diff --git a/include/libtorrent/aux_/storage_utils.hpp b/include/libtorrent/aux_/storage_utils.hpp index 3bf5de97b4c..6b9ce0311bb 100644 --- a/include/libtorrent/aux_/storage_utils.hpp +++ b/include/libtorrent/aux_/storage_utils.hpp @@ -18,33 +18,15 @@ see LICENSE file. #include "libtorrent/config.hpp" #include "libtorrent/fwd.hpp" #include "libtorrent/span.hpp" -#include "libtorrent/span.hpp" #include "libtorrent/units.hpp" #include "libtorrent/disk_interface.hpp" // for status_t #include "libtorrent/session_types.hpp" #include "libtorrent/error_code.hpp" -namespace libtorrent { - - // TODO: 3 remove this typedef, and use span for disk write - // operations - using iovec_t = span; - -namespace aux { +namespace libtorrent::aux { struct stat_cache; - // this is a read or write operation so that readwrite() knows - // what to do when it's actually touching the file - using fileop = std::function, storage_error&)>; - - // 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). - TORRENT_EXTRA_EXPORT int readwrite(file_storage const& files - , span buf, piece_index_t piece, int offset - , storage_error& ec, fileop op); - // 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. @@ -102,6 +84,6 @@ namespace aux { TORRENT_EXTRA_EXPORT void copy_file(std::string const& f , std::string const& newf, storage_error& se); -}} +} #endif diff --git a/include/libtorrent/aux_/utp_stream.hpp b/include/libtorrent/aux_/utp_stream.hpp index 596e56c4d79..3d5180ea2da 100644 --- a/include/libtorrent/aux_/utp_stream.hpp +++ b/include/libtorrent/aux_/utp_stream.hpp @@ -23,7 +23,6 @@ see LICENSE file. #include "libtorrent/aux_/sliding_average.hpp" #include "libtorrent/address.hpp" #include "libtorrent/aux_/invariant_check.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t #include @@ -35,6 +34,8 @@ see LICENSE file. namespace libtorrent::aux { + using iovec_t = span; + #ifndef TORRENT_UTP_LOG_ENABLE #define TORRENT_UTP_LOG 0 #define TORRENT_VERBOSE_UTP_LOG 0 diff --git a/src/mmap_storage.cpp b/src/mmap_storage.cpp index 77a696196a7..cfb83b95a6a 100644 --- a/src/mmap_storage.cpp +++ b/src/mmap_storage.cpp @@ -39,6 +39,7 @@ see LICENSE file. #include "libtorrent/aux_/file_view_pool.hpp" #include "libtorrent/aux_/drive_info.hpp" #include "libtorrent/aux_/stat_cache.hpp" +#include "libtorrent/aux_/readwrite.hpp" #include "libtorrent/hex.hpp" // to_hex #if TORRENT_HAVE_MMAP || TORRENT_HAVE_MAP_VIEW_OF_FILE @@ -582,7 +583,7 @@ error_code translate_error(std::error_code const& err, bool const write) } int mmap_storage::write(settings_interface const& sett - , span buffer + , span buffer , piece_index_t const piece, int const offset , aux::open_mode_t const mode , disk_job_flags_t const flags @@ -594,7 +595,7 @@ error_code translate_error(std::error_code const& err, bool const write) return readwrite(files(), buffer, piece, offset, error , [this, mode, flags, &sett](file_index_t const file_index , std::int64_t const file_offset - , span buf, storage_error& ec) + , span buf, storage_error& ec) { if (files().pad_file_at(file_index)) { @@ -682,10 +683,10 @@ error_code translate_error(std::error_code const& err, bool const write) char dummy; std::vector scratch; - return readwrite(files(), {&dummy, len}, piece, offset, error + return readwrite(files(), span{&dummy, len}, piece, offset, error , [this, mode, flags, &ph, &sett, &scratch](file_index_t const file_index , std::int64_t const file_offset - , span const buf, storage_error& ec) + , span const buf, storage_error& ec) { if (files().pad_file_at(file_index)) return aux::hash_zeroes(ph, buf.size()); diff --git a/src/part_file.cpp b/src/part_file.cpp index 4a8808a5b8e..016db7398b1 100644 --- a/src/part_file.cpp +++ b/src/part_file.cpp @@ -46,7 +46,6 @@ see LICENSE file. #include "libtorrent/assert.hpp" #include "libtorrent/aux_/vector.hpp" #include "libtorrent/aux_/path.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t #include // for std::function #include @@ -149,7 +148,7 @@ namespace libtorrent::aux { return slot; } - int part_file::write(span buf, piece_index_t const piece + int part_file::write(span buf, piece_index_t const piece , int const offset, error_code& ec) { TORRENT_ASSERT(offset >= 0); diff --git a/src/posix_part_file.cpp b/src/posix_part_file.cpp index 9622d670de5..9497bcdc59b 100644 --- a/src/posix_part_file.cpp +++ b/src/posix_part_file.cpp @@ -47,7 +47,7 @@ see LICENSE file. #include "libtorrent/assert.hpp" #include "libtorrent/aux_/vector.hpp" #include "libtorrent/aux_/path.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t +#include "libtorrent/aux_/storage_utils.hpp" // for copy_file #include // for std::function #include @@ -153,7 +153,7 @@ namespace aux { return slot; } - int posix_part_file::write(span buf, piece_index_t const piece + int posix_part_file::write(span buf, piece_index_t const piece , int const offset, error_code& ec) { TORRENT_ASSERT(offset >= 0); diff --git a/src/posix_storage.cpp b/src/posix_storage.cpp index 450b579b9ae..7e14c60322e 100644 --- a/src/posix_storage.cpp +++ b/src/posix_storage.cpp @@ -17,6 +17,8 @@ see LICENSE file. #include "libtorrent/aux_/open_mode.hpp" #include "libtorrent/aux_/file_pointer.hpp" #include "libtorrent/torrent_status.hpp" +#include "libtorrent/aux_/storage_utils.hpp" // for read_zeroes, move_storage +#include "libtorrent/aux_/readwrite.hpp" using namespace libtorrent::flags; // for flag operators @@ -197,7 +199,7 @@ namespace aux { } int posix_storage::write(settings_interface const& - , span buffer + , span buffer , piece_index_t const piece, int const offset , storage_error& error) { @@ -207,7 +209,7 @@ namespace aux { return readwrite(files(), buffer, piece, offset, error , [this](file_index_t const file_index , std::int64_t const file_offset - , span buf, storage_error& ec) + , span buf, storage_error& ec) { if (files().pad_file_at(file_index)) { diff --git a/src/storage_utils.cpp b/src/storage_utils.cpp index bfcfa42f8ce..4519831457a 100644 --- a/src/storage_utils.cpp +++ b/src/storage_utils.cpp @@ -33,91 +33,6 @@ see LICENSE file. namespace libtorrent { namespace aux { - // much of what needs to be done when reading and writing is buffer - // management and piece to file mapping. Most of that is the same for reading - // and writing. This function is a template, and the fileop decides what to - // do with the file and the buffers. - int readwrite(file_storage const& files, span buf - , piece_index_t const piece, const int offset - , storage_error& ec, fileop op) - { - TORRENT_ASSERT(piece >= piece_index_t(0)); - TORRENT_ASSERT(piece < files.end_piece()); - TORRENT_ASSERT(offset >= 0); - TORRENT_ASSERT(buf.size() > 0); - - TORRENT_ASSERT(buf.size() > 0); - TORRENT_ASSERT(static_cast(piece) * static_cast(files.piece_length()) - + offset + buf.size() <= files.total_size()); - - // find the file iterator and file offset - std::int64_t const torrent_offset = static_cast(piece) * std::int64_t(files.piece_length()) + offset; - file_index_t file_index = files.file_index_at_offset(torrent_offset); - TORRENT_ASSERT(torrent_offset >= files.file_offset(file_index)); - TORRENT_ASSERT(torrent_offset < files.file_offset(file_index) + files.file_size(file_index)); - std::int64_t file_offset = torrent_offset - files.file_offset(file_index); - - int ret = 0; - - while (buf.size() > 0) - { - // the number of bytes left to read in the current file (specified by - // file_index). This is the minimum of (file_size - file_offset) and - // buf.size(). - int file_bytes_left = int(buf.size()); - if (file_offset + file_bytes_left > files.file_size(file_index)) - file_bytes_left = std::max(static_cast(files.file_size(file_index) - file_offset), 0); - - // there are no bytes left in this file, move to the next one - // this loop skips over empty files - if (file_bytes_left == 0) - { - do - { - ++file_index; - file_offset = 0; - TORRENT_ASSERT(file_index < files.end_file()); - - // this should not happen. buf.size() should be clamped by the total - // size of the torrent, so we should never run off the end of it - if (file_index >= files.end_file()) return ret; - - // skip empty files - } - while (files.file_size(file_index) == 0); - - file_bytes_left = int(buf.size()); - if (file_offset + file_bytes_left > files.file_size(file_index)) - file_bytes_left = std::max(static_cast(files.file_size(file_index) - file_offset), 0); - TORRENT_ASSERT(file_bytes_left > 0); - } - - int const bytes_transferred = op(file_index, file_offset - , buf.first(file_bytes_left), ec); - TORRENT_ASSERT(bytes_transferred <= file_bytes_left); - if (ec) - { - ec.file(file_index); - return ret; - } - - buf = buf.subspan(bytes_transferred); - file_offset += bytes_transferred; - ret += bytes_transferred; - - // if the file operation returned 0, we've hit end-of-file. We're done - if (bytes_transferred == 0 && file_bytes_left > 0 ) - { - // fill in this information in case the caller wants to treat - // a short-read as an error - ec.operation = operation_t::file_read; - ec.ec = boost::asio::error::eof; - ec.file(file_index); - } - } - return ret; - } - std::pair move_storage(file_storage const& f , std::string save_path , std::string const& destination_save_path diff --git a/src/utp_stream.cpp b/src/utp_stream.cpp index 47b1d7e3a36..a30fbe687e8 100644 --- a/src/utp_stream.cpp +++ b/src/utp_stream.cpp @@ -23,7 +23,6 @@ see LICENSE file. #include "libtorrent/aux_/invariant_check.hpp" #include "libtorrent/performance_counters.hpp" #include "libtorrent/io_context.hpp" -#include "libtorrent/aux_/storage_utils.hpp" // for iovec_t #include #include diff --git a/test/test_storage.cpp b/test/test_storage.cpp index 1af85d76af2..c51a81476b7 100644 --- a/test/test_storage.cpp +++ b/test/test_storage.cpp @@ -35,6 +35,7 @@ see LICENSE file. #include "libtorrent/mmap_disk_io.hpp" #include "libtorrent/posix_disk_io.hpp" #include "libtorrent/flags.hpp" +#include "libtorrent/aux_/readwrite.hpp" #include #include // for bind @@ -1179,8 +1180,7 @@ TORRENT_TEST(readwrite_stripe_1) std::vector buf(std::size_t(fs.total_size())); fill_pattern(buf); - int ret = readwrite(fs, buf, 0_piece, 0, ec - , std::ref(fop)); + int ret = readwrite(fs, span(buf), 0_piece, 0, ec, std::ref(fop)); TEST_EQUAL(ret, fs.total_size()); TEST_EQUAL(fop.m_file_data.size(), 4); @@ -1204,7 +1204,7 @@ TORRENT_TEST(readwrite_single_buffer) std::vector buf(size_t(fs.total_size())); fill_pattern(buf); - int ret = readwrite(fs, buf, 0_piece, 0, ec, std::ref(fop)); + int ret = readwrite(fs, span(buf), 0_piece, 0, ec, std::ref(fop)); TEST_EQUAL(ret, fs.total_size()); TEST_EQUAL(fop.m_file_data.size(), 4); @@ -1228,7 +1228,7 @@ TORRENT_TEST(readwrite_read) std::vector buf(size_t(fs.total_size())); // read everything - int ret = readwrite(fs, buf, 0_piece, 0, ec, std::ref(fop)); + int ret = readwrite(fs, span(buf), 0_piece, 0, ec, std::ref(fop)); TEST_EQUAL(ret, fs.total_size()); TEST_CHECK(check_pattern(buf, 0)); @@ -1243,7 +1243,7 @@ TORRENT_TEST(readwrite_read_short) std::vector buf(size_t(fs.total_size())); // read everything - int ret = readwrite(fs, buf, 0_piece, 0, ec, std::ref(fop)); + int ret = readwrite(fs, span(buf), 0_piece, 0, ec, std::ref(fop)); TEST_EQUAL(static_cast(ec.file()), 3); @@ -1261,7 +1261,7 @@ TORRENT_TEST(readwrite_error) std::vector buf(size_t(fs.total_size())); // read everything - int ret = readwrite(fs, buf, 0_piece, 0, ec, std::ref(fop)); + int ret = readwrite(fs, span(buf), 0_piece, 0, ec, std::ref(fop)); TEST_EQUAL(ret, 12); TEST_EQUAL(static_cast(ec.file()), 2); @@ -1286,7 +1286,7 @@ TORRENT_TEST(readwrite_zero_size_files) std::vector buf(size_t(fs.total_size())); // read everything - int ret = readwrite(fs, buf, 0_piece, 0, ec, std::ref(fop)); + int ret = readwrite(fs, span(buf), 0_piece, 0, ec, std::ref(fop)); TEST_EQUAL(ret, fs.total_size()); TEST_CHECK(check_pattern(buf, 0));