Base: Make base::File use ScopedFD on POSIX.

base/files/file_posix.cc

Index: base/files/file_posix.cc
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index a62ad98bd75974ad6f3f4548cf6648c7e8bf0702..f62491d21782b9d4791ddbb5663d98b4c00afce2 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -202,18 +202,20 @@ void File::InitializeUnsafe(const FilePath& name, uint32 flags) {
   else
     error_details_ = File::OSErrorToFileError(errno);
 
-  file_ = descriptor;
+  file_.reset(descriptor);
 }
 #endif  // !defined(OS_NACL)
 
 bool File::IsValid() const {
-  return file_ >= 0;
+  return file_.is_valid();

[rvargas (doing something else), 2014/03/17 18:38:49]

This is interesting, as the current code considers any negative value as
invalid, but ScopedFD only considers -5 to be a valid FD.

This file will not store intentionally an invalid value on file_, but the API
allows callers to instantiate this object with a FD, so it is possible to hit
this case.

Maybe we need something else on ScopedFD?

[rvargas (doing something else), 2014/03/17 18:40:46]

I meant, "ScopedFD considers -5 to be a valid FD", as in -1 is the only invalid
value.

[brettw, 2014/03/17 19:38:50]

I thought about this but it seems that the system libraries only really return
-1, so how you could get something like -5 floating around would indicate a
pretty big error. Maybe File should assert that it's positive or -1?

[rvargas (doing something else), 2014/03/17 21:39:28]

OK. I added a dcheck on that constructor.

+}
+
+PlatformFile File::GetPlatformFile() const {
+  return file_.get();
 }
 
 PlatformFile File::TakePlatformFile() {
-  PlatformFile file = file_;
-  file_ = kInvalidPlatformFileValue;
-  return file;
+  return file_.release();
 }
 
 void File::Close() {
@@ -221,17 +223,17 @@ void File::Close() {
     return;
 
   base::ThreadRestrictions::AssertIOAllowed();
-  if (!IGNORE_EINTR(close(file_)))
-    file_ = kInvalidPlatformFileValue;
+  file_.reset();
 }
 
 int64 File::Seek(Whence whence, int64 offset) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
-  if (file_ < 0 || offset < 0)
+  if (offset < 0)
     return -1;
 
-  return lseek(file_, static_cast<off_t>(offset), static_cast<int>(whence));
+  return lseek(file_.get(), static_cast<off_t>(offset),
+               static_cast<int>(whence));
 }
 
 int File::Read(int64 offset, char* data, int size) {
@@ -243,7 +245,7 @@ int File::Read(int64 offset, char* data, int size) {
   int bytes_read = 0;
   int rv;
   do {
-    rv = HANDLE_EINTR(pread(file_, data + bytes_read,
+    rv = HANDLE_EINTR(pread(file_.get(), data + bytes_read,
                             size - bytes_read, offset + bytes_read));
     if (rv <= 0)
       break;
@@ -263,7 +265,7 @@ int File::ReadAtCurrentPos(char* data, int size) {
   int bytes_read = 0;
   int rv;
   do {
-    rv = HANDLE_EINTR(read(file_, data, size));
+    rv = HANDLE_EINTR(read(file_.get(), data, size));
     if (rv <= 0)
       break;
 
@@ -277,7 +279,7 @@ int File::ReadNoBestEffort(int64 offset, char* data, int size) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
 
-  return HANDLE_EINTR(pread(file_, data, size, offset));
+  return HANDLE_EINTR(pread(file_.get(), data, size, offset));
 }
 
 int File::ReadAtCurrentPosNoBestEffort(char* data, int size) {
@@ -286,13 +288,13 @@ int File::ReadAtCurrentPosNoBestEffort(char* data, int size) {
   if (size < 0)
     return -1;
 
-  return HANDLE_EINTR(read(file_, data, size));
+  return HANDLE_EINTR(read(file_.get(), data, size));
 }
 
 int File::Write(int64 offset, const char* data, int size) {
   base::ThreadRestrictions::AssertIOAllowed();
 
-  if (IsOpenAppend(file_))
+  if (IsOpenAppend(file_.get()))
     return WriteAtCurrentPos(data, size);
 
   DCHECK(IsValid());
@@ -302,7 +304,7 @@ int File::Write(int64 offset, const char* data, int size) {
   int bytes_written = 0;
   int rv;
   do {
-    rv = HANDLE_EINTR(pwrite(file_, data + bytes_written,
+    rv = HANDLE_EINTR(pwrite(file_.get(), data + bytes_written,
                              size - bytes_written, offset + bytes_written));
     if (rv <= 0)
       break;
@@ -322,7 +324,7 @@ int File::WriteAtCurrentPos(const char* data, int size) {
   int bytes_written = 0;
   int rv;
   do {
-    rv = HANDLE_EINTR(write(file_, data, size));
+    rv = HANDLE_EINTR(write(file_.get(), data, size));
     if (rv <= 0)
       break;
 
@@ -338,14 +340,14 @@ int File::WriteAtCurrentPosNoBestEffort(const char* data, int size) {
   if (size < 0)
     return -1;
 
-  return HANDLE_EINTR(write(file_, data, size));
+  return HANDLE_EINTR(write(file_.get(), data, size));
 }
 
 int64 File::GetLength() {
   DCHECK(IsValid());
 
   stat_wrapper_t file_info;
-  if (CallFstat(file_, &file_info))
+  if (CallFstat(file_.get(), &file_info))
     return false;
 
   return file_info.st_size;
@@ -354,13 +356,13 @@ int64 File::GetLength() {
 bool File::SetLength(int64 length) {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
-  return !CallFtruncate(file_, length);
+  return !CallFtruncate(file_.get(), length);
 }
 
 bool File::Flush() {
   base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(IsValid());
-  return !CallFsync(file_);
+  return !CallFsync(file_.get());
 }
 
 bool File::SetTimes(Time last_access_time, Time last_modified_time) {
@@ -371,14 +373,14 @@ bool File::SetTimes(Time last_access_time, Time last_modified_time) {
   times[0] = last_access_time.ToTimeVal();
   times[1] = last_modified_time.ToTimeVal();
 
-  return !CallFutimes(file_, times);
+  return !CallFutimes(file_.get(), times);
 }
 
 bool File::GetInfo(Info* info) {
   DCHECK(IsValid());
 
   stat_wrapper_t file_info;
-  if (CallFstat(file_, &file_info))
+  if (CallFstat(file_.get(), &file_info))
     return false;
 
   info->is_directory = S_ISDIR(file_info.st_mode);
@@ -432,11 +434,11 @@ bool File::GetInfo(Info* info) {
 }
 
 File::Error File::Lock() {
-  return CallFctnlFlock(file_, true);
+  return CallFctnlFlock(file_.get(), true);
 }
 
 File::Error File::Unlock() {
-  return CallFctnlFlock(file_, false);
+  return CallFctnlFlock(file_.get(), false);
 }
 
 // Static.
@@ -473,8 +475,8 @@ File::Error File::OSErrorToFileError(int saved_errno) {
 }
 
 void File::SetPlatformFile(PlatformFile file) {
-  DCHECK_EQ(file_, kInvalidPlatformFileValue);
-  file_ = file;
+  DCHECK(!file_.is_valid());
+  file_.reset(file);
 }
 
 }  // namespace base