Refactor AppendToFile and WriteFileDescriptor

- Unify the behavior of the windows and posix implementations of these
  functions.
- Simplify the interface by having them just indicate success or failure
  instead of making callers deal with partial writes.

BUG=418837

Signed-off-by: Chirantan Ekbote <chirantan@chromium.org>

Review URL: https://codereview.chromium.org/614893004

Cr-Commit-Position: refs/heads/master@{#298604}

6 files changed, 45 insertions, 34 deletions

diff --git a/base/files/file_util.h b/base/files/file_util.h
index feebeed..ecc0d58 100644
--- a/base/files/file_util.h
+++ b/base/files/file_util.h
@@ -326,14 +326,16 @@ BASE_EXPORT int WriteFile(const FilePath& filename, const char* data,
                           int size);
 
 #if defined(OS_POSIX)
-// Append the data to |fd|. Does not close |fd| when done.
-BASE_EXPORT int WriteFileDescriptor(const int fd, const char* data, int size);
+// Appends |data| to |fd|. Does not close |fd| when done.  Returns true iff
+// |size| bytes of |data| were written to |fd|.
+BASE_EXPORT bool WriteFileDescriptor(const int fd, const char* data, int size);
 #endif
 
-// Append the given buffer into the file. Returns the number of bytes written,
-// or -1 on error.
-BASE_EXPORT int AppendToFile(const FilePath& filename,
-                             const char* data, int size);
+// Appends |data| to |filename|.  Returns true iff |size| bytes of |data| were
+// written to |filename|.
+BASE_EXPORT bool AppendToFile(const FilePath& filename,
+                              const char* data,
+                              int size);
 
 // Gets the current working directory for the process.
 BASE_EXPORT bool GetCurrentDirectory(FilePath* path);
diff --git a/base/files/file_util_posix.cc b/base/files/file_util_posix.cc
index 07c21d1..561f5c7 100644
--- a/base/files/file_util_posix.cc
+++ b/base/files/file_util_posix.cc
@@ -682,13 +682,13 @@ int WriteFile(const FilePath& filename, const char* data, int size) {
   if (fd < 0)
     return -1;
 
-  int bytes_written = WriteFileDescriptor(fd, data, size);
+  int bytes_written = WriteFileDescriptor(fd, data, size) ? size : -1;
   if (IGNORE_EINTR(close(fd)) < 0)
     return -1;
   return bytes_written;
 }
 
-int WriteFileDescriptor(const int fd, const char* data, int size) {
+bool WriteFileDescriptor(const int fd, const char* data, int size) {
   // Allow for partial writes.
   ssize_t bytes_written_total = 0;
   for (ssize_t bytes_written_partial = 0; bytes_written_total < size;
@@ -697,22 +697,33 @@ int WriteFileDescriptor(const int fd, const char* data, int size) {
         HANDLE_EINTR(write(fd, data + bytes_written_total,
                            size - bytes_written_total));
     if (bytes_written_partial < 0)
-      return -1;
+      return false;
   }
 
-  return bytes_written_total;
+  return true;
 }
 
-int AppendToFile(const FilePath& filename, const char* data, int size) {
+bool AppendToFile(const FilePath& filename, const char* data, int size) {
   ThreadRestrictions::AssertIOAllowed();
+  bool ret = true;
   int fd = HANDLE_EINTR(open(filename.value().c_str(), O_WRONLY | O_APPEND));
-  if (fd < 0)
-    return -1;
+  if (fd < 0) {
+    VPLOG(1) << "Unable to create file " << filename.value();
+    return false;
+  }
 
-  int bytes_written = WriteFileDescriptor(fd, data, size);
-  if (IGNORE_EINTR(close(fd)) < 0)
-    return -1;
-  return bytes_written;
+  // This call will either write all of the data or return false.
+  if (!WriteFileDescriptor(fd, data, size)) {
+    VPLOG(1) << "Error while writing to file " << filename.value();
+    ret = false;
+  }
+
+  if (IGNORE_EINTR(close(fd)) < 0) {
+    VPLOG(1) << "Error while closing file " << filename.value();
+    return false;
+  }
+
+  return ret;
 }
 
 // Gets the current working directory for the process.
diff --git a/base/files/file_util_unittest.cc b/base/files/file_util_unittest.cc
index b3e345c..bd4839a 100644
--- a/base/files/file_util_unittest.cc
+++ b/base/files/file_util_unittest.cc
@@ -1977,11 +1977,10 @@ TEST_F(FileUtilTest, AppendToFile) {
   FilePath foobar(data_dir.Append(FILE_PATH_LITERAL("foobar.txt")));
 
   std::string data("hello");
-  EXPECT_EQ(-1, AppendToFile(foobar, data.c_str(), data.length()));
+  EXPECT_FALSE(AppendToFile(foobar, data.c_str(), data.size()));
   EXPECT_EQ(static_cast<int>(data.length()),
             WriteFile(foobar, data.c_str(), data.length()));
-  EXPECT_EQ(static_cast<int>(data.length()),
-            AppendToFile(foobar, data.c_str(), data.length()));
+  EXPECT_TRUE(AppendToFile(foobar, data.c_str(), data.size()));
 
   const std::wstring read_content = ReadTextFile(foobar);
   EXPECT_EQ(L"hellohello", read_content);
diff --git a/base/files/file_util_win.cc b/base/files/file_util_win.cc
index b586191..733c32c 100644
--- a/base/files/file_util_win.cc
+++ b/base/files/file_util_win.cc
@@ -644,7 +644,7 @@ int WriteFile(const FilePath& filename, const char* data, int size) {
   return -1;
 }
 
-int AppendToFile(const FilePath& filename, const char* data, int size) {
+bool AppendToFile(const FilePath& filename, const char* data, int size) {
   ThreadRestrictions::AssertIOAllowed();
   base::win::ScopedHandle file(CreateFile(filename.value().c_str(),
                                           FILE_APPEND_DATA,
@@ -654,26 +654,24 @@ int AppendToFile(const FilePath& filename, const char* data, int size) {
                                           0,
                                           NULL));
   if (!file.IsValid()) {
-    DPLOG(WARNING) << "CreateFile failed for path "
-                   << UTF16ToUTF8(filename.value());
-    return -1;
+    VPLOG(1) << "CreateFile failed for path " << UTF16ToUTF8(filename.value());
+    return false;
   }
 
   DWORD written;
   BOOL result = ::WriteFile(file.Get(), data, size, &written, NULL);
   if (result && static_cast<int>(written) == size)
-    return written;
+    return true;
 
   if (!result) {
     // WriteFile failed.
-    DPLOG(WARNING) << "writing file " << UTF16ToUTF8(filename.value())
-                   << " failed";
+    VPLOG(1) << "Writing file " << UTF16ToUTF8(filename.value()) << " failed";
   } else {
     // Didn't write all the bytes.
-    DLOG(WARNING) << "wrote" << written << " bytes to "
-                  << UTF16ToUTF8(filename.value()) << " expected " << size;
+    VPLOG(1) << "Only wrote " << written << " out of " << size << " byte(s) to "
+             << UTF16ToUTF8(filename.value());
   }
-  return -1;
+  return false;
 }
 
 // Gets the current working directory for the process.
diff --git a/base/sync_socket_posix.cc b/base/sync_socket_posix.cc
index 86b3c34..51b38a5 100644
--- a/base/sync_socket_posix.cc
+++ b/base/sync_socket_posix.cc
@@ -36,8 +36,9 @@ size_t SendHelper(SyncSocket::Handle handle,
   DCHECK_LE(length, kMaxMessageLength);
   DCHECK_NE(handle, SyncSocket::kInvalidHandle);
   const char* charbuffer = static_cast<const char*>(buffer);
-  const int len = WriteFileDescriptor(handle, charbuffer, length);
-  return len < 0 ? 0 : static_cast<size_t>(len);
+  return WriteFileDescriptor(handle, charbuffer, length)
+             ? static_cast<size_t>(length)
+             : 0;
 }
 
 bool CloseHandle(SyncSocket::Handle handle) {
diff --git a/base/test/trace_to_file.cc b/base/test/trace_to_file.cc
index 6caaf47..423f65c 100644
--- a/base/test/trace_to_file.cc
+++ b/base/test/trace_to_file.cc
@@ -67,8 +67,8 @@ void TraceToFile::AppendFileFooter() {
 }
 
 void TraceToFile::TraceOutputCallback(const std::string& data) {
-  int ret = AppendToFile(path_, data.c_str(), static_cast<int>(data.size()));
-  DCHECK_NE(-1, ret);
+  bool ret = AppendToFile(path_, data.c_str(), static_cast<int>(data.size()));
+  DCHECK(ret);
 }
 
 static void OnTraceDataCollected(