From fa389d902fbf5fac19fba8c5d0c5e0a25f15ca63 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 4 Jul 2023 18:59:49 +0200 Subject: [PATCH 1/3] refactor: Drop unused fclose() from BufferedFile This was only explicitly used in the tests, where it can be replaced by wrapping the original raw file pointer into a CAutoFile on creation and then calling CAutoFile::fclose(). Also, it was used in LoadExternalBlockFile(), where it can also be replaced by the (implicit call to the) CAutoFile destructor after wrapping the original raw file pointer in a CAutoFile. --- src/bench/load_external.cpp | 5 ++-- src/bench/streams_findbyte.cpp | 16 +++++++----- src/node/blockstorage.cpp | 12 ++++----- src/streams.h | 17 ------------ src/test/fuzz/buffered_file.cpp | 9 +++---- src/test/fuzz/load_external_block_file.cpp | 9 ++++--- src/test/streams_tests.cpp | 30 +++++++++++----------- src/validation.cpp | 1 - 8 files changed, 42 insertions(+), 57 deletions(-) diff --git a/src/bench/load_external.cpp b/src/bench/load_external.cpp index 252cbb163b6..7cfade9c794 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,8 +55,8 @@ 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")}; - testing_setup->m_node.chainman->LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); + CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION}; + testing_setup->m_node.chainman->LoadExternalBlockFile(file.Get(), &pos, &blocks_with_unknown_parent); }); fs::remove(blkfile); } diff --git a/src/bench/streams_findbyte.cpp b/src/bench/streams_findbyte.cpp index 22b8f1b356b..b97f130c673 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.Get(), /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0}; 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/node/blockstorage.cpp b/src/node/blockstorage.cpp index f21c94c0a04..d0213ea632b 100644 --- a/src/node/blockstorage.cpp +++ b/src/node/blockstorage.cpp @@ -1015,12 +1015,12 @@ 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), CLIENT_VERSION}; + if (file.IsNull()) { break; // This error is logged in OpenBlockFile } LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile); - chainman.LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent); + chainman.LoadExternalBlockFile(file.Get(), &pos, &blocks_with_unknown_parent); if (chainman.m_interrupt) { LogPrintf("Interrupt requested. Exit %s\n", __func__); return; @@ -1036,10 +1036,10 @@ 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); + chainman.LoadExternalBlockFile(file.Get()); if (chainman.m_interrupt) { LogPrintf("Interrupt requested. Exit %s\n", __func__); return; diff --git a/src/streams.h b/src/streams.h index f9a817c9b64..0e9805fa9c0 100644 --- a/src/streams.h +++ b/src/streams.h @@ -637,25 +637,8 @@ class BufferedFile 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; - } - } - //! check whether we're at the end of the source file bool eof() const { return m_read_pos == nSrcPos && feof(src); diff --git a/src/test/fuzz/buffered_file.cpp b/src/test/fuzz/buffered_file.cpp index 1116274e3d2..b6320ceab3a 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.Get(), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange(0, 4096), fuzzed_data_provider.ConsumeIntegral()); } 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..3ca2aa711bf 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,17 +28,17 @@ 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()) { // 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, &flat_file_pos, &blocks_with_unknown_parent); + g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file.Get(), &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); + g_setup->m_node.chainman->LoadExternalBlockFile(fuzzed_block_file.Get()); } } diff --git a/src/test/streams_tests.cpp b/src/test/streams_tests.cpp index 99740ee7792..833d706fefe 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.Get(), 25, 25, 333}; 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.Get(), 25, 10, 333}; 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.Get(), 25, 10, 333}; 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.Get(), bufSize, rewindSize, 333}; 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 1d4786bb172..090fa34a933 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4589,7 +4589,6 @@ 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}; // nRewind indicates where to resume scanning in case something goes wrong, // such as a block fails to deserialize. From 9999b89cd37fb2a23c5ebcd91d9cb31d69375933 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 4 Jul 2023 17:09:13 +0200 Subject: [PATCH 2/3] 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 7cfade9c794..3b100d97b0b 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 b97f130c673..4aaccb2af84 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 d0213ea632b..0003a082579 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 0e9805fa9c0..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,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 b6320ceab3a..636f11b381b 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 3ca2aa711bf..fc903e5ec25 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 833d706fefe..8ff65b5377a 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 090fa34a933..1c9806ded75 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 f1ff6bb6719..e453d5aaae6 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); From fa56c421be04af846f479c30749b17e6663ab418 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Fri, 30 Jun 2023 16:25:13 +0200 Subject: [PATCH 3/3] Return CAutoFile from BlockManager::Open*File() This is a refactor. --- src/index/txindex.cpp | 2 +- src/node/blockstorage.cpp | 22 +++++++++++----------- src/node/blockstorage.h | 5 +++-- src/test/blockmanager_tests.cpp | 6 +++--- 4 files changed, 18 insertions(+), 17 deletions(-) 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 0003a082579..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,7 +1015,7 @@ void ImportBlocks(ChainstateManager& chainman, std::vector vImportFile if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) { break; // No block files left to reindex } - CAutoFile file{chainman.m_blockman.OpenBlockFile(pos, true), CLIENT_VERSION}; + CAutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)}; if (file.IsNull()) { break; // This error is logged in OpenBlockFile } 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/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)