From 5f5e342584355bb495d12b54846a29041163e88c Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 19 Dec 2014 13:33:01 -0800 Subject: [PATCH] Switch [String]FileWriter to use new file_io.h functions/types Also add ScopedFileHandle as cross-platform version of ScopedFD/ScopedFileHANDLE. R=mark@chromium.org BUG=crashpad:1 Review URL: https://codereview.chromium.org/815053004 --- tools/generate_dump.cc | 4 +-- util/file/file_io.h | 15 +++++--- util/file/file_writer.cc | 64 ++++++++++++++++++--------------- util/file/file_writer.h | 45 +++++++++++------------ util/file/string_file_writer.cc | 17 ++++----- util/file/string_file_writer.h | 2 +- util/win/scoped_handle.cc | 3 +- 7 files changed, 82 insertions(+), 68 deletions(-) diff --git a/tools/generate_dump.cc b/tools/generate_dump.cc index c94aa983..d9361497 100644 --- a/tools/generate_dump.cc +++ b/tools/generate_dump.cc @@ -152,8 +152,8 @@ int GenerateDumpMain(int argc, char* argv[]) { FileWriter file_writer; if (!file_writer.Open(base::FilePath(options.dump_path), - O_WRONLY | O_CREAT | O_TRUNC, - 0644)) { + FileWriteMode::kTruncateOrCreate, + true)) { return EXIT_FAILURE; } diff --git a/util/file/file_io.h b/util/file/file_io.h index 2c59f598..901566c7 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -19,8 +19,11 @@ #include "build/build_config.h" -#if defined(OS_WIN) +#if defined(OS_POSIX) +#include "base/files/scoped_file.h" +#elif defined(OS_WIN) #include +#include "util/win/scoped_handle.h" #endif namespace base { @@ -37,10 +40,14 @@ using FileHandle = int; //! \brief Platform-specific alias for a position in an open file. using FileOffset = off_t; +//! \brief Scoped wrapper of a FileHandle. +using ScopedFileHandle = base::ScopedFD; + #elif defined(OS_WIN) using FileHandle = HANDLE; using FileOffset = LONGLONG; +using ScopedFileHandle = ScopedFileHANDLE; #endif @@ -152,8 +159,7 @@ void CheckedReadFileAtEOF(FileHandle file); //! //! \return The newly opened FileHandle, or an invalid FileHandle on failure. //! -//! \sa ScopedFD -//! \sa ScopedFileHANDLE +//! \sa ScopedFileHandle FileHandle LoggingOpenFileForRead(const base::FilePath& path); //! \brief Wraps `open()` or `CreateFile()`, creating a file for output. Logs @@ -168,8 +174,7 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path); //! \return The newly opened FileHandle, or an invalid FileHandle on failure. //! //! \sa FileWriteMode -//! \sa ScopedFD -//! \sa ScopedFileHANDLE +//! \sa ScopedFileHandle FileHandle LoggingOpenFileForWrite(const base::FilePath& path, FileWriteMode write_mode, bool world_readable); diff --git a/util/file/file_writer.cc b/util/file/file_writer.cc index 4f833051..d6ab9e7c 100644 --- a/util/file/file_writer.cc +++ b/util/file/file_writer.cc @@ -19,49 +19,49 @@ #include #include "base/logging.h" + +#if defined(OS_POSIX) +#include +#include #include "base/posix/eintr_wrapper.h" -#include "util/file/file_io.h" +#endif // OS_POSIX namespace crashpad { +#if defined(OS_POSIX) // Ensure type compatibility between WritableIoVec and iovec. static_assert(sizeof(WritableIoVec) == sizeof(iovec), "WritableIoVec size"); static_assert(offsetof(WritableIoVec, iov_base) == offsetof(iovec, iov_base), "WritableIoVec base offset"); static_assert(offsetof(WritableIoVec, iov_len) == offsetof(iovec, iov_len), "WritableIoVec len offset"); +#endif // OS_POSIX -FileWriter::FileWriter() : fd_() { +FileWriter::FileWriter() : file_() { } FileWriter::~FileWriter() { } -bool FileWriter::Open(const base::FilePath& path, int oflag, mode_t mode) { - CHECK(!fd_.is_valid()); - - DCHECK((oflag & O_WRONLY) || (oflag & O_RDWR)); - - fd_.reset(HANDLE_EINTR(open(path.value().c_str(), oflag, mode))); - if (!fd_.is_valid()) { - PLOG(ERROR) << "open " << path.value(); - return false; - } - - return true; +bool FileWriter::Open(const base::FilePath& path, + FileWriteMode write_mode, + bool world_readable) { + CHECK(!file_.is_valid()); + file_.reset(LoggingOpenFileForWrite(path, write_mode, world_readable)); + return file_.is_valid(); } void FileWriter::Close() { - CHECK(fd_.is_valid()); + CHECK(file_.is_valid()); - fd_.reset(); + file_.reset(); } bool FileWriter::Write(const void* data, size_t size) { - DCHECK(fd_.is_valid()); + DCHECK(file_.is_valid()); // TODO(mark): Write no more than SSIZE_MAX bytes in a single call. - ssize_t written = WriteFile(fd_.get(), data, size); + ssize_t written = WriteFile(file_.get(), data, size); if (written < 0) { PLOG(ERROR) << "write"; return false; @@ -74,7 +74,9 @@ bool FileWriter::Write(const void* data, size_t size) { } bool FileWriter::WriteIoVec(std::vector* iovecs) { - DCHECK(fd_.is_valid()); + DCHECK(file_.is_valid()); + +#if defined(OS_POSIX) ssize_t size = 0; for (const WritableIoVec& iov : *iovecs) { @@ -94,7 +96,8 @@ bool FileWriter::WriteIoVec(std::vector* iovecs) { while (size > 0) { size_t writev_iovec_count = std::min(remaining_iovecs, implicit_cast(IOV_MAX)); - ssize_t written = HANDLE_EINTR(writev(fd_.get(), iov, writev_iovec_count)); + ssize_t written = + HANDLE_EINTR(writev(file_.get(), iov, writev_iovec_count)); if (written < 0) { PLOG(ERROR) << "writev"; return false; @@ -128,6 +131,15 @@ bool FileWriter::WriteIoVec(std::vector* iovecs) { DCHECK_EQ(remaining_iovecs, 0u); +#else // !OS_POSIX + + for (const WritableIoVec& iov : *iovecs) { + if (!Write(iov.iov_base, iov.iov_len)) + return false; + } + +#endif // OS_POSIX + #ifndef NDEBUG // The interface says that |iovecs| is not sacred, so scramble it to make sure // that nobody depends on it. @@ -137,15 +149,9 @@ bool FileWriter::WriteIoVec(std::vector* iovecs) { return true; } -off_t FileWriter::Seek(off_t offset, int whence) { - DCHECK(fd_.is_valid()); - - off_t rv = lseek(fd_.get(), offset, whence); - if (rv < 0) { - PLOG(ERROR) << "lseek"; - } - - return rv; +FileOffset FileWriter::Seek(FileOffset offset, int whence) { + DCHECK(file_.is_valid()); + return LoggingSeekFile(file_.get(), offset, whence); } } // namespace crashpad diff --git a/util/file/file_writer.h b/util/file/file_writer.h index 54c59c92..d0c4a359 100644 --- a/util/file/file_writer.h +++ b/util/file/file_writer.h @@ -17,15 +17,14 @@ #include #include -#include -#include #include #include #include "base/basictypes.h" #include "base/files/file_path.h" -#include "base/files/scoped_file.h" +#include "build/build_config.h" +#include "util/file/file_io.h" namespace crashpad { @@ -43,21 +42,21 @@ struct WritableIoVec { size_t iov_len; }; -//! \brief An interface to write to files and other file-like objects with POSIX -//! semantics. +//! \brief An interface to write to files and other file-like objects with +//! semantics matching the underlying platform (POSIX or Windows). class FileWriterInterface { public: - //! \brief Wraps `write()` or provides an alternate implementation with - //! identical semantics. This method will write the entire buffer, - //! continuing after a short write or after being interrupted. + //! \brief Wraps WriteFile(), or provides an implementation with identical + //! semantics. //! //! \return `true` if the operation succeeded, `false` if it failed, with an //! error message logged. virtual bool Write(const void* data, size_t size) = 0; - //! \brief Wraps `writev()` or provides an alternate implementation with - //! identical semantics. This method will write the entire buffer, - //! continuing after a short write or after being interrupted. + //! \brief Wraps `writev()` on POSIX or provides an alternate implementation + //! with identical semantics. This method will write entire buffers, + //! continuing after a short write or after being interrupted. On + //! non-POSIX this is a simple wrapper around Write(). //! //! \return `true` if the operation succeeded, `false` if it failed, with an //! error message logged. @@ -65,34 +64,36 @@ class FileWriterInterface { //! \note The contents of \a iovecs are undefined when this method returns. virtual bool WriteIoVec(std::vector* iovecs) = 0; - //! \brief Wraps `lseek()` or provides an alternate implementation with - //! identical semantics. + //! \brief Wraps LoggingFileSeek() or provides an alternate implementation + //! with identical semantics. //! - //! \return The return value of `lseek()`. `-1` on failure, with an error - //! message logged. - virtual off_t Seek(off_t offset, int whence) = 0; + //! \return The return value of LoggingFileSeek(). `-1` on failure, + //! with an error message logged. + virtual FileOffset Seek(FileOffset offset, int whence) = 0; protected: ~FileWriterInterface() {} }; -//! \brief A file writer implementation that wraps traditional POSIX file +//! \brief A file writer implementation that wraps traditional system file //! operations on files accessed through the filesystem. class FileWriter : public FileWriterInterface { public: FileWriter(); ~FileWriter(); - //! \brief Wraps `open()`. + //! \brief Wraps LoggingOpenFileForWrite(). //! //! \return `true` if the operation succeeded, `false` if it failed, with an //! error message logged. //! //! \note After a successful call, this method cannot be called again until //! after Close(). - bool Open(const base::FilePath& path, int oflag, mode_t mode); + bool Open(const base::FilePath& path, + FileWriteMode write_mode, + bool world_readable); - //! \brief Wraps `close().` + //! \brief Wraps CheckedCloseHandle(). //! //! \note It is only valid to call this method on an object that has had a //! successful Open() that has not yet been matched by a subsequent call @@ -117,10 +118,10 @@ class FileWriter : public FileWriterInterface { //! //! \note It is only valid to call this method between a successful Open() and //! a Close(). - off_t Seek(off_t offset, int whence) override; + FileOffset Seek(FileOffset offset, int whence) override; private: - base::ScopedFD fd_; + ScopedFileHandle file_; DISALLOW_COPY_AND_ASSIGN(FileWriter); }; diff --git a/util/file/string_file_writer.cc b/util/file/string_file_writer.cc index 5875a327..e2669b32 100644 --- a/util/file/string_file_writer.cc +++ b/util/file/string_file_writer.cc @@ -87,7 +87,7 @@ bool StringFileWriter::WriteIoVec(std::vector* iovecs) { return true; } -off_t StringFileWriter::Seek(off_t offset, int whence) { +FileOffset StringFileWriter::Seek(FileOffset offset, int whence) { DCHECK(offset_.IsValid()); size_t base_offset; @@ -110,21 +110,22 @@ off_t StringFileWriter::Seek(off_t offset, int whence) { return -1; } - off_t base_offset_offt; - if (!AssignIfInRange(&base_offset_offt, base_offset)) { - LOG(ERROR) << "Seek(): base_offset " << base_offset << " invalid for off_t"; + FileOffset base_offset_fileoffset; + if (!AssignIfInRange(&base_offset_fileoffset, base_offset)) { + LOG(ERROR) << "Seek(): base_offset " << base_offset + << " invalid for FileOffset"; return -1; } - base::CheckedNumeric new_offset(base_offset_offt); + base::CheckedNumeric new_offset(base_offset_fileoffset); new_offset += offset; if (!new_offset.IsValid()) { LOG(ERROR) << "Seek(): new_offset invalid"; return -1; } - off_t new_offset_offt = new_offset.ValueOrDie(); + FileOffset new_offset_fileoffset = new_offset.ValueOrDie(); size_t new_offset_sizet; - if (!AssignIfInRange(&new_offset_sizet, new_offset_offt)) { - LOG(ERROR) << "Seek(): new_offset " << new_offset_offt + if (!AssignIfInRange(&new_offset_sizet, new_offset_fileoffset)) { + LOG(ERROR) << "Seek(): new_offset " << new_offset_fileoffset << " invalid for size_t"; return -1; } diff --git a/util/file/string_file_writer.h b/util/file/string_file_writer.h index fa1e7de9..bdfb5e55 100644 --- a/util/file/string_file_writer.h +++ b/util/file/string_file_writer.h @@ -47,7 +47,7 @@ class StringFileWriter : public FileWriterInterface { // FileWriterInterface: bool Write(const void* data, size_t size) override; bool WriteIoVec(std::vector* iovecs) override; - off_t Seek(off_t offset, int whence) override; + FileOffset Seek(FileOffset offset, int whence) override; private: //! \brief The virtual file’s contents. diff --git a/util/win/scoped_handle.cc b/util/win/scoped_handle.cc index 34c1ec7b..5eb440e0 100644 --- a/util/win/scoped_handle.cc +++ b/util/win/scoped_handle.cc @@ -15,12 +15,13 @@ #include "util/win/scoped_handle.h" #include "base/logging.h" +#include "util/file/file_io.h" namespace crashpad { namespace internal { void ScopedFileHANDLECloseTraits::Free(HANDLE handle) { - PCHECK(CloseHandle(handle)); + CheckedCloseFile(handle); } void ScopedKernelHANDLECloseTraits::Free(HANDLE handle) {