Add WeakFileHandleFileWriter, a FileWriterInterface implementation that

deals solely with a weak FileHandle.

CrashReportDatabase::PrepareNewCrashReport() provides its caller with
both a FileHandle and a FilePath. While it’s possible to create a
FileWriter from the FilePath, it’s not necessary to have two FileHandles
open to the same file. Also, there’s no FileWriteMode::kReuseOrFail
option because it didn’t seem necessary[1], and although it would
actually be the most desirable option for a FileWriter here, allowing
the FileHandle to be used directly without reopening the file sidesteps
the problem entirely.

FileWriter is adapted to use WeakFileHandleFileWriter to minimize
duplication.

[1] https://codereview.chromium.org/818433002/diff/80001/util/file/file_io.h#newcode138

R=rsesek@chromium.org, scottmg@chromium.org

Review URL: https://codereview.chromium.org/871193010

util/file/file_io.h

@@ -43,12 +43,17 @@ using FileOffset = off_t;
 //! \brief Scoped wrapper of a FileHandle.
 using ScopedFileHandle = base::ScopedFD;
 
+//! \brief A value that can never be a valid FileHandle.
+const FileHandle kInvalidFileHandle = -1;
+
 #elif defined(OS_WIN)
 
 using FileHandle = HANDLE;
 using FileOffset = LONGLONG;
 using ScopedFileHandle = ScopedFileHANDLE;
 
+const FileHandle kInvalidFileHandle = INVALID_HANDLE_VALUE;
+
 #endif
 
 //! \brief Determines the mode that LoggingOpenFileForWrite() uses.

util/file/file_writer.cc

@@ -37,31 +37,18 @@ static_assert(offsetof(WritableIoVec, iov_len) == offsetof(iovec, iov_len),
               "WritableIoVec len offset");
 #endif  // OS_POSIX
 
