[PATCH] D38570: Support: Rewrite Windows implementation of sys::fs::rename to be more POSIXy.

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

So if I understand the problem correctly, you don't have any issue with `ReplaceFile`'s behavior on the destination, only the source.  So, for example, you call `ReplaceFile`, it returns, and then some other process tries to open the //source// file and fails.

If this is correct, then what about this idea?

1. Move the source file to a temporary file name.
2. Call `ReplaceFile` specifying the new temporary file as the source.

This is still not atomic, but neither is the code here, so it's a question of which tradeoffs do we prefer.  With a truly atomic rename, there are two outcomes:

1. Dest exists before rename.  After the rename, there was never a window of time where Dest existed but not Source.  Either They both exist, or exactly 1 exists at it is Dest with Source's original content.
2. Dest doesn't exist before rename.  After the rename, there was never a window of time where 0 or 2 files existed.  There was always exactly 1 file.

With the approach mentioned above, you can still satisfy 2, but not 1.  There will be a short window of time where Dest is moved to a temporary location but not yet overwritten onto Source.



================
Comment at: llvm/lib/Support/Windows/Path.inc:368-369
 
-  // Retry while we see recoverable errors.
-  // System scanners (eg. indexer) might open the source file when it is written
-  // and closed.
+  std::vector<char> RenameInfoBuf(sizeof(FILE_RENAME_INFO) +
+                                  (ToWide.size() * sizeof(wchar_t)));
+  FILE_RENAME_INFO &RenameInfo =
----------------
I think this is allocating 1 character too many.  `ToWide.size()` already includes the null terminator, but so does `sizeof(FILE_RENAME_INFO)` since it has a 1 character array.


================
Comment at: llvm/lib/Support/Windows/Path.inc:374
+  RenameInfo.RootDirectory = 0;
+  RenameInfo.FileNameLength = ToWide.size();
+  std::copy(ToWide.begin(), ToWide.end(), RenameInfo.FileName);
----------------
If I understand correctly, `ToWide.size()` contains the number of characters including null terminator, but this value should be number of characters *not* including null terminator.


https://reviews.llvm.org/D38570