From 9999b89cd37fb2a23c5ebcd91d9cb31d69375933 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 4 Jul 2023 17:09:13 +0200 Subject: [PATCH] Make BufferedFile to be a CAutoFile wrapper This refactor allows to forward some calls to the underlying CAutoFile, instead of re-implementing the logic in the buffered file. --- src/bench/load_external.cpp | 2 +- src/bench/streams_findbyte.cpp | 2 +- src/node/blockstorage.cpp | 4 ++-- src/streams.h | 19 ++++++++----------- src/test/fuzz/buffered_file.cpp | 2 +- src/test/fuzz/load_external_block_file.cpp | 4 ++-- src/test/streams_tests.cpp | 8 ++++---- src/validation.cpp | 4 ++-- src/validation.h | 5 ++--- 9 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp index 7cfade9c7944b..3b100d97b0b5d 100644 --- a/src/bench/load_external.cpp +++ b/src/bench/load_external.cpp @@ -56,7 +56,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench) // "rb" is "binary, O_RDONLY", positioned to the start of the file. // The file will be closed by LoadExternalBlockFile(). CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION}; - testing_setup->m_node.chainman->LoadExternalBlockFile(file.Get(), &pos, &blocks_with_unknown_parent); + testing_setup->m_node.chainman->LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); }); fs::remove(blkfile); } diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp index b97f130c673a3..4aaccb2af8422 100644 --- a/src/bench/streams_findbyte.cpp +++ b/src/bench/streams_findbyte.cpp @@ -20,7 +20,7 @@ static void FindByte(benchmark::Bench& bench) data[file_size-1] = 1; file << data; std::rewind(file.Get()); - BufferedFile bf{file.Get(), /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0}; + BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size}; bench.run([&] { bf.SetPos(0); diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index d0213ea632b17..0003a08257925 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -1020,7 +1020,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile break; // This error is logged in OpenBlockFile } LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile); - chainman.LoadExternalBlockFile(file.Get(), &pos, &blocks_with_unknown_parent); + chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); if (chainman.m_interrupt) { LogPrintf("Interrupt requested. Exit %s\n", __func__); return; @@ -1039,7 +1039,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile CAutoFile file{fsbridge::fopen(path, "rb"), CLIENT_VERSION}; if (!file.IsNull()) { LogPrintf("Importing blocks file %s...\n", fs::PathToString(path)); - chainman.LoadExternalBlockFile(file.Get()); + chainman.LoadExternalBlockFile(file); if (chainman.m_interrupt) { LogPrintf("Interrupt requested. Exit %s\n", __func__); return; diff --git a/src/streams.h b/src/streams.h index 0e9805fa9c041..e069719d89e7d 100644 --- a/src/streams.h +++ b/src/streams.h @@ -571,7 +571,7 @@ class CAutoFile : public AutoFile } }; -/** Non-refcounted RAII wrapper around a FILE* that implements a ring buffer to +/** Wrapper around a CAutoFile& that implements a ring buffer to * deserialize from. It guarantees the ability to rewind a given number of bytes. * * Will automatically close the file when it goes out of scope if not null. @@ -580,9 +580,7 @@ class CAutoFile : public AutoFile class BufferedFile { private: - const int nVersion; - - FILE *src; //!< source file + CAutoFile& m_src; uint64_t nSrcPos{0}; //!< how many bytes have been read from source uint64_t m_read_pos{0}; //!< how many bytes have been read from this uint64_t nReadLimit; //!< up to which position we're allowed to read @@ -598,9 +596,9 @@ class BufferedFile readNow = nAvail; if (readNow == 0) return false; - size_t nBytes = fread((void*)&vchBuf[pos], 1, readNow, src); + size_t nBytes{m_src.detail_fread(Span{vchBuf}.subspan(pos, readNow))}; if (nBytes == 0) { - throw std::ios_base::failure(feof(src) ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed"); + throw std::ios_base::failure{m_src.feof() ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed"}; } nSrcPos += nBytes; return true; @@ -629,19 +627,18 @@ class BufferedFile } public: - BufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nVersionIn) - : nVersion{nVersionIn}, nReadLimit{std::numeric_limits::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0}) + BufferedFile(CAutoFile& file, uint64_t nBufSize, uint64_t nRewindIn) + : m_src{file}, nReadLimit{std::numeric_limits::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0}) { if (nRewindIn >= nBufSize) throw std::ios_base::failure("Rewind limit must be less than buffer size"); - src = fileIn; } - int GetVersion() const { return nVersion; } + int GetVersion() const { return m_src.GetVersion(); } //! check whether we're at the end of the source file bool eof() const { - return m_read_pos == nSrcPos && feof(src); + return m_read_pos == nSrcPos && m_src.feof(); } //! read a number of bytes diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp index b6320ceab3a0f..636f11b381b2d 100644 --- a/src/test/fuzz/buffered_file.cpp +++ b/src/test/fuzz/buffered_file.cpp @@ -21,7 +21,7 @@ FUZZ_TARGET(buffered_file) std::optional opt_buffered_file; CAutoFile fuzzed_file{fuzzed_file_provider.open(), 0}; try { - opt_buffered_file.emplace(fuzzed_file.Get(), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegral()); + opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096)); } catch (const std::ios_base::failure&) { } if (opt_buffered_file && !fuzzed_file.IsNull()) { diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp index 3ca2aa711bf14..fc903e5ec25d8 100644 --- a/src/test/fuzz/load_external_block_file.cpp +++ b/src/test/fuzz/load_external_block_file.cpp @@ -36,9 +36,9 @@ FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_fil // Corresponds to the -reindex case (track orphan blocks across files). FlatFilePos flat_file_pos; std::multimap blocks_with_unknown_parent; - g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file.Get(), &flat_file_pos, &blocks_with_unknown_parent); + g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file, &flat_file_pos, &blocks_with_unknown_parent); } else { // Corresponds to the -loadblock= case (orphan blocks aren't tracked across files). - g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file.Get()); + g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file); } } diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 833d706fefe55..8ff65b5377a35 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -260,7 +260,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) // The buffer size (second arg) must be greater than the rewind // amount (third arg). try { - BufferedFile bfbad{file.Get(), 25, 25, 333}; + BufferedFile bfbad{file, 25, 25}; BOOST_CHECK(false); } catch (const std::exception& e) { BOOST_CHECK(strstr(e.what(), @@ -268,7 +268,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) } // The buffer is 25 bytes, allow rewinding 10 bytes. - BufferedFile bf{file.Get(), 25, 10, 333}; + BufferedFile bf{file, 25, 10}; BOOST_CHECK(!bf.eof()); // This member has no functional effect. @@ -391,7 +391,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) std::rewind(file.Get()); // The buffer is 25 bytes, allow rewinding 10 bytes. - BufferedFile bf{file.Get(), 25, 10, 333}; + BufferedFile bf{file, 25, 10}; uint8_t i; // This is like bf >> (7-byte-variable), in that it will cause data @@ -445,7 +445,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) size_t bufSize = InsecureRandRange(300) + 1; size_t rewindSize = InsecureRandRange(bufSize); - BufferedFile bf{file.Get(), bufSize, rewindSize, 333}; + BufferedFile bf{file, bufSize, rewindSize}; size_t currentPos = 0; size_t maxPos = 0; for (int step = 0; step < 100; ++step) { diff --git a/src/validation.cpp b/src/validation.cpp index 090fa34a9335e..1c9806ded7584 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4577,7 +4577,7 @@ bool Chainstate::LoadGenesisBlock() } void ChainstateManager::LoadExternalBlockFile( - FILE* fileIn, + CAutoFile& file_in, FlatFilePos* dbp, std::multimap* blocks_with_unknown_parent) { @@ -4589,7 +4589,7 @@ void ChainstateManager::LoadExternalBlockFile( int nLoaded = 0; try { - BufferedFile blkdat{fileIn, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8, CLIENT_VERSION}; + BufferedFile blkdat{file_in, 2 * MAX_BLOCK_SERIALIZED_SIZE, MAX_BLOCK_SERIALIZED_SIZE + 8}; // nRewind indicates where to resume scanning in case something goes wrong, // such as a block fails to deserialize. uint64_t nRewind = blkdat.GetPos(); diff --git a/src/validation.h b/src/validation.h index f1ff6bb671932..e453d5aaae658 100644 --- a/src/validation.h +++ b/src/validation.h @@ -607,7 +607,6 @@ class Chainstate bool ResizeCoinsCaches(size_t coinstip_size, size_t coinsdb_size) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - /** * Update the on-disk chain state. * The caches and indexes are flushed depending on the mode we're called with @@ -1087,14 +1086,14 @@ class ChainstateManager * -loadblock= option. There's no unknown-parent tracking, so the last two arguments are omitted. * * - * @param[in] fileIn FILE handle to file containing blocks to read + * @param[in] file_in File containing blocks to read * @param[in] dbp (optional) Disk block position (only for reindex) * @param[in,out] blocks_with_unknown_parent (optional) Map of disk positions for blocks with * unknown parent, key is parent block hash * (only used for reindex) * */ void LoadExternalBlockFile( - FILE* fileIn, + CAutoFile& file_in, FlatFilePos* dbp = nullptr, std::multimap* blocks_with_unknown_parent = nullptr);