util: Remove DirIsWritable, GetUniquePath #28075

maflcko · 2023-07-14T10:26:23Z

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 · 2023-07-14T10:26:25Z

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.

fanquake · 2023-07-14T10:35:10Z

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

src/Makefile.am

TheCharlatan · 2023-07-26T09:32:04Z

Nice, Concept ACK.

TheCharlatan

Nice, ACK fa5bcb6

TheCharlatan · 2023-07-28T08:31:02Z

src/init.cpp

    switch (util::LockDirectory(datadir, ".lock", probeOnly)) {
+    case util::LockResult::ErrorWrite:
+        return InitError(strprintf(_("Cannot write to data directory '%s'; check permissions."), fs::PathToString(datadir)));


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.

Correct. Or you can disable the settings feature.

TheCharlatan · 2023-07-28T08:32:52Z

src/wallet/bdb.cpp

@@ -149,7 +149,7 @@ bool BerkeleyEnvironment::Open(bilingual_str& err)

    fs::path pathIn = fs::PathFromString(strPath);
    TryCreateDirectories(pathIn);
-    if (!LockDirectory(pathIn, ".walletlock")) {
+    if (util::LockDirectory(pathIn, ".walletlock") != util::LockResult::Success) {


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

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.

hebasto · 2023-10-04T10:40:13Z

Concept ACK.

theuni

Concept ACK.

theuni · 2023-10-25T21:35:32Z

src/test/util_tests.cpp

@@ -1122,6 +1122,7 @@ static constexpr char ExitCommand = 'X';
            ch = [&] {
                switch (util::LockDirectory(dirname, lockname)) {
                case util::LockResult::Success: return 1;
+                case util::LockResult::ErrorWrite: return 2;


Can you help me understand what 2 means here?

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.

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

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

This allows the caller to remove a call to DirIsWritable(), which did a similar check. Users should not notice any different behavior.

TheCharlatan

Re-ACK fa3da62

hebasto

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

DrahtBot changed the title ~~util: Remove DirIsWritable, GetUniquePath~~ util: Remove DirIsWritable, GetUniquePath Jul 14, 2023

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

maflcko force-pushed the 2307-remove-fs-code- branch from fac69b3 to 9999d82 Compare July 14, 2023 10:35

DrahtBot added the CI failed label Jul 14, 2023

maflcko force-pushed the 2307-remove-fs-code- branch from 9999d82 to fa4c289 Compare July 14, 2023 10:49

DrahtBot removed the CI failed label Jul 14, 2023

This was referenced Jul 14, 2023

util: Show descriptive error messages when FileCommit fails #26654

Merged

sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es #25390

Closed

DrahtBot added the Needs rebase label Jul 20, 2023

maflcko force-pushed the 2307-remove-fs-code- branch from fa4c289 to fa5bcb6 Compare July 20, 2023 13:12

DrahtBot removed the Needs rebase label Jul 20, 2023

maflcko commented Jul 26, 2023

View reviewed changes

src/Makefile.am Outdated Show resolved Hide resolved

TheCharlatan approved these changes Jul 28, 2023

View reviewed changes

maflcko requested a review from stickies-v August 15, 2023 10:04

maflcko requested a review from theuni August 30, 2023 14:35

DrahtBot added CI failed and removed CI failed labels Sep 15, 2023

DrahtBot mentioned this pull request Sep 28, 2023

build: Move message.{h,cpp} from libbitcoin_util to libbitcoin_common #28549

Closed

maflcko requested a review from hebasto October 3, 2023 11:21

This was referenced Oct 20, 2023

build: Introduce internal kernel library #28690

Draft

Remove the legacy wallet and BDB dependency #28710

Draft

DrahtBot added the CI failed label Oct 25, 2023

maflcko force-pushed the 2307-remove-fs-code- branch from fa5bcb6 to fa61bfc Compare October 25, 2023 21:00

theuni reviewed Oct 25, 2023

View reviewed changes

DrahtBot removed the CI failed label Oct 25, 2023

refactor: Return enum in LockDirectory

fa0afe7

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

MarcoFalke added 2 commits October 26, 2023 10:32

Return LockResult::ErrorWrite in LockDirectory

fad3a97

This allows the caller to remove a call to DirIsWritable(), which did a similar check. Users should not notice any different behavior.

Remove DirIsWritable, GetUniquePath

fa3da62

maflcko force-pushed the 2307-remove-fs-code- branch from fa61bfc to fa3da62 Compare October 26, 2023 08:46

TheCharlatan approved these changes Oct 26, 2023

View reviewed changes

DrahtBot requested a review from theuni October 26, 2023 12:37

maflcko removed request for stickies-v and hebasto December 6, 2023 15:28

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

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

maflcko requested a review from hebasto December 6, 2023 15:30

hebasto approved these changes Dec 9, 2023

View reviewed changes

maflcko requested a review from fanquake December 11, 2023 17:23

fanquake merged commit f48a789 into bitcoin:master Dec 13, 2023
16 checks passed

maflcko deleted the 2307-remove-fs-code- branch December 13, 2023 10:10

furszy mentioned this pull request Dec 21, 2023

test: test_bitcoin: allow -testdatadir=<datadir> #26564

Merged

hebasto mentioned this pull request Jan 9, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: Remove DirIsWritable, GetUniquePath #28075

util: Remove DirIsWritable, GetUniquePath #28075

maflcko commented Jul 14, 2023

DrahtBot commented Jul 14, 2023 •

edited

fanquake commented Jul 14, 2023

TheCharlatan commented Jul 26, 2023

TheCharlatan left a comment

TheCharlatan Jul 28, 2023

maflcko Jul 28, 2023

TheCharlatan Jul 28, 2023

maflcko Jul 28, 2023

hebasto commented Oct 4, 2023

theuni left a comment

theuni Oct 25, 2023

maflcko Oct 26, 2023 •

edited

maflcko Oct 26, 2023

TheCharlatan left a comment

hebasto left a comment

util: Remove DirIsWritable, GetUniquePath #28075

util: Remove DirIsWritable, GetUniquePath #28075

Conversation

maflcko commented Jul 14, 2023

DrahtBot commented Jul 14, 2023 • edited

Code Coverage

Reviews

Conflicts

fanquake commented Jul 14, 2023

TheCharlatan commented Jul 26, 2023

TheCharlatan left a comment

Choose a reason for hiding this comment

TheCharlatan Jul 28, 2023

Choose a reason for hiding this comment

maflcko Jul 28, 2023

Choose a reason for hiding this comment

TheCharlatan Jul 28, 2023

Choose a reason for hiding this comment

maflcko Jul 28, 2023

Choose a reason for hiding this comment

hebasto commented Oct 4, 2023

theuni left a comment

Choose a reason for hiding this comment

theuni Oct 25, 2023

Choose a reason for hiding this comment

maflcko Oct 26, 2023 • edited

Choose a reason for hiding this comment

maflcko Oct 26, 2023

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

hebasto left a comment

Choose a reason for hiding this comment

DrahtBot commented Jul 14, 2023 •

edited

maflcko Oct 26, 2023 •

edited