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
Block unsafe std::string fs::path conversion copy_file calls #24026
Conversation
There is no change in behavior. This just helps prepare for the transition from boost::filesystem to std::filesystem by avoiding copy_file calls that will be unsafe after the transition to std::filesystem to due lack of a boost::filesystem::path::imbue equivalent and inability to set a predictable locale.
`fs::path` looks more native than `std::string` for a parameter which represents a backup file. This change eliminates back-and-forth type conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -326,7 +327,7 @@ class WalletLoader : public ChainClient | |||
virtual std::string getWalletDir() = 0; | |||
|
|||
//! Restore backup wallet | |||
virtual std::unique_ptr<Wallet> restoreWallet(const std::string& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0; | |||
virtual std::unique_ptr<Wallet> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In commit "Change type of backup_file
parameter in RestoreWallet/restoreWallet" (3a45dc3)
Hmm, interesting that this method doesn't seem to be called anywhere. I guess whatever PR which will use it will also need to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoin-core/gui#471 is the PR using it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
post-merge ACK 3a45dc3 |
…y_file calls 3a45dc3 Change type of `backup_file` parameter in RestoreWallet/restoreWallet (Hennadii Stepanov) 213172c refactor: Block unsafe std::string fs::path conversion copy_file calls (Hennadii Stepanov) Pull request description: This PR is an optional prerequisite for bitcoin#20744 "Use std::filesystem. Remove Boost Filesystem & System" which: - makes further code changes safer - prevents [some](https://cirrus-ci.com/task/6525835388649472) test failures on native Windows ACKs for top commit: ryanofsky: Code review ACK 3a45dc3. Looks great! Thanks for debugging and fixing this and making bitcoin#20744 smaller! Tree-SHA512: c6dfaef6b45b9c268bc9ee9b943b9d679152c9d565ca4f86da8d33f8eb9b3cdbe9ba6df7b7578eacc0d00db6551048beff97419f86eb4b1d3182c43e2b4eb9a5
This PR is an optional prerequisite for #20744 "Use std::filesystem. Remove Boost Filesystem & System" which: