util: use stronger-guarantee rename method#24308
Merged
maflcko merged 1 commit intobitcoin:masterfrom Feb 11, 2022
Merged
Conversation
Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case.
vasild
approved these changes
Feb 10, 2022
Contributor
vasild
left a comment
vasild
left a comment
There was a problem hiding this comment.
ACK ee822d8
Add a test for this assumption?
Unit test for RenameOver()
commit 23074edf054845b1b74c43618cf7a8cc58aa5518 (HEAD -> pull/24308_1644480965_ee822d85d6__20435_rebased)
Parent: ee822d85d6de7db85416190cf843ad74147519cf
Author: Vasil Dimov <[email protected]>
AuthorDate: Thu Feb 10 14:11:35 2022 +0100
Commit: Vasil Dimov <[email protected]>
CommitDate: Thu Feb 10 14:14:58 2022 +0100
test: add a unit test for RenameOver()
diff --git a/src/test/fs_tests.cpp b/src/test/fs_tests.cpp
index 1256395849..8a5f025e5f 100644
--- a/src/test/fs_tests.cpp
+++ b/src/test/fs_tests.cpp
@@ -115,7 +115,40 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream)
BOOST_CHECK(p1 != p2);
BOOST_CHECK(p2 != p3);
BOOST_CHECK(p1 != p3);
}
}
-BOOST_AUTO_TEST_SUITE_END()
\ No newline at end of file
+BOOST_AUTO_TEST_CASE(rename)
+{
+ const fs::path tmpfolder{m_args.GetDataDirBase()};
+
+ const fs::path path1{GetUniquePath(tmpfolder)};
+ const fs::path path2{GetUniquePath(tmpfolder)};
+
+ const std::string path1_contents{"1111"};
+ const std::string path2_contents{"2222"};
+
+ {
+ std::ofstream file{path1};
+ file << path1_contents;
+ }
+
+ {
+ std::ofstream file{path2};
+ file << path2_contents;
+ }
+
+ // Rename path1 -> path2.
+ BOOST_CHECK(RenameOver(path1, path2));
+
+ BOOST_CHECK(!fs::exists(path1));
+
+ {
+ std::ifstream file{path2};
+ std::string contents;
+ file >> contents;
+ BOOST_CHECK_EQUAL(contents, path1_contents);
+ }
+}
+
+BOOST_AUTO_TEST_SUITE_END()
Member
|
Concept ACK. Checked the relevant documentation that the behavior is what we want for
- https://en.cppreference.com/w/cpp/filesystem/rename
- https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html Also, it is a nice cleanup, no more win32-specific code here. |
Member
|
review ACK ee822d8 |
Member
|
(Also reviewed the unit test in #24308 (review) but didn't run it) |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 11, 2022
ee822d8 util: use stronger-guarantee rename method (Vasil Dimov) Pull request description: Use std::filesystem::rename() instead of std::rename(). We rely on the destination to be overwritten if it exists, but std::rename()'s behavior is implementation-defined in this case. This is a rebase of bitcoin#20435 by vasild. ACKs for top commit: MarcoFalke: review ACK ee822d8 hebasto: Approach ACK ee822d8. vasild: ACK ee822d8 Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a
Member
|
It seems to break Windows builds:
|
Member
Btw, it fails for mingw build: https://cirrus-ci.com/task/6670169794674688 |
laanwj
added a commit
to bitcoin-core/gui
that referenced
this pull request
Feb 17, 2022
… MinGW-w64 dc01cbc test: Add fs_tests/rename unit test (Hennadii Stepanov) d4999d4 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov) Pull request description: Unfortunately, bitcoin/bitcoin#24308 introduced a [regression](bitcoin/bitcoin#24308 (comment)) for mingw builds. The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior > If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_... fails with the "File exists" error. This PR reverts back the `MoveFileExW` call, and adds the [suggested](bitcoin/bitcoin#24308 (review)) unit test. ACKs for top commit: vasild: ACK dc01cbc Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 18, 2022
dc01cbc test: Add fs_tests/rename unit test (Hennadii Stepanov) d4999d4 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov) Pull request description: Unfortunately, bitcoin#24308 introduced a [regression](bitcoin#24308 (comment)) for mingw builds. The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior > If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_... fails with the "File exists" error. This PR reverts back the `MoveFileExW` call, and adds the [suggested](bitcoin#24308 (review)) unit test. ACKs for top commit: vasild: ACK dc01cbc Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
This is a rebase of #20435 by vasild.