From ed4fad9b1868bbdddfe58d4aaf55032ba3eac70a Mon Sep 17 00:00:00 2001 From: Yat Ho Date: Wed, 29 Nov 2023 00:51:13 +0800 Subject: [PATCH] feat: support creating 0 byte files (#6232) --- libtransmission/cache.cc | 10 - libtransmission/cache.h | 3 - libtransmission/file-piece-map.cc | 8 +- libtransmission/file-piece-map.h | 9 +- libtransmission/inout.cc | 25 ++- libtransmission/lru-cache.h | 64 +------ libtransmission/makemeta.cc | 6 +- libtransmission/open-files.cc | 36 ++-- libtransmission/open-files.h | 7 +- libtransmission/torrent-metainfo.cc | 5 - libtransmission/torrent.cc | 44 +++++ libtransmission/torrent.h | 5 +- libtransmission/torrents.cc | 2 +- libtransmission/torrents.h | 2 +- libtransmission/verify.cc | 14 +- libtransmission/verify.h | 2 +- libtransmission/webseed.cc | 2 +- tests/libtransmission/file-piece-map-test.cc | 186 ++++++++----------- tests/libtransmission/makemeta-test.cc | 7 +- tests/libtransmission/open-files-test.cc | 49 +++-- 20 files changed, 207 insertions(+), 279 deletions(-) diff --git a/libtransmission/cache.cc b/libtransmission/cache.cc index e836bd712..c2c18fe70 100644 --- a/libtransmission/cache.cc +++ b/libtransmission/cache.cc @@ -116,8 +116,6 @@ int Cache::write_contiguous(CIter const begin, CIter const end) const int Cache::set_limit(Memory const max_size) { - auto const lock = std::lock_guard{ mutex_ }; - max_blocks_ = get_max_blocks(max_size); tr_logAddDebug(fmt::format("Maximum cache size set to {} ({} blocks)", max_size.to_string(), max_blocks_)); @@ -145,8 +143,6 @@ int Cache::write_block(tr_torrent_id_t tor_id, tr_block_index_t block, std::uniq return tr_ioWrite(tor, tor->block_loc(block), std::size(*writeme), std::data(*writeme)); } - auto const lock = std::lock_guard{ mutex_ }; - auto const key = Key{ tor_id, block }; auto iter = std::lower_bound(std::begin(blocks_), std::end(blocks_), key, CompareCacheBlockByKey); if (iter == std::end(blocks_) || iter->key != key) @@ -180,25 +176,21 @@ Cache::CIter Cache::get_block(tr_torrent const* torrent, tr_block_info::Location int Cache::read_block(tr_torrent* torrent, tr_block_info::Location const& loc, size_t len, uint8_t* setme) { - auto lock = std::unique_lock{ mutex_ }; if (auto const iter = get_block(torrent, loc); iter != std::end(blocks_)) { std::copy_n(std::begin(*iter->buf), len, setme); return {}; } - lock.unlock(); return tr_ioRead(torrent, loc, len, setme); } int Cache::prefetch_block(tr_torrent* torrent, tr_block_info::Location const& loc, size_t len) { - auto lock = std::unique_lock{ mutex_ }; if (auto const iter = get_block(torrent, loc); iter != std::end(blocks_)) { return {}; // already have it } - lock.unlock(); return tr_ioPrefetch(torrent, loc, len); } @@ -228,7 +220,6 @@ int Cache::flush_file(tr_torrent const* const torrent, tr_file_index_t const fil auto const tor_id = torrent->id(); auto const [block_begin, block_end] = torrent->block_span_for_file(file); - auto const lock = std::lock_guard{ mutex_ }; return flush_span( std::lower_bound(std::begin(blocks_), std::end(blocks_), std::make_pair(tor_id, block_begin), CompareCacheBlockByKey), std::lower_bound(std::begin(blocks_), std::end(blocks_), std::make_pair(tor_id, block_end), CompareCacheBlockByKey)); @@ -238,7 +229,6 @@ int Cache::flush_torrent(tr_torrent const* torrent) { auto const tor_id = torrent->id(); - auto const lock = std::lock_guard{ mutex_ }; return flush_span( std::lower_bound(std::begin(blocks_), std::end(blocks_), std::make_pair(tor_id, 0), CompareCacheBlockByKey), std::lower_bound(std::begin(blocks_), std::end(blocks_), std::make_pair(tor_id + 1, 0), CompareCacheBlockByKey)); diff --git a/libtransmission/cache.h b/libtransmission/cache.h index ab6979ada..26660464c 100644 --- a/libtransmission/cache.h +++ b/libtransmission/cache.h @@ -12,7 +12,6 @@ #include // for size_t #include // for intX_t, uintX_t #include // for std::unique_ptr -#include #include // for std::pair #include @@ -91,8 +90,6 @@ private: mutable size_t cache_writes_ = 0; mutable size_t cache_write_bytes_ = 0; - mutable std::mutex mutex_; - static constexpr struct { [[nodiscard]] constexpr bool operator()(Key const& key, CacheBlock const& block) diff --git a/libtransmission/file-piece-map.cc b/libtransmission/file-piece-map.cc index 232c8a362..dbd6985c6 100644 --- a/libtransmission/file-piece-map.cc +++ b/libtransmission/file-piece-map.cc @@ -77,17 +77,17 @@ void tr_file_piece_map::reset(tr_torrent_metainfo const& tm) tr_file_piece_map::file_span_t tr_file_piece_map::file_span(tr_piece_index_t piece) const { - static constexpr auto Compare = CompareToSpan{ false }; + static constexpr auto Compare = CompareToSpan{}; auto const begin = std::begin(file_pieces_); auto const& [equal_begin, equal_end] = std::equal_range(begin, std::end(file_pieces_), piece, Compare); return { static_cast(equal_begin - begin), static_cast(equal_end - begin) }; } -tr_file_piece_map::file_offset_t tr_file_piece_map::file_offset(uint64_t offset, bool include_empty_files) const +tr_file_piece_map::file_offset_t tr_file_piece_map::file_offset(uint64_t offset) const { - auto const compare = CompareToSpan{ include_empty_files }; + static constexpr auto Compare = CompareToSpan{}; auto const begin = std::begin(file_bytes_); - auto const it = std::lower_bound(begin, std::end(file_bytes_), offset, compare); + auto const it = std::lower_bound(begin, std::end(file_bytes_), offset, Compare); tr_file_index_t const file_index = std::distance(begin, it); auto const file_offset = offset - it->begin; return file_offset_t{ file_index, file_offset }; diff --git a/libtransmission/file-piece-map.h b/libtransmission/file-piece-map.h index 217cf6658..5ce2328ed 100644 --- a/libtransmission/file-piece-map.h +++ b/libtransmission/file-piece-map.h @@ -63,7 +63,7 @@ public: [[nodiscard]] file_span_t file_span(tr_piece_index_t piece) const; - [[nodiscard]] file_offset_t file_offset(uint64_t offset, bool include_empty_files) const; + [[nodiscard]] file_offset_t file_offset(uint64_t offset) const; [[nodiscard]] TR_CONSTEXPR20 size_t size() const { @@ -104,11 +104,6 @@ private: [[nodiscard]] constexpr int compare(T item, span_t span) const // <=> { - if (include_empty_span && span.begin == span.end) - { - return tr_compare_3way(item, span.begin); - } - if (item < span.begin) { return -1; @@ -136,8 +131,6 @@ private: { return compare(span, item) < 0; } - - bool const include_empty_span; }; }; diff --git a/libtransmission/inout.cc b/libtransmission/inout.cc index 53fa7b10d..5216a8605 100644 --- a/libtransmission/inout.cc +++ b/libtransmission/inout.cc @@ -78,7 +78,7 @@ enum class IoMode Write }; -bool getFilename(tr_pathbuf& setme, tr_torrent const* tor, tr_file_index_t file_index, IoMode io_mode) +bool getFilename(tr_pathbuf& setme, tr_torrent const* tor, tr_file_index_t file_index, bool do_write) { if (auto found = tor->find_file(file_index); found) { @@ -86,7 +86,7 @@ bool getFilename(tr_pathbuf& setme, tr_torrent const* tor, tr_file_index_t file_ return true; } - if (io_mode != IoMode::Write) + if (!do_write) { return false; } @@ -129,9 +129,9 @@ void readOrWriteBytes( // --- Find the fd - auto fd_top = session->openFiles().get(tor->id(), file_index, do_write); + auto fd = session->openFiles().get(tor->id(), file_index, do_write); auto filename = tr_pathbuf{}; - if (!fd_top && !getFilename(filename, tor, file_index, io_mode)) + if (!fd && !getFilename(filename, tor, file_index, do_write)) { auto const err = ENOENT; error->set( @@ -144,20 +144,20 @@ void readOrWriteBytes( return; } - if (!fd_top) // not in the cache, so open or create it now + if (!fd) // not in the cache, so open or create it now { // open (and maybe create) the file auto const prealloc = (!do_write || !tor->file_is_wanted(file_index)) ? TR_PREALLOCATE_NONE : tor->session->preallocationMode(); - fd_top = session->openFiles().get(tor->id(), file_index, do_write, filename, prealloc, file_size); - if (fd_top && do_write) + fd = session->openFiles().get(tor->id(), file_index, do_write, filename, prealloc, file_size); + if (fd && do_write) { // make a note that we just created a file tor->session->add_file_created(); } } - if (!fd_top) // couldn't create/open it either + if (!fd) // couldn't create/open it either { auto const errnum = errno; error->set( @@ -171,11 +171,10 @@ void readOrWriteBytes( return; } - auto const& [fd, tag] = *fd_top; switch (io_mode) { case IoMode::Read: - if (!readEntireBuf(fd, file_offset, buf, buflen, error) && *error) + if (!readEntireBuf(*fd, file_offset, buf, buflen, error) && *error) { tr_logAddErrorTor( tor, @@ -189,7 +188,7 @@ void readOrWriteBytes( break; case IoMode::Write: - if (!writeEntireBuf(fd, file_offset, buf, buflen, error) && *error) + if (!writeEntireBuf(*fd, file_offset, buf, buflen, error) && *error) { tr_logAddErrorTor( tor, @@ -203,7 +202,7 @@ void readOrWriteBytes( break; case IoMode::Prefetch: - tr_sys_file_advise(fd, file_offset, buflen, TR_SYS_FILE_ADVICE_WILL_NEED); + tr_sys_file_advise(*fd, file_offset, buflen, TR_SYS_FILE_ADVICE_WILL_NEED); break; } } @@ -216,7 +215,7 @@ int readOrWritePiece(tr_torrent* tor, IoMode io_mode, tr_block_info::Location lo return EINVAL; } - auto [file_index, file_offset] = tor->file_offset(loc, false); + auto [file_index, file_offset] = tor->file_offset(loc); while (buflen != 0U) { diff --git a/libtransmission/lru-cache.h b/libtransmission/lru-cache.h index 50a708403..faf9ce61b 100644 --- a/libtransmission/lru-cache.h +++ b/libtransmission/lru-cache.h @@ -6,53 +6,44 @@ #pragma once #include -#include #include // size_t #include #include -#include -#include #include -#include "libtransmission/observable.h" - // A fixed-size cache that erases least-recently-used items to make room for new ones. template class tr_lru_cache { public: - [[nodiscard]] TR_CONSTEXPR23 std::optional> get(Key const& key) noexcept + [[nodiscard]] constexpr Val* get(Key const& key) noexcept { - auto const lock = std::lock_guard{ mutex_ }; - if (auto* const found = find(key); found != nullptr) + if (auto const found = find(key); found != nullptr) { found->sequence_ = next_sequence_++; - return lock_and_get_entry(*found); + return &found->val_; } - return {}; + return nullptr; } - [[nodiscard]] TR_CONSTEXPR23 bool contains(Key const& key) const noexcept + [[nodiscard]] constexpr bool contains(Key const& key) const noexcept { return find(key) != nullptr; } - std::pair add(Key&& key) + Val& add(Key&& key) { - auto const lock = std::lock_guard{ mutex_ }; - auto& entry = get_free_slot(); entry.key_ = std::move(key); entry.sequence_ = next_sequence_++; key = {}; - return lock_and_get_entry(entry); + return entry.val_; } void erase(Key const& key) { - auto const lock = std::lock_guard{ mutex_ }; if (auto* const found = find(key); found != nullptr) { this->erase(*found); @@ -61,7 +52,6 @@ public: void erase_if(std::function test) { - auto const lock = std::lock_guard{ mutex_ }; for (auto& entry : entries_) { if (entry.sequence_ != InvalidSeq && test(entry.key_, entry.val_)) @@ -73,7 +63,6 @@ public: void clear() { - auto const lock = std::lock_guard{ mutex_ }; for (auto& entry : entries_) { erase(entry); @@ -85,38 +74,18 @@ private: { Key key_ = {}; Val val_ = {}; - size_t in_use_ = 0U; uint64_t sequence_ = InvalidSeq; }; - std::pair lock_and_get_entry(Entry& entry) - { - auto const lock = std::lock_guard{ mutex_ }; - ++entry.in_use_; - return std::make_pair( - std::ref(entry.val_), - libtransmission::ObserverTag{ [this, &entry]() - { - auto lock2 = std::unique_lock{ mutex_ }; - --entry.in_use_; - TR_ASSERT(entry.in_use_ >= 0U); - lock2.unlock(); - cv_.notify_one(); - } }); - } - void erase(Entry& entry) { - auto const lock = std::lock_guard{ mutex_ }; - entry.key_ = {}; entry.val_ = {}; entry.sequence_ = InvalidSeq; } - [[nodiscard]] TR_CONSTEXPR23 Entry* find(Key const& key) noexcept + [[nodiscard]] constexpr Entry* find(Key const& key) noexcept { - auto const lock = std::lock_guard{ mutex_ }; for (auto& entry : entries_) { if (entry.sequence_ != InvalidSeq && entry.key_ == key) @@ -128,9 +97,8 @@ private: return nullptr; } - [[nodiscard]] TR_CONSTEXPR23 Entry const* find(Key const& key) const noexcept + [[nodiscard]] constexpr Entry const* find(Key const& key) const noexcept { - auto const lock = std::lock_guard{ mutex_ }; for (auto const& entry : entries_) { if (entry.sequence_ != InvalidSeq && entry.key_ == key) @@ -144,19 +112,10 @@ private: Entry& get_free_slot() { - auto lock = std::unique_lock{ mutex_ }; - cv_.wait( - lock, - [this]() { - return std::any_of( - std::begin(entries_), - std::end(entries_), - [](auto const& entry) { return entry.in_use_ == 0U; }); - }); auto const iter = std::min_element( std::begin(entries_), std::end(entries_), - [](auto const& a, auto const& b) { return b.in_use_ > 0U || a.sequence_ < b.sequence_; }); + [](auto const& a, auto const& b) { return a.sequence_ < b.sequence_; }); erase(*iter); return *iter; } @@ -164,7 +123,4 @@ private: std::array entries_; uint64_t next_sequence_ = 1U; static uint64_t constexpr InvalidSeq = 0U; - - mutable std::recursive_mutex mutex_; - std::condition_variable_any cv_; }; diff --git a/libtransmission/makemeta.cc b/libtransmission/makemeta.cc index 669b47561..e2cb79b2b 100644 --- a/libtransmission/makemeta.cc +++ b/libtransmission/makemeta.cc @@ -158,7 +158,7 @@ bool tr_metainfo_builder::blocking_make_checksums(tr_error* error) { if (error != nullptr) { - error->set_from_errno(ENOENT); + error->set(ENOENT, "zero-length torrents are not allowed"sv); } return false; @@ -374,12 +374,10 @@ std::string tr_metainfo_builder::benc(tr_error* error) const uint32_t tr_metainfo_builder::default_piece_size(uint64_t total_size) noexcept { - TR_ASSERT(total_size != 0); - // Ideally, we want approximately 2^10 = 1024 pieces, give or take a few hundred pieces. // So we subtract 10 from the log2 of total size. // The ideal number of pieces is up for debate. - auto exp = std::log2(total_size) - 10; + auto exp = (total_size > 0U ? std::log2(total_size) : 0) - 10; // We want a piece size between 16KiB (2^14 bytes) and 16MiB (2^24 bytes) for maximum compatibility exp = std::clamp(exp, 14., 24.); diff --git a/libtransmission/open-files.cc b/libtransmission/open-files.cc index 8e4727c8e..53a60428f 100644 --- a/libtransmission/open-files.cc +++ b/libtransmission/open-files.cc @@ -17,14 +17,11 @@ #include "libtransmission/error.h" #include "libtransmission/file.h" #include "libtransmission/log.h" -#include "libtransmission/observable.h" #include "libtransmission/open-files.h" #include "libtransmission/tr-assert.h" #include "libtransmission/tr-strbuf.h" #include "libtransmission/utils.h" // _() -using namespace std::literals; - namespace { @@ -125,26 +122,22 @@ bool preallocate_file_full(tr_sys_file_t fd, uint64_t length, tr_error* error) // --- -std::optional> tr_open_files::get( - tr_torrent_id_t tor_id, - tr_file_index_t file_num, - bool writable) +std::optional tr_open_files::get(tr_torrent_id_t tor_id, tr_file_index_t file_num, bool writable) { - if (auto found = pool_.get(make_key(tor_id, file_num)); found) + if (auto* const found = pool_.get(make_key(tor_id, file_num)); found != nullptr) { - auto& [entry, tag] = *found; - if (writable && !entry.writable_) + if (writable && !found->writable_) { return {}; } - return std::pair{ entry.fd_, std::move(tag) }; + return found->fd_; } return {}; } -std::optional> tr_open_files::get( +std::optional tr_open_files::get( tr_torrent_id_t tor_id, tr_file_index_t file_num, bool writable, @@ -154,12 +147,11 @@ std::optional> tr_open_fi { // is there already an entry auto key = make_key(tor_id, file_num); - if (auto found = pool_.get(key); found) + if (auto* const found = pool_.get(key); found != nullptr) { - auto& [entry, tag] = *found; - if (!writable || entry.writable_) + if (!writable || found->writable_) { - return std::pair{ entry.fd_, std::move(tag) }; + return found->fd_; } pool_.erase(key); // close so we can re-open as writable @@ -207,20 +199,20 @@ std::optional> tr_open_fi if (writable && !already_existed && allocation != TR_PREALLOCATE_NONE) { bool success = false; - auto type = std::string_view{}; + char const* type = nullptr; if (allocation == TR_PREALLOCATE_FULL) { success = preallocate_file_full(fd, file_size, &error); - type = "full"sv; + type = "full"; } else if (allocation == TR_PREALLOCATE_SPARSE) { success = preallocate_file_sparse(fd, file_size, &error); - type = "sparse"sv; + type = "sparse"; } - TR_ASSERT(!std::empty(type)); + TR_ASSERT(type != nullptr); if (!success) { @@ -253,11 +245,11 @@ std::optional> tr_open_fi } // cache it - auto [entry, tag] = pool_.add(std::move(key)); + auto& entry = pool_.add(std::move(key)); entry.fd_ = fd; entry.writable_ = writable; - return std::pair{ fd, std::move(tag) }; + return fd; } void tr_open_files::close_all() diff --git a/libtransmission/open-files.h b/libtransmission/open-files.h index 4c2d8bbb8..9680561ee 100644 --- a/libtransmission/open-files.h +++ b/libtransmission/open-files.h @@ -24,12 +24,9 @@ class tr_open_files { public: - [[nodiscard]] std::optional> get( - tr_torrent_id_t tor_id, - tr_file_index_t file_num, - bool writable); + [[nodiscard]] std::optional get(tr_torrent_id_t tor_id, tr_file_index_t file_num, bool writable); - [[nodiscard]] std::optional> get( + [[nodiscard]] std::optional get( tr_torrent_id_t tor_id, tr_file_index_t file_num, bool writable, diff --git a/libtransmission/torrent-metainfo.cc b/libtransmission/torrent-metainfo.cc index a1102f9f9..63eb6fe95 100644 --- a/libtransmission/torrent-metainfo.cc +++ b/libtransmission/torrent-metainfo.cc @@ -460,11 +460,6 @@ private: { bool ok = true; - if (file_length_ == 0) - { - return ok; - } - // FIXME: Check to see if we already added this file. This is a safeguard // for hybrid torrents with duplicate info between "file tree" and "files" if (std::empty(file_subpath_)) diff --git a/libtransmission/torrent.cc b/libtransmission/torrent.cc index d5c81c3fd..0fdfe2805 100644 --- a/libtransmission/torrent.cc +++ b/libtransmission/torrent.cc @@ -720,6 +720,10 @@ void tr_torrent::start_in_session_thread() TR_ASSERT(session->am_in_session_thread()); auto const lock = unique_lock(); + // We are after `torrentStart` and before announcing to trackers/peers, + // so now is the best time to create wanted empty files. + create_empty_files(); + recheck_completeness(); set_is_queued(false); @@ -882,6 +886,9 @@ void tr_torrent::on_metainfo_completed() if (session->shouldFullyVerifyAddedTorrents() || !is_new_torrent_a_seed()) { + // Potentially, we are in `tr_torrentNew`, + // and we don't want any file created before `torrentStart` + // so we Verify but we don't Create files. tr_torrentVerify(this); } else @@ -1703,6 +1710,43 @@ namespace completeness_helpers } // namespace completeness_helpers } // namespace +void tr_torrent::create_empty_files() const +{ + auto const base = current_dir(); + TR_ASSERT(!std::empty(base)); + if (!has_metainfo() || std::empty(base)) + { + return; + } + + auto const file_count = this->file_count(); + for (tr_file_index_t file_index = 0U; file_index < file_count; ++file_index) + { + if (file_size(file_index) != 0U || !file_is_wanted(file_index) || find_file(file_index)) + { + continue; + } + + // torrent contains a wanted zero-bytes file and that file isn't on disk yet. + // We attempt to create that file. + auto filename = tr_pathbuf{}; + auto const& subpath = file_subpath(file_index); + filename.assign(base, '/', subpath); + + // create subfolders, if any + auto dir = tr_pathbuf{ filename.sv() }; + dir.popdir(); + tr_sys_dir_create(dir, TR_SYS_DIR_CREATE_PARENTS, 0777); + + // create the file + if (auto const fd = tr_sys_file_open(filename, TR_SYS_FILE_WRITE | TR_SYS_FILE_CREATE | TR_SYS_FILE_SEQUENTIAL, 0666); + fd != TR_BAD_SYS_FILE) + { + tr_sys_file_close(fd); + } + } +} + void tr_torrent::recheck_completeness() { using namespace completeness_helpers; diff --git a/libtransmission/torrent.h b/libtransmission/torrent.h index 072832929..4a3b7e837 100644 --- a/libtransmission/torrent.h +++ b/libtransmission/torrent.h @@ -388,9 +388,9 @@ public: return fpm_.piece_span(file); } - [[nodiscard]] auto file_offset(tr_block_info::Location loc, bool include_empty_files) const + [[nodiscard]] auto file_offset(tr_block_info::Location loc) const { - return fpm_.file_offset(loc.byte, include_empty_files); + return fpm_.file_offset(loc.byte); } /// WANTED @@ -1229,6 +1229,7 @@ private: void on_file_completed(tr_file_index_t file); void on_tracker_response(tr_tracker_event const* event); + void create_empty_files() const; void recheck_completeness(); void set_location_in_session_thread(std::string_view path, bool move_from_old_path, int volatile* setme_state); diff --git a/libtransmission/torrents.cc b/libtransmission/torrents.cc index 560b6f24f..a4552c66a 100644 --- a/libtransmission/torrents.cc +++ b/libtransmission/torrents.cc @@ -57,7 +57,7 @@ tr_torrent* tr_torrents::get(tr_sha1_digest_t const& hash) const return begin == end ? nullptr : *begin; } -tr_torrent* tr_torrents::find_from_obfuscated_hash(tr_sha1_digest_t const& obfuscated_hash) +tr_torrent* tr_torrents::find_from_obfuscated_hash(tr_sha1_digest_t const& obfuscated_hash) const { for (auto* const tor : *this) { diff --git a/libtransmission/torrents.h b/libtransmission/torrents.h index d67bb6455..f8003663b 100644 --- a/libtransmission/torrents.h +++ b/libtransmission/torrents.h @@ -50,7 +50,7 @@ public: } // O(n)} - [[nodiscard]] tr_torrent* find_from_obfuscated_hash(tr_sha1_digest_t const& obfuscated_hash); + [[nodiscard]] tr_torrent* find_from_obfuscated_hash(tr_sha1_digest_t const& obfuscated_hash) const; // These convenience functions use get(tr_sha1_digest_t const&) // after parsing the magnet link to get the info hash. If you have diff --git a/libtransmission/verify.cc b/libtransmission/verify.cc index cfa5fd029..3641a6bc1 100644 --- a/libtransmission/verify.cc +++ b/libtransmission/verify.cc @@ -34,7 +34,7 @@ auto constexpr SleepPerSecondDuringVerify = 100ms; } } // namespace -void tr_verify_worker::verify_torrent(Mediator& verify_mediator, std::atomic const& abort_flag) +void tr_verify_worker::verify_torrent(Mediator& verify_mediator, bool const abort_flag) { verify_mediator.on_verify_started(); @@ -54,7 +54,7 @@ void tr_verify_worker::verify_torrent(Mediator& verify_mediator, std::atomicc_str(), TR_SYS_FILE_READ | TR_SYS_FILE_SEQUENTIAL, 0); @@ -71,7 +71,7 @@ void tr_verify_worker::verify_torrent(Mediator& verify_mediator, std::atomic 0) + if (tr_sys_file_read_at(fd, std::data(buffer), bytes_this_pass, file_pos, &num_read) && num_read > 0U) { bytes_this_pass = num_read; sha->add(std::data(buffer), bytes_this_pass); @@ -86,7 +86,7 @@ void tr_verify_worker::verify_torrent(Mediator& verify_mediator, std::atomicfinish() == metainfo.piece_hash(piece); verify_mediator.on_piece_checked(piece, has_piece); @@ -101,11 +101,11 @@ void tr_verify_worker::verify_torrent(Mediator& verify_mediator, std::atomicclear(); ++piece; - piece_pos = 0; + piece_pos = 0U; } /* if we're finishing a file... */ - if (left_in_file == 0) + if (left_in_file == 0U) { if (fd != TR_BAD_SYS_FILE) { @@ -114,7 +114,7 @@ void tr_verify_worker::verify_torrent(Mediator& verify_mediator, std::atomic const& abort_flag); + static void verify_torrent(Mediator& verify_mediator, bool abort_flag); void verify_thread_func(); diff --git a/libtransmission/webseed.cc b/libtransmission/webseed.cc index 8e2d0825b..74dc52ebd 100644 --- a/libtransmission/webseed.cc +++ b/libtransmission/webseed.cc @@ -502,7 +502,7 @@ void task_request_next_chunk(tr_webseed_task* task) auto const loc = tor->byte_loc(task->loc.byte + evbuffer_get_length(task->content())); - auto const [file_index, file_offset] = tor->file_offset(loc, false); + auto const [file_index, file_offset] = tor->file_offset(loc); auto const left_in_file = tor->file_size(file_index) - file_offset; auto const left_in_task = task->end_byte - loc.byte; auto const this_chunk = std::min(left_in_file, left_in_task); diff --git a/tests/libtransmission/file-piece-map-test.cc b/tests/libtransmission/file-piece-map-test.cc index c6c94ce0c..89209bef3 100644 --- a/tests/libtransmission/file-piece-map-test.cc +++ b/tests/libtransmission/file-piece-map-test.cc @@ -25,13 +25,12 @@ protected: static constexpr size_t TotalSize{ 10 * PieceSize + 1 }; tr_block_info const block_info_{ TotalSize, PieceSize }; - static constexpr std::array FileSizes{ - 0U, // [offset 0] zero-sized file - PieceSize, // [offset 0] begins and ends on a piece boundary (preceded by zero sized file) - 4U * PieceSize, // [offset P] begins and ends on a piece boundary + static constexpr std::array FileSizes{ + 5U * PieceSize, // [offset 0] begins and ends on a piece boundary 0U, // [offset 5 P] zero-sized files 0U, 0U, + 0U, PieceSize / 2U, // [offset 5 P] begins on a piece boundary PieceSize, // [offset 5.5 P] neither begins nor ends on a piece boundary, spans >1 piece 10U, // [offset 6.5 P] small files all contained in a single piece @@ -52,7 +51,7 @@ protected: static_assert( FileSizes[0] + FileSizes[1] + FileSizes[2] + FileSizes[3] + FileSizes[4] + FileSizes[5] + FileSizes[6] + FileSizes[7] + FileSizes[8] + FileSizes[9] + FileSizes[10] + FileSizes[11] + FileSizes[12] + FileSizes[13] + - FileSizes[14] + FileSizes[15] + FileSizes[16] + FileSizes[17] == + FileSizes[14] + FileSizes[15] + FileSizes[16] == TotalSize); EXPECT_EQ(11U, block_info_.piece_count()); @@ -62,59 +61,33 @@ protected: } }; -TEST_F(FilePieceMapTest, fileOffsetNoEmptyFiles) +TEST_F(FilePieceMapTest, fileOffset) { auto const fpm = tr_file_piece_map{ block_info_, std::data(FileSizes), std::size(FileSizes) }; - // first byte of file #1 (first nonzero file) - auto file_offset = fpm.file_offset(0U, false); - EXPECT_EQ(1U, file_offset.index); - EXPECT_EQ(0U, file_offset.offset); - - // final byte of file #1 (first nonzero file) - file_offset = fpm.file_offset(FileSizes[1] - 1U, false); - EXPECT_EQ(1U, file_offset.index); - EXPECT_EQ(FileSizes[1] - 1U, file_offset.offset); - - // first byte of file #6 (nonzero file preceded by zero file) - // NB: this is an edge case, file #3 is 0 bytes. - // The next nonzero file is file #6 - file_offset = fpm.file_offset(FileSizes[1] + FileSizes[2], false); - EXPECT_EQ(6U, file_offset.index); - EXPECT_EQ(0U, file_offset.offset); - - // the last byte of in the torrent. - // NB: reverse of previous edge case, since - // the final 4 files in the torrent are all 0 bytes - // the fifth file from end will be selected - file_offset = fpm.file_offset(TotalSize - 1U, false); - EXPECT_EQ(13U, file_offset.index); - EXPECT_EQ(FileSizes[13] - 1U, file_offset.offset); -} - -TEST_F(FilePieceMapTest, fileOffsetWithEmptyFiles) -{ - auto const fpm = tr_file_piece_map{ block_info_, std::data(FileSizes), std::size(FileSizes) }; - - // first byte of file #0 (zero file) - auto file_offset = fpm.file_offset(0U, true); + // first byte of the first file + auto file_offset = fpm.file_offset(0U); EXPECT_EQ(0U, file_offset.index); EXPECT_EQ(0U, file_offset.offset); - // first byte of file #3 (zero file preceded by nonzero file) - // NB: this is an edge case, file #3 is 0 bytes. - // The next nonzero file is file #6 - file_offset = fpm.file_offset(FileSizes[1] + FileSizes[2], true); - EXPECT_EQ(3U, file_offset.index); + // final byte of the first file + file_offset = fpm.file_offset(FileSizes[0] - 1); + EXPECT_EQ(0U, file_offset.index); + EXPECT_EQ(FileSizes[0] - 1, file_offset.offset); + + // first byte of the second file + // NB: this is an edge case, second file is 0 bytes. + // The second nonzero file is file #5 + file_offset = fpm.file_offset(FileSizes[0]); + EXPECT_EQ(5U, file_offset.index); EXPECT_EQ(0U, file_offset.offset); // the last byte of in the torrent. // NB: reverse of previous edge case, since // the final 4 files in the torrent are all 0 bytes - // the fifth file from end will be selected - file_offset = fpm.file_offset(TotalSize - 1U, true); - EXPECT_EQ(13U, file_offset.index); - EXPECT_EQ(FileSizes[13] - 1U, file_offset.offset); + file_offset = fpm.file_offset(TotalSize - 1); + EXPECT_EQ(12U, file_offset.index); + EXPECT_EQ(FileSizes[12] - 1, file_offset.offset); } TEST_F(FilePieceMapTest, pieceSpan) @@ -122,10 +95,9 @@ TEST_F(FilePieceMapTest, pieceSpan) // Note to reviewers: it's easy to see a nonexistent fencepost error here. // Remember everything is zero-indexed, so the 11 valid pieces are [0..10] // and that last piece #10 has one byte in it. Piece #11 is the 'end' iterator position. - static auto constexpr ExpectedPieceSpans = std::array{ { - { 0U, 1U }, - { 0U, 1U }, - { 1U, 5U }, + auto constexpr ExpectedPieceSpans = std::array{ { + { 0U, 5U }, + { 5U, 6U }, { 5U, 6U }, { 5U, 6U }, { 5U, 6U }, @@ -200,14 +172,13 @@ TEST_F(FilePieceMapTest, priorities) mark_file_endpoints_as_high_priority(); compare_to_expected(); - // set the file #2 as high priority. + // set the first file as high priority. // since this begins and ends on a piece boundary, - // and it is not preceded by a zero sized file, // this shouldn't affect any other files' pieces auto pri = TR_PRI_HIGH; - file_priorities.set(2U, pri); - expected_file_priorities[2] = pri; - for (size_t i = 1U; i < 5U; ++i) + file_priorities.set(0U, pri); + expected_file_priorities[0] = pri; + for (size_t i = 0U; i < 5U; ++i) { expected_piece_priorities[i] = pri; } @@ -216,27 +187,27 @@ TEST_F(FilePieceMapTest, priorities) // This file shares a piece with another file. // If _either_ is set to high, the piece's priority should be high. - // file #6: byte [500..550) piece [5, 6) - // file #7: byte [550..650) piece [5, 7) + // file #5: byte [500..550) piece [5, 6) + // file #6: byte [550..650) piece [5, 7) // - // first test setting file #6... + // first test setting file #5... pri = TR_PRI_HIGH; - file_priorities.set(6U, pri); - expected_file_priorities[6] = pri; + file_priorities.set(5U, pri); + expected_file_priorities[5] = pri; expected_piece_priorities[5] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); // ...and that shared piece should still be the same when both are high... - file_priorities.set(7U, pri); - expected_file_priorities[7] = pri; + file_priorities.set(6U, pri); + expected_file_priorities[6] = pri; expected_piece_priorities[5] = pri; expected_piece_priorities[6] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); - // ...and that shared piece should still be the same when only 7 is high... + // ...and that shared piece should still be the same when only 6 is high... pri = TR_PRI_NORMAL; - file_priorities.set(6U, pri); - expected_file_priorities[6] = pri; + file_priorities.set(5U, pri); + expected_file_priorities[5] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); @@ -253,19 +224,19 @@ TEST_F(FilePieceMapTest, priorities) // Raise the priority of a small 1-piece file. // Since it's the highest priority in the piece, piecePriority() should return its value. - // file #9: byte [650, 659) piece [6, 7) + // file #8: byte [650, 659) piece [6, 7) pri = TR_PRI_NORMAL; - file_priorities.set(9U, pri); - expected_file_priorities[9] = pri; + file_priorities.set(8U, pri); + expected_file_priorities[8] = pri; expected_piece_priorities[6] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); // Raise the priority of another small 1-piece file in the same piece. // Since _it_ now has the highest priority in the piece, piecePriority should return _its_ value. - // file #10: byte [659, 667) piece [6, 7) + // file #9: byte [659, 667) piece [6, 7) pri = TR_PRI_HIGH; - file_priorities.set(10U, pri); - expected_file_priorities[10] = pri; + file_priorities.set(9U, pri); + expected_file_priorities[9] = pri; expected_piece_priorities[6] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); @@ -288,17 +259,17 @@ TEST_F(FilePieceMapTest, priorities) // other does no real harm. Let's KISS. // // Check that even zero-sized files can change a piece's priority - // file #3: byte [500, 500) piece [5, 6) + // file #1: byte [500, 500) piece [5, 6) pri = TR_PRI_HIGH; - file_priorities.set(3U, pri); - expected_file_priorities[3] = pri; + file_priorities.set(1U, pri); + expected_file_priorities[1] = pri; expected_piece_priorities[5] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); // Check that zero-sized files at the end of a torrent change the last piece's priority. - // file #17 byte [1001, 1001) piece [10, 11) - file_priorities.set(17U, pri); - expected_file_priorities[17] = pri; + // file #16 byte [1001, 1001) piece [10, 11) + file_priorities.set(16U, pri); + expected_file_priorities[16] = pri; expected_piece_priorities[10] = pri; mark_file_endpoints_as_high_priority(); compare_to_expected(); @@ -350,54 +321,55 @@ TEST_F(FilePieceMapTest, wanted) expected_pieces_wanted.set_has_all(); compare_to_expected(); - // set file #2 as not wanted. - // since this file begins and ends on a piece boundary, - // and it is not preceded by a zero sized file, - // this shouldn't normally affect any other files' pieces + // set the first file as not wanted. + // since this begins and ends on a piece boundary, + // this shouldn't affect any other files' pieces bool const wanted = false; - files_wanted.set(2U, wanted); - expected_files_wanted.set(2U, wanted); - expected_pieces_wanted.set_span(1U, 5U, wanted); + files_wanted.set(0U, wanted); + expected_files_wanted.set(0U, wanted); + expected_pieces_wanted.set_span(0U, 5U, wanted); compare_to_expected(); // now test when a piece has >1 file. // if *any* file in that piece is wanted, then we want the piece too. + // file #1: byte [100..100) piece [5, 6) (zero-byte file) + // file #2: byte [100..100) piece [5, 6) (zero-byte file) // file #3: byte [100..100) piece [5, 6) (zero-byte file) // file #4: byte [100..100) piece [5, 6) (zero-byte file) - // file #5: byte [100..100) piece [5, 6) (zero-byte file) - // file #6: byte [500..550) piece [5, 6) - // file #7: byte [550..650) piece [5, 7) + // file #5: byte [500..550) piece [5, 6) + // file #6: byte [550..650) piece [5, 7) // - // first test setting file #6... - files_wanted.set(6U, false); - expected_files_wanted.unset(6U); + // first test setting file #5... + files_wanted.set(5U, false); + expected_files_wanted.unset(5U); compare_to_expected(); // marking all the files in the piece as unwanted // should cause the piece to become unwanted + files_wanted.set(1U, false); + files_wanted.set(2U, false); files_wanted.set(3U, false); files_wanted.set(4U, false); files_wanted.set(5U, false); files_wanted.set(6U, false); - files_wanted.set(7U, false); - expected_files_wanted.set_span(3U, 8U, false); + expected_files_wanted.set_span(1U, 7U, false); expected_pieces_wanted.unset(5U); compare_to_expected(); // but as soon as any of them is turned back to wanted, // the piece should pop back. - files_wanted.set(7U, true); - expected_files_wanted.set(7U, true); - expected_pieces_wanted.set(5U); - compare_to_expected(); files_wanted.set(6U, true); - files_wanted.set(7U, false); - expected_files_wanted.set(6U); - expected_files_wanted.unset(7U); + expected_files_wanted.set(6U, true); + expected_pieces_wanted.set(5U); compare_to_expected(); files_wanted.set(5U, true); files_wanted.set(6U, false); expected_files_wanted.set(5U); expected_files_wanted.unset(6U); compare_to_expected(); + files_wanted.set(4U, true); + files_wanted.set(5U, false); + expected_files_wanted.set(4U); + expected_files_wanted.unset(5U); + compare_to_expected(); // Prep for the next test: set all files to unwanted for (tr_file_index_t i = 0U; i < n_files; ++i) @@ -413,16 +385,16 @@ TEST_F(FilePieceMapTest, wanted) // since you can't download a zero-byte file. But that would complicate // the code for a stupid use case, so let's KISS. // - // Check that even zero-sized files can change a piece's 'wanted' state - // file #3: byte [500, 500) piece [5, 6) - files_wanted.set(3U, true); - expected_files_wanted.set(3U); + // Check that even zero-sized files can change a file's 'wanted' state + // file #1: byte [500, 500) piece [5, 6) + files_wanted.set(1U, true); + expected_files_wanted.set(1U); expected_pieces_wanted.set(5U); compare_to_expected(); // Check that zero-sized files at the end of a torrent change the last piece's state. - // file #17 byte [1001, 1001) piece [10, 11) - files_wanted.set(17U, true); - expected_files_wanted.set(17U); + // file #16 byte [1001, 1001) piece [10, 11) + files_wanted.set(16U, true); + expected_files_wanted.set(16U); expected_pieces_wanted.set(10U); compare_to_expected(); diff --git a/tests/libtransmission/makemeta-test.cc b/tests/libtransmission/makemeta-test.cc index e8aff8b92..e3615bae9 100644 --- a/tests/libtransmission/makemeta-test.cc +++ b/tests/libtransmission/makemeta-test.cc @@ -47,12 +47,7 @@ protected: for (size_t i = 0; i < n_files; ++i) { auto payload = std::vector{}; - // TODO(5.0.0): zero-sized files are disabled in these test - // because tr_torrent_metainfo discards them, throwing off the - // builder-to-metainfo comparisons here. tr_torrent_metainfo - // will behave when BEP52 support is added in Transmission 5. - static auto constexpr MinFileSize = size_t{ 1U }; - payload.resize(std::max(MinFileSize, static_cast(tr_rand_int(max_size)))); + payload.resize(static_cast(tr_rand_int(max_size))); tr_rand_buffer(std::data(payload), std::size(payload)); auto filename = tr_pathbuf{ top, '/', "test.XXXXXX" }; diff --git a/tests/libtransmission/open-files-test.cc b/tests/libtransmission/open-files-test.cc index 97bd877bf..434e2ddf7 100644 --- a/tests/libtransmission/open-files-test.cc +++ b/tests/libtransmission/open-files-test.cc @@ -28,8 +28,8 @@ using OpenFilesTest = libtransmission::test::SessionTest; TEST_F(OpenFilesTest, getCachedFailsIfNotCached) { - auto const fd_top = session_->openFiles().get(0, 0, false); - EXPECT_FALSE(fd_top); + auto const fd = session_->openFiles().get(0, 0, false); + EXPECT_FALSE(fd); } TEST_F(OpenFilesTest, getOpensIfNotCached) @@ -42,16 +42,15 @@ TEST_F(OpenFilesTest, getOpensIfNotCached) EXPECT_FALSE(session_->openFiles().get(0, 0, false)); // confirm that we can cache the file - auto fd_top = session_->openFiles().get(0, 0, false, filename, TR_PREALLOCATE_FULL, std::size(Contents)); - EXPECT_TRUE(fd_top.has_value()); - assert(fd_top.has_value()); - auto& [fd, tag] = *fd_top; - EXPECT_NE(TR_BAD_SYS_FILE, fd); + auto fd = session_->openFiles().get(0, 0, false, filename, TR_PREALLOCATE_FULL, std::size(Contents)); + EXPECT_TRUE(fd.has_value()); + assert(fd.has_value()); + EXPECT_NE(TR_BAD_SYS_FILE, *fd); // test the file contents to confirm that fd points to the right file auto buf = std::array{}; auto bytes_read = uint64_t{}; - EXPECT_TRUE(tr_sys_file_read_at(fd, std::data(buf), std::size(Contents), 0, &bytes_read)); + EXPECT_TRUE(tr_sys_file_read_at(*fd, std::data(buf), std::size(Contents), 0, &bytes_read)); auto const contents = std::string_view{ std::data(buf), static_cast(bytes_read) }; EXPECT_EQ(Contents, contents); } @@ -74,13 +73,13 @@ TEST_F(OpenFilesTest, getCachedReturnsTheSameFd) createFileWithContents(filename, Contents); EXPECT_FALSE(session_->openFiles().get(0, 0, false)); - auto const fd1_top = session_->openFiles().get(0, 0, false, filename, TR_PREALLOCATE_FULL, std::size(Contents)); - auto const fd2_top = session_->openFiles().get(0, 0, false); - EXPECT_TRUE(fd1_top.has_value()); - EXPECT_TRUE(fd2_top.has_value()); - assert(fd1_top.has_value()); - assert(fd2_top.has_value()); - EXPECT_EQ(fd1_top->first, fd2_top->first); + auto const fd1 = session_->openFiles().get(0, 0, false, filename, TR_PREALLOCATE_FULL, std::size(Contents)); + auto const fd2 = session_->openFiles().get(0, 0, false); + EXPECT_TRUE(fd1.has_value()); + EXPECT_TRUE(fd2.has_value()); + assert(fd1.has_value()); + assert(fd2.has_value()); + EXPECT_EQ(*fd1, *fd2); } TEST_F(OpenFilesTest, getCachedFailsIfWrongPermissions) @@ -105,13 +104,13 @@ TEST_F(OpenFilesTest, opensInReadOnlyUnlessWritableIsRequested) createFileWithContents(filename, Contents); // cache a file read-only mode - auto fd_top = session_->openFiles().get(0, 0, false, filename, TR_PREALLOCATE_FULL, std::size(Contents)); - EXPECT_TRUE(fd_top.has_value()); - assert(fd_top.has_value()); + auto fd = session_->openFiles().get(0, 0, false, filename, TR_PREALLOCATE_FULL, std::size(Contents)); + EXPECT_TRUE(fd.has_value()); + assert(fd.has_value()); // confirm that writing to it fails auto error = tr_error{}; - EXPECT_FALSE(tr_sys_file_write(fd_top->first, std::data(Contents), std::size(Contents), nullptr, &error)); + EXPECT_FALSE(tr_sys_file_write(*fd, std::data(Contents), std::size(Contents), nullptr, &error)); EXPECT_TRUE(error); } @@ -121,14 +120,14 @@ TEST_F(OpenFilesTest, createsMissingFileIfWriteRequested) auto filename = tr_pathbuf{ sandboxDir(), "/test-file.txt" }; EXPECT_FALSE(tr_sys_path_exists(filename)); - auto fd_top = session_->openFiles().get(0, 0, false); - EXPECT_FALSE(fd_top); + auto fd = session_->openFiles().get(0, 0, false); + EXPECT_FALSE(fd); EXPECT_FALSE(tr_sys_path_exists(filename)); - fd_top = session_->openFiles().get(0, 0, true, filename, TR_PREALLOCATE_FULL, std::size(Contents)); - EXPECT_TRUE(fd_top.has_value()); - assert(fd_top.has_value()); - EXPECT_NE(TR_BAD_SYS_FILE, fd_top->first); + fd = session_->openFiles().get(0, 0, true, filename, TR_PREALLOCATE_FULL, std::size(Contents)); + EXPECT_TRUE(fd.has_value()); + assert(fd.has_value()); + EXPECT_NE(TR_BAD_SYS_FILE, *fd); EXPECT_TRUE(tr_sys_path_exists(filename)); }