Remove fs::relative call and fix listwalletdir tests #14561

promag · 2018-10-24T11:30:45Z

The implementation of fs::relative resolves symlinks which is not intended
in ListWalletDir. The replacement does what is required, and listwalletdir RPC
tests are fixed accordingly.

Also, fs::recursive_directory_iterator iteration is fixed to build with boost 1.47.

Based on #14559

ryanofsky

utACK ee15451f65c73bc51a7fb2e3fda105488ce16915 (just this commit, not base commits). I think this fix is preferable to the one in #14531 because it avoids getting confused by symlinks and duplicating work already done by the directory iterator.

ryanofsky · 2018-10-24T14:00:04Z

src/wallet/walletutil.cpp

-    for (auto it = fs::recursive_directory_iterator(wallet_dir); it != end(it); ++it) {
+    for (auto it = fs::recursive_directory_iterator(wallet_dir); it != fs::recursive_directory_iterator(); ++it) {
+        assert(it->path().string().compare(0, base.size(), base) == 0);
+        const std::string relative = it->path().string().substr(base.size() + 1); // Account for directory separator


It might be good to pull this logic out into a reusable fs::path LexicallyRelative(fs::path path, fs::path base) function. Then later we could replace it with standard lexically_relative.

ryanofsky

utACK 7879ac6ee8efbefafcecef7a0f7fcbdf888bb957 (last commit only). Only change is adding LexicallyRelative function. There are some other changes I might want to make on top of this:

Writing tests for LexicallyRelative
Adding comment saying it could be replaced in the future with c++17 path::lexically_relative
Throwing exception instead of asserting
Checking path.string()[base.string().size()] character is a separator.

But I think 7879ac6ee8efbefafcecef7a0f7fcbdf888bb957 is a nice minimal fix that will restore compatibility with previous boost versions.

promag · 2018-10-24T15:21:31Z

I will rebase once base is merged.

promag · 2018-10-24T19:26:20Z

Rebased.

ken2812221

utACK 8c38f991f8925834c3db390d3c15e42a13c8984c

laanwj · 2018-10-25T13:07:08Z

src/wallet/walletutil.cpp

@@ -49,15 +49,21 @@ static bool IsBerkeleyBtree(const fs::path& path)
    return data == 0x00053162 || data == 0x62310500;
 }

+static fs::path LexicallyRelative(const fs::path& path, const fs::path& base)
+{
+    assert(path.string().compare(0, base.string().size(), base.string()) == 0);


I don't like using assert as error handling here

I don't like using assert as error handling here

I previously suggested an exception instead of aborting (#14561 (review)), but this isn't runtime error handling. It is checking an assumption the list code is making about the behavior of recursive_directory_iterator. In the new version of this PR that drops the assert, the assumption is no longer documented or checked, and if it's violated ListWalletDirs will return garbage instead of aborting.

promag · 2018-10-25T14:02:09Z

@ryanofsky @ken2812221 please review updated version which inlines the function.

laanwj · 2018-10-25T14:02:59Z

utACK

ryanofsky

utACK a0daca2342dbab588db0461ad292d4a4a0d8879c which inlines the lexically relative logic again. I think this is less readable and more fragile than what I suggested, but it's fine if others like this approach.

The implementation of fs::relative resolves symlinks which is not intended in ListWalletDir. The replacement does what is required, and listwalletdir tests are fixed accordingly. Also, building with boost 1.47 required 2 changes: - replace fs::relative with an alternative implementation; - fix fs::recursive_directory_iterator iteration.

promag · 2018-10-25T14:35:27Z

Squashed.

laanwj · 2018-10-25T14:48:45Z

utACK a0daca2 which inlines the lexically relative logic again. I think this is less readable and more fragile than what I suggested, but it's fine if others like this approach.

we had some discussion about this on IRC. I was fine with having it an utility function, I just believe utility functions need proper error handling and not 'assert', that was my only point that caused @promag to change it to this.

ryanofsky

utACK squashed ed2e183

promag · 2018-10-25T14:53:32Z

Honestly I don't want to provide the utility function, it wasn't needed until now, I doubt it will be needed and sadly boost 1.47 doesn't have it. But if needed this can be extracted and unit tested. Until then this is covered by listwalletdir RPC and soon also exposed in the UI.

I'm also happy to follow up with previous @ryanofsky suggestions. I just think that the goal should be to fix boost build and listwalletdir tests first.

BTW, thanks for your time reviewers.

kallewoof · 2018-10-25T23:28:00Z

Compiles and tests run fine on Debian Jessie PPC (big endian).

laanwj · 2018-10-26T11:25:20Z

Honestly I don't want to provide the utility function, it wasn't needed until now, I doubt it will be needed

Thank you for being clear, that's completely acceptable.

FWIW to me, too, it seems to small of a code unit to have much advantage of clarity of factoring it out. Let's just leave it at this.

ed2e183 Remove fs::relative call and fix listwalletdir tests (João Barbosa) Pull request description: The implementation of `fs::relative` resolves symlinks which is not intended in ListWalletDir. The replacement does what is required, and `listwalletdir` RPC tests are fixed accordingly. Also, `fs::recursive_directory_iterator` iteration is fixed to build with boost 1.47. Based on #14559 Tree-SHA512: 1da516226073f195285d10d9d9648c90cce0158c5d1eb9c31217bb4abb575cd37f07c00787c5a850554d6120bbc5a3cbc5cb47d4488b32ac6bcb52bc1882d600

Summary: ed2e18398b3ab657e98e3e1fe135cbf8dd94fda3 Remove fs::relative call and fix listwalletdir tests (João Barbosa) Pull request description: The implementation of `fs::relative` resolves symlinks which is not intended in ListWalletDir. The replacement does what is required, and `listwalletdir` RPC tests are fixed accordingly. Also, `fs::recursive_directory_iterator` iteration is fixed to build with boost 1.47. Based on #14559 Tree-SHA512: 1da516226073f195285d10d9d9648c90cce0158c5d1eb9c31217bb4abb575cd37f07c00787c5a850554d6120bbc5a3cbc5cb47d4488b32ac6bcb52bc1882d600 Backport of Core [[bitcoin/bitcoin#14561 | PR14561]] Depends on D5546 Test Plan: ``` ninja test_runner.py wallet_multiwallet ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5547

…ests ed2e183 Remove fs::relative call and fix listwalletdir tests (João Barbosa) Pull request description: The implementation of `fs::relative` resolves symlinks which is not intended in ListWalletDir. The replacement does what is required, and `listwalletdir` RPC tests are fixed accordingly. Also, `fs::recursive_directory_iterator` iteration is fixed to build with boost 1.47. Based on bitcoin#14559 Tree-SHA512: 1da516226073f195285d10d9d9648c90cce0158c5d1eb9c31217bb4abb575cd37f07c00787c5a850554d6120bbc5a3cbc5cb47d4488b32ac6bcb52bc1882d600

Merge bitcoin#14291, bitcoin#14275, bitcoin#14465, bitcoin#14559, bitcoin#14561

fanquake added the Wallet label Oct 24, 2018

promag mentioned this pull request Oct 24, 2018

Replace fs::relative call with custom GetRelativePath #14531

Closed

promag force-pushed the 2018-10-fix-listwalletdir branch from be120e3 to ee15451 Compare October 24, 2018 13:53

ryanofsky reviewed Oct 24, 2018

View reviewed changes

promag force-pushed the 2018-10-fix-listwalletdir branch from ee15451 to 7879ac6 Compare October 24, 2018 14:25

ryanofsky approved these changes Oct 24, 2018

View reviewed changes

DrahtBot added the Needs rebase label Oct 24, 2018

promag force-pushed the 2018-10-fix-listwalletdir branch from 7879ac6 to 8c38f99 Compare October 24, 2018 18:57

DrahtBot removed the Needs rebase label Oct 24, 2018

ken2812221 approved these changes Oct 25, 2018

View reviewed changes

promag mentioned this pull request Oct 25, 2018

loadwallet command crashes bitcoind #14538

Closed

laanwj reviewed Oct 25, 2018

View reviewed changes

fanquake mentioned this pull request Oct 25, 2018

Error compiling bitcoin core from master branch #14570

Closed

ryanofsky approved these changes Oct 25, 2018

View reviewed changes

promag force-pushed the 2018-10-fix-listwalletdir branch from a0daca2 to ed2e183 Compare October 25, 2018 14:34

ryanofsky approved these changes Oct 25, 2018

View reviewed changes

laanwj merged commit ed2e183 into bitcoin:master Oct 26, 2018

promag deleted the 2018-10-fix-listwalletdir branch October 26, 2018 12:52

hebasto mentioned this pull request Oct 4, 2020

Strip any trailing / in -datadir and -blocksdir paths #20080

Merged

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Aug 16, 2021

Merge pull request #4339 from 5tefan/bp018a

f370f41

Merge bitcoin#14291, bitcoin#14275, bitcoin#14465, bitcoin#14559, bitcoin#14561

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Remove fs::relative call and fix listwalletdir tests #14561

Remove fs::relative call and fix listwalletdir tests #14561

Uh oh!

Conversation

promag commented Oct 24, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

promag Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 24, 2018

Uh oh!

promag commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 25, 2018

Uh oh!

laanwj commented Oct 25, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 25, 2018

Uh oh!

laanwj commented Oct 25, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Oct 25, 2018

Uh oh!

laanwj commented Oct 26, 2018

Uh oh!

Uh oh!

ryanofsky left a comment •

edited

Loading

promag commented Oct 24, 2018 •

edited

Loading

promag commented Oct 25, 2018 •

edited

Loading