Merge "Clean up reading and writing in init."

14 files changed, 259 insertions, 173 deletions

diff --git a/include/utils/file.h b/include/utils/file.h
index 8685ee2..a80afb1 100644
--- a/include/utils/file.h
+++ b/include/utils/file.h
@@ -22,8 +22,11 @@
 
 namespace android {
 
+bool ReadFdToString(int fd, std::string* content);
 bool ReadFileToString(const std::string& path, std::string* content);
+
 bool WriteStringToFile(const std::string& content, const std::string& path);
+bool WriteStringToFd(const std::string& content, int fd);
 
 #if !defined(_WIN32)
 bool WriteStringToFile(const std::string& content, const std::string& path,
diff --git a/init/Android.mk b/init/Android.mk
index bf8dea5..7f3788a 100644
--- a/init/Android.mk
+++ b/init/Android.mk
@@ -1,48 +1,55 @@
 # Copyright 2005 The Android Open Source Project
 
 LOCAL_PATH:= $(call my-dir)
-include $(CLEAR_VARS)
 
 # --
 
 ifeq ($(strip $(INIT_BOOTCHART)),true)
-LOCAL_CPPFLAGS  += -DBOOTCHART=1
+init_options += -DBOOTCHART=1
 else
-LOCAL_CPPFLAGS  += -DBOOTCHART=0
+init_options  += -DBOOTCHART=0
 endif
 
 ifneq (,$(filter userdebug eng,$(TARGET_BUILD_VARIANT)))
-LOCAL_CPPFLAGS += -DALLOW_LOCAL_PROP_OVERRIDE=1 -DALLOW_DISABLE_SELINUX=1
+init_options += -DALLOW_LOCAL_PROP_OVERRIDE=1 -DALLOW_DISABLE_SELINUX=1
 else
-LOCAL_CPPFLAGS += -DALLOW_LOCAL_PROP_OVERRIDE=0 -DALLOW_DISABLE_SELINUX=0
+init_options += -DALLOW_LOCAL_PROP_OVERRIDE=0 -DALLOW_DISABLE_SELINUX=0
 endif
 
-LOCAL_CPPFLAGS += -DLOG_UEVENTS=0
+init_options += -DLOG_UEVENTS=0
+
+init_cflags += \
+    $(init_options) \
+    -Wall -Wextra \
+    -Wno-unused-parameter \
+    -Werror \
 
 # --
 
+include $(CLEAR_VARS)
+LOCAL_CPPFLAGS := $(init_cflags)
+LOCAL_SRC_FILES:= \
+    init_parser.cpp \
+    parser.cpp \
+    util.cpp \
+
+LOCAL_MODULE := libinit
+include $(BUILD_STATIC_LIBRARY)
+
+include $(CLEAR_VARS)
+LOCAL_CPPFLAGS := $(init_cflags)
 LOCAL_SRC_FILES:= \
     bootchart.cpp \
     builtins.cpp \
     devices.cpp \
     init.cpp \
-    init_parser.cpp \
     keychords.cpp \
-    parser.cpp \
     property_service.cpp \
     signal_handler.cpp \
     ueventd.cpp \
     ueventd_parser.cpp \
-    util.cpp \
     watchdogd.cpp \
 
-#LOCAL_CLANG := true
-
-LOCAL_CPPFLAGS += \
-    -Wall -Wextra \
-    -Werror -Wno-error=deprecated-declarations \
-    -Wno-unused-parameter \
-
 LOCAL_MODULE:= init
 
 LOCAL_FORCE_STATIC_EXECUTABLE := true
@@ -50,14 +57,16 @@ LOCAL_MODULE_PATH := $(TARGET_ROOT_OUT)
 LOCAL_UNSTRIPPED_PATH := $(TARGET_ROOT_OUT_UNSTRIPPED)
 
 LOCAL_STATIC_LIBRARIES := \
-	libfs_mgr \
-	liblogwrap \
-	libcutils \
-	liblog \
-	libc \
-	libselinux \
-	libmincrypt \
-	libext4_utils_static
+    libinit \
+    libfs_mgr \
+    liblogwrap \
+    libcutils \
+    libutils \
+    liblog \
+    libc \
+    libselinux \
+    libmincrypt \
+    libext4_utils_static
 
 # Create symlinks
 LOCAL_POST_INSTALL_CMD := $(hide) mkdir -p $(TARGET_ROOT_OUT)/sbin; \
@@ -65,3 +74,18 @@ LOCAL_POST_INSTALL_CMD := $(hide) mkdir -p $(TARGET_ROOT_OUT)/sbin; \
     ln -sf ../init $(TARGET_ROOT_OUT)/sbin/watchdogd
 
 include $(BUILD_EXECUTABLE)
+
+
+
+
+include $(CLEAR_VARS)
+LOCAL_MODULE := init_tests
+LOCAL_SRC_FILES := \
+    util_test.cpp \
+
+LOCAL_SHARED_LIBRARIES += \
+    libcutils \
+    libutils \
+
+LOCAL_STATIC_LIBRARIES := libinit
+include $(BUILD_NATIVE_TEST)
diff --git a/init/builtins.cpp b/init/builtins.cpp
index 9ead340..31c6a99 100644
--- a/init/builtins.cpp
+++ b/init/builtins.cpp
@@ -56,42 +56,15 @@ int add_environment(const char *name, const char *value);
 // System call provided by bionic but not in any header file.
 extern "C" int init_module(void *, unsigned long, const char *);
 
-static int write_file(const char *path, const char *value)
-{
-    int fd, ret, len;
-
-    fd = open(path, O_WRONLY|O_CREAT|O_NOFOLLOW|O_CLOEXEC, 0600);
-
-    if (fd < 0)
-        return -errno;
-
-    len = strlen(value);
-
-    ret = TEMP_FAILURE_RETRY(write(fd, value, len));
-
-    close(fd);
-    if (ret < 0) {
-        return -errno;
-    } else {
-        return 0;
-    }
-}
-
 static int insmod(const char *filename, char *options)
 {
-    void *module;
-    unsigned size;
-    int ret;
-
-    module = read_file(filename, &size);
-    if (!module)
+    std::string module;
+    if (!read_file(filename, &module)) {
         return -1;
+    }
 
-    ret = init_module(module, size, options);
-
-    free(module);
-
-    return ret;
+    // TODO: use finit_module for >= 3.8 kernels.
+    return init_module(&module[0], module.size(), options);
 }
 
 static int setkey(struct kbentry *kbe)
@@ -743,15 +716,13 @@ int do_write(int nargs, char **args)
 {
     const char *path = args[1];
     const char *value = args[2];
-    char prop_val[PROP_VALUE_MAX];
-    int ret;
 
-    ret = expand_props(prop_val, value, sizeof(prop_val));
-    if (ret) {
+    char expanded_value[PROP_VALUE_MAX];
+    if (expand_props(expanded_value, value, sizeof(expanded_value))) {
         ERROR("cannot expand '%s' while writing to '%s'\n", value, path);
         return -EINVAL;
     }
-    return write_file(path, prop_val);
+    return write_file(path, expanded_value);
 }
 
 int do_copy(int nargs, char **args)
diff --git a/init/devices.cpp b/init/devices.cpp
index b55933c..9275439 100644
--- a/init/devices.cpp
+++ b/init/devices.cpp
@@ -48,8 +48,6 @@
 #include "util.h"
 #include "log.h"
 
-#define UNUSED __attribute__((__unused__))
-
 #define SYSFS_PREFIX    "/sys"
 #define FIRMWARE_DIR1   "/etc/firmware"
 #define FIRMWARE_DIR2   "/vendor/firmware"
@@ -231,7 +229,7 @@ static mode_t get_device_perm(const char *path, const char **links,
 }
 
 static void make_device(const char *path,
-                        const char *upath UNUSED,
+                        const char */*upath*/,
                         int block, int major, int minor,
                         const char **links)
 {
@@ -652,11 +650,6 @@ static void mkdir_recursive_for_devpath(const char *devpath)
     mkdir_recursive(dir, 0755);
 }
 
-static inline void __attribute__((__deprecated__)) kernel_logger()
-{
-    INFO("kernel logger is deprecated\n");
-}
-
 static void handle_generic_device_event(struct uevent *uevent)
 {
     const char *base;
@@ -743,7 +736,7 @@ static void handle_generic_device_event(struct uevent *uevent)
          make_dir(base, 0755);
      } else if(!strncmp(uevent->subsystem, "misc", 4) &&
                  !strncmp(name, "log_", 4)) {
-         kernel_logger();
+         INFO("kernel logger is deprecated\n");
          base = "/dev/log/";
          make_dir(base, 0755);
          name += 4;
diff --git a/init/init.cpp b/init/init.cpp
index da3fe96..3e3be2e 100644
--- a/init/init.cpp
+++ b/init/init.cpp
@@ -940,7 +940,7 @@ int selinux_reload_policy(void)
     return 0;
 }
 
-static int audit_callback(void *data, security_class_t cls __attribute__((unused)), char *buf, size_t len)
+static int audit_callback(void *data, security_class_t /*cls*/, char *buf, size_t len)
 {
     snprintf(buf, len, "property=%s", !data ? "NULL" : (char *)data);
     return 0;
@@ -1058,7 +1058,6 @@ int main(int argc, char **argv)
     INFO("property init\n");
     property_load_boot_defaults();
 
-    INFO("reading config file\n");
     init_parse_config_file("/init.rc");
 
     action_for_each_trigger("early-init", action_add_queue_tail);
@@ -1088,7 +1087,6 @@ int main(int argc, char **argv)
     /* run all property triggers based on current state of the properties */
     queue_builtin_action(queue_property_triggers_action, "queue_property_triggers");
 
-
     if (BOOTCHART) {
         queue_builtin_action(bootchart_init_action, "bootchart_init");
     }
diff --git a/init/init_parser.cpp b/init/init_parser.cpp
index facb0cf..65fc1a6 100644
--- a/init/init_parser.cpp
+++ b/init/init_parser.cpp
@@ -116,7 +116,7 @@ static int lookup_keyword(const char *s)
 {
     switch (*s++) {
     case 'c':
-    if (!strcmp(s, "opy")) return K_copy;
+        if (!strcmp(s, "opy")) return K_copy;
         if (!strcmp(s, "apability")) return K_capability;
         if (!strcmp(s, "hdir")) return K_chdir;
         if (!strcmp(s, "hroot")) return K_chroot;
@@ -171,6 +171,7 @@ static int lookup_keyword(const char *s)
         break;
     case 'p':
         if (!strcmp(s, "owerctl")) return K_powerctl;
+        break;
     case 'r':
         if (!strcmp(s, "estart")) return K_restart;
         if (!strcmp(s, "estorecon")) return K_restorecon;
@@ -209,8 +210,7 @@ static int lookup_keyword(const char *s)
     return K_UNKNOWN;
 }
 
-static void parse_line_no_op(struct parse_state *state, int nargs, char **args)
-{
+static void parse_line_no_op(struct parse_state*, int, char**) {
 }
 
 static int push_chars(char **dst, int *len, const char *chars, int cnt)
@@ -378,7 +378,7 @@ static void parse_new_section(struct parse_state *state, int kw,
     state->parse_line = parse_line_no_op;
 }
 
-static void parse_config(const char *fn, char *s)
+static void parse_config(const char *fn, const std::string& data)
 {
     struct parse_state state;
     struct listnode import_list;
@@ -389,7 +389,7 @@ static void parse_config(const char *fn, char *s)
     nargs = 0;
     state.filename = fn;
     state.line = 0;
-    state.ptr = s;
+    state.ptr = strdup(data.c_str());  // TODO: fix this code!
     state.nexttoken = 0;
     state.parse_line = parse_line_no_op;
 
@@ -427,7 +427,6 @@ parser_done:
          struct import *import = node_to_item(node, struct import, list);
          int ret;
 
-         INFO("importing '%s'", import->filename);
          ret = init_parse_config_file(import->filename);
          if (ret)
              ERROR("could not import file '%s' from '%s'\n",
@@ -435,13 +434,14 @@ parser_done:
     }
 }
 
-int init_parse_config_file(const char *fn)
-{
-    char *data;
-    data = read_file(fn, 0);
-    if (!data) return -1;
+int init_parse_config_file(const char* path) {
+    INFO("Parsing %s...", path);
+    std::string data;
+    if (!read_file(path, &data)) {
+        return -1;
+    }
 
-    parse_config(fn, data);
+    parse_config(path, data);
     dump_parser_state();
     return 0;
 }
diff --git a/init/property_service.cpp b/init/property_service.cpp
index cc1ee34..05c03d6 100644
--- a/init/property_service.cpp
+++ b/init/property_service.cpp
@@ -30,6 +30,8 @@
 #include <cutils/sockets.h>
 #include <cutils/multiuser.h>
 
+#include <utils/file.h>
+
 #define _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_
 #include <sys/_system_properties.h>
 
@@ -416,14 +418,9 @@ static void load_properties(char *data, const char *filter)
  */
 static void load_properties_from_file(const char *fn, const char *filter)
 {
-    char *data;
-    unsigned sz;
-
-    data = read_file(fn, &sz);
-
-    if(data != 0) {
-        load_properties(data, filter);
-        free(data);
+    std::string data;
+    if (read_file(fn, &data)) {
+        load_properties(&data[0], filter);
     }
 }
 
diff --git a/init/readme.txt b/init/readme.txt
index e5406ed..16a9186 100644
--- a/init/readme.txt
+++ b/init/readme.txt
@@ -191,10 +191,12 @@ enable <servicename>
      on property:ro.boot.myfancyhardware=1
         enable my_fancy_service_for_my_fancy_hardware
 
-
 insmod <path>
    Install the module at <path>
 
+loglevel <level>
+   Sets the kernel log level to level. Properties are expanded within <level>.
+
 mkdir <path> [mode] [owner] [group]
    Create a directory at <path>, optionally with the given mode, owner, and
    group. If not provided, the directory is created with permissions 755 and
@@ -229,7 +231,8 @@ setkey
    TBD
 
 setprop <name> <value>
-   Set system property <name> to <value>.
+   Set system property <name> to <value>. Properties are expanded
+   within <value>.
 
 setrlimit <resource> <cur> <max>
    Set the rlimit for a resource.
@@ -259,9 +262,10 @@ wait <path> [ <timeout> ]
   or the timeout has been reached. If timeout is not specified it
   currently defaults to five seconds.
 
-write <path> <string>
-   Open the file at <path> and write a string to it with write(2)
-   without appending.
+write <path> <content>
+   Open the file at <path> and write a string to it with write(2).
+   If the file does not exist, it will be created. If it does exist,
+   it will be truncated. Properties are expanded within <content>.
 
 
 Properties
@@ -338,8 +342,23 @@ Debugging notes
 ---------------
 By default, programs executed by init will drop stdout and stderr into
 /dev/null. To help with debugging, you can execute your program via the
-Andoird program logwrapper. This will redirect stdout/stderr into the
+Android program logwrapper. This will redirect stdout/stderr into the
 Android logging system (accessed via logcat).
 
 For example
 service akmd /system/bin/logwrapper /sbin/akmd
+
+For quicker turnaround when working on init itself, use:
+
+  mm
+  m ramdisk-nodeps
+  m bootimage-nodeps
+  adb reboot bootloader
+  fastboot boot $ANDROID_PRODUCT_OUT/boot.img
+
+Alternatively, use the emulator:
+
+  emulator -partition-size 1024 -verbose -show-kernel -no-window
+
+You might want to call klog_set_level(6) after the klog_init() call
+so you see the kernel logging in dmesg (or the emulator output).
diff --git a/init/ueventd_parser.cpp b/init/ueventd_parser.cpp
index 2ae251f..7a4841f 100644
--- a/init/ueventd_parser.cpp
+++ b/init/ueventd_parser.cpp
@@ -67,9 +67,7 @@ static int lookup_keyword(const char *s)
     return K_UNKNOWN;
 }
 
-static void parse_line_no_op(struct parse_state *state __attribute__((unused)),
-        int nargs __attribute__((unused)), char **args  __attribute__((unused)))
-{
+static void parse_line_no_op(struct parse_state*, int, char**) {
 }
 
 static int valid_name(const char *name)
@@ -97,9 +95,7 @@ struct ueventd_subsystem *ueventd_subsystem_find_by_name(const char *name)
     return 0;
 }
 
-static void *parse_subsystem(struct parse_state *state,
-        int nargs __attribute__((unused)), char **args)
-{
+static void *parse_subsystem(parse_state* state, int /*nargs*/, char** args) {
     if (!valid_name(args[1])) {
         parse_error(state, "invalid subsystem name '%s'\n", args[1]);
         return 0;
@@ -195,7 +191,7 @@ static void parse_line(struct parse_state *state, char **args, int nargs)
     }
 }
 
-static void parse_config(const char *fn, char *s)
+static void parse_config(const char *fn, const std::string& data)
 {
     struct parse_state state;
     char *args[UEVENTD_PARSER_MAXARGS];
@@ -203,7 +199,7 @@ static void parse_config(const char *fn, char *s)
     nargs = 0;
     state.filename = fn;
     state.line = 1;
-    state.ptr = s;
+    state.ptr = strdup(data.c_str());  // TODO: fix this code!
     state.nexttoken = 0;
     state.parse_line = parse_line_no_op;
     for (;;) {
@@ -230,17 +226,16 @@ static void parse_config(const char *fn, char *s)
 
 int ueventd_parse_config_file(const char *fn)
 {
-    char *data;
-    data = read_file(fn, 0);
-    if (!data) return -1;
+    std::string data;
+    if (!read_file(fn, &data)) {
+        return -1;
+    }
 
     parse_config(fn, data);
     dump_parser_state();
     return 0;
 }
 
-static void parse_line_device(struct parse_state *state __attribute__((unused)),
-        int nargs, char **args)
-{
+static void parse_line_device(parse_state*, int nargs, char** args) {
     set_device_permission(nargs, args);
 }
diff --git a/init/util.cpp b/init/util.cpp
index 84fe536..3dddb15 100644
--- a/init/util.cpp
+++ b/init/util.cpp
@@ -35,6 +35,8 @@
 /* for ANDROID_SOCKET_* */
 #include <cutils/sockets.h>
 
+#include <utils/file.h>
+
 #include <private/android_filesystem_config.h>
 
 #include "init.h"
@@ -146,48 +148,42 @@ out_close:
     return -1;
 }
 
-/* reads a file, making sure it is terminated with \n \0 */
-char *read_file(const char *fn, unsigned *_sz)
-{
-    char *data = NULL;
-    int sz;
-    int fd;
-    struct stat sb;
+bool read_file(const char* path, std::string* content) {
+    content->clear();
 
-    data = 0;
-    fd = open(fn, O_RDONLY|O_CLOEXEC);
-    if(fd < 0) return 0;
+    int fd = TEMP_FAILURE_RETRY(open(path, O_RDONLY|O_NOFOLLOW|O_CLOEXEC));
+    if (fd == -1) {
+        return false;
+    }
 
-    // for security reasons, disallow world-writable
-    // or group-writable files
-    if (fstat(fd, &sb) < 0) {
-        ERROR("fstat failed for '%s'\n", fn);
-        goto oops;
+    // For security reasons, disallow world-writable
+    // or group-writable files.
+    struct stat sb;
+    if (fstat(fd, &sb) == -1) {
+        ERROR("fstat failed for '%s': %s\n", path, strerror(errno));
+        return false;
     }
     if ((sb.st_mode & (S_IWGRP | S_IWOTH)) != 0) {
-        ERROR("skipping insecure file '%s'\n", fn);
-        goto oops;
+        ERROR("skipping insecure file '%s'\n", path);
+        return false;
     }
 
-    sz = lseek(fd, 0, SEEK_END);
-    if(sz < 0) goto oops;
-
-    if(lseek(fd, 0, SEEK_SET) != 0) goto oops;
-
-    data = (char*) malloc(sz + 2);
-    if(data == 0) goto oops;
-
-    if(read(fd, data, sz) != sz) goto oops;
-    close(fd);
-    data[sz] = '\n';
-    data[sz+1] = 0;
-    if(_sz) *_sz = sz;
-    return data;
+    bool okay = android::ReadFdToString(fd, content);
+    TEMP_FAILURE_RETRY(close(fd));
+    if (okay) {
+        content->append("\n", 1);
+    }
+    return okay;
+}
 
-oops:
-    close(fd);
-    free(data);
-    return 0;
+int write_file(const char* path, const char* content) {
+    int fd = TEMP_FAILURE_RETRY(open(path, O_WRONLY|O_CREAT|O_NOFOLLOW|O_CLOEXEC, 0600));
+    if (fd == -1) {
+        return -errno;
+    }
+    int result = android::WriteStringToFd(content, fd) ? 0 : -errno;
+    TEMP_FAILURE_RETRY(close(fd));
+    return result;
 }
 
 #define MAX_MTD_PARTITIONS 16
diff --git a/init/util.h b/init/util.h
index 1f9ecbe..77da3ac 100644
--- a/init/util.h
+++ b/init/util.h
@@ -20,6 +20,8 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 
+#include <string>
+
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 
 #define COLDBOOT_DONE "/dev/.coldboot_done"
@@ -27,7 +29,10 @@
 int mtd_name_to_number(const char *name);
 int create_socket(const char *name, int type, mode_t perm,
                   uid_t uid, gid_t gid, const char *socketcon);
-char *read_file(const char *fn, unsigned *_sz);
+
+bool read_file(const char* path, std::string* content);
+int write_file(const char* path, const char* content);
+
 time_t gettime(void);
 unsigned int decode_uid(const char *s);
 
diff --git a/init/util_test.cpp b/init/util_test.cpp
new file mode 100644
index 0000000..e9a1581
--- /dev/null
+++ b/init/util_test.cpp
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "util.h"
+
+#include <errno.h>
+#include <gtest/gtest.h>
+
+TEST(util, read_file_ENOENT) {
+  std::string s("hello");
+  errno = 0;
+  EXPECT_FALSE(read_file("/proc/does-not-exist", &s));
+  EXPECT_EQ(ENOENT, errno);
+  EXPECT_EQ("", s); // s was cleared.
+}
+
+TEST(util, read_file_success) {
+  std::string s("hello");
+  EXPECT_TRUE(read_file("/proc/version", &s));
+  EXPECT_GT(s.length(), 6U);
+  EXPECT_EQ('\n', s[s.length() - 1]);
+  s[5] = 0;
+  EXPECT_STREQ("Linux", s.c_str());
+}
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);
+}