util: Remove DirIsWritable, GetUniquePath #28075

pull maflcko wants to merge 3 commits into bitcoin:master from maflcko:2307-remove-fs-code- changing 9 files +67 −126
  1. maflcko commented at 10:26 AM on July 14, 2023: member

    GetUniquePath is only used in tests and in DirIsWritable. The check by DirIsWritable is redundant with the check done in LockDirectory.

    Fix the redundancy by removing everything, except LockDirectory.

  2. DrahtBot commented at 10:26 AM on July 14, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan, hebasto
    Concept ACK theuni

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #28690 (build: Introduce internal kernel library by TheCharlatan)
    • #25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)

    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:
    util: Remove DirIsWritable, GetUniquePath
    util: Remove DirIsWritable, GetUniquePath
    on Jul 14, 2023
  4. DrahtBot added the label Utils/log/libs on Jul 14, 2023
  5. fanquake commented at 10:35 AM on July 14, 2023: member 
    make[2]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-arm-linux-gnueabihf/src'
/usr/bin/ccache arm-linux-gnueabihf-g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config  -fmacro-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-arm-linux-gnueabihf=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /tmp/cirrus-ci-build/depends/arm-linux-gnueabihf/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE   -isystem /tmp/cirrus-ci-build/depends/arm-linux-gnueabihf/include -pthread -I/tmp/cirrus-ci-build/depends/arm-linux-gnueabihf/include  -I/tmp/cirrus-ci-build/depends/arm-linux-gnueabihf/include/  -fdebug-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-arm-linux-gnueabihf=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fstack-clash-protection   -Werror    -fno-extended-identifiers -fvisibility=hidden -fPIE -pipe -std=c++17 -O2 -Wno-psabi -c -o libbitcoin_node_a-init.o `test -f 'init.cpp' || echo './'`init.cpp
init.cpp: In function ‘bool LockDataDirectory(bool)’:
init.cpp:1021:1: error: control reaches end of non-void function [-Werror=return-type]
 1021 | }
      | ^
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:10093: libbitcoin_node_a-init.o] Error 1
  6. maflcko force-pushed on Jul 14, 2023
  7. DrahtBot added the label CI failed on Jul 14, 2023
  8. maflcko force-pushed on Jul 14, 2023
  9. DrahtBot removed the label CI failed on Jul 14, 2023
  10. DrahtBot added the label Needs rebase on Jul 20, 2023
  11. maflcko force-pushed on Jul 20, 2023
  12. DrahtBot removed the label Needs rebase on Jul 20, 2023
  13. in src/Makefile.am:1 in fa5bcb6daf
    maflcko commented at 8:36 AM on July 26, 2023:

    cc @TheCharlatan (libbitcoinkernel)

  14. TheCharlatan commented at 9:32 AM on July 26, 2023: contributor

    Nice, Concept ACK.

  15. in src/init.cpp:1033 in fa5baee943 outdated 
    1011 | @@ -1012,10 +1012,9 @@ static bool LockDataDirectory(bool probeOnly)
