Refactor FileRotatingStream to use FileWrapper rather than FileStream

Bug: webrtc:6463
Change-Id: I77df2c77a658e9c5614554fb5ef8f2dc053031e6
Reviewed-on: https://webrtc-review.googlesource.com/c/117620
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26355}

rtc_base/BUILD.gn

@@ -868,6 +868,7 @@ rtc_static_library("rtc_base") {
     "..:webrtc_common",
     "../api:array_view",
     "network:sent_packet",
+    "system:file_wrapper",
     "third_party/base64",
     "third_party/sigslot",
     "//third_party/abseil-cpp/absl/memory",

rtc_base/file_rotating_stream.cc

@@ -175,7 +175,6 @@ FileRotatingStream::FileRotatingStream(const std::string& dir_path,
                                        size_t num_files)
     : dir_path_(AddTrailingPathDelimiterIfNeeded(dir_path)),
       file_prefix_(file_prefix),
-      file_stream_(nullptr),
       max_file_size_(max_file_size),
       current_file_index_(0),
       rotation_index_(0),
@@ -194,10 +193,7 @@ FileRotatingStream::FileRotatingStream(const std::string& dir_path,
 FileRotatingStream::~FileRotatingStream() {}
 
 StreamState FileRotatingStream::GetState() const {
-  if (!file_stream_) {
-    return SS_CLOSED;
-  }
-  return file_stream_->GetState();
+  return (file_.is_open() ? SS_OPEN : SS_CLOSED);
 }
 
 StreamResult FileRotatingStream::Read(void* buffer,
@@ -213,7 +209,7 @@ StreamResult FileRotatingStream::Write(const void* data,
                                        size_t data_len,
                                        size_t* written,
                                        int* error) {
-  if (!file_stream_) {
+  if (!file_.is_open()) {
     std::fprintf(stderr, "Open() must be called before Write.\n");
     return SR_ERROR;
   }
@@ -221,26 +217,31 @@ StreamResult FileRotatingStream::Write(const void* data,
   RTC_DCHECK_LT(current_bytes_written_, max_file_size_);
   size_t remaining_bytes = max_file_size_ - current_bytes_written_;
   size_t write_length = std::min(data_len, remaining_bytes);
-  size_t local_written = 0;
-  if (!written) {
-    written = &local_written;
-  }
-  StreamResult result = file_stream_->Write(data, write_length, written, error);
-  current_bytes_written_ += *written;
 
+  if (!file_.Write(data, write_length)) {
+    return SR_ERROR;
+  }
+  if (disable_buffering_ && !file_.Flush()) {
+    return SR_ERROR;
+  }
+
+  current_bytes_written_ += write_length;
+  if (written) {
+    *written = write_length;
+  }
   // If we're done with this file, rotate it out.
   if (current_bytes_written_ >= max_file_size_) {
     RTC_DCHECK_EQ(current_bytes_written_, max_file_size_);
     RotateFiles();
   }
-  return result;
+  return SR_SUCCESS;
 }
 
 bool FileRotatingStream::Flush() {
-  if (!file_stream_) {
+  if (!file_.is_open()) {
     return false;
   }
-  return file_stream_->Flush();
+  return file_.Flush();
 }
 
 void FileRotatingStream::Close() {
@@ -261,11 +262,7 @@ bool FileRotatingStream::Open() {
 
 bool FileRotatingStream::DisableBuffering() {
   disable_buffering_ = true;
-  if (!file_stream_) {
-    std::fprintf(stderr, "Open() must be called before DisableBuffering().\n");
-    return false;
-  }
-  return file_stream_->DisableBuffering();
+  return true;
 }
 
 std::string FileRotatingStream::GetFilePath(size_t index) const {
@@ -279,29 +276,25 @@ bool FileRotatingStream::OpenCurrentFile() {
   // Opens the appropriate file in the appropriate mode.
   RTC_DCHECK_LT(current_file_index_, file_names_.size());
   std::string file_path = file_names_[current_file_index_];
-  file_stream_.reset(new FileStream());
 
   // We should always be writing to the zero-th file.
   RTC_DCHECK_EQ(current_file_index_, 0);
-  int error = 0;
-  if (!file_stream_->Open(file_path, "w+", &error)) {
-    std::fprintf(stderr, "Failed to open: %s Error: %i\n", file_path.c_str(),
+  int error;
+  file_ = webrtc::FileWrapper::OpenWriteOnly(file_path, &error);
+  if (!file_.is_open()) {
+    std::fprintf(stderr, "Failed to open: %s Error: %d\n", file_path.c_str(),
                  error);
-    file_stream_.reset();
     return false;
   }
-  if (disable_buffering_) {
-    file_stream_->DisableBuffering();
-  }
   return true;
 }
 
 void FileRotatingStream::CloseCurrentFile() {
-  if (!file_stream_) {
+  if (!file_.is_open()) {
     return;
   }
   current_bytes_written_ = 0;
-  file_stream_.reset();
+  file_.Close();
 }
 
 void FileRotatingStream::RotateFiles() {
@@ -416,12 +409,11 @@ size_t FileRotatingStreamReader::ReadAll(void* buffer, size_t size) const {
   size_t done = 0;
   for (const auto& file_name : file_names_) {
     if (done < size) {
-      FILE* f = fopen(file_name.c_str(), "rb");
-      if (!f) {
+      webrtc::FileWrapper f = webrtc::FileWrapper::OpenReadOnly(file_name);
+      if (!f.is_open()) {
         break;
       }
-      done += fread(static_cast<char*>(buffer) + done, 1, size - done, f);
-      fclose(f);
+      done += f.Read(static_cast<char*>(buffer) + done, size - done);
     } else {
       break;
     }

rtc_base/file_rotating_stream.h

@@ -18,6 +18,7 @@
 
 #include "rtc_base/constructor_magic.h"
 #include "rtc_base/stream.h"
+#include "rtc_base/system/file_wrapper.h"
 
 namespace rtc {
 
@@ -98,8 +99,8 @@ class FileRotatingStream : public StreamInterface {
   const std::string dir_path_;
   const std::string file_prefix_;
 
-  // FileStream is used to write to the current file.
-  std::unique_ptr<FileStream> file_stream_;
+  // File we're currently writing to.
+  webrtc::FileWrapper file_;
   // Convenience storage for file names so we don't generate them over and over.
   std::vector<std::string> file_names_;
   size_t max_file_size_;

rtc_base/file_rotating_stream_unittest.cc

@@ -92,11 +92,9 @@ class MAYBE_FileRotatingStreamTest : public ::testing::Test {
                           const size_t expected_length,
                           const std::string& file_path) {
     std::unique_ptr<uint8_t[]> buffer(new uint8_t[expected_length + 1]);
-    FileStream stream;
-    ASSERT_TRUE(stream.Open(file_path, "r", nullptr));
-    size_t size_read = 0;
-    EXPECT_EQ(rtc::SR_EOS, stream.ReadAll(buffer.get(), expected_length + 1,
-                                          &size_read, nullptr));
+    webrtc::FileWrapper f = webrtc::FileWrapper::OpenReadOnly(file_path);
+    ASSERT_TRUE(f.is_open());
+    size_t size_read = f.Read(buffer.get(), expected_length + 1);
     EXPECT_EQ(size_read, expected_length);
     EXPECT_EQ(0, memcmp(expected_contents, buffer.get(),
                         std::min(expected_length, size_read)));
@@ -129,12 +127,10 @@ TEST_F(MAYBE_FileRotatingStreamTest, EmptyWrite) {
   WriteAndFlush("a", 0);
 
   std::string logfile_path = stream_->GetFilePath(0);
-  FileStream stream;
-  ASSERT_TRUE(stream.Open(logfile_path, "r", nullptr));
+  webrtc::FileWrapper f = webrtc::FileWrapper::OpenReadOnly(logfile_path);
+  ASSERT_TRUE(f.is_open());
   char buf[1];
-  size_t read_nbytes;
-  int read_error;
-  EXPECT_EQ(SR_EOS, stream.Read(buf, sizeof(buf), &read_nbytes, &read_error));
+  EXPECT_EQ(0u, f.Read(buf, sizeof(buf)));
 }
 
 // Tests that a write operation followed by a read returns the expected data

rtc_base/stream.h

@@ -192,6 +192,7 @@ class StreamAdapterInterface : public StreamInterface,
 // support asynchronous notification.
 ///////////////////////////////////////////////////////////////////////////////
 
+// TODO(bugs.webrtc.org/6463): Delete this class.
 class FileStream : public StreamInterface {
  public:
   FileStream();

rtc_base/system/file_wrapper.cc

@@ -10,6 +10,8 @@
 
 #include "rtc_base/system/file_wrapper.h"
 
+#include <cerrno>
+
 #ifdef _WIN32
 #include <Windows.h>
 #else
@@ -20,7 +22,7 @@
 
 namespace webrtc {
 namespace {
-FILE* FileOpen(const char* file_name_utf8, bool read_only) {
+FILE* FileOpen(const char* file_name_utf8, bool read_only, int* error) {
 #if defined(_WIN32)
   int len = MultiByteToWideChar(CP_UTF8, 0, file_name_utf8, -1, nullptr, 0);
   std::wstring wstr(len, 0);
@@ -29,18 +31,40 @@ FILE* FileOpen(const char* file_name_utf8, bool read_only) {
 #else
   FILE* file = fopen(file_name_utf8, read_only ? "rb" : "wb");
 #endif
+  if (!file && error) {
+    *error = errno;
+  }
   return file;
 }
+
+const char* GetCstrCheckNoEmbeddedNul(const std::string& s) {
+  const char* p = s.c_str();
+  RTC_CHECK_EQ(strlen(p), s.size())
+      << "Invalid filename, containing NUL character";
+  return p;
+}
 }  // namespace
 
 // static
 FileWrapper FileWrapper::OpenReadOnly(const char* file_name_utf8) {
-  return FileWrapper(FileOpen(file_name_utf8, true));
+  return FileWrapper(FileOpen(file_name_utf8, true, nullptr));
 }
 
 // static
-FileWrapper FileWrapper::OpenWriteOnly(const char* file_name_utf8) {
-  return FileWrapper(FileOpen(file_name_utf8, false));
+FileWrapper FileWrapper::OpenReadOnly(const std::string& file_name_utf8) {
+  return OpenReadOnly(GetCstrCheckNoEmbeddedNul(file_name_utf8));
+}
+
+// static
+FileWrapper FileWrapper::OpenWriteOnly(const char* file_name_utf8,
+                                       int* error /*=nullptr*/) {
+  return FileWrapper(FileOpen(file_name_utf8, false, error));
+}
+
+// static
+FileWrapper FileWrapper::OpenWriteOnly(const std::string& file_name_utf8,
+                                       int* error /*=nullptr*/) {
+  return OpenWriteOnly(GetCstrCheckNoEmbeddedNul(file_name_utf8), error);
 }
 
 FileWrapper::FileWrapper(FileWrapper&& other) {

rtc_base/system/file_wrapper.h

@@ -20,13 +20,27 @@
 
 namespace webrtc {
 
+// This class is a thin wrapper around FILE*. It's main features are that it
+// owns the FILE*, calling fclose on destruction, and that on windows, file
+// names passed to the open methods are always treated as utf-8, regardless of
+// system code page.
+
+// Most of the methods return only a success/fail indication. When needed, an
+// optional argument |int* error| should be added to all methods, in the same
+// way as for the OpenWriteOnly methods.
 class FileWrapper final {
  public:
   // Opens a file, in read or write mode. Use the is_open() method on the
-  // returned object to check if the open operation was successful. The file is
-  // closed by the destructor.
+  // returned object to check if the open operation was successful. On failure,
+  // and if |error| is non-null, the system errno value is stored at |*error|.
+  // The file is closed by the destructor.
   static FileWrapper OpenReadOnly(const char* file_name_utf8);
-  static FileWrapper OpenWriteOnly(const char* file_name_utf8);
+  static FileWrapper OpenReadOnly(const std::string& file_name_utf8);
+  static FileWrapper OpenWriteOnly(const char* file_name_utf8,
+                                   int* error = nullptr);
+
+  static FileWrapper OpenWriteOnly(const std::string& file_name_utf8,
+                                   int* error = nullptr);
 
   FileWrapper() = default;
 
@@ -53,7 +67,6 @@ class FileWrapper final {
 
   // Write any buffered data to the underlying file. Returns true on success,
   // false on write error. Note: Flushing when closing, is not required.
-  // TODO(nisse): Delete this method.
   bool Flush();
 
   // Seeks to the beginning of file. Returns true on success, false on failure,