Convert base::MemoryMappedFile to use File instead of PlatformFile.
This also modifies consumers of MemoryMappedFile and fixes double handle
close on MediaFileChecker, media_file_checker_unittest and data_pack_unittests.
BUG=322664
TEST=unit tests
R=cpu@chromium.org, dalecurtis@chromium.org (media)
TBR (owners):
tony@chromium.org (resource)
jochen@chromium.org (chrome-content)
thakis@chromium.org (base)
Review URL: https://codereview.chromium.org/109273002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@242937 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 58ba3eab5af91f112de6a26856f07cdd46cde35e
diff --git a/base/files/file.cc b/base/files/file.cc
index 4902f15..508103a 100644
--- a/base/files/file.cc
+++ b/base/files/file.cc
@@ -31,14 +31,17 @@
error_(FILE_OK),
created_(false),
async_(false) {
- if (name.ReferencesParent()) {
- error_ = FILE_ERROR_ACCESS_DENIED;
- return;
- }
- CreateBaseFileUnsafe(name, flags);
+ Initialize(name, flags);
}
#endif
+File::File(PlatformFile platform_file)
+ : file_(platform_file),
+ error_(FILE_OK),
+ created_(false),
+ async_(false) {
+}
+
File::File(RValue other)
: file_(other.object->TakePlatformFile()),
error_(other.object->error()),
@@ -61,4 +64,14 @@
return *this;
}
+#if !defined(OS_NACL)
+void File::Initialize(const FilePath& name, uint32 flags) {
+ if (name.ReferencesParent()) {
+ error_ = FILE_ERROR_ACCESS_DENIED;
+ return;
+ }
+ InitializeUnsafe(name, flags);
+}
+#endif
+
} // namespace base
diff --git a/base/files/file.h b/base/files/file.h
index d1e0e8c..e87dd7b 100644
--- a/base/files/file.h
+++ b/base/files/file.h
@@ -157,9 +157,12 @@
// Move operator= for C++03 move emulation of this type.
File& operator=(RValue other);
+ // Creates or opens the given file.
+ void Initialize(const FilePath& name, uint32 flags);
+
// Creates or opens the given file, allowing paths with traversal ('..')
// components. Use only with extreme care.
- void CreateBaseFileUnsafe(const FilePath& name, uint32 flags);
+ void InitializeUnsafe(const FilePath& name, uint32 flags);
bool IsValid() const;
@@ -216,10 +219,13 @@
// platforms. Returns the number of bytes written, or -1 on error.
int WriteAtCurrentPosNoBestEffort(const char* data, int size);
+ // Returns the current size of this file, or a negative number on failure.
+ int64 GetLength();
+
// Truncates the file to the given length. If |length| is greater than the
// current size of the file, the file is extended with zeros. If the file
// doesn't exist, |false| is returned.
- bool Truncate(int64 length);
+ bool SetLength(int64 length);
// Flushes the buffers.
bool Flush();
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index 9d97c33..e8b4d77 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -122,7 +122,7 @@
// NaCl doesn't implement system calls to open files directly.
#if !defined(OS_NACL)
// TODO(erikkay): does it make sense to support FLAG_EXCLUSIVE_* here?
-void File::CreateBaseFileUnsafe(const FilePath& name, uint32 flags) {
+void File::InitializeUnsafe(const FilePath& name, uint32 flags) {
base::ThreadRestrictions::AssertIOAllowed();
DCHECK(!IsValid());
DCHECK(!(flags & FLAG_ASYNC));
@@ -341,7 +341,17 @@
return HANDLE_EINTR(write(file_, data, size));
}
-bool File::Truncate(int64 length) {
+int64 File::GetLength() {
+ DCHECK(IsValid());
+
+ stat_wrapper_t file_info;
+ if (CallFstat(file_, &file_info))
+ return false;
+
+ return file_info.st_size;
+}
+
+bool File::SetLength(int64 length) {
base::ThreadRestrictions::AssertIOAllowed();
DCHECK(IsValid());
return !CallFtruncate(file_, length);
diff --git a/base/files/file_unittest.cc b/base/files/file_unittest.cc
index b2e855d..16eece1 100644
--- a/base/files/file_unittest.cc
+++ b/base/files/file_unittest.cc
@@ -44,6 +44,19 @@
}
{
+ // Open an existing file through Initialize
+ File file;
+ file.Initialize(file_path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+ EXPECT_TRUE(file.IsValid());
+ EXPECT_FALSE(file.created());
+ EXPECT_EQ(base::File::FILE_OK, file.error());
+
+ // This time verify closing the file.
+ file.Close();
+ EXPECT_FALSE(file.IsValid());
+ }
+
+ {
// Create a file that exists.
File file(file_path, base::File::FLAG_CREATE | base::File::FLAG_READ);
EXPECT_FALSE(file.IsValid());
@@ -221,7 +234,7 @@
}
-TEST(File, Truncate) {
+TEST(File, Length) {
base::ScopedTempDir temp_dir;
ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
FilePath file_path = temp_dir.path().AppendASCII("truncate_file");
@@ -229,6 +242,7 @@
base::File::FLAG_CREATE | base::File::FLAG_READ |
base::File::FLAG_WRITE);
ASSERT_TRUE(file.IsValid());
+ EXPECT_EQ(0, file.GetLength());
// Write "test" to the file.
char data_to_write[] = "test";
@@ -239,7 +253,8 @@
// Extend the file.
const int kExtendedFileLength = 10;
int64 file_size = 0;
- EXPECT_TRUE(file.Truncate(kExtendedFileLength));
+ EXPECT_TRUE(file.SetLength(kExtendedFileLength));
+ EXPECT_EQ(kExtendedFileLength, file.GetLength());
EXPECT_TRUE(GetFileSize(file_path, &file_size));
EXPECT_EQ(kExtendedFileLength, file_size);
@@ -254,7 +269,8 @@
// Truncate the file.
const int kTruncatedFileLength = 2;
- EXPECT_TRUE(file.Truncate(kTruncatedFileLength));
+ EXPECT_TRUE(file.SetLength(kTruncatedFileLength));
+ EXPECT_EQ(kTruncatedFileLength, file.GetLength());
EXPECT_TRUE(GetFileSize(file_path, &file_size));
EXPECT_EQ(kTruncatedFileLength, file_size);
diff --git a/base/files/memory_mapped_file.cc b/base/files/memory_mapped_file.cc
index a48ec0c..ace4e11 100644
--- a/base/files/memory_mapped_file.cc
+++ b/base/files/memory_mapped_file.cc
@@ -17,7 +17,14 @@
if (IsValid())
return false;
- if (!MapFileToMemory(file_name)) {
+ file_.Initialize(file_name, File::FLAG_OPEN | File::FLAG_READ);
+
+ if (!file_.IsValid()) {
+ DLOG(ERROR) << "Couldn't open " << file_name.AsUTF8Unsafe();
+ return false;
+ }
+
+ if (!MapFileToMemory()) {
CloseHandles();
return false;
}
@@ -25,13 +32,13 @@
return true;
}
-bool MemoryMappedFile::Initialize(PlatformFile file) {
+bool MemoryMappedFile::Initialize(File file) {
if (IsValid())
return false;
- file_ = file;
+ file_ = file.Pass();
- if (!MapFileToMemoryInternal()) {
+ if (!MapFileToMemory()) {
CloseHandles();
return false;
}
@@ -43,16 +50,4 @@
return data_ != NULL;
}
-bool MemoryMappedFile::MapFileToMemory(const FilePath& file_name) {
- file_ = CreatePlatformFile(file_name, PLATFORM_FILE_OPEN | PLATFORM_FILE_READ,
- NULL, NULL);
-
- if (file_ == kInvalidPlatformFileValue) {
- DLOG(ERROR) << "Couldn't open " << file_name.AsUTF8Unsafe();
- return false;
- }
-
- return MapFileToMemoryInternal();
-}
-
} // namespace base
diff --git a/base/files/memory_mapped_file.h b/base/files/memory_mapped_file.h
index 6df1bad..b02d8cf 100644
--- a/base/files/memory_mapped_file.h
+++ b/base/files/memory_mapped_file.h
@@ -7,7 +7,7 @@
#include "base/base_export.h"
#include "base/basictypes.h"
-#include "base/platform_file.h"
+#include "base/files/file.h"
#include "build/build_config.h"
#if defined(OS_WIN)
@@ -30,9 +30,10 @@
// the file does not exist, or the memory mapping fails, it will return false.
// Later we may want to allow the user to specify access.
bool Initialize(const FilePath& file_name);
- // As above, but works with an already-opened file. MemoryMappedFile will take
- // ownership of |file| and close it when done.
- bool Initialize(PlatformFile file);
+
+ // As above, but works with an already-opened file. MemoryMappedFile takes
+ // ownership of |file| and closes it when done.
+ bool Initialize(File file);
#if defined(OS_WIN)
// Opens an existing file and maps it as an image section. Please refer to
@@ -47,27 +48,22 @@
bool IsValid() const;
private:
- // Open the given file and pass it to MapFileToMemoryInternal().
- bool MapFileToMemory(const FilePath& file_name);
-
// Map the file to memory, set data_ to that memory address. Return true on
// success, false on any kind of failure. This is a helper for Initialize().
- bool MapFileToMemoryInternal();
+ bool MapFileToMemory();
- // Closes all open handles. Later we may want to make this public.
+ // Closes all open handles.
void CloseHandles();
-#if defined(OS_WIN)
- // MapFileToMemoryInternal calls this function. It provides the ability to
- // pass in flags which control the mapped section.
- bool MapFileToMemoryInternalEx(int flags);
-
- HANDLE file_mapping_;
-#endif
- PlatformFile file_;
+ File file_;
uint8* data_;
size_t length_;
+#if defined(OS_WIN)
+ win::ScopedHandle file_mapping_;
+ bool image_; // Map as an image.
+#endif
+
DISALLOW_COPY_AND_ASSIGN(MemoryMappedFile);
};
diff --git a/base/files/memory_mapped_file_posix.cc b/base/files/memory_mapped_file_posix.cc
index c4c477a..5d7e007 100644
--- a/base/files/memory_mapped_file_posix.cc
+++ b/base/files/memory_mapped_file_posix.cc
@@ -13,26 +13,23 @@
namespace base {
-MemoryMappedFile::MemoryMappedFile()
- : file_(kInvalidPlatformFileValue),
- data_(NULL),
- length_(0) {
+MemoryMappedFile::MemoryMappedFile() : data_(NULL), length_(0) {
}
-bool MemoryMappedFile::MapFileToMemoryInternal() {
+bool MemoryMappedFile::MapFileToMemory() {
ThreadRestrictions::AssertIOAllowed();
struct stat file_stat;
- if (fstat(file_, &file_stat) == kInvalidPlatformFileValue) {
- DPLOG(ERROR) << "fstat " << file_;
+ if (fstat(file_.GetPlatformFile(), &file_stat) == -1 ) {
+ DPLOG(ERROR) << "fstat " << file_.GetPlatformFile();
return false;
}
length_ = file_stat.st_size;
data_ = static_cast<uint8*>(
- mmap(NULL, length_, PROT_READ, MAP_SHARED, file_, 0));
+ mmap(NULL, length_, PROT_READ, MAP_SHARED, file_.GetPlatformFile(), 0));
if (data_ == MAP_FAILED)
- DPLOG(ERROR) << "mmap " << file_;
+ DPLOG(ERROR) << "mmap " << file_.GetPlatformFile();
return data_ != MAP_FAILED;
}
@@ -42,12 +39,10 @@
if (data_ != NULL)
munmap(data_, length_);
- if (file_ != kInvalidPlatformFileValue)
- close(file_);
+ file_.Close();
data_ = NULL;
length_ = 0;
- file_ = kInvalidPlatformFileValue;
}
} // namespace base