[Chromecast] Remove usage of nonreentrant functions.

chromecast/crash/linux/synchronized_minidump_manager.cc

Index: chromecast/crash/linux/synchronized_minidump_manager.cc
diff --git a/chromecast/crash/linux/synchronized_minidump_manager.cc b/chromecast/crash/linux/synchronized_minidump_manager.cc
index 523916d349b896cd297d461697e2efae350b3e7d..f87978ee187a5e7ee26b56c7ff4e18b6f2370038 100644
--- a/chromecast/crash/linux/synchronized_minidump_manager.cc
+++ b/chromecast/crash/linux/synchronized_minidump_manager.cc
@@ -4,17 +4,11 @@
 
 #include "chromecast/crash/linux/synchronized_minidump_manager.h"
 
-#include <dirent.h>
 #include <errno.h>
-#include <fcntl.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <string.h>
-#include <sys/file.h>
-#include <sys/stat.h>
 #include <sys/types.h>

[bcf, 2016/08/05 03:33:23]

This is unused now right?

[ameyak, 2016/08/05 18:45:06]

We still used size_t, but I have removed its usage and replaced it with
uint64_t.

-#include <time.h>
-#include <unistd.h>
 
 #include <utility>
 
@@ -22,8 +16,10 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
+#include "chromecast/base/file_utils.h"
 #include "chromecast/base/path_utils.h"
 #include "chromecast/base/serializers.h"
 #include "chromecast/crash/linux/dump_info.h"
