From 78592537bcc3dfa720227344b7281be553533322 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 7 Oct 2015 11:40:02 -0400 Subject: [PATCH] Add non-logging OpenFileForWrite() and OpenFileForReadAndWrite() BUG=crashpad:63 TEST=crashpad_util_test FileIO.*OpenFileFor* R=scottmg@chromium.org Review URL: https://codereview.chromium.org/1395543002 . --- util/file/file_io.h | 68 ++++++++++++++++++++++++++-------- util/file/file_io_posix.cc | 44 ++++++++++++++++------ util/file/file_io_test.cc | 75 +++++++++++++++++++++++--------------- util/file/file_io_win.cc | 70 +++++++++++++++++++++++------------ 4 files changed, 176 insertions(+), 81 deletions(-) diff --git a/util/file/file_io.h b/util/file/file_io.h index 7bc96735..ec89d62b 100644 --- a/util/file/file_io.h +++ b/util/file/file_io.h @@ -202,15 +202,17 @@ void CheckedWriteFile(FileHandle file, const void* buffer, size_t size); void CheckedReadFileAtEOF(FileHandle file); //! \brief Wraps `open()` or `CreateFile()`, opening an existing file for -//! reading. Logs an error if the operation fails. +//! reading. //! //! \return The newly opened FileHandle, or an invalid FileHandle on failure. //! //! \sa ScopedFileHandle -FileHandle LoggingOpenFileForRead(const base::FilePath& path); +//! \sa OpenFileForWrite +//! \sa OpenFileForReadAndWrite +//! \sa LoggingOpenFileForRead +FileHandle OpenFileForRead(const base::FilePath& path); -//! \brief Wraps `open()` or `CreateFile()`, creating a file for output. Logs -//! an error if the operation fails. +//! \brief Wraps `open()` or `CreateFile()`, creating a file for output. //! //! \a mode determines the style (truncate, reuse, etc.) that is used to open //! the file. On POSIX, \a permissions determines the value that is passed as @@ -220,27 +222,61 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path); //! //! \return The newly opened FileHandle, or an invalid FileHandle on failure. //! -//! \sa FileWriteMode -//! \sa FilePermissions //! \sa ScopedFileHandle +//! \sa OpenFileForRead +//! \sa OpenFileForReadAndWrite +//! \sa LoggingOpenFileForWrite +FileHandle OpenFileForWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions); + +//! \brief Wraps `open()` or `CreateFile()`, creating a file for both input and +//! output. +//! +//! \a mode determines the style (truncate, reuse, etc.) that is used to open +//! the file. On POSIX, \a permissions determines the value that is passed as +//! `mode` to `open()`. On Windows, the file is always opened in binary mode +//! (that is, no CRLF translation). On Windows, the file is opened for sharing, +//! see LoggingLockFile() and LoggingUnlockFile() to control concurrent access. +//! +//! \return The newly opened FileHandle, or an invalid FileHandle on failure. +//! +//! \sa ScopedFileHandle +//! \sa OpenFileForRead +//! \sa OpenFileForWrite +//! \sa LoggingOpenFileForReadAndWrite +FileHandle OpenFileForReadAndWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions); + +//! \brief Wraps OpenFileForRead(), logging an error if the operation fails. +//! +//! \return The newly opened FileHandle, or an invalid FileHandle on failure. +//! +//! \sa ScopedFileHandle +//! \sa LoggingOpenFileForWrite +//! \sa LoggingOpenFileForReadAndWrite +FileHandle LoggingOpenFileForRead(const base::FilePath& path); + +//! \brief Wraps OpenFileForWrite(), logging an error if the operation fails. +//! +//! \return The newly opened FileHandle, or an invalid FileHandle on failure. +//! +//! \sa ScopedFileHandle +//! \sa LoggingOpenFileForRead +//! \sa LoggingOpenFileForReadAndWrite FileHandle LoggingOpenFileForWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions); -//! \brief Wraps `open()` or `CreateFile()`, creating a file for both input and -//! output. Logs an error if the operation fails. -//! -//! \a mode determines the style (truncate, reuse, etc.) that is used to open -//! the file. On POSIX, \a permissions determines the value that is passed as -//! `mode` to `open()`. On Windows, the file is always opened in binary mode -//! (that is, no CRLF translation). On Windows, the file is opened for sharing, -//! see LoggingLockFile() and LoggingUnlockFile() to control concurrent access. +//! \brief Wraps OpenFileForReadAndWrite(), logging an error if the operation +//! fails. //! //! \return The newly opened FileHandle, or an invalid FileHandle on failure. //! -//! \sa FileWriteMode -//! \sa FilePermissions //! \sa ScopedFileHandle +//! \sa LoggingOpenFileForRead +//! \sa LoggingOpenFileForWrite FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions); diff --git a/util/file/file_io_posix.cc b/util/file/file_io_posix.cc index 29cdfec5..c0969dcf 100644 --- a/util/file/file_io_posix.cc +++ b/util/file/file_io_posix.cc @@ -71,12 +71,14 @@ namespace crashpad { namespace { -FileHandle LoggingOpenFileForOutput(int rdwr_or_wronly, - const base::FilePath& path, - FileWriteMode mode, - FilePermissions permissions) { - int flags = rdwr_or_wronly & (O_RDWR | O_WRONLY); - DCHECK_NE(flags, 0); +FileHandle OpenFileForOutput(int rdwr_or_wronly, + const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + DCHECK(rdwr_or_wronly & (O_RDWR | O_WRONLY)); + DCHECK_EQ(rdwr_or_wronly & ~(O_RDWR | O_WRONLY), 0); + + int flags = rdwr_or_wronly; switch (mode) { case FileWriteMode::kReuseOrFail: @@ -92,12 +94,10 @@ FileHandle LoggingOpenFileForOutput(int rdwr_or_wronly, break; } - int fd = HANDLE_EINTR( + return HANDLE_EINTR( open(path.value().c_str(), flags, permissions == FilePermissions::kWorldReadable ? 0644 : 0600)); - PLOG_IF(ERROR, fd < 0) << "open " << path.value(); - return fd; } } // namespace @@ -110,8 +110,24 @@ ssize_t WriteFile(FileHandle file, const void* buffer, size_t size) { return ReadOrWrite(file, buffer, size); } +FileHandle OpenFileForRead(const base::FilePath& path) { + return HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)); +} + +FileHandle OpenFileForWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + return OpenFileForOutput(O_WRONLY, path, mode, permissions); +} + +FileHandle OpenFileForReadAndWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + return OpenFileForOutput(O_RDWR, path, mode, permissions); +} + FileHandle LoggingOpenFileForRead(const base::FilePath& path) { - int fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)); + FileHandle fd = OpenFileForRead(path); PLOG_IF(ERROR, fd < 0) << "open " << path.value(); return fd; } @@ -119,13 +135,17 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path) { FileHandle LoggingOpenFileForWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions) { - return LoggingOpenFileForOutput(O_WRONLY, path, mode, permissions); + FileHandle fd = OpenFileForWrite(path, mode, permissions); + PLOG_IF(ERROR, fd < 0) << "open " << path.value(); + return fd; } FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions) { - return LoggingOpenFileForOutput(O_RDWR, path, mode, permissions); + FileHandle fd = OpenFileForReadAndWrite(path, mode, permissions); + PLOG_IF(ERROR, fd < 0) << "open " << path.value(); + return fd; } bool LoggingLockFile(FileHandle file, FileLocking locking) { diff --git a/util/file/file_io_test.cc b/util/file/file_io_test.cc index 29cf3204..77529ea5 100644 --- a/util/file/file_io_test.cc +++ b/util/file/file_io_test.cc @@ -28,29 +28,30 @@ namespace crashpad { namespace test { namespace { -TEST(FileIO, OpenFileForWrite) { +void TestOpenFileForWrite(FileHandle (*opener)(const base::FilePath&, + FileWriteMode, + FilePermissions)) { ScopedTempDir temp_dir; base::FilePath file_path_1 = temp_dir.path().Append(FILE_PATH_LITERAL("file_1")); ASSERT_FALSE(FileExists(file_path_1)); - ScopedFileHandle - file_handle(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kReuseOrFail, - FilePermissions::kWorldReadable)); + ScopedFileHandle file_handle(opener(file_path_1, + FileWriteMode::kReuseOrFail, + FilePermissions::kWorldReadable)); EXPECT_EQ(kInvalidFileHandle, file_handle); EXPECT_FALSE(FileExists(file_path_1)); - file_handle.reset(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kCreateOrFail, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_1, + FileWriteMode::kCreateOrFail, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_1)); EXPECT_EQ(0, FileSize(file_path_1)); - file_handle.reset(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kReuseOrCreate, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_1, + FileWriteMode::kReuseOrCreate, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_1)); EXPECT_EQ(0, FileSize(file_path_1)); @@ -62,30 +63,30 @@ TEST(FileIO, OpenFileForWrite) { file_handle.reset(); EXPECT_EQ(implicit_cast(sizeof(data)), FileSize(file_path_1)); - file_handle.reset(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kReuseOrCreate, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_1, + FileWriteMode::kReuseOrCreate, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_1)); EXPECT_EQ(implicit_cast(sizeof(data)), FileSize(file_path_1)); - file_handle.reset(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kCreateOrFail, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_1, + FileWriteMode::kCreateOrFail, + FilePermissions::kWorldReadable)); EXPECT_EQ(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_1)); EXPECT_EQ(implicit_cast(sizeof(data)), FileSize(file_path_1)); - file_handle.reset(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kReuseOrFail, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_1, + FileWriteMode::kReuseOrFail, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_1)); EXPECT_EQ(implicit_cast(sizeof(data)), FileSize(file_path_1)); - file_handle.reset(LoggingOpenFileForWrite(file_path_1, - FileWriteMode::kTruncateOrCreate, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_1, + FileWriteMode::kTruncateOrCreate, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_1)); EXPECT_EQ(0, FileSize(file_path_1)); @@ -94,9 +95,9 @@ TEST(FileIO, OpenFileForWrite) { temp_dir.path().Append(FILE_PATH_LITERAL("file_2")); ASSERT_FALSE(FileExists(file_path_2)); - file_handle.reset(LoggingOpenFileForWrite(file_path_2, - FileWriteMode::kTruncateOrCreate, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_2, + FileWriteMode::kTruncateOrCreate, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_2)); EXPECT_EQ(0, FileSize(file_path_2)); @@ -105,14 +106,30 @@ TEST(FileIO, OpenFileForWrite) { temp_dir.path().Append(FILE_PATH_LITERAL("file_3")); ASSERT_FALSE(FileExists(file_path_3)); - file_handle.reset(LoggingOpenFileForWrite(file_path_3, - FileWriteMode::kReuseOrCreate, - FilePermissions::kWorldReadable)); + file_handle.reset(opener(file_path_3, + FileWriteMode::kReuseOrCreate, + FilePermissions::kWorldReadable)); EXPECT_NE(kInvalidFileHandle, file_handle); EXPECT_TRUE(FileExists(file_path_3)); EXPECT_EQ(0, FileSize(file_path_3)); } +TEST(FileIO, OpenFileForWrite) { + TestOpenFileForWrite(OpenFileForWrite); +} + +TEST(FileIO, OpenFileForReadAndWrite) { + TestOpenFileForWrite(OpenFileForReadAndWrite); +} + +TEST(FileIO, LoggingOpenFileForWrite) { + TestOpenFileForWrite(LoggingOpenFileForWrite); +} + +TEST(FileIO, LoggingOpenFileForReadAndWrite) { + TestOpenFileForWrite(LoggingOpenFileForReadAndWrite); +} + enum class ReadOrWrite : bool { kRead, kWrite, diff --git a/util/file/file_io_win.cc b/util/file/file_io_win.cc index dce1518a..8b9da455 100644 --- a/util/file/file_io_win.cc +++ b/util/file/file_io_win.cc @@ -37,10 +37,13 @@ namespace crashpad { namespace { -FileHandle LoggingOpenFileForOutput(DWORD access, - const base::FilePath& path, - FileWriteMode mode, - FilePermissions permissions) { +FileHandle OpenFileForOutput(DWORD access, + const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + DCHECK(access & GENERIC_WRITE); + DCHECK_EQ(access & ~(GENERIC_READ | GENERIC_WRITE), 0u); + DWORD disposition = 0; switch (mode) { case FileWriteMode::kReuseOrFail: @@ -56,16 +59,13 @@ FileHandle LoggingOpenFileForOutput(DWORD access, disposition = CREATE_NEW; break; } - HANDLE file = CreateFile(path.value().c_str(), - access, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - disposition, - FILE_ATTRIBUTE_NORMAL, - nullptr); - PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) - << "CreateFile " << base::UTF16ToUTF8(path.value()); - return file; + return CreateFile(path.value().c_str(), + access, + FILE_SHARE_READ | FILE_SHARE_WRITE, + nullptr, + disposition, + FILE_ATTRIBUTE_NORMAL, + nullptr); } } // namespace @@ -115,14 +115,31 @@ ssize_t WriteFile(FileHandle file, const void* buffer, size_t size) { return bytes_written; } +FileHandle OpenFileForRead(const base::FilePath& path) { + return CreateFile(path.value().c_str(), + GENERIC_READ, + FILE_SHARE_READ | FILE_SHARE_WRITE, + nullptr, + OPEN_EXISTING, + 0, + nullptr); +} + +FileHandle OpenFileForWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + return OpenFileForOutput(GENERIC_WRITE, path, mode, permissions); +} + +FileHandle OpenFileForReadAndWrite(const base::FilePath& path, + FileWriteMode mode, + FilePermissions permissions) { + return OpenFileForOutput( + GENERIC_READ | GENERIC_WRITE, path, mode, permissions); +} + FileHandle LoggingOpenFileForRead(const base::FilePath& path) { - HANDLE file = CreateFile(path.value().c_str(), - GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE, - nullptr, - OPEN_EXISTING, - 0, - nullptr); + FileHandle file = OpenFileForRead(path); PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) << "CreateFile " << base::UTF16ToUTF8(path.value()); return file; @@ -131,14 +148,19 @@ FileHandle LoggingOpenFileForRead(const base::FilePath& path) { FileHandle LoggingOpenFileForWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions) { - return LoggingOpenFileForOutput(GENERIC_WRITE, path, mode, permissions); + FileHandle file = OpenFileForWrite(path, mode, permissions); + PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) + << "CreateFile " << base::UTF16ToUTF8(path.value()); + return file; } FileHandle LoggingOpenFileForReadAndWrite(const base::FilePath& path, FileWriteMode mode, FilePermissions permissions) { - return LoggingOpenFileForOutput( - GENERIC_READ | GENERIC_WRITE, path, mode, permissions); + FileHandle file = OpenFileForReadAndWrite(path, mode, permissions); + PLOG_IF(ERROR, file == INVALID_HANDLE_VALUE) + << "CreateFile " << base::UTF16ToUTF8(path.value()); + return file; } bool LoggingLockFile(FileHandle file, FileLocking locking) {