bench: fix used file that is not opened #31693

rex4539 commented at 7:18 PM on January 20, 2025: contributor

Fixes

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

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.

DrahtBot added the label Tests on Jan 20, 2025

bench: fix used file that is not opened f90c3dde71

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.

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.

maflcko commented at 8:11 AM on January 21, 2025: member

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

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

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.

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 :)

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.

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.

maflcko closed this on Jan 21, 2025

bitcoin locked this on Jan 21, 2026