From df8789252252c77660daf5d602d425b60b344b08 Mon Sep 17 00:00:00 2001 From: Andreas Gampe Date: Thu, 13 Aug 2015 16:44:54 -0700 Subject: [PATCH] ART: Add FdFile constructors Make Open protected, and expose constructors instead. Add a move constructor and move assignment operator. Add OS functions that return the FdFile non-pointer version. Add tests. Bug: 21192156 Test: m test-art-host Test: m test-art-target (shamu) Change-Id: I83e390edde7cd37c900e9d5c3e4d21da22981b3f --- runtime/base/unix_file/fd_file.cc | 32 ++++++++++++++++++++- runtime/base/unix_file/fd_file.h | 52 +++++++++++++++++++++++++++++++--- runtime/base/unix_file/fd_file_test.cc | 51 ++++++++++++++++++++------------- runtime/os_linux.cc | 5 ++-- runtime/trace.cc | 4 +-- runtime/utils.cc | 8 +++--- 6 files changed, 120 insertions(+), 32 deletions(-) diff --git a/runtime/base/unix_file/fd_file.cc b/runtime/base/unix_file/fd_file.cc index e4097dd3d..6f0e1251d 100644 --- a/runtime/base/unix_file/fd_file.cc +++ b/runtime/base/unix_file/fd_file.cc @@ -53,7 +53,15 @@ FdFile::FdFile(int fd, const std::string& path, bool check_usage, bool read_only fd_(fd), file_path_(path), auto_close_(true), read_only_mode_(read_only_mode) { } -FdFile::~FdFile() { +FdFile::FdFile(const std::string& path, int flags, mode_t mode, bool check_usage) + : fd_(-1), auto_close_(true) { + Open(path, flags, mode); + if (!check_usage || !IsOpened()) { + guard_state_ = GuardState::kNoCheck; + } +} + +void FdFile::Destroy() { if (kCheckSafeUsage && (guard_state_ < GuardState::kNoCheck)) { if (guard_state_ < GuardState::kFlushed) { LOG(::art::ERROR) << "File " << file_path_ << " wasn't explicitly flushed before destruction."; @@ -70,6 +78,28 @@ FdFile::~FdFile() { } } +FdFile& FdFile::operator=(FdFile&& other) { + if (this == &other) { + return *this; + } + + if (this->fd_ != other.fd_) { + Destroy(); // Free old state. + } + + guard_state_ = other.guard_state_; + fd_ = other.fd_; + file_path_ = std::move(other.file_path_); + auto_close_ = other.auto_close_; + other.Release(); // Release other. + + return *this; +} + +FdFile::~FdFile() { + Destroy(); +} + void FdFile::moveTo(GuardState target, GuardState warn_threshold, const char* warning) { if (kCheckSafeUsage) { if (guard_state_ < GuardState::kNoCheck) { diff --git a/runtime/base/unix_file/fd_file.h b/runtime/base/unix_file/fd_file.h index 16cd44f4e..d896ee9ec 100644 --- a/runtime/base/unix_file/fd_file.h +++ b/runtime/base/unix_file/fd_file.h @@ -18,7 +18,9 @@ #define ART_RUNTIME_BASE_UNIX_FILE_FD_FILE_H_ #include + #include + #include "base/unix_file/random_access_file.h" #include "base/macros.h" @@ -39,6 +41,46 @@ class FdFile : public RandomAccessFile { FdFile(int fd, const std::string& path, bool checkUsage); FdFile(int fd, const std::string& path, bool checkUsage, bool read_only_mode); + FdFile(const std::string& path, int flags, bool checkUsage) + : FdFile(path, flags, 0640, checkUsage) {} + FdFile(const std::string& path, int flags, mode_t mode, bool checkUsage); + + // Move constructor. + FdFile(FdFile&& other) + : guard_state_(other.guard_state_), + fd_(other.fd_), + file_path_(std::move(other.file_path_)), + auto_close_(other.auto_close_), + read_only_mode_(other.read_only_mode_) { + other.Release(); // Release the src. + } + + // Move assignment operator. + FdFile& operator=(FdFile&& other); + + // Release the file descriptor. This will make further accesses to this FdFile invalid. Disables + // all further state checking. + int Release() { + int tmp_fd = fd_; + fd_ = -1; + guard_state_ = GuardState::kNoCheck; + auto_close_ = false; + return tmp_fd; + } + + void Reset(int fd, bool check_usage) { + if (fd_ != -1 && fd_ != fd) { + Destroy(); + } + fd_ = fd; + if (check_usage) { + guard_state_ = fd == -1 ? GuardState::kNoCheck : GuardState::kBase; + } else { + guard_state_ = GuardState::kNoCheck; + } + // Keep the auto_close_ state. + } + // Destroys an FdFile, closing the file descriptor if Close hasn't already // been called. (If you care about the return value of Close, call it // yourself; this is meant to handle failure cases and read-only accesses. @@ -46,10 +88,6 @@ class FdFile : public RandomAccessFile { // guarantee that data actually made it to stable storage.) virtual ~FdFile(); - // Opens file 'file_path' using 'flags' and 'mode'. - bool Open(const std::string& file_path, int flags); - bool Open(const std::string& file_path, int flags, mode_t mode); - // RandomAccessFile API. int Close() OVERRIDE WARN_UNUSED; int64_t Read(char* buf, int64_t byte_count, int64_t offset) const OVERRIDE WARN_UNUSED; @@ -119,10 +157,16 @@ class FdFile : public RandomAccessFile { GuardState guard_state_; + // Opens file 'file_path' using 'flags' and 'mode'. + bool Open(const std::string& file_path, int flags); + bool Open(const std::string& file_path, int flags, mode_t mode); + private: template bool WriteFullyGeneric(const void* buffer, size_t byte_count, size_t offset); + void Destroy(); // For ~FdFile and operator=(&&). + int fd_; std::string file_path_; bool auto_close_; diff --git a/runtime/base/unix_file/fd_file_test.cc b/runtime/base/unix_file/fd_file_test.cc index 9bc87e5bb..db3a44f9b 100644 --- a/runtime/base/unix_file/fd_file_test.cc +++ b/runtime/base/unix_file/fd_file_test.cc @@ -49,29 +49,28 @@ TEST_F(FdFileTest, UnopenedFile) { TEST_F(FdFileTest, OpenClose) { std::string good_path(GetTmpPath("some-file.txt")); - FdFile file; - ASSERT_TRUE(file.Open(good_path, O_CREAT | O_WRONLY)); + FdFile file(good_path, O_CREAT | O_WRONLY, true); + ASSERT_TRUE(file.IsOpened()); EXPECT_GE(file.Fd(), 0); EXPECT_TRUE(file.IsOpened()); EXPECT_EQ(0, file.Flush()); EXPECT_EQ(0, file.Close()); EXPECT_EQ(-1, file.Fd()); EXPECT_FALSE(file.IsOpened()); - EXPECT_TRUE(file.Open(good_path, O_RDONLY)); - EXPECT_GE(file.Fd(), 0); - EXPECT_TRUE(file.IsOpened()); + FdFile file2(good_path, O_RDONLY, true); + EXPECT_TRUE(file2.IsOpened()); + EXPECT_GE(file2.Fd(), 0); - ASSERT_EQ(file.Close(), 0); + ASSERT_EQ(file2.Close(), 0); ASSERT_EQ(unlink(good_path.c_str()), 0); } TEST_F(FdFileTest, ReadFullyEmptyFile) { // New scratch file, zero-length. art::ScratchFile tmp; - FdFile file; - ASSERT_TRUE(file.Open(tmp.GetFilename(), O_RDONLY)); + FdFile file(tmp.GetFilename(), O_RDONLY, false); + ASSERT_TRUE(file.IsOpened()); EXPECT_GE(file.Fd(), 0); - EXPECT_TRUE(file.IsOpened()); uint8_t buffer[16]; EXPECT_FALSE(file.ReadFully(&buffer, 4)); } @@ -84,10 +83,9 @@ static void NullTerminateCharArray(char (&array)[Size]) { TEST_F(FdFileTest, ReadFullyWithOffset) { // New scratch file, zero-length. art::ScratchFile tmp; - FdFile file; - ASSERT_TRUE(file.Open(tmp.GetFilename(), O_RDWR)); + FdFile file(tmp.GetFilename(), O_RDWR, false); + ASSERT_TRUE(file.IsOpened()); EXPECT_GE(file.Fd(), 0); - EXPECT_TRUE(file.IsOpened()); char ignore_prefix[20] = {'a', }; NullTerminateCharArray(ignore_prefix); @@ -113,9 +111,8 @@ TEST_F(FdFileTest, ReadFullyWithOffset) { TEST_F(FdFileTest, ReadWriteFullyWithOffset) { // New scratch file, zero-length. art::ScratchFile tmp; - FdFile file; - ASSERT_TRUE(file.Open(tmp.GetFilename(), O_RDWR)); - EXPECT_GE(file.Fd(), 0); + FdFile file(tmp.GetFilename(), O_RDWR, false); + ASSERT_GE(file.Fd(), 0); EXPECT_TRUE(file.IsOpened()); const char* test_string = "This is a test string"; @@ -140,8 +137,7 @@ TEST_F(FdFileTest, ReadWriteFullyWithOffset) { TEST_F(FdFileTest, Copy) { art::ScratchFile src_tmp; - FdFile src; - ASSERT_TRUE(src.Open(src_tmp.GetFilename(), O_RDWR)); + FdFile src(src_tmp.GetFilename(), O_RDWR, false); ASSERT_GE(src.Fd(), 0); ASSERT_TRUE(src.IsOpened()); @@ -151,8 +147,7 @@ TEST_F(FdFileTest, Copy) { ASSERT_EQ(static_cast(sizeof(src_data)), src.GetLength()); art::ScratchFile dest_tmp; - FdFile dest; - ASSERT_TRUE(dest.Open(src_tmp.GetFilename(), O_RDWR)); + FdFile dest(src_tmp.GetFilename(), O_RDWR, false); ASSERT_GE(dest.Fd(), 0); ASSERT_TRUE(dest.IsOpened()); @@ -168,4 +163,22 @@ TEST_F(FdFileTest, Copy) { ASSERT_EQ(0, src.Close()); } +TEST_F(FdFileTest, MoveConstructor) { + // New scratch file, zero-length. + art::ScratchFile tmp; + FdFile file(tmp.GetFilename(), O_RDWR, false); + ASSERT_TRUE(file.IsOpened()); + EXPECT_GE(file.Fd(), 0); + + int old_fd = file.Fd(); + + FdFile file2(std::move(file)); + EXPECT_FALSE(file.IsOpened()); + EXPECT_TRUE(file2.IsOpened()); + EXPECT_EQ(old_fd, file2.Fd()); + + ASSERT_EQ(file2.Flush(), 0); + ASSERT_EQ(file2.Close(), 0); +} + } // namespace unix_file diff --git a/runtime/os_linux.cc b/runtime/os_linux.cc index f45e9f603..1d1413bd7 100644 --- a/runtime/os_linux.cc +++ b/runtime/os_linux.cc @@ -53,8 +53,9 @@ File* OS::CreateEmptyFileWriteOnly(const char* name) { File* OS::OpenFileWithFlags(const char* name, int flags) { CHECK(name != nullptr); - std::unique_ptr file(new File); - if (!file->Open(name, flags, 0666)) { + bool read_only = (flags == O_RDONLY); + std::unique_ptr file(new File(name, flags, 0666, !read_only)); + if (!file->IsOpened()) { return nullptr; } return file.release(); diff --git a/runtime/trace.cc b/runtime/trace.cc index b8793556d..0acc54d0f 100644 --- a/runtime/trace.cc +++ b/runtime/trace.cc @@ -715,8 +715,8 @@ void Trace::FinishTracing() { std::string header(os.str()); if (trace_output_mode_ == TraceOutputMode::kStreaming) { - File file; - if (!file.Open(streaming_file_name_ + ".sec", O_CREAT | O_WRONLY)) { + File file(streaming_file_name_ + ".sec", O_CREAT | O_WRONLY, true); + if (!file.IsOpened()) { LOG(WARNING) << "Could not open secondary trace file!"; return; } diff --git a/runtime/utils.cc b/runtime/utils.cc index 6a50b8eee..3f779df15 100644 --- a/runtime/utils.cc +++ b/runtime/utils.cc @@ -136,8 +136,8 @@ void GetThreadStack(pthread_t thread, void** stack_base, size_t* stack_size, siz } bool ReadFileToString(const std::string& file_name, std::string* result) { - File file; - if (!file.Open(file_name, O_RDONLY)) { + File file(file_name, O_RDONLY, false); + if (!file.IsOpened()) { return false; } @@ -155,8 +155,8 @@ bool ReadFileToString(const std::string& file_name, std::string* result) { } bool PrintFileToLog(const std::string& file_name, LogSeverity level) { - File file; - if (!file.Open(file_name, O_RDONLY)) { + File file(file_name, O_RDONLY, false); + if (!file.IsOpened()) { return false; } -- 2.11.0