From 23213d94ffeb3bee3562684601182e44ef06aba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 22 Jan 2019 11:01:24 +0100 Subject: [PATCH] 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 Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#26355} --- rtc_base/BUILD.gn | 1 + rtc_base/file_rotating_stream.cc | 60 ++++++++++------------- rtc_base/file_rotating_stream.h | 5 +- rtc_base/file_rotating_stream_unittest.cc | 16 +++--- rtc_base/stream.h | 1 + rtc_base/system/file_wrapper.cc | 32 ++++++++++-- rtc_base/system/file_wrapper.h | 21 ++++++-- 7 files changed, 82 insertions(+), 54 deletions(-) diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index f4766f5cb5..08199fa0dd 100644 --- a/rtc_base/BUILD.gn +++ b/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", diff --git a/rtc_base/file_rotating_stream.cc b/rtc_base/file_rotating_stream.cc index 01429b6629..86b852fdcf 100644 --- a/rtc_base/file_rotating_stream.cc +++ b/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(buffer) + done, 1, size - done, f); - fclose(f); + done += f.Read(static_cast(buffer) + done, size - done); } else { break; } diff --git a/rtc_base/file_rotating_stream.h b/rtc_base/file_rotating_stream.h index bda1ee16ba..78e2983445 100644 --- a/rtc_base/file_rotating_stream.h +++ b/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 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 file_names_; size_t max_file_size_; diff --git a/rtc_base/file_rotating_stream_unittest.cc b/rtc_base/file_rotating_stream_unittest.cc index 214cde95ba..22e247f225 100644 --- a/rtc_base/file_rotating_stream_unittest.cc +++ b/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 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 diff --git a/rtc_base/stream.h b/rtc_base/stream.h index 06c3d63e38..a7c6266304 100644 --- a/rtc_base/stream.h +++ b/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(); diff --git a/rtc_base/system/file_wrapper.cc b/rtc_base/system/file_wrapper.cc index dbea1ca907..9d99cefaac 100644 --- a/rtc_base/system/file_wrapper.cc +++ b/rtc_base/system/file_wrapper.cc @@ -10,6 +10,8 @@ #include "rtc_base/system/file_wrapper.h" +#include + #ifdef _WIN32 #include #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) { diff --git a/rtc_base/system/file_wrapper.h b/rtc_base/system/file_wrapper.h index d56e131cfa..8a3adc555a 100644 --- a/rtc_base/system/file_wrapper.h +++ b/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,