-FileWriter::FileWriter() : file_() {
+WeakFileHandleFileWriter::WeakFileHandleFileWriter(FileHandle file_handle)
+    : file_handle_(file_handle) {
 }
 
-FileWriter::~FileWriter() {
+WeakFileHandleFileWriter::~WeakFileHandleFileWriter() {
 }
 
-bool FileWriter::Open(const base::FilePath& path,
+bool WeakFileHandleFileWriter::Write(const void* data, size_t size) {
-                      FileWriteMode write_mode,
+  DCHECK_NE(file_handle_, kInvalidFileHandle);
-                      FilePermissions permissions) {
-  CHECK(!file_.is_valid());
-  file_.reset(LoggingOpenFileForWrite(path, write_mode, permissions));
-  return file_.is_valid();
-}
-
-void FileWriter::Close() {
-  CHECK(file_.is_valid());
-
-  file_.reset();
-}
-
-bool FileWriter::Write(const void* data, size_t size) {
-  DCHECK(file_.is_valid());
 
   // TODO(mark): Write no more than SSIZE_MAX bytes in a single call.
-  ssize_t written = WriteFile(file_.get(), data, size);
+  ssize_t written = WriteFile(file_handle_, data, size);
   if (written < 0) {
     PLOG(ERROR) << "write";
     return false;
@@ -73,8 +60,8 @@ bool FileWriter::Write(const void* data, size_t size) {
   return true;
 }
 
-bool FileWriter::WriteIoVec(std::vector<WritableIoVec>* iovecs) {
+bool WeakFileHandleFileWriter::WriteIoVec(std::vector<WritableIoVec>* iovecs) {
-  DCHECK(file_.is_valid());
+  DCHECK_NE(file_handle_, kInvalidFileHandle);
 
 #if defined(OS_POSIX)
 
@@ -97,7 +84,7 @@ bool FileWriter::WriteIoVec(std::vector<WritableIoVec>* iovecs) {
     size_t writev_iovec_count =
         std::min(remaining_iovecs, implicit_cast<size_t>(IOV_MAX));
     ssize_t written =
-        HANDLE_EINTR(writev(file_.get(), iov, writev_iovec_count));
+        HANDLE_EINTR(writev(file_handle_, iov, writev_iovec_count));
     if (written < 0) {
       PLOG(ERROR) << "writev";
       return false;
@@ -149,9 +136,52 @@ bool FileWriter::WriteIoVec(std::vector<WritableIoVec>* iovecs) {
   return true;
 }
 
+FileOffset WeakFileHandleFileWriter::Seek(FileOffset offset, int whence) {
+  DCHECK_NE(file_handle_, kInvalidFileHandle);
+  return LoggingSeekFile(file_handle_, offset, whence);
+}
+
+FileWriter::FileWriter()
+    : file_(),
+      weak_file_handle_file_writer_(kInvalidFileHandle) {
+}
+
+FileWriter::~FileWriter() {
+}
+
+bool FileWriter::Open(const base::FilePath& path,
+                      FileWriteMode write_mode,
+                      FilePermissions permissions) {
+  CHECK(!file_.is_valid());
+  file_.reset(LoggingOpenFileForWrite(path, write_mode, permissions));
+  if (!file_.is_valid()) {
+    return false;
+  }
+
+  weak_file_handle_file_writer_.set_file_handle(file_.get());
+  return true;
+}
+
+void FileWriter::Close() {
+  CHECK(file_.is_valid());
+
+  weak_file_handle_file_writer_.set_file_handle(kInvalidFileHandle);
+  file_.reset();
+}
+
+bool FileWriter::Write(const void* data, size_t size) {
+  DCHECK(file_.is_valid());
+  return weak_file_handle_file_writer_.Write(data, size);
+}
+
+bool FileWriter::WriteIoVec(std::vector<WritableIoVec>* iovecs) {
+  DCHECK(file_.is_valid());
+  return weak_file_handle_file_writer_.WriteIoVec(iovecs);
+}
+
 FileOffset FileWriter::Seek(FileOffset offset, int whence) {
   DCHECK(file_.is_valid());
-  return LoggingSeekFile(file_.get(), offset, whence);
+  return weak_file_handle_file_writer_.Seek(offset, whence);
 }
 
 }  // namespace crashpad

util/file/file_writer.h

@@ -75,6 +75,50 @@ class FileWriterInterface {
   ~FileWriterInterface() {}
 };
 
+//! \brief A file writer backed by a FileHandle.
+//!
+//! FileWriter requires users to provide a FilePath to open, but this class
+//! accepts an already-open FileHandle instead. Like FileWriter, this class may
+//! write to a filesystem-based file, but unlike FileWriter, this class is not
+//! responsible for creating or closing the file. Users of this class must
+//! ensure that the file handle is closed appropriately elsewhere. Objects of
+//! this class may be used to write to file handles not associated with
+//! filesystem-based files, although special attention should be paid to the
+//! Seek() method, which may not function on file handles that do not refer to
+//! disk-based files.
+//!
+//! This class is expected to be used when other code is responsible for
+//! creating files and already provides file handles.
+class WeakFileHandleFileWriter : public FileWriterInterface {
+ public:
+  explicit WeakFileHandleFileWriter(FileHandle file_handle);
+  ~WeakFileHandleFileWriter();
+
+  // FileWriterInterface:
+  bool Write(const void* data, size_t size) override;
+  bool WriteIoVec(std::vector<WritableIoVec>* iovecs) override;
+
+  //! \copydoc FileWriterInterface::Seek()
+  //!
+  //! \note This method is only guaranteed to function on file handles referring
+  //!     to disk-based files.
+  FileOffset Seek(FileOffset offset, int whence) override;
+
+ private:
+  void set_file_handle(FileHandle file_handle) { file_handle_ = file_handle; }
+
+  FileHandle file_handle_;  // weak
+
+  // FileWriter uses this class as its internal implementation, and it needs to
+  // be able to call set_file_handle(). FileWriter cannot initialize an
+  // WeakFileHandleFileWriter with a correct file descriptor at the time of
+  // construction because no file descriptor will be available until
+  // FileWriter::Open() is called.
+  friend class FileWriter;
+
+  DISALLOW_COPY_AND_ASSIGN(WeakFileHandleFileWriter);
+};
+
 //! \brief A file writer implementation that wraps traditional system file
 //!     operations on files accessed through the filesystem.
 class FileWriter : public FileWriterInterface {
@@ -122,6 +166,7 @@ class FileWriter : public FileWriterInterface {
 
  private:
   ScopedFileHandle file_;
+  WeakFileHandleFileWriter weak_file_handle_file_writer_;
 
   DISALLOW_COPY_AND_ASSIGN(FileWriter);
 };