bench: fix used file that is not opened #31693

1. 
   rex4539 commented at 7:18 pm on January 20, 2025: contributor

   Fixes

   0src/bench/load_external.cpp:57:17: error: Used file that is not opened. [useClosedFile]
   1            if (fwrite(ss.data(), 1, ss.size(), file) != ss.size()) {
   2                ^
   3src/bench/load_external.cpp:61:9: error: Used file that is not opened. [useClosedFile]
   4        fclose(file);
   5        ^
   

2. 
   DrahtBot commented at 7:18 pm on January 20, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31693.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. DrahtBot added the label Tests on Jan 20, 2025
4. bench: fix used file that is not opened f90c3dde71
5. in src/bench/load_external.cpp:53 in ae7daab20b outdated

   47@@ -48,18 +48,29 @@ static void LoadExternalBlockFile(benchmark::Bench& bench)
   48     // because that first writes a compact size.
   49     ss << Span{benchmark::data::block413567};
   50 
   51-    // Create the test file.
   52-    {
   53+    // Ensure the directory exists
   54+    if (!fs::exists(testing_setup.get()->m_path_root)) {
   55+    fs::create_directories(testing_setup.get()->m_path_root);
   

   ---

   ---

   

   l0rinc commented at 7:26 pm on January 20, 2025:

   formatting is completely off - is this still a draft?

   ---

   

   rex4539 commented at 7:34 pm on January 20, 2025:

   Forgot to format before committing. Should be ok now :)

   ---

   

   l0rinc commented at 7:37 pm on January 20, 2025:

   Did you test it before committing?

   ---

   

   rex4539 commented at 7:56 pm on January 20, 2025:

   I compiled and ran bench_bitcoin.
6. rex4539 force-pushed on Jan 20, 2025
7. in src/bench/load_external.cpp:54 in f90c3dde71

   58-                throw std::runtime_error("write to test file failed\n");
   59-            }
   60+    // Ensure the directory exists
   61+    if (!fs::exists(testing_setup.get()->m_path_root)) {
   62+        fs::create_directories(testing_setup.get()->m_path_root);
   63+    }
   

   ---

   ---

   

   maflcko commented at 8:11 am on January 21, 2025:

   Why would it not exist? If this can happen, I don’t think the right fix will be to add this code to every unit/bench/fuzz test.
8. 
   maflcko commented at 8:11 am on January 21, 2025: member

   What are the exact steps to reproduce or test this problem and fix?
9. 
   rex4539 commented at 8:16 am on January 21, 2025: contributor

   What are the exact steps to reproduce or test this problem and fix?

   cppcheck src/bench/load_external.cpp

10. 
    maflcko commented at 8:32 am on January 21, 2025: member

    What are the exact steps to reproduce or test this problem and fix?

    cppcheck src/bench/load_external.cpp

    cppcheck is a static code analysis tool. A finding does not imply a bug and removing a finding does not imply a fix.

    With “reproduce” I meant how a real user/developer can run into this problem.

    Using static analysers to find (and fix) bugs will overall result in more bugs (Source: https://www.sqlite.org/testing.html#static_analysis). Looking at the diff in this pull confirms this sqlite finding.

11. 
    rex4539 commented at 8:47 am on January 21, 2025: contributor

    Hey, if everyone feels this PR is not helpful, feel free to close. No hard feelings :)
12. 
    maflcko commented at 8:59 am on January 21, 2025: member

    The burden to motivate a change and explain the change is on the pull request author.

    If you genuinely think there is benefit in this change and a real user/developer can run into this problem, it would be good to explain it and put it into context. Also, the pull request author should answer any outstanding review questions.

13. 
    rex4539 commented at 9:09 am on January 21, 2025: contributor

    This PR doesn’t have any seriousness. I had some free time and ran cppcheck for fun. I tested a fix proposal that I’m not sure if it’s even correct. It appears to fix the issue and not introduce any new bugs (by my testing and CI). I definitely cannot motivate or convince anyone about it. It was made purely for fun.
14. maflcko closed this on Jan 21, 2025

    ---

Contributors
rex4539 DrahtBot l0rinc maflcko

Labels
Tests