Clean up reading and writing in init.

This isn't particularly useful in and of itself, but it does introduce the
first (trivial) unit test, improves the documentation (including details
about how to debug init crashes), and made me aware of how unpleasant the
existing parser is.

I also fixed a bug in passing --- unless you thought the "peboot" and "pm"
commands were features...

Bug: 19217569
Change-Id: I6ab76129a543ce3ed3dab52ef2c638009874c3de

2 files changed, 72 insertions, 24 deletions

diff --git a/libutils/file.cpp b/libutils/file.cpp
index 5b1ce88..577df78 100644
--- a/libutils/file.cpp
+++ b/libutils/file.cpp
@@ -23,6 +23,17 @@
 
 #include <utils/Compat.h> // For TEMP_FAILURE_RETRY on Darwin.
 
+bool android::ReadFdToString(int fd, std::string* content) {
+  content->clear();
+
+  char buf[BUFSIZ];
+  ssize_t n;
+  while ((n = TEMP_FAILURE_RETRY(read(fd, &buf[0], sizeof(buf)))) > 0) {
+    content->append(buf, n);
+  }
+  return (n == 0) ? true : false;
+}
+
 bool android::ReadFileToString(const std::string& path, std::string* content) {
   content->clear();
 
@@ -30,35 +41,22 @@ bool android::ReadFileToString(const std::string& path, std::string* content) {
   if (fd == -1) {
     return false;
   }
-
-  while (true) {
-    char buf[BUFSIZ];
-    ssize_t n = TEMP_FAILURE_RETRY(read(fd, &buf[0], sizeof(buf)));
-    if (n == -1) {
-      TEMP_FAILURE_RETRY(close(fd));
-      return false;
-    }
-    if (n == 0) {
-      TEMP_FAILURE_RETRY(close(fd));
-      return true;
-    }
-    content->append(buf, n);
-  }
+  bool result = ReadFdToString(fd, content);
+  TEMP_FAILURE_RETRY(close(fd));
+  return result;
 }
 
-static bool WriteStringToFd(const std::string& content, int fd) {
+bool android::WriteStringToFd(const std::string& content, int fd) {
   const char* p = content.data();
   size_t left = content.size();
   while (left > 0) {
     ssize_t n = TEMP_FAILURE_RETRY(write(fd, p, left));
     if (n == -1) {
-      TEMP_FAILURE_RETRY(close(fd));
       return false;
     }
     p += n;
     left -= n;
   }
-  TEMP_FAILURE_RETRY(close(fd));
   return true;
 }
 
@@ -79,12 +77,12 @@ bool android::WriteStringToFile(const std::string& content, const std::string& p
   if (fd == -1) {
     return false;
   }
+
   // We do an explicit fchmod here because we assume that the caller really meant what they
   // said and doesn't want the umask-influenced mode.
-  if (fchmod(fd, mode) != -1 && fchown(fd, owner, group) == -1 && WriteStringToFd(content, fd)) {
-    return true;
-  }
-  return CleanUpAfterFailedWrite(path);
+  bool result = (fchmod(fd, mode) != -1 && fchown(fd, owner, group) == -1 && WriteStringToFd(content, fd));
+  TEMP_FAILURE_RETRY(close(fd));
+  return result || CleanUpAfterFailedWrite(path);
 }
 #endif
 
@@ -95,5 +93,8 @@ bool android::WriteStringToFile(const std::string& content, const std::string& p
   if (fd == -1) {
     return false;
   }
-  return WriteStringToFd(content, fd) || CleanUpAfterFailedWrite(path);
+
+  bool result = WriteStringToFd(content, fd);
+  TEMP_FAILURE_RETRY(close(fd));
+  return result || CleanUpAfterFailedWrite(path);
 }
diff --git a/libutils/tests/file_test.cpp b/libutils/tests/file_test.cpp
index acf66a6..3703a49 100644
--- a/libutils/tests/file_test.cpp
+++ b/libutils/tests/file_test.cpp
@@ -17,21 +17,68 @@
 #include "utils/file.h"
 
 #include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
 #include <gtest/gtest.h>
 
+class TemporaryFile {
+ public:
+  TemporaryFile() {
+    init("/data/local/tmp");
+    if (fd == -1) {
+      init("/tmp");
+    }
+  }
+
+  ~TemporaryFile() {
+    close(fd);
+    unlink(filename);
+  }
+
+  int fd;
+  char filename[1024];
+
+ private:
+  void init(const char* tmp_dir) {
+    snprintf(filename, sizeof(filename), "%s/TemporaryFile-XXXXXX", tmp_dir);
+    fd = mkstemp(filename);
+  }
+};
+
 TEST(file, ReadFileToString_ENOENT) {
   std::string s("hello");
   errno = 0;
-  EXPECT_FALSE(android::ReadFileToString("/proc/does-not-exist", &s));
+  ASSERT_FALSE(android::ReadFileToString("/proc/does-not-exist", &s));
   EXPECT_EQ(ENOENT, errno);
   EXPECT_EQ("", s); // s was cleared.
 }
 
 TEST(file, ReadFileToString_success) {
   std::string s("hello");
-  EXPECT_TRUE(android::ReadFileToString("/proc/version", &s));
+  ASSERT_TRUE(android::ReadFileToString("/proc/version", &s)) << errno;
   EXPECT_GT(s.length(), 6U);
   EXPECT_EQ('\n', s[s.length() - 1]);
   s[5] = 0;
   EXPECT_STREQ("Linux", s.c_str());
 }
+
+TEST(file, WriteStringToFile) {
+  TemporaryFile tf;
+  ASSERT_TRUE(tf.fd != -1);
+  ASSERT_TRUE(android::WriteStringToFile("abc", tf.filename)) << errno;
+  std::string s;
+  ASSERT_TRUE(android::ReadFileToString(tf.filename, &s)) << errno;
+  EXPECT_EQ("abc", s);
+}
+
+TEST(file, WriteStringToFd) {
+  TemporaryFile tf;
+  ASSERT_TRUE(tf.fd != -1);
+  ASSERT_TRUE(android::WriteStringToFd("abc", tf.fd));
+
+  ASSERT_EQ(0, lseek(tf.fd, 0, SEEK_SET)) << errno;
+
+  std::string s;
+  ASSERT_TRUE(android::ReadFdToString(tf.fd, &s)) << errno;
+  EXPECT_EQ("abc", s);
+}