Skip to content

Commit c9f2882

Browse files
fanquakefanquake
committed
Merge bitcoin#28483: refactor: Return CAutoFile from BlockManager::Open*File()
fa56c42 Return CAutoFile from BlockManager::Open*File() (MarcoFalke) 9999b89 Make BufferedFile to be a CAutoFile wrapper (MarcoFalke) fa389d9 refactor: Drop unused fclose() from BufferedFile (MarcoFalke) Pull request description: This is required for bitcoin#28052, but makes sense on its own, because offloading logic to `CAutoFile` instead of re-implementing it allows to delete code and complexity. ACKs for top commit: TheCharlatan: Re-ACK fa56c42 willcl-ark: tACK fa56c42 Tree-SHA512: fe4638f3a6bd3f9d968cfb9ae3259c9d6cd278fe2912cbc90289851311c8c781099db4c160e775960975c4739098d9af801a8d2d12603f371f8edfe134d8f85a
2 parents dcfbf3c + fa56c42 commit c9f2882

12 files changed

+66
-84
lines changed

‎src/bench/load_external.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include <bench/bench.h>
66
#include <bench/data.h>
77
#include <chainparams.h>
8+
#include <clientversion.h>
89
#include <test/util/setup_common.h>
910
#include <util/chaintype.h>
1011
#include <validation.h>
@@ -54,7 +55,7 @@ static void LoadExternalBlockFile(benchmark::Bench& bench)
5455
bench.run([&] {
5556
// "rb" is "binary, O_RDONLY", positioned to the start of the file.
5657
// The file will be closed by LoadExternalBlockFile().
57-
FILE* file{fsbridge::fopen(blkfile, "rb")};
58+
CAutoFile file{fsbridge::fopen(blkfile, "rb"), CLIENT_VERSION};
5859
testing_setup->m_node.chainman->LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
5960
});
6061
fs::remove(blkfile);

‎src/bench/streams_findbyte.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,31 @@
44

55
#include <bench/bench.h>
66

7-
#include <util/fs.h>
87
#include <streams.h>
8+
#include <util/fs.h>
9+
10+
#include <cstddef>
11+
#include <cstdint>
12+
#include <cstdio>
913

1014
static void FindByte(benchmark::Bench& bench)
1115
{
1216
// Setup
13-
FILE* file = fsbridge::fopen("streams_tmp", "w+b");
17+
CAutoFile file{fsbridge::fopen("streams_tmp", "w+b"), 0};
1418
const size_t file_size = 200;
1519
uint8_t data[file_size] = {0};
1620
data[file_size-1] = 1;
17-
fwrite(&data, sizeof(uint8_t), file_size, file);
18-
rewind(file);
19-
BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0};
21+
file << data;
22+
std::rewind(file.Get());
23+
BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size};
2024

2125
bench.run([&] {
2226
bf.SetPos(0);
2327
bf.FindByte(std::byte(1));
2428
});
2529

2630
// Cleanup
27-
bf.fclose();
31+
file.fclose();
2832
fs::remove("streams_tmp");
2933
}
3034

‎src/index/txindex.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe
7979
return false;
8080
}
8181

82-
CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true), CLIENT_VERSION};
82+
CAutoFile file{m_chainstate->m_blockman.OpenBlockFile(postx, true)};
8383
if (file.IsNull()) {
8484
return error("%s: OpenBlockFile failed", __func__);
8585
}