@@ -40,8 +36,6 @@ namespace chromecast {
 
 namespace {
 
-const mode_t kDirMode = 0770;
-const mode_t kFileMode = 0660;
 const char kLockfileName[] = "lockfile";
 const char kMetadataName[] = "metadata";
 const char kMinidumpsDir[] = "minidumps";
@@ -65,39 +59,30 @@ base::DictionaryValue* GetRatelimitParams(base::Value* metadata) {
 }
 
 // Returns the time of the current ratelimit period's start in |metadata|.
-// Returns (time_t)-1 if an error occurs.
-time_t GetRatelimitPeriodStart(base::Value* metadata) {
-  time_t result = static_cast<time_t>(-1);
+// Returns base::Time() if an error occurs.
+base::Time GetRatelimitPeriodStart(base::Value* metadata) {
   base::DictionaryValue* ratelimit_params = GetRatelimitParams(metadata);
-  RCHECK(ratelimit_params, result);
+  RCHECK(ratelimit_params, base::Time());
 
-  std::string period_start_str;
-  RCHECK(ratelimit_params->GetString(kLockfileRatelimitPeriodStartKey,
-                                     &period_start_str),
-         result);
+  double seconds;
+  RCHECK(
+      ratelimit_params->GetDouble(kLockfileRatelimitPeriodStartKey, &seconds),
+      base::Time());
 
-  errno = 0;
-  result = static_cast<time_t>(strtoll(period_start_str.c_str(), nullptr, 0));
-  if (errno != 0) {
-    return static_cast<time_t>(-1);
-  }
-
-  return result;
+  return seconds ? base::Time::FromDoubleT(seconds) : base::Time::UnixEpoch();

[bcf, 2016/08/05 03:33:23]

Add comment why this is necessary:

// 0 means "not initialized", so we need a special case.

[ameyak, 2016/08/05 18:45:06]

Done.

 }
 
 // Sets the time of the current ratelimit period's start in |metadata| to
-// |period_start|. Returns 0 on success, < 0 on error.
-int SetRatelimitPeriodStart(base::Value* metadata, time_t period_start) {
-  DCHECK_GE(period_start, 0);
+// |period_start|. Returns true on success, false on error.
+bool SetRatelimitPeriodStart(base::Value* metadata, base::Time period_start) {
+  DCHECK(!period_start.is_null());
 
   base::DictionaryValue* ratelimit_params = GetRatelimitParams(metadata);
-  RCHECK(ratelimit_params, -1);
+  RCHECK(ratelimit_params, false);
 
-  std::string period_start_str =
-      base::StringPrintf("%lld", static_cast<long long>(period_start));
-  ratelimit_params->SetString(kLockfileRatelimitPeriodStartKey,
-                              period_start_str);
-  return 0;
+  ratelimit_params->SetDouble(kLockfileRatelimitPeriodStartKey,
+                              period_start.ToDoubleT());
+  return true;
 }
 
 // Gets the number of dumps added to |metadata| in the current ratelimit
@@ -137,7 +122,7 @@ bool ValidateMetadata(base::Value* metadata) {
 
   return ratelimit_params &&
          ratelimit_params->size() == kLockfileNumRatelimitParams &&
-         GetRatelimitPeriodStart(metadata) >= 0 &&
+         !GetRatelimitPeriodStart(metadata).is_null() &&
          GetRatelimitPeriodDumps(metadata) >= 0;
 }
 
@@ -148,12 +133,10 @@ const int SynchronizedMinidumpManager::kRatelimitPeriodSeconds = 24 * 3600;
 const int SynchronizedMinidumpManager::kRatelimitPeriodMaxDumps = 100;
 
 SynchronizedMinidumpManager::SynchronizedMinidumpManager()
-    : non_blocking_(false),
-      dump_path_(GetHomePathASCII(kMinidumpsDir)),
+    : dump_path_(GetHomePathASCII(kMinidumpsDir)),
       lockfile_path_(dump_path_.Append(kLockfileName).value()),
       metadata_path_(dump_path_.Append(kMetadataName).value()),
-      lockfile_fd_(-1) {
-}
+      lockfile_fd_(-1) {}
 
 SynchronizedMinidumpManager::~SynchronizedMinidumpManager() {
   // Release the lock if held.
@@ -162,40 +145,36 @@ SynchronizedMinidumpManager::~SynchronizedMinidumpManager() {
 
 // TODO(slan): Move some of this pruning logic to ReleaseLockFile?
 int SynchronizedMinidumpManager::GetNumDumps(bool delete_all_dumps) {
-  DIR* dirp;
-  struct dirent* dptr;
   int num_dumps = 0;
 
-  // folder does not exist
-  dirp = opendir(dump_path_.value().c_str());
-  if (dirp == NULL) {
+  base::DirReaderPosix reader(dump_path_.value().c_str());
+  if (!reader.IsValid()) {
     LOG(ERROR) << "Unable to open directory " << dump_path_.value();
     return 0;
   }
 
-  while ((dptr = readdir(dirp)) != NULL) {
-    struct stat buf;
-    const std::string file_fullname = dump_path_.value() + "/" + dptr->d_name;
-    if (lstat(file_fullname.c_str(), &buf) == -1 || !S_ISREG(buf.st_mode)) {
-      // if we cannot lstat this file, it is probably bad, so skip
-      // if the file is not regular, skip
+  while (reader.Next()) {
+    if (strcmp(reader.name(), ".") == 0 || strcmp(reader.name(), "..") == 0)
       continue;
-    }
 
-    // 'lockfile' and 'metadata' is not counted
-    if (lockfile_path_ != file_fullname && metadata_path_ != file_fullname) {
+    const base::FilePath dump_file(dump_path_.Append(reader.name()));
+    // If file cannot be found, skip.
+    if (!base::PathExists(dump_file))
+      continue;
+
+    // Do not count |lockfile_path_| and |metadata_path_|.
+    if (lockfile_path_ != dump_file && metadata_path_ != dump_file) {
       ++num_dumps;
       if (delete_all_dumps) {
-        LOG(INFO) << "Removing " << dptr->d_name
+        LOG(INFO) << "Removing " << reader.name()
                   << "which was not in the lockfile";
-        if (remove(file_fullname.c_str()) < 0) {
+        if (!base::DeleteFile(dump_file, false)) {
           LOG(INFO) << "remove failed. error " << strerror(errno);
         }
       }
     }
   }
 
-  closedir(dirp);
   return num_dumps;
 }
 
@@ -211,28 +190,23 @@ int SynchronizedMinidumpManager::AcquireLockAndDoWork() {
 int SynchronizedMinidumpManager::AcquireLockFile() {
   DCHECK_LT(lockfile_fd_, 0);
   // Make the directory for the minidumps if it does not exist.
-  if (mkdir(dump_path_.value().c_str(), kDirMode) < 0 && errno != EEXIST) {
-    LOG(ERROR) << "mkdir for " << dump_path_.value().c_str()
-               << " failed. error = " << strerror(errno);
+  if (!CreateDirectory(dump_path_)) {
+    LOG(ERROR) << "mkdir for " << dump_path_.value() << " failed.";

[bcf, 2016/08/05 03:33:23]

Nit:

We're not using mkdir anymore.

"Failed to create directory: ". Also lets use CreateDirectoryAndGetError so we
can keep the error message.

[ameyak, 2016/08/05 18:45:06]

Done.

     return -1;
   }
 
   // Open the lockfile. Create it if it does not exist.
-  lockfile_fd_ = open(lockfile_path_.c_str(), O_RDWR | O_CREAT, kFileMode);
+  base::File lockfile(lockfile_path_, base::File::FLAG_OPEN_ALWAYS);
 
   // If opening or creating the lockfile failed, we don't want to proceed
   // with dump writing for fear of exhausting up system resources.
-  if (lockfile_fd_ < 0) {
-    LOG(ERROR) << "open lockfile failed " << lockfile_path_;
+  if (!lockfile.IsValid()) {
+    LOG(ERROR) << "open lockfile failed " << lockfile_path_.value();
     return -1;
   }
 
-  // Acquire the lock on the file. Whether or not we are in non-blocking mode,
-  // flock failure means that we did not acquire it and this method should fail.
-  int operation_mode = non_blocking_ ? (LOCK_EX | LOCK_NB) : LOCK_EX;
-  if (flock(lockfile_fd_, operation_mode) < 0) {
+  if ((lockfile_fd_ = OpenAndLockFile(lockfile_path_)) < 0) {
     ReleaseLockFile();
-    LOG(INFO) << "flock lockfile failed, error = " << strerror(errno);
     return -1;
   }
 
@@ -259,7 +233,7 @@ int SynchronizedMinidumpManager::ParseFiles() {
   DCHECK(!metadata_);
 
   std::string lockfile;
-  RCHECK(ReadFileToString(base::FilePath(lockfile_path_), &lockfile), -1);
+  RCHECK(ReadFileToString(lockfile_path_, &lockfile), -1);
 
   std::vector<std::string> lines = base::SplitString(
       lockfile, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
@@ -278,7 +252,7 @@ int SynchronizedMinidumpManager::ParseFiles() {
   }
 
   std::unique_ptr<base::Value> metadata =
-      DeserializeJsonFromFile(base::FilePath(metadata_path_));
+      DeserializeJsonFromFile(metadata_path_);
   RCHECK(ValidateMetadata(metadata.get()), -1);
 
   dumps_ = std::move(dumps);
@@ -299,14 +273,11 @@ int SynchronizedMinidumpManager::WriteFiles(const base::ListValue* dumps,
     lockfile += "\n";  // Add line seperatators
   }
 
-  if (WriteFile(base::FilePath(lockfile_path_),
-                lockfile.c_str(),
-                lockfile.size()) < 0) {
+  if (WriteFile(lockfile_path_, lockfile.c_str(), lockfile.size()) < 0) {
     return -1;
   }
 
-  return SerializeJsonToFile(base::FilePath(metadata_path_), *metadata) ? 0
-                                                                        : -1;
+  return SerializeJsonToFile(metadata_path_, *metadata) ? 0 : -1;
 }
 
 int SynchronizedMinidumpManager::InitializeFiles() {
@@ -315,7 +286,7 @@ int SynchronizedMinidumpManager::InitializeFiles() {
 
   base::DictionaryValue* ratelimit_fields = new base::DictionaryValue();
   metadata->Set(kLockfileRatelimitKey, base::WrapUnique(ratelimit_fields));
-  ratelimit_fields->SetString(kLockfileRatelimitPeriodStartKey, "0");
+  ratelimit_fields->SetDouble(kLockfileRatelimitPeriodStartKey, 0);
   ratelimit_fields->SetInteger(kLockfileRatelimitPeriodDumpsKey, 0);
 
   std::unique_ptr<base::ListValue> dumps =
@@ -325,7 +296,7 @@ int SynchronizedMinidumpManager::InitializeFiles() {
 }
 
 int SynchronizedMinidumpManager::AddEntryToLockFile(const DumpInfo& dump_info) {
-  DCHECK_LE(0, lockfile_fd_);
+  DCHECK_GE(lockfile_fd_, 0);
   DCHECK(dumps_);
 
   // Make sure dump_info is valid.
@@ -350,10 +321,7 @@ void SynchronizedMinidumpManager::ReleaseLockFile() {
     if (dumps_)
       WriteFiles(dumps_.get(), metadata_.get());
 
-    flock(lockfile_fd_, LOCK_UN);
-    close(lockfile_fd_);
-
-    // We may use this object again, so we should reset this.
+    chromecast::UnlockAndCloseFile(lockfile_fd_);
     lockfile_fd_ = -1;
   }
 
@@ -390,8 +358,8 @@ int SynchronizedMinidumpManager::IncrementNumDumpsInCurrentPeriod() {
 }
 
 bool SynchronizedMinidumpManager::CanUploadDump() {
-  time_t cur_time = time(nullptr);
-  time_t period_start = GetRatelimitPeriodStart(metadata_.get());
+  base::Time cur_time = base::Time::Now();
+  base::Time period_start = GetRatelimitPeriodStart(metadata_.get());
   int period_dumps_count = GetRatelimitPeriodDumps(metadata_.get());
 
   // If we're in invalid state, or we passed the period, reset the ratelimit.
@@ -400,8 +368,9 @@ bool SynchronizedMinidumpManager::CanUploadDump() {
   // |period_start| invalid when |cur_time| is less if |cur_time| is not very
   // close to 0.
   if (period_dumps_count < 0 ||
-      (cur_time < period_start && cur_time > kRatelimitPeriodSeconds) ||
-      difftime(cur_time, period_start) >= kRatelimitPeriodSeconds) {
+      (cur_time < period_start &&
+       cur_time.ToDoubleT() > kRatelimitPeriodSeconds) ||
+      (cur_time - period_start).InSeconds() >= kRatelimitPeriodSeconds) {
     period_start = cur_time;
     period_dumps_count = 0;
     SetRatelimitPeriodStart(metadata_.get(), period_start);
@@ -414,7 +383,7 @@ bool SynchronizedMinidumpManager::CanUploadDump() {
 bool SynchronizedMinidumpManager::HasDumps() {
   // Check if lockfile has entries.
   int64_t size = 0;
-  if (GetFileSize(base::FilePath(lockfile_path_), &size) && size > 0)
+  if (base::GetFileSize(lockfile_path_, &size) && size > 0)
     return true;
 
   // Check if any files are in minidump directory
@@ -428,7 +397,7 @@ bool SynchronizedMinidumpManager::HasDumps() {
     if (strcmp(reader.name(), ".") == 0 || strcmp(reader.name(), "..") == 0)
       continue;
 
-    const std::string file_path = dump_path_.Append(reader.name()).value();
+    const base::FilePath file_path = dump_path_.Append(reader.name());
     if (file_path != lockfile_path_ && file_path != metadata_path_)
       return true;
   }