[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..1ff68f0798e24027b81f4a743025a7da557eb4a6 100644
--- a/chromecast/crash/linux/synchronized_minidump_manager.cc
+++ b/chromecast/crash/linux/synchronized_minidump_manager.cc
@@ -4,13 +4,10 @@
 
 #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>

[bcf, 2016/08/03 02:40:24]

Can usage of stat.h, types,h, unistd.h be removed?

[ameyak, 2016/08/03 18:50:32]

time_t/size_t are required and defined in sys/types.h. I have attempted to
remove time.h functions (time(1), difftime(2)) in lieu of base::Time

sys/stat.h is no longer necessary and has been removed
unistd.h is no longer necessary and has been removed

[bcf, 2016/08/03 20:45:10]

Can you try to remove time_t usage?

[ameyak, 2016/08/04 15:25:30]

Done.

 #include <sys/types.h>
 #include <time.h>
@@ -22,8 +19,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 +39,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";
@@ -76,12 +73,7 @@ time_t GetRatelimitPeriodStart(base::Value* metadata) {
                                      &period_start_str),
          result);
 
-  errno = 0;
-  result = static_cast<time_t>(strtoll(period_start_str.c_str(), nullptr, 0));
-  if (errno != 0) {
-    return static_cast<time_t>(-1);
-  }
-
+  base::StringToInt64(period_start_str, &result);

[bcf, 2016/08/03 02:40:23]

Check return value.

[ameyak, 2016/08/03 18:50:32]

Done. Overlooked that result can be changed even on a failure.

   return result;
 }
 
@@ -148,12 +140,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) {
-}
+      initialized(false) {}

[bcf, 2016/08/03 02:40:24]

How about instead call it "lock_held_"

[ameyak, 2016/08/03 18:50:32]

Done.

 
 SynchronizedMinidumpManager::~SynchronizedMinidumpManager() {
   // Release the lock if held.
@@ -162,40 +152,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;
+
+    const base::FilePath dump_file(dump_path_.Append(reader.name()));
+    // If file cannot be found, skip.
+    if (!base::PathExists(dump_file))
       continue;
-    }
 
-    // 'lockfile' and 'metadata' is not counted
-    if (lockfile_path_ != file_fullname && metadata_path_ != file_fullname) {
+    // 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;
 }
 
@@ -209,33 +195,30 @@ int SynchronizedMinidumpManager::AcquireLockAndDoWork() {
 }
 
 int SynchronizedMinidumpManager::AcquireLockFile() {
-  DCHECK_LT(lockfile_fd_, 0);
+  DCHECK(!initialized);
   // 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_)) {

[bcf, 2016/08/03 02:40:24]

This uses mode 0700. Can you make sure this works?
https://cs.chromium.org/chromium/src/base/files/file_util_posix.cc?sq=package...

[ameyak, 2016/08/03 18:50:32]

Acknowledged. Seems ok, as discussed.

+    LOG(ERROR) << "mkdir for " << dump_path_.value() << " failed.";
     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(lockfile_path_)) {
     ReleaseLockFile();
-    LOG(INFO) << "flock lockfile failed, error = " << strerror(errno);
     return -1;
   }
 
+  initialized = true;
+
   // The lockfile is open and locked. Parse it to provide subclasses with a
   // record of all the current dumps.
   if (ParseFiles() < 0) {
@@ -254,12 +237,12 @@ int SynchronizedMinidumpManager::AcquireLockFile() {
 }
 
 int SynchronizedMinidumpManager::ParseFiles() {
-  DCHECK_GE(lockfile_fd_, 0);
+  DCHECK(initialized);
   DCHECK(!dumps_);
   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 +261,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 +282,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() {
@@ -325,7 +305,7 @@ int SynchronizedMinidumpManager::InitializeFiles() {
 }
 
 int SynchronizedMinidumpManager::AddEntryToLockFile(const DumpInfo& dump_info) {
-  DCHECK_LE(0, lockfile_fd_);
+  DCHECK(initialized);
   DCHECK(dumps_);
 
   // Make sure dump_info is valid.
@@ -346,15 +326,12 @@ int SynchronizedMinidumpManager::RemoveEntryFromLockFile(int index) {
 void SynchronizedMinidumpManager::ReleaseLockFile() {
   // flock is associated with the fd entry in the open fd table, so closing
   // all fd's will release the lock. To be safe, we explicitly unlock.
-  if (lockfile_fd_ >= 0) {
+  if (initialized) {
     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.
-    lockfile_fd_ = -1;
+    chromecast::UnlockFile(lockfile_path_);
+    initialized = false;
   }
 
   dumps_.reset();
@@ -414,7 +391,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 +405,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;
   }