Remove WeakStdioFileReader and WeakStdioFileWriter

These classes were a bit of a hack, and one of the the reasons that
WeakStdioFileReader was introduced, accurate detection of EOF when stdin
is a terminal, will be obsolete once
https://chromium-review.googlesource.com/456676/ lands. In fact,
WeakStdioFileReader didn’t even work properly for this purpose on
Windows.

Use WeakFile{Reader,Writer} in place of these classes (there were only
two use sites). Provide a StdioFileHandle() function to access the
proper values to use as a FileHandle for native file I/O given each OS’
own interface.

Change-Id: I35e8d49982162bb9813855f41739cc77597ea74d
Reviewed-on: https://chromium-review.googlesource.com/456358
Reviewed-by: Robert Sesek <rsesek@chromium.org>

tools/crashpad_database_util.cc

@@ -560,13 +560,21 @@ int DatabaseUtilMain(int argc, char* argv[]) {
     return EXIT_FAILURE;
   }
 
+  bool used_stdin = false;
   for (const base::FilePath new_report_path : options.new_report_paths) {
     std::unique_ptr<FileReaderInterface> file_reader;
 
-    bool is_stdin = false;
     if (new_report_path.value() == FILE_PATH_LITERAL("-")) {
-      is_stdin = true;
+      if (used_stdin) {
-      file_reader.reset(new WeakStdioFileReader(stdin));
+        fprintf(stderr,
+                "%" PRFilePath
+                ": Only one --new-report may be read from standard input\n",
+                me.value().c_str());
+        return EXIT_FAILURE;
+      }
+      used_stdin = true;
+      file_reader.reset(new WeakFileHandleFileReader(
+          StdioFileHandle(StdioStream::kStandardInput)));
     } else {
       std::unique_ptr<FileReader> file_path_reader(new FileReader());
       if (!file_path_reader->Open(new_report_path)) {
@@ -607,13 +615,6 @@ int DatabaseUtilMain(int argc, char* argv[]) {
       return EXIT_FAILURE;
     }
 
-    file_reader.reset();
-    if (is_stdin) {
-      if (fclose(stdin) == EOF) {
-        STDIO_PLOG(ERROR) << "fclose";
-      }
-    }
-
     const char* prefix = (show_operations > 1) ? "New report ID: " : "";
     printf("%s%s\n", prefix, uuid.ToString().c_str());
   }

tools/crashpad_http_upload.cc

@@ -163,7 +163,8 @@ int HTTPUploadMain(int argc, char* argv[]) {
       return EXIT_FAILURE;
     }
   } else {
-    file_writer.reset(new WeakStdioFileWriter(stdout));
+    file_writer.reset(new WeakFileHandleFileWriter(
+        StdioFileHandle(StdioStream::kStandardOutput)));
   }
 
   http_multipart_builder.SetGzipEnabled(options.upload_gzip);

util/file/file_io.h

@@ -26,27 +26,6 @@
 #include "util/win/scoped_handle.h"
 #endif
 
-//! \file
-
-#if defined(OS_POSIX) || DOXYGEN
-
-//! \brief A `PLOG()` macro usable for standard input/output error conditions.
-//!
-//! The `PLOG()` macro uses `errno` on POSIX and is appropriate to report
-//! errors from standard input/output functions. On Windows, `PLOG()` uses
-//! `GetLastError()`, and cannot be used to report errors from standard
-//! input/output functions. This macro uses `PLOG()` when appropriate for
-//! standard I/O functions, and `LOG()` otherwise.
-#define STDIO_PLOG(x) PLOG(x)
-
-#else
-
-#define STDIO_PLOG(x) LOG(x)
-#define fseeko(file, offset, whence) _fseeki64(file, offset, whence)
-#define ftello(file) _ftelli64(file)
-
-#endif
-
 namespace base {
 class FilePath;
 }  // namespace base
@@ -114,6 +93,18 @@ enum class FileLocking : bool {
   kExclusive,
 };
 
+//! \brief Determines the FileHandle that StdioFileHandle() returns.
+enum class StdioStream {
+  //! \brief Standard input, or `stdin`.
+  kStandardInput,
+
+  //! \brief Standard output, or `stdout`.
+  kStandardOutput,
+
+  //! \brief Standard error, or `stderr`.
+  kStandardError,
+};
+
 //! \brief The name of the native read function used by ReadFile().
 //!
 //! This value may be useful for logging.
@@ -381,12 +372,27 @@ void CheckedCloseFile(FileHandle file);
 //! \brief Determines the size of a file.
 //!
 //! \param[in] file The handle to the file for which the size should be
-//!     retrived.
+//!     retrieved.
 //!
 //! \return The size of the file. If an error occurs when attempting to
 //!     determine its size, returns `-1` with an error logged.
 FileOffset LoggingFileSizeByHandle(FileHandle file);
 
+//! \brief Returns a FileHandle corresponding to the requested standard I/O
+//!     stream.
+//!
+//! The returned FileHandle should not be closed on POSIX, where it is
+//! important to maintain valid file descriptors occupying the slots reserved
+//! for these streams. If a need to close such a stream arises on POSIX,
+//! `dup2()` should instead be used to replace the existing file descriptor with
+//! one opened to `/dev/null`. See CloseStdinAndStdout().
+//!
+//! \param[in] stdio_stream The requested standard I/O stream.
+//!
+//! \return A corresponding FileHandle on success. kInvalidFileHandle on error,
+//!     with a message logged.
+FileHandle StdioFileHandle(StdioStream stdio_stream);
+
 }  // namespace crashpad
 
 #endif  // CRASHPAD_UTIL_FILE_FILE_IO_H_