‎src/node/blockstorage.cpp

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ bool BlockManager::LoadBlockIndexDB()
459459
}
460460
for (std::set<int>::iterator it = setBlkDataFiles.begin(); it != setBlkDataFiles.end(); it++) {
461461
FlatFilePos pos(*it, 0);
462-
if (AutoFile{OpenBlockFile(pos, true)}.IsNull()) {
462+
if (OpenBlockFile(pos, true).IsNull()) {
463463
return false;
464464
}
465465
}
@@ -592,7 +592,7 @@ CBlockFileInfo* BlockManager::GetBlockFileInfo(size_t n)
592592
bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const
593593
{
594594
// Open history file to append
595-
AutoFile fileout{OpenUndoFile(pos)};
595+
CAutoFile fileout{OpenUndoFile(pos)};
596596
if (fileout.IsNull()) {
597597
return error("%s: OpenUndoFile failed", __func__);
598598
}
@@ -627,7 +627,7 @@ bool BlockManager::UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex& in
627627
}
628628

629629
// Open history file to read
630-
AutoFile filein{OpenUndoFile(pos, true)};
630+
CAutoFile filein{OpenUndoFile(pos, true)};
631631
if (filein.IsNull()) {
632632
return error("%s: OpenUndoFile failed", __func__);
633633
}
@@ -715,15 +715,15 @@ FlatFileSeq BlockManager::UndoFileSeq() const
715715
return FlatFileSeq(m_opts.blocks_dir, "rev", UNDOFILE_CHUNK_SIZE);
716716
}
717717

718-
FILE* BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
718+
CAutoFile BlockManager::OpenBlockFile(const FlatFilePos& pos, bool fReadOnly) const
719719
{
720-
return BlockFileSeq().Open(pos, fReadOnly);
720+
return CAutoFile{BlockFileSeq().Open(pos, fReadOnly), CLIENT_VERSION};
721721
}
722722

723723
/** Open an undo file (rev?????.dat) */
724-
FILE* BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
724+
CAutoFile BlockManager::OpenUndoFile(const FlatFilePos& pos, bool fReadOnly) const
725725
{
726-
return UndoFileSeq().Open(pos, fReadOnly);
726+
return CAutoFile{UndoFileSeq().Open(pos, fReadOnly), CLIENT_VERSION};
727727
}
728728

729729
fs::path BlockManager::GetBlockPosFilename(const FlatFilePos& pos) const
@@ -824,7 +824,7 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
824824
bool BlockManager::WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const
825825
{
826826
// Open history file to append
827-
CAutoFile fileout{OpenBlockFile(pos), CLIENT_VERSION};
827+
CAutoFile fileout{OpenBlockFile(pos)};
828828
if (fileout.IsNull()) {
829829
return error("WriteBlockToDisk: OpenBlockFile failed");
830830
}
@@ -880,7 +880,7 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos) cons
880880
block.SetNull();
881881

