From e14d4e63deefeed08c46c4e3802a65c491f15371 Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Mon, 13 Nov 2017 18:33:44 +0000 Subject: [PATCH] Create a TempFile class. This just adds a TempFile class and replaces the use in FileOutputBuffer with it. The only difference for now is better error handling. Followup work includes: - Convert other user of temporary files to it. - Add support for automatically deleting on windows. - Add a createUnnamed method that returns a potentially unnamed file. It would be actually unnamed on modern linux and have a unknown name on windows. llvm-svn: 318069 --- include/llvm/Support/FileSystem.h | 35 ++++++++++++++++++++ lib/Support/FileOutputBuffer.cpp | 39 +++++++++++------------ lib/Support/Path.cpp | 53 +++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 20 deletions(-) diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h index 03015a0ca3b..a9b4c987e11 100644 --- a/include/llvm/Support/FileSystem.h +++ b/include/llvm/Support/FileSystem.h @@ -31,6 +31,7 @@ #include "llvm/ADT/StringRef.h" #include "llvm/ADT/Twine.h" #include "llvm/Support/Chrono.h" +#include "llvm/Support/Error.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/ErrorOr.h" #include "llvm/Support/MD5.h" @@ -694,6 +695,40 @@ std::error_code createUniqueFile(const Twine &Model, int &ResultFD, std::error_code createUniqueFile(const Twine &Model, SmallVectorImpl &ResultPath); +/// Represents a temporary file. +/// +/// The temporary file must be eventually discarded or given a final name and +/// kept. +/// +/// The destructor doesn't implicitly discard because there is no way to +/// properly handle errors in a destructor. +class TempFile { + bool Done = false; + TempFile(StringRef Name, int FD); + +public: + /// This creates a temporary file with createUniqueFile and schedules it for + /// deletion with sys::RemoveFileOnSignal. + static Expected create(const Twine &Model, + unsigned Mode = all_read | all_write); + TempFile(TempFile &&Other); + + // Name of the temporary file. + std::string TmpName; + + // The open file descriptor. + int FD = -1; + + // Keep this with the given name. + Error keep(const Twine &Name); + + // Delete the file. + Error discard(); + + // This checks that keep or delete was called. + ~TempFile(); +}; + /// @brief Create a file in the system temporary directory. /// /// The filename is of the form prefix-random_chars.suffix. Since the directory diff --git a/lib/Support/FileOutputBuffer.cpp b/lib/Support/FileOutputBuffer.cpp index db8ff38e5b5..1b1960395e2 100644 --- a/lib/Support/FileOutputBuffer.cpp +++ b/lib/Support/FileOutputBuffer.cpp @@ -17,7 +17,6 @@ #include "llvm/Support/Errc.h" #include "llvm/Support/Memory.h" #include "llvm/Support/Path.h" -#include "llvm/Support/Signals.h" #include #if !defined(_MSC_VER) && !defined(__MINGW32__) @@ -34,9 +33,9 @@ using namespace llvm::sys; // with the temporary file on commit(). class OnDiskBuffer : public FileOutputBuffer { public: - OnDiskBuffer(StringRef Path, StringRef TempPath, + OnDiskBuffer(StringRef Path, fs::TempFile Temp, std::unique_ptr Buf) - : FileOutputBuffer(Path), Buffer(std::move(Buf)), TempPath(TempPath) {} + : FileOutputBuffer(Path), Buffer(std::move(Buf)), Temp(std::move(Temp)) {} uint8_t *getBufferStart() const override { return (uint8_t *)Buffer->data(); } @@ -51,21 +50,19 @@ public: Buffer.reset(); // Atomically replace the existing file with the new one. - auto EC = fs::rename(TempPath, FinalPath); - sys::DontRemoveFileOnSignal(TempPath); - return errorCodeToError(EC); + return Temp.keep(FinalPath); } ~OnDiskBuffer() override { // Close the mapping before deleting the temp file, so that the removal // succeeds. Buffer.reset(); - fs::remove(TempPath); + consumeError(Temp.discard()); } private: std::unique_ptr Buffer; - std::string TempPath; + fs::TempFile Temp; }; // A FileOutputBuffer which keeps data in memory and writes to the final @@ -110,13 +107,11 @@ createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) { static Expected> createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) { - // Create new file in same directory but with random name. - SmallString<128> TempPath; - int FD; - if (auto EC = fs::createUniqueFile(Path + ".tmp%%%%%%%", FD, TempPath, Mode)) - return errorCodeToError(EC); - - sys::RemoveFileOnSignal(TempPath); + Expected FileOrErr = + fs::TempFile::create(Path + ".tmp%%%%%%%", Mode); + if (!FileOrErr) + return FileOrErr.takeError(); + fs::TempFile File = std::move(*FileOrErr); #ifndef LLVM_ON_WIN32 // On Windows, CreateFileMapping (the mmap function on Windows) @@ -124,18 +119,22 @@ createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) { // extend the file beforehand. _chsize (ftruncate on Windows) is // pretty slow just like it writes specified amount of bytes, // so we should avoid calling that function. - if (auto EC = fs::resize_file(FD, Size)) + if (auto EC = fs::resize_file(File.FD, Size)) { + consumeError(File.discard()); return errorCodeToError(EC); + } #endif // Mmap it. std::error_code EC; auto MappedFile = llvm::make_unique( - FD, fs::mapped_file_region::readwrite, Size, 0, EC); - close(FD); - if (EC) + File.FD, fs::mapped_file_region::readwrite, Size, 0, EC); + if (EC) { + consumeError(File.discard()); return errorCodeToError(EC); - return llvm::make_unique(Path, TempPath, std::move(MappedFile)); + } + return llvm::make_unique(Path, std::move(File), + std::move(MappedFile)); } // Create an instance of FileOutputBuffer. diff --git a/lib/Support/Path.cpp b/lib/Support/Path.cpp index 9692acb5283..54ea76b0f92 100644 --- a/lib/Support/Path.cpp +++ b/lib/Support/Path.cpp @@ -18,6 +18,7 @@ #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Process.h" +#include "llvm/Support/Signals.h" #include #include @@ -759,6 +760,58 @@ std::error_code createUniqueFile(const Twine &Model, return createUniqueEntity(Model, Dummy, ResultPath, false, 0, FS_Name); } +TempFile::TempFile(StringRef Name, int FD) : TmpName(Name), FD(FD) {} +TempFile::TempFile(TempFile &&Other) { + TmpName = std::move(Other.TmpName); + FD = Other.FD; + Other.Done = true; +} + +TempFile::~TempFile() { assert(Done); } + +Error TempFile::discard() { + if (Done) + return Error::success(); + Done = true; + // Always try to close and remove. + std::error_code RemoveEC = fs::remove(TmpName); + sys::DontRemoveFileOnSignal(TmpName); + if (close(FD) == -1) { + std::error_code EC = std::error_code(errno, std::generic_category()); + return errorCodeToError(EC); + } + return errorCodeToError(RemoveEC); +} + +Error TempFile::keep(const Twine &Name) { + assert(!Done); + Done = true; + // Always try to close and rename. + std::error_code RenameEC = fs::rename(TmpName, Name); + sys::DontRemoveFileOnSignal(TmpName); + if (close(FD) == -1) { + std::error_code EC(errno, std::generic_category()); + return errorCodeToError(EC); + } + return errorCodeToError(RenameEC); +} + +Expected TempFile::create(const Twine &Model, unsigned Mode) { + int FD; + SmallString<128> ResultPath; + if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Mode)) + return errorCodeToError(EC); + + // Make sure we delete the file when RemoveFileOnSignal fails. + TempFile Ret(ResultPath, FD); + if (sys::RemoveFileOnSignal(ResultPath)) { + consumeError(Ret.discard()); + std::error_code EC(errc::operation_not_permitted); + return errorCodeToError(EC); + } + return std::move(Ret); +} + static std::error_code createTemporaryFile(const Twine &Model, int &ResultFD, llvm::SmallVectorImpl &ResultPath, FSEntity Type) {