util/file/file_io_posix.cc

@@ -181,4 +181,15 @@ FileOffset LoggingFileSizeByHandle(FileHandle file) {
   return st.st_size;
 }
 
+FileHandle StdioFileHandle(StdioStream stdio_stream) {
+  switch (stdio_stream) {
+    case StdioStream::kStandardInput:
+      return STDIN_FILENO;
+    case StdioStream::kStandardOutput:
+      return STDOUT_FILENO;
+    case StdioStream::kStandardError:
+      return STDERR_FILENO;
+  }
+}
+
 }  // namespace crashpad

util/file/file_io_test.cc

@@ -14,6 +14,8 @@
 
 #include "util/file/file_io.h"
 
+#include <stdio.h>
+
 #include "base/atomicops.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -324,6 +326,26 @@ TEST(FileIO, FileSizeByHandle) {
   EXPECT_EQ(9, LoggingFileSizeByHandle(file_handle.get()));
 }
 
+FileHandle FileHandleForFILE(FILE* file) {
+  int fd = fileno(file);
+#if defined(OS_POSIX)
+  return fd;
+#elif defined(OS_WIN)
+  return reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+#else
+#error Port
+#endif
+}
+
+TEST(FileIO, StdioFileHandle) {
+  EXPECT_EQ(FileHandleForFILE(stdin),
+            StdioFileHandle(StdioStream::kStandardInput));
+  EXPECT_EQ(FileHandleForFILE(stdout),
+            StdioFileHandle(StdioStream::kStandardOutput));
+  EXPECT_EQ(FileHandleForFILE(stderr),
+            StdioFileHandle(StdioStream::kStandardError));
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad

util/file/file_io_win.cc

@@ -246,4 +246,26 @@ FileOffset LoggingFileSizeByHandle(FileHandle file) {
   return file_size.QuadPart;
 }
 
+FileHandle StdioFileHandle(StdioStream stdio_stream) {
+  DWORD standard_handle;
+  switch (stdio_stream) {
+    case StdioStream::kStandardInput:
+      standard_handle = STD_INPUT_HANDLE;
+      break;
+    case StdioStream::kStandardOutput:
+      standard_handle = STD_OUTPUT_HANDLE;
+      break;
+    case StdioStream::kStandardError:
+      standard_handle = STD_ERROR_HANDLE;
+      break;
+    default:
+      NOTREACHED();
+      return INVALID_HANDLE_VALUE;
+  }
+
+  HANDLE handle = GetStdHandle(standard_handle);
+  PLOG_IF(ERROR, handle == INVALID_HANDLE_VALUE) << "GetStdHandle";
+  return handle;
+}
+
 }  // namespace crashpad