882882
// Open history file to read
883-
CAutoFile filein{OpenBlockFile(pos, true), CLIENT_VERSION};
883+
CAutoFile filein{OpenBlockFile(pos, true)};
884884
if (filein.IsNull()) {
885885
return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString());
886886
}
@@ -923,7 +923,7 @@ bool BlockManager::ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatF
923923
{
924924
FlatFilePos hpos = pos;
925925
hpos.nPos -= 8; // Seek back 8 bytes for meta header
926-
AutoFile filein{OpenBlockFile(hpos, true)};
926+
CAutoFile filein{OpenBlockFile(hpos, true)};
927927
if (filein.IsNull()) {
928928
return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
929929
}
@@ -1015,8 +1015,8 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
10151015
if (!fs::exists(chainman.m_blockman.GetBlockPosFilename(pos))) {
10161016
break; // No block files left to reindex
10171017
}
1018-
FILE* file = chainman.m_blockman.OpenBlockFile(pos, true);
1019-
if (!file) {
1018+
CAutoFile file{chainman.m_blockman.OpenBlockFile(pos, true)};
1019+
if (file.IsNull()) {
10201020
break; // This error is logged in OpenBlockFile
10211021
}
10221022
LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
@@ -1036,8 +1036,8 @@ void ImportBlocks(ChainstateManager& chainman, std::vector<fs::path> vImportFile
10361036

10371037
// -loadblock=
10381038
for (const fs::path& path : vImportFiles) {
1039-
FILE* file = fsbridge::fopen(path, "rb");
1040-
if (file) {
1039+
CAutoFile file{fsbridge::fopen(path, "rb"), CLIENT_VERSION};
1040+
if (!file.IsNull()) {
10411041
LogPrintf("Importing blocks file %s...\n", fs::PathToString(path));
10421042
chainman.LoadExternalBlockFile(file);
10431043
if (chainman.m_interrupt) {

‎src/node/blockstorage.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include <vector>
3030

3131
class BlockValidationState;
32+
class CAutoFile;
3233
class CBlock;
3334
class CBlockFileInfo;
3435
class CBlockUndo;
@@ -126,7 +127,7 @@ class BlockManager
126127
FlatFileSeq BlockFileSeq() const;
127128
FlatFileSeq UndoFileSeq() const;
128129

129-
FILE* OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const;
130+
CAutoFile OpenUndoFile(const FlatFilePos& pos, bool fReadOnly = false) const;
130131

131132
bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const;
132133
bool UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos, const uint256& hashBlock) const;
@@ -278,7 +279,7 @@ class BlockManager
278279
void UpdatePruneLock(const std::string& name, const PruneLockInfo& lock_info) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
279280

280281
/** Open a block file (blk?????.dat) */
281-
FILE* OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const;
282+
CAutoFile OpenBlockFile(const FlatFilePos& pos, bool fReadOnly = false) const;
282283

283284
/** Translation to a filesystem path */
284285
fs::path GetBlockPosFilename(const FlatFilePos& pos) const;

‎src/streams.h

Lines changed: 8 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -571,7 +571,7 @@ class CAutoFile : public AutoFile
571571
}
572572
};
573573

574-
/** Non-refcounted RAII wrapper around a FILE* that implements a ring buffer to
574+
/** Wrapper around a CAutoFile& that implements a ring buffer to
575575
* deserialize from. It guarantees the ability to rewind a given number of bytes.
576576
*
577577
* Will automatically close the file when it goes out of scope if not null.
@@ -580,9 +580,7 @@ class CAutoFile : public AutoFile
580580
class BufferedFile
581581
{
582582
private:
583-
const int nVersion;
584-
585-
FILE *src; //!< source file
583+
CAutoFile& m_src;
586584
uint64_t nSrcPos{0}; //!< how many bytes have been read from source
587585
uint64_t m_read_pos{0}; //!< how many bytes have been read from this
588586
uint64_t nReadLimit; //!< up to which position we're allowed to read
@@ -598,9 +596,9 @@ class BufferedFile
598596
readNow = nAvail;
599597
if (readNow == 0)
600598
return false;
601-
size_t nBytes = fread((void*)&vchBuf[pos], 1, readNow, src);
599+
size_t nBytes{m_src.detail_fread(Span{vchBuf}.subspan(pos, readNow))};
602600
if (nBytes == 0) {
603-
throw std::ios_base::failure(feof(src) ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed");
601+
throw std::ios_base::failure{m_src.feof() ? "BufferedFile::Fill: end of file" : "BufferedFile::Fill: fread failed"};
604602
}
605603
nSrcPos += nBytes;
606604
return true;
@@ -629,36 +627,18 @@ class BufferedFile
629627
}
630628

631629
public:
632-
BufferedFile(FILE* fileIn, uint64_t nBufSize, uint64_t nRewindIn, int nVersionIn)
633-
: nVersion{nVersionIn}, nReadLimit{std::numeric_limits<uint64_t>::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0})
630+
BufferedFile(CAutoFile& file, uint64_t nBufSize, uint64_t nRewindIn)
631+
: m_src{file}, nReadLimit{std::numeric_limits<uint64_t>::max()}, nRewind{nRewindIn}, vchBuf(nBufSize, std::byte{0})
634632
{
635633
if (nRewindIn >= nBufSize)
636634
throw std::ios_base::failure("Rewind limit must be less than buffer size");
637-
src = fileIn;
638-
}
639-
640-
~BufferedFile()
641-
{
642-
fclose();
643635
}
644636

645-
// Disallow copies
646-
BufferedFile(const BufferedFile&) = delete;
647-
BufferedFile& operator=(const BufferedFile&) = delete;
648-
649-
int GetVersion() const { return nVersion; }
650-
651-
void fclose()
652-
{
653-
if (src) {
654-
::fclose(src);
655-
src = nullptr;
656-
}
657-
}
637+
int GetVersion() const { return m_src.GetVersion(); }
658638

659639
//! check whether we're at the end of the source file
660640
bool eof() const {
661-
return m_read_pos == nSrcPos && feof(src);
641+
return m_read_pos == nSrcPos && m_src.feof();
662642
}
663643

664644
//! read a number of bytes

‎src/test/blockmanager_tests.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,13 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
7474
// Check that the file is not unlinked after ScanAndUnlinkAlreadyPrunedFiles
7575
// if m_have_pruned is not yet set
7676
WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
77-
BOOST_CHECK(!AutoFile(blockman.OpenBlockFile(pos, true)).IsNull());
77+
BOOST_CHECK(!blockman.OpenBlockFile(pos, true).IsNull());
7878

7979
// Check that the file is unlinked after ScanAndUnlinkAlreadyPrunedFiles
8080
// once m_have_pruned is set
8181
blockman.m_have_pruned = true;
8282
WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
83-
BOOST_CHECK(AutoFile(blockman.OpenBlockFile(pos, true)).IsNull());
83+
BOOST_CHECK(blockman.OpenBlockFile(pos, true).IsNull());
8484

8585
// Check that calling with already pruned files doesn't cause an error
8686
WITH_LOCK(chainman->GetMutex(), blockman.ScanAndUnlinkAlreadyPrunedFiles());
@@ -90,7 +90,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_scan_unlink_already_pruned_files, TestChain
9090
BOOST_CHECK_NE(old_tip, new_tip);
9191
const int new_file_number{WITH_LOCK(chainman->GetMutex(), return new_tip->GetBlockPos().nFile)};
9292
const FlatFilePos new_pos(new_file_number, 0);
93-
BOOST_CHECK(!AutoFile(blockman.OpenBlockFile(new_pos, true)).IsNull());
93+
BOOST_CHECK(!blockman.OpenBlockFile(new_pos, true).IsNull());
9494
}
9595

9696
BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)

‎src/test/fuzz/buffered_file.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,12 @@ FUZZ_TARGET(buffered_file)
1919
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
2020
FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
2121
std::optional<BufferedFile> opt_buffered_file;
22-
FILE* fuzzed_file = fuzzed_file_provider.open();
22+
CAutoFile fuzzed_file{fuzzed_file_provider.open(), 0};
2323
try {
24-
opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegral<int>());
24+
opt_buffered_file.emplace(fuzzed_file, fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096), fuzzed_data_provider.ConsumeIntegralInRange<uint64_t>(0, 4096));
2525
} catch (const std::ios_base::failure&) {
26-
if (fuzzed_file != nullptr) {
27-
fclose(fuzzed_file);
28-
}
2926
}
30-
if (opt_buffered_file && fuzzed_file != nullptr) {
27+
if (opt_buffered_file && !fuzzed_file.IsNull()) {
3128
bool setpos_fail = false;
3229
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
3330
CallOneOf(

‎src/test/fuzz/load_external_block_file.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

55
#include <chainparams.h>
6+
#include <clientversion.h>
67
#include <flatfile.h>
78
#include <test/fuzz/FuzzedDataProvider.h>
89
#include <test/fuzz/fuzz.h>
@@ -27,8 +28,8 @@ FUZZ_TARGET(load_external_block_file, .init = initialize_load_external_block_fil
2728
{
2829
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
2930
FuzzedFileProvider fuzzed_file_provider = ConsumeFile(fuzzed_data_provider);
30-
FILE* fuzzed_block_file = fuzzed_file_provider.open();
31-
if (fuzzed_block_file == nullptr) {
31+
CAutoFile fuzzed_block_file{fuzzed_file_provider.open(), CLIENT_VERSION};
32+
if (fuzzed_block_file.IsNull()) {
3233
return;
3334
}
3435
if (fuzzed_data_provider.ConsumeBool()) {

0 commit comments

Comments
 (0)