util: Remove DirIsWritable, GetUniquePath

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.

DrahtBot commented at 10:26 am on July 14, 2023: contributor

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

Code Coverage

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

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.

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.

DrahtBot renamed this:
~~util: Remove DirIsWritable, GetUniquePath~~
util: Remove DirIsWritable, GetUniquePath
on Jul 14, 2023

DrahtBot added the label Utils/log/libs on Jul 14, 2023

fanquake commented at 10:35 am on July 14, 2023: member

0make[2]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-arm-linux-gnueabihf/src'
1/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
2init.cpp: In function ‘bool LockDataDirectory(bool)’:
3init.cpp:1021:1: error: control reaches end of non-void function [-Werror=return-type]
4 1021 | }
5      | ^
6cc1plus: all warnings being treated as errors
7make[2]: *** [Makefile:10093: libbitcoin_node_a-init.o] Error 1

maflcko force-pushed on Jul 14, 2023

DrahtBot added the label CI failed on Jul 14, 2023

maflcko force-pushed on Jul 14, 2023

DrahtBot removed the label CI failed on Jul 14, 2023

DrahtBot added the label Needs rebase on Jul 20, 2023

maflcko force-pushed on Jul 20, 2023

DrahtBot removed the label Needs rebase on Jul 20, 2023

in src/Makefile.am:1 in fa5bcb6daf

maflcko commented at 8:36 am on July 26, 2023:

cc @TheCharlatan (libbitcoinkernel)

TheCharlatan commented at 9:32 am on July 26, 2023: contributor

Nice, Concept ACK.

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.

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.

TheCharlatan approved

TheCharlatan commented at 8:33 am on July 28, 2023: contributor

Nice, ACK fa5bcb6dafe376f81a669bf9324eedecf3feeb8b

maflcko requested review from stickies-v on Aug 15, 2023

maflcko requested review from theuni on Aug 30, 2023

DrahtBot added the label CI failed on Sep 15, 2023

DrahtBot removed the label CI failed on Sep 20, 2023

maflcko requested review from hebasto on Oct 3, 2023

hebasto commented at 10:40 am on October 4, 2023: member

Concept ACK.

DrahtBot added the label CI failed on Oct 25, 2023

maflcko force-pushed on Oct 25, 2023

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.

theuni commented at 9:36 pm on October 25, 2023: member

Concept ACK.

DrahtBot removed the label CI failed on Oct 25, 2023

refactor: Return enum in LockDirectory

This makes it easier to add more Error cases in the future. Also, add
missing util namespace.

fa0afe7408

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

Remove DirIsWritable, GetUniquePath fa3da629a1

maflcko force-pushed on Oct 26, 2023

TheCharlatan approved

TheCharlatan commented at 12:37 pm on October 26, 2023: contributor

Re-ACK fa3da629a1aebcb4500803d7417feed8e34285b0

DrahtBot requested review from theuni on Oct 26, 2023

maflcko removed review request from stickies-v on Dec 6, 2023

maflcko removed review request from hebasto on Dec 6, 2023

maflcko removed the label Utils/log/libs on Dec 6, 2023

DrahtBot added the label Utils/log/libs on Dec 6, 2023

maflcko requested review from hebasto on Dec 6, 2023

hebasto approved

hebasto commented at 5:34 pm on December 9, 2023: member

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

maflcko requested review from fanquake on Dec 11, 2023

fanquake merged this on Dec 13, 2023

fanquake closed this on Dec 13, 2023

maflcko deleted the branch on Dec 13, 2023

util: Remove DirIsWritable, GetUniquePath #28075

Code Coverage

Reviews

Conflicts