util/file/file_reader.cc

@@ -113,43 +113,4 @@ FileOffset FileReader::Seek(FileOffset offset, int whence) {
   return weak_file_handle_file_reader_.Seek(offset, whence);
 }
 
-WeakStdioFileReader::WeakStdioFileReader(FILE* file)
-    : file_(file) {
-}
-
-WeakStdioFileReader::~WeakStdioFileReader() {
-}
-
-FileOperationResult WeakStdioFileReader::Read(void* data, size_t size) {
-  DCHECK(file_);
-
-  size_t rv = fread(data, 1, size, file_);
-  if (rv != size && ferror(file_)) {
-    STDIO_PLOG(ERROR) << "fread";
-    return -1;
-  }
-  if (rv > size) {
-    LOG(ERROR) << "fread: expected " << size << ", observed " << rv;
-    return -1;
-  }
-
-  return rv;
-}
-
-FileOffset WeakStdioFileReader::Seek(FileOffset offset, int whence) {
-  DCHECK(file_);
-  if (fseeko(file_, offset, whence) == -1) {
-    STDIO_PLOG(ERROR) << "fseeko";
-    return -1;
-  }
-
-  FileOffset new_offset = ftello(file_);
-  if (new_offset == -1) {
-    STDIO_PLOG(ERROR) << "ftello";
-    return -1;
-  }
-
-  return new_offset;
-}
-
 }  // namespace crashpad

util/file/file_reader.h

@@ -15,7 +15,6 @@
 #ifndef CRASHPAD_UTIL_FILE_FILE_READER_H_
 #define CRASHPAD_UTIL_FILE_FILE_READER_H_
 
-#include <stdio.h>
 #include <sys/types.h>
 
 #include "base/files/file_path.h"
@@ -142,40 +141,6 @@ class FileReader : public FileReaderInterface {
   DISALLOW_COPY_AND_ASSIGN(FileReader);
 };
 
-//! \brief A file reader backed by a standard input/output `FILE*`.
-//!
-//! This class accepts an already-open `FILE*`. It is not responsible for
-//! opening or closing this `FILE*`. Users of this class must ensure that the
-//! `FILE*` is closed appropriately elsewhere. Objects of this class may be used
-//! to read from `FILE*` objects not associated with filesystem-based files,
-//! although special attention should be paid to the Seek() method, which may
-//! not function on `FILE*` objects that do not refer to disk-based files.
-//!
-//! This class is expected to be used when other code is responsible for
-//! opening `FILE*` objects and already provides `FILE*` objects. A good use
-//! would be a WeakStdioFileReader for `stdin`.
-class WeakStdioFileReader : public FileReaderInterface {
- public:
-  explicit WeakStdioFileReader(FILE* file);
-  ~WeakStdioFileReader() override;
-
-  // FileReaderInterface:
-  FileOperationResult Read(void* data, size_t size) override;
-
-  // FileSeekerInterface:
-
-  //! \copydoc FileReaderInterface::Seek()
-  //!
-  //! \note This method is only guaranteed to function on `FILE*` objects
-  //!     referring to disk-based files.
-  FileOffset Seek(FileOffset offset, int whence) override;
-
- private:
-  FILE* file_;  // weak
-
-  DISALLOW_COPY_AND_ASSIGN(WeakStdioFileReader);
-};
-
 }  // namespace crashpad
 
 #endif  // CRASHPAD_UTIL_FILE_FILE_READER_H_

util/file/file_writer.cc

