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

[Zachary Turner via llvm-commits]

Ok, lgtm then
On Tue, Sep 26, 2017 at 5:03 PM Rui Ueyama <ruiu at google.com> wrote:

> 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/20170927/9d38c67f/attachment.html>