diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp index 252cbb163b6..3b100d97b0b 100644 --- a/src/bench/load_external.cpp +++ b/src/bench/load_external.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -54,7 +55,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench) bench.run([&] { // "rb" is "binary, O_RDONLY", positioned to the start of the file. // The file will be closed by LoadExternalBlockFile(). - FILE* file{fsbridge::fopen(blkfile, "rb")}; + CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION}; 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 22b8f1b356b..4aaccb2af84 100644 --- a/src/bench/streams_findbyte.cpp +++ b/src/bench/streams_findbyte.cpp @@ -4,19 +4,23 @@ #include -#include #include +#include + +#include +#include +#include static void FindByte(benchmark::Bench& bench) { // Setup - FILE* file = fsbridge::fopen("streams_tmp", "w+b"); + CAutoFile file{fsbridge::fopen("streams_tmp", "w+b"), 0}; const size_t file_size = 200; uint8_t data[file_size] = {0}; data[file_size-1] = 1; - fwrite(&data, sizeof(uint8_t), file_size, file); - rewind(file); - BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0}; + file << data; + std::rewind(file.Get()); + BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size}; bench.run([&] { bf.SetPos(0); @@ -24,7 +28,7 @@ static void FindByte(benchmark::Bench& bench) }); // Cleanup - bf.fclose(); + file.fclose(); fs::remove("streams_tmp"); } diff --git a/src/index/txindex.cpp b/src/index/txindex.cpp index 6b38e19d813..e16dd0f8bd1 100644 --- a/src/index/txindex.cpp +++ b/src/index/txindex.cpp @@ -79,7 +79,7 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe return false; } - CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true), CLIENT_VERSION}; + CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true)}; if (file.IsNull()) { return error("%s: OpenBlockFile failed", __func__); } diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp index f21c94c0a04..ae63d12ef70 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -459,7 +459,7 @@ bool BlockManager::LoadBlockIndexDB() } for (std::set::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) { FlatFilePos pos(*it, 0); - if (AutoFile{OpenBlockFile(pos, true)}.IsNull()) { + if (OpenBlockFile(pos, true).IsNull()) { return false; } } @@ -592,7 +592,7 @@ CBlockFileInfo* BlockManager::GetBlockFileInfo(size_t n) bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const { // Open history file to append - AutoFile fileout{OpenUndoFile(pos)}; + CAutoFile fileout{OpenUndoFile(pos)}; if (fileout.IsNull()) { return error("%s: OpenUndoFile failed", __func__); } @@ -627,7 +627,7 @@ bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& in } // Open history file to read - AutoFile filein{OpenUndoFile(pos, true)}; + CAutoFile filein{OpenUndoFile(pos, true)}; if (filein.IsNull()) { return error("%s: OpenUndoFile failed", __func__); } @@ -715,15 +715,15 @@ FlatFileSeq BlockManager::UndoFileSeq() const return FlatFileSeq(m_opts.blocks_dir, "rev", UNDOFILE_CHUNK_SIZE); } -FILE* BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const +CAutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const { - return BlockFileSeq().Open(pos, fReadOnly); + return CAutoFile{BlockFileSeq().Open(pos, fReadOnly), CLIENT_VERSION}; } /** Open an undo file (rev?????.dat) */ -FILE* BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const +CAutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const { - return UndoFileSeq().Open(pos, fReadOnly); + return CAutoFile{UndoFileSeq().Open(pos, fReadOnly), CLIENT_VERSION}; } fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const @@ -824,7 +824,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP bool BlockManager::WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const { // Open history file to append - CAutoFile fileout{OpenBlockFile(pos), CLIENT_VERSION}; + CAutoFile fileout{OpenBlockFile(pos)}; if (fileout.IsNull()) { return error("WriteBlockToDisk: OpenBlockFile failed"); } @@ -880,7 +880,7 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) cons block.SetNull(); // Open history file to read - CAutoFile filein{OpenBlockFile(pos, true), CLIENT_VERSION}; + CAutoFile filein{OpenBlockFile(pos, true)}; if (filein.IsNull()) { return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString()); } @@ -923,7 +923,7 @@ bool BlockManager::ReadRawBlockFromDisk(std::vector& block, const FlatF { FlatFilePos hpos = pos; hpos.nPos -= 8; // Seek back 8 bytes for meta header - AutoFile filein{OpenBlockFile(hpos, true)}; + CAutoFile filein{OpenBlockFile(hpos, true)}; if (filein.IsNull()) { return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString()); } @@ -1015,8 +1015,8 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) { break; // No block files left to reindex } - FILE* file = chainman.m_blockman.OpenBlockFile(pos, true); - if (!file) { + CAutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)}; + if (file.IsNull()) { break; // This error is logged in OpenBlockFile } LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile); @@ -1036,8 +1036,8 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile // -loadblock= for (const fs::path& path : vImportFiles) { - FILE* file = fsbridge::fopen(path, "rb"); - if (file) { + CAutoFile file{fsbridge::fopen(path, "rb"), CLIENT_VERSION}; + if (!file.IsNull()) { LogPrintf("Importing blocks file %s...\n", fs::PathToString(path)); chainman.LoadExternalBlockFile(file); if (chainman.m_interrupt) { diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h index b251ece31f2..a89fa7f76e1 100644 --- a/src/node/blockstorage.h +++ b/src/node/blockstorage.h @@ -29,6 +29,7 @@ #include class BlockValidationState; +class CAutoFile; class CBlock; class CBlockFileInfo; class CBlockUndo; @@ -126,7 +127,7 @@ class BlockManager FlatFileSeq BlockFileSeq() const; FlatFileSeq UndoFileSeq() const; - FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const; + CAutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const; bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const; bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const; @@ -278,7 +279,7 @@ class BlockManager void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); /** Open a block file (blk?????.dat) */ - FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const; + CAutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const; /** Translation to a filesystem path */ fs::path GetBlockPosFilename(const FlatFilePos& pos) const; diff --git a/src/streams.h b/src/streams.h index f9a817c9b64..e069719d89e 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,36 +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; - } - - ~BufferedFile() - { - fclose(); } - // Disallow copies - BufferedFile(const BufferedFile&) = delete; - BufferedFile& operator=(const BufferedFile&) = delete; - - int GetVersion() const { return nVersion; } - - void fclose() - { - if (src) { - ::fclose(src); - src = nullptr; - } - } + 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/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp index 636f5b9388f..c6800c498bf 100644 --- a/src/test/blockmanager_tests.cpp +++ b/src/test/blockmanager_tests.cpp @@ -74,13 +74,13 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain // Check that the file is not unlinked after ScanAndUnlinkAlreadyPrunedFiles // if m_have_pruned is not yet set WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles()); - BOOST_CHECK(!AutoFile(blockman.OpenBlockFile(pos, true)).IsNull()); + BOOST_CHECK(!blockman.OpenBlockFile(pos, true).IsNull()); // Check that the file is unlinked after ScanAndUnlinkAlreadyPrunedFiles // once m_have_pruned is set blockman.m_have_pruned = true; WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles()); - BOOST_CHECK(AutoFile(blockman.OpenBlockFile(pos, true)).IsNull()); + BOOST_CHECK(blockman.OpenBlockFile(pos, true).IsNull()); // Check that calling with already pruned files doesn't cause an error WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles()); @@ -90,7 +90,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain BOOST_CHECK_NE(old_tip, new_tip); const int new_file_number{WITH_LOCK(chainman->GetMutex(), return new_tip->GetBlockPos().nFile)}; const FlatFilePos new_pos(new_file_number, 0); - BOOST_CHECK(!AutoFile(blockman.OpenBlockFile(new_pos, true)).IsNull()); + BOOST_CHECK(!blockman.OpenBlockFile(new_pos, true).IsNull()); } BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup) diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp index 1116274e3d2..636f11b381b 100644 --- a/src/test/fuzz/buffered_file.cpp +++ b/src/test/fuzz/buffered_file.cpp @@ -19,15 +19,12 @@ FUZZ_TARGET(buffered_file) FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider); std::optional opt_buffered_file; - FILE* fuzzed_file = fuzzed_file_provider.open(); + CAutoFile fuzzed_file{fuzzed_file_provider.open(), 0}; try { - opt_buffered_file.emplace(fuzzed_file, 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 (fuzzed_file != nullptr) { - fclose(fuzzed_file); - } } - if (opt_buffered_file && fuzzed_file != nullptr) { + if (opt_buffered_file && !fuzzed_file.IsNull()) { bool setpos_fail = false; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { CallOneOf( diff --git a/src/test/fuzz/load_external_block_file.cpp b/src/test/fuzz/load_external_block_file.cpp index bdaa4ad1b8f..fc903e5ec25 100644 --- a/src/test/fuzz/load_external_block_file.cpp +++ b/src/test/fuzz/load_external_block_file.cpp @@ -3,6 +3,7 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include #include #include @@ -27,8 +28,8 @@ FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_fil { FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider); - FILE* fuzzed_block_file = fuzzed_file_provider.open(); - if (fuzzed_block_file == nullptr) { + CAutoFile fuzzed_block_file{fuzzed_file_provider.open(), CLIENT_VERSION}; + if (fuzzed_block_file.IsNull()) { return; } if (fuzzed_data_provider.ConsumeBool()) { diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 99740ee7792..8ff65b5377a 100644 --- a/src/test/streams_tests.cpp +++ b/src/test/streams_tests.cpp @@ -249,18 +249,18 @@ BOOST_AUTO_TEST_CASE(streams_serializedata_xor) BOOST_AUTO_TEST_CASE(streams_buffered_file) { fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp"; - FILE* file = fsbridge::fopen(streams_test_filename, "w+b"); + CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333}; // The value at each offset is the offset. for (uint8_t j = 0; j < 40; ++j) { - fwrite(&j, 1, 1, file); + file << j; } - rewind(file); + std::rewind(file.Get()); // The buffer size (second arg) must be greater than the rewind // amount (third arg). try { - BufferedFile bfbad{file, 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, 25, 10, 333}; + BufferedFile bf{file, 25, 10}; BOOST_CHECK(!bf.eof()); // This member has no functional effect. @@ -375,7 +375,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) BOOST_CHECK(bf.GetPos() <= 30U); // We can explicitly close the file, or the destructor will do it. - bf.fclose(); + file.fclose(); fs::remove(streams_test_filename); } @@ -383,15 +383,15 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file) BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) { fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp"; - FILE* file = fsbridge::fopen(streams_test_filename, "w+b"); + CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333}; // The value at each offset is the byte offset (e.g. byte 1 in the file has the value 0x01). for (uint8_t j = 0; j < 40; ++j) { - fwrite(&j, 1, 1, file); + file << j; } - rewind(file); + std::rewind(file.Get()); // The buffer is 25 bytes, allow rewinding 10 bytes. - BufferedFile bf{file, 25, 10, 333}; + BufferedFile bf{file, 25, 10}; uint8_t i; // This is like bf >> (7-byte-variable), in that it will cause data @@ -425,7 +425,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip) bf.SkipTo(13); BOOST_CHECK_EQUAL(bf.GetPos(), 13U); - bf.fclose(); + file.fclose(); fs::remove(streams_test_filename); } @@ -436,16 +436,16 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand) fs::path streams_test_filename = m_args.GetDataDirBase() / "streams_test_tmp"; for (int rep = 0; rep < 50; ++rep) { - FILE* file = fsbridge::fopen(streams_test_filename, "w+b"); + CAutoFile file{fsbridge::fopen(streams_test_filename, "w+b"), 333}; size_t fileSize = InsecureRandRange(256); for (uint8_t i = 0; i < fileSize; ++i) { - fwrite(&i, 1, 1, file); + file << i; } - rewind(file); + std::rewind(file.Get()); size_t bufSize = InsecureRandRange(300) + 1; size_t rewindSize = InsecureRandRange(bufSize); - BufferedFile bf{file, 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 0ce29021959..4acd5c7cb0e 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,8 +4589,7 @@ void ChainstateManager::LoadExternalBlockFile( int nLoaded = 0; try { - // This takes over fileIn and calls fclose() on it in the BufferedFile destructor - 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 6640bb5b50a..3f0a2312b52 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);