[PATCH] D38283: Do not remove a target file in FileOutputBuffer::create().

[Rui Ueyama via llvm-commits]

It should replace an existing file both on Unix and Windows. On Unix, it
uses rename() which replaces a destination file if exists. On Windows, it
uses MoveFileExW with MOVEFILE_REPLACE_EXISTING flag, so it should replace
a file too.

On Tue, Sep 26, 2017 at 4:30 PM, Zachary Turner <zturner at google.com> wrote:

> I don’t think this will replace an existing file will it? It looks like it
> will fail if the file already exists
> On Tue, Sep 26, 2017 at 3:31 PM Rui Ueyama via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> ruiu created this revision.
>> Herald added a subscriber: hiraditya.
>>
>> FileOutputBuffer::create() attempts to remove a target file if the file
>> is a regular one, which results in an unexpected result in a failure
>> scenario.
>>
>> If something goes wrong and the user of FileOutputBuffer decides to not
>> call commit(), it leaves nothing. An existing file was removed, and no
>> new file was created.
>>
>> What we should do is to atomically replace an existing file with a new
>> file using rename(), so that it wouldn't remove an existing file without
>> creating a new one.
>>
>>
>> https://reviews.llvm.org/D38283
>>
>> Files:
>>   llvm/lib/Support/FileOutputBuffer.cpp
>>
>>
>> Index: llvm/lib/Support/FileOutputBuffer.cpp
>> ===================================================================
>> --- llvm/lib/Support/FileOutputBuffer.cpp
>> +++ llvm/lib/Support/FileOutputBuffer.cpp
>> @@ -65,13 +65,6 @@
>>        IsRegular = false;
>>    }
>>
>> -  if (IsRegular) {
>> -    // Delete target file.
>> -    EC = sys::fs::remove(FilePath);
>> -    if (EC)
>> -      return EC;
>> -  }
>> -
>>    SmallString<128> TempFilePath;
>>    int FD;
>>    if (IsRegular) {
>> @@ -125,7 +118,7 @@
>>
>>    std::error_code EC;
>>    if (IsRegular) {
>> -    // Rename file to final name.
>> +    // Atomically replace the existing file with the new one.
>>      EC = sys::fs::rename(Twine(TempPath), Twine(FinalPath));
>>      sys::DontRemoveFileOnSignal(TempPath);
>>    } else {
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170926/aef577d8/attachment.html>