@@ -192,66 +192,4 @@ FileOffset FileWriter::Seek(FileOffset offset, int whence) {
   return weak_file_handle_file_writer_.Seek(offset, whence);
 }
 
-WeakStdioFileWriter::WeakStdioFileWriter(FILE* file)
-    : file_(file) {
-}
-
-WeakStdioFileWriter::~WeakStdioFileWriter() {
-}
-
-bool WeakStdioFileWriter::Write(const void* data, size_t size) {
-  DCHECK(file_);
-
-  size_t rv = fwrite(data, 1, size, file_);
-  if (rv != size) {
-    if (ferror(file_)) {
-      STDIO_PLOG(ERROR) << "fwrite";
-    } else {
-      LOG(ERROR) << "fwrite: expected " << size << ", observed " << rv;
-    }
-    return false;
-  }
-
-  return true;
-}
-
-bool WeakStdioFileWriter::WriteIoVec(std::vector<WritableIoVec>* iovecs) {
-  DCHECK(file_);
-
-  if (iovecs->empty()) {
-    LOG(ERROR) << "WriteIoVec(): no iovecs";
-    return false;
-  }
-
-  for (const WritableIoVec& iov : *iovecs) {
-    if (!Write(iov.iov_base, iov.iov_len)) {
-      return false;
-    }
-  }
-
-#ifndef NDEBUG
-  // The interface says that |iovecs| is not sacred, so scramble it to make sure
-  // that nobody depends on it.
-  memset(&(*iovecs)[0], 0xa5, sizeof((*iovecs)[0]) * iovecs->size());
-#endif
-
-  return true;
-}
-
-FileOffset WeakStdioFileWriter::Seek(FileOffset offset, int whence) {
-  DCHECK(file_);
-  if (fseeko(file_, offset, whence) == -1) {
-    STDIO_PLOG(ERROR) << "fseeko";
-    return -1;
-  }
-
-  FileOffset new_offset = ftello(file_);
-  if (new_offset == -1) {
-    STDIO_PLOG(ERROR) << "ftello";
-    return -1;
-  }
-
-  return new_offset;
-}
-
 }  // namespace crashpad

util/file/file_writer.h

@@ -167,41 +167,6 @@ class FileWriter : public FileWriterInterface {
   DISALLOW_COPY_AND_ASSIGN(FileWriter);
 };
 
-//! \brief A file writer backed by a standard input/output `FILE*`.
-//!
-//! This class accepts an already-open `FILE*`. It is not responsible for
-//! opening or closing this `FILE*`. Users of this class must ensure that the
-//! `FILE*` is closed appropriately elsewhere. Objects of this class may be used
-//! to write to `FILE*` objects not associated with filesystem-based files,
-//! although special attention should be paid to the Seek() method, which may
-//! not function on `FILE*` objects that do not refer to disk-based files.
-//!
-//! This class is expected to be used when other code is responsible for
-//! opening `FILE*` objects and already provides `FILE*` objects. A good use
-//! would be a WeakStdioFileWriter for `stdout`.
-class WeakStdioFileWriter : public FileWriterInterface {
- public:
-  explicit WeakStdioFileWriter(FILE* file);
-  ~WeakStdioFileWriter() override;
-
-  // FileWriterInterface:
-  bool Write(const void* data, size_t size) override;
-  bool WriteIoVec(std::vector<WritableIoVec>* iovecs) override;
-
-  // FileSeekerInterface:
-
-  //! \copydoc FileWriterInterface::Seek()
-  //!
-  //! \note This method is only guaranteed to function on `FILE*` objects
-  //!     referring to disk-based files.
-  FileOffset Seek(FileOffset offset, int whence) override;
-
- private:
-  FILE* file_;  // weak
-
-  DISALLOW_COPY_AND_ASSIGN(WeakStdioFileWriter);
-};
-
 }  // namespace crashpad
 
 #endif  // CRASHPAD_UTIL_FILE_FILE_WRITER_H_