Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Oct 26, 2018

Conversation

Projects
None yet
7 participants
@promag
Copy link
Member

commented Oct 24, 2018

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

@fanquake fanquake added the Wallet label Oct 24, 2018

@promag promag force-pushed the promag:2018-10-fix-listwalletdir branch Oct 24, 2018

@ryanofsky
Copy link
Contributor

left a comment

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.

src/wallet/walletutil.cpp Outdated
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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 24, 2018

Contributor

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.

This comment has been minimized.

Copy link
@promag

promag Oct 24, 2018

Author Member

Done.

@promag promag force-pushed the promag:2018-10-fix-listwalletdir branch Oct 24, 2018

@ryanofsky
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

I will rebase once base is merged.

@promag promag force-pushed the promag:2018-10-fix-listwalletdir branch Oct 24, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Oct 24, 2018

@fanquake fanquake added this to Blockers in High-priority for review Oct 25, 2018

@ken2812221
Copy link
Member

left a comment

utACK 8c38f991f8925834c3db390d3c15e42a13c8984c

src/wallet/walletutil.cpp Outdated
@@ -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);

This comment has been minimized.

Copy link
@laanwj

laanwj Oct 25, 2018

Member

I don't like using assert as error handling here

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 25, 2018

Contributor

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

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

utACK

@ryanofsky
Copy link
Contributor

left a comment

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.

Remove fs::relative call and fix listwalletdir tests
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 promag force-pushed the promag:2018-10-fix-listwalletdir branch to ed2e183 Oct 25, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

Squashed.

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

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
Copy link
Contributor

left a comment

utACK squashed ed2e183

@promag

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

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.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 26, 2018

Merge #14561: Remove fs::relative call and fix listwalletdir tests
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

@promag promag deleted the promag:2018-10-fix-listwalletdir branch Oct 26, 2018

@fanquake fanquake removed this from Blockers in High-priority for review Oct 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.