1012 |  {
1013 |      // Make sure only a single Bitcoin process is using the data directory.
1014 |      const fs::path& datadir = gArgs.GetDataDirNet();
1015 | -    if (!DirIsWritable(datadir)) {
1016 | -        return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir)));
1017 | -    }
1018 |      switch (util::LockDirectory(datadir, ".lock", probeOnly)) {
1019 | +    case util::LockResult::ErrorWrite:
1020 | +        return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir)));
    TheCharlatan commented at 8:31 AM on July 28, 2023:

    While testing I noticed that the settings file in the datadir is already interacted with before we do this check. So I think the only way to trigger this is if the lock file has bad permissions.

    maflcko commented at 8:46 AM on July 28, 2023:

    Correct. Or you can disable the settings feature.

  16. in src/wallet/bdb.cpp:152 in dddd818bb2 outdated 
     148 | @@ -149,7 +149,7 @@ bool BerkeleyEnvironment::Open(bilingual_str& err)
 149 |  
 150 |      fs::path pathIn = fs::PathFromString(strPath);
 151 |      TryCreateDirectories(pathIn);
 152 | -    if (!LockDirectory(pathIn, ".walletlock")) {
 153 | +    if (util::LockDirectory(pathIn, ".walletlock") != util::LockResult::Success) {
    TheCharlatan commented at 8:32 AM on July 28, 2023:

    Could probably improve the logging a bit here now that there is a diagnostic difference between a locking error and a lock write error.

    maflcko commented at 8:48 AM on July 28, 2023:

    I don't think there is a difference, at least when the wallets are placed inside the datadir. Maybe for wallets outside the datadir this is different? In any case, it seems unrelated, as this pull is not changing any behavior.

  17. TheCharlatan approved
  18. TheCharlatan commented at 8:33 AM on July 28, 2023: contributor

    Nice, ACK fa5bcb6dafe376f81a669bf9324eedecf3feeb8b

  19. maflcko requested review from stickies-v on Aug 15, 2023
  20. maflcko requested review from theuni on Aug 30, 2023
  21. DrahtBot added the label CI failed on Sep 15, 2023
  22. DrahtBot removed the label CI failed on Sep 20, 2023
  23. maflcko requested review from hebasto on Oct 3, 2023
  24. hebasto commented at 10:40 AM on October 4, 2023: member

    Concept ACK.

  25. DrahtBot added the label CI failed on Oct 25, 2023
  26. maflcko force-pushed on Oct 25, 2023
  27. in src/test/util_tests.cpp:1125 in fa5baee943 outdated 
    1121 | @@ -1122,6 +1122,7 @@ static constexpr char ExitCommand = 'X';
1122 |              ch = [&] {
1123 |                  switch (util::LockDirectory(dirname, lockname)) {
1124 |                  case util::LockResult::Success: return 1;
1125 | +                case util::LockResult::ErrorWrite: return 2;
    theuni commented at 9:35 PM on October 25, 2023:

    Can you help me understand what 2 means here?

    maflcko commented at 6:56 AM on October 26, 2023:

    This piece of code translates the return value of the LockDirectory function to a value of type char, so that it can be written as one byte in the next line. Previously, it would assign the boolean return value to char, so I picked 1 for true/Success and 0 for false/ErrorLock.

    Now that the LockResult enum can hold more than a binary result, I am mapping the new return value ErrorWrite to a char value of 2.

    However, that value is never read and the code is never hit, so it can be anything.

    Happy to change to anything else, if you have suggestions.

    maflcko commented at 8:47 AM on October 26, 2023:

    Force pushed to rewrite this test completely to enumerate all possible "return values" of TestOtherProcess.

  28. theuni commented at 9:36 PM on October 25, 2023: member

    Concept ACK.

  29. DrahtBot removed the label CI failed on Oct 25, 2023
  30. refactor: Return enum in LockDirectory
    This makes it easier to add more Error cases in the future. Also, add
missing util namespace.
    fa0afe7408
  31. Return LockResult::ErrorWrite in LockDirectory
    This allows the caller to remove a call to DirIsWritable(), which did a
similar check. Users should not notice any different behavior.
    fad3a9793b
  32. Remove DirIsWritable, GetUniquePath fa3da629a1
  33. maflcko force-pushed on Oct 26, 2023
  34. TheCharlatan approved
  35. TheCharlatan commented at 12:37 PM on October 26, 2023: contributor

    Re-ACK fa3da629a1aebcb4500803d7417feed8e34285b0

  36. DrahtBot requested review from theuni on Oct 26, 2023
  37. maflcko removed review request from stickies-v on Dec 6, 2023
  38. maflcko removed review request from hebasto on Dec 6, 2023
  39. maflcko removed the label Utils/log/libs on Dec 6, 2023
  40. DrahtBot added the label Utils/log/libs on Dec 6, 2023
  41. maflcko requested review from hebasto on Dec 6, 2023
  42. hebasto approved
  43. hebasto commented at 5:34 PM on December 9, 2023: member

    ACK fa3da629a1aebcb4500803d7417feed8e34285b0, I have reviewed the code and it looks OK.

  44. maflcko requested review from fanquake on Dec 11, 2023
  45. fanquake merged this on Dec 13, 2023
  46. fanquake closed this on Dec 13, 2023
  47. maflcko deleted the branch on Dec 13, 2023
  48. bitcoin locked this on Dec 12, 2024
Contributors
maflckoDrahtBotfanquakeTheCharlatanhebastotheuni


theunifanquake

Labels
Utils/log/libs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-03 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me