Block unsafe std::string fs::path conversion copy_file calls #24026

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:220110-copyfile changing 6 files +15 −6

hebasto commented at 10:04 PM on January 10, 2022: member
This PR is an optional prerequisite for bitcoin/bitcoin#20744 "Use std::filesystem. Remove Boost Filesystem & System" which:
- makes further code changes safer
- prevents some test failures on native Windows

refactor: Block unsafe std::string fs::path conversion copy_file calls

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.

213172c734

Change type of `backup_file` parameter in RestoreWallet/restoreWallet

`fs::path` looks more native than `std::string` for a parameter which
represents a backup file. This change eliminates back-and-forth type
conversions.

3a45dc36a6

DrahtBot added the label interfaces on Jan 10, 2022
DrahtBot added the label RPC/REST/ZMQ on Jan 10, 2022
DrahtBot added the label Wallet on Jan 10, 2022

in src/interfaces/wallet.h:330 in 3a45dc36a6

 326 | @@ -326,7 +327,7 @@ class WalletLoader : public ChainClient
 327 |     virtual std::string getWalletDir() = 0;
 328 |  
 329 |     //! Restore backup wallet
 330 | -   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;
 331 | +   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;

ryanofsky commented at 10:12 PM on January 10, 2022:

In commit "Change type of backup_file parameter in RestoreWallet/restoreWallet" (3a45dc36a663ea67f13e7d5c413518b9385ec594)

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.

achow101 commented at 10:39 PM on January 10, 2022:

https://github.com/bitcoin-core/gui/pull/471 is the PR using it.

ryanofsky approved
ryanofsky commented at 10:13 PM on January 10, 2022: member

Code review ACK 3a45dc36a663ea67f13e7d5c413518b9385ec594. Looks great! Thanks for debugging and fixing this and making #20744 smaller!
DrahtBot commented at 2:06 AM on January 11, 2022: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20744 (Use std::filesystem. Remove Boost Filesystem & System by fanquake)
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 merged this on Jan 11, 2022
fanquake closed this on Jan 11, 2022
achow101 commented at 5:48 PM on January 11, 2022: member

post-merge ACK 3a45dc36a663ea67f13e7d5c413518b9385ec594
hebasto deleted the branch on Jan 11, 2022
sidhujag referenced this in commit 7f43055fac on Jan 12, 2022
DrahtBot locked this on Jan 11, 2023

Contributors

Labels

Wallet RPC/REST/ZMQ interfaces

Block unsafe std::string fs::path conversion copy_file calls #24026

Conflicts