refactor: Return CAutoFile from BlockManager::Open*File() #28483

1. 
   maflcko commented at 3:13 pm on September 14, 2023: member

   This is required for #28052, but makes sense on its own, because offloading logic to CAutoFile instead of re-implementing it allows to delete code and complexity.
2. 
   DrahtBot commented at 3:13 pm on September 14, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, willcl-ark

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #28052 (blockstorage: XOR blocksdir *.dat files by MarcoFalke)
   • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot renamed this:
   refactor: Return CAutoFile from BlockManager::Open*File()
   refactor: Return CAutoFile from BlockManager::Open*File()
   on Sep 14, 2023
4. DrahtBot added the label Refactoring on Sep 14, 2023
5. in src/streams.h:647 in faa45328e6 outdated

   642-        fclose();
   643-    }
   644-
   645-    // Disallow copies
   646-    BufferedFile(const BufferedFile&) = delete;
   647-    BufferedFile& operator=(const BufferedFile&) = delete;
   

   ---

   ---

   

   TheCharlatan commented at 11:28 am on September 15, 2023:

   Nit: Would removing this in the second commit when the FILE * is changed to CAutoFile make more sense?

   ---

   

   maflcko commented at 12:21 pm on September 15, 2023:

   Now that fclose is removed, copies are allowed, because they no longer lead to a double-free undefined behavior segfault (Calling fclose twice on the same pointer)
6. in src/streams.h:599 in fa33b6fba2 outdated

   595@@ -598,9 +596,9 @@ class BufferedFile
   596             readNow = nAvail;
   597         if (readNow == 0)
   598             return false;
   599-        size_t nBytes = fread((void*)&vchBuf[pos], 1, readNow, src);
   600+        size_t nBytes{m_src.detail_fread(Span{vchBuf}.subspan(pos,readNow))};
   

   ---

   ---

   

   TheCharlatan commented at 12:01 pm on September 15, 2023:

   Nit (clang-format): Space after pos,.

   ---

   

   maflcko commented at 12:38 pm on September 15, 2023:

   Thanks, done
7. in src/bench/streams_findbyte.cpp:22 in fa33b6fba2 outdated

   17     data[file_size-1] = 1;
   18-    fwrite(&data, sizeof(uint8_t), file_size, file);
   19-    rewind(file);
   20-    BufferedFile bf{file, /*nBufSize=*/file_size + 1, /*nRewindIn=*/file_size, 0};
   21+    file << data;
   22+    std::rewind(file.Get());
   

   ---

   ---

   

   TheCharlatan commented at 12:08 pm on September 15, 2023:

   Nit: Include cstdio where this is used?

   ---

   

   maflcko commented at 12:38 pm on September 15, 2023:

   Added iwyu suggestions here
8. TheCharlatan approved
9. 
   TheCharlatan commented at 12:11 pm on September 15, 2023: contributor

   ACK fa33b6fba270e5867f208b242f50145e1ed3550e
10. 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.

    fa389d902f
11. 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.

    9999b89cd3
12. Return CAutoFile from BlockManager::Open*File()

    This is a refactor.

    fa56c421be
13. maflcko force-pushed on Sep 15, 2023
14. DrahtBot added the label CI failed on Sep 15, 2023
15. DrahtBot removed the label CI failed on Sep 15, 2023
16. TheCharlatan approved
17. 
    TheCharlatan commented at 7:05 pm on September 15, 2023: contributor

    Re-ACK fa56c421be04af846f479c30749b17e6663ab418

    Just addressing nits.

18. fanquake requested review from stickies-v on Sep 16, 2023
19. willcl-ark approved
20. 
    willcl-ark commented at 12:43 pm on September 26, 2023: member

    tACK fa56c421be

    Clean refactor needed for future work.

21. fanquake merged this on Sep 26, 2023
22. fanquake closed this on Sep 26, 2023

    ---

23. Frank-GER referenced this in commit 45c80f5aa8 on Sep 27, 2023
24. maflcko deleted the branch on Sep 28, 2023
25. bitcoin locked this on Dec 3, 2024

Contributors
maflcko DrahtBot TheCharlatan willcl-ark

Review Requested
stickies-v

Labels
Refactoring