[Chromecast] Remove usage of nonreetrant functions.

Remove usage of dmtime & readdir_r Replace usage of time_t with base::Time::ToDoubleT() Replace int error codes with bool error codes Remove unix headers in favor of chromium-equivalent functions These changes are applied to files in directory crash/linux/ BUG=internal b/27252596 TEST=builds, cast_crash_unittests Review-Url: https://codereview.chromium.org/2203123003 Cr-Commit-Position: refs/heads/master@{#410441}
Denger-Network · Aug 8, 2016 · 0625111 · 0625111
1 parent c8c2305
commit 0625111
Show file tree

Hide file tree

Showing 14 changed files with 303 additions and 343 deletions.
diff --git a/chromecast/base/BUILD.gn b/chromecast/base/BUILD.gn
@@ -41,6 +41,8 @@ source_set("base") {
     "device_capabilities_impl.h",
     "error_codes.cc",
     "error_codes.h",
+    "file_utils.cc",
+    "file_utils.h",
     "init_command_line_shlib.cc",
     "init_command_line_shlib.h",
     "path_utils.cc",

diff --git a/chromecast/base/file_utils.cc b/chromecast/base/file_utils.cc
@@ -0,0 +1,47 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromecast/base/file_utils.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/file.h>
+
+namespace {
+
+// Calls flock on valid file descriptor |fd| with flag |flag|. Returns true
+// on success, false on failure.
+bool CallFlockOnFileWithFlag(const int fd, int flag) {
+  int ret = -1;
+  if ((ret = TEMP_FAILURE_RETRY(flock(fd, flag))) < 0)
+    PLOG(ERROR) << "Error locking " << fd;
+
+  return !ret;
+}
+
+}  // namespace
+
+namespace chromecast {
+
+int OpenAndLockFile(const base::FilePath& path, bool write) {
+  int fd = -1;
+  const char* file = path.value().c_str();
+
+  if ((fd = open(file, write ? O_RDWR : O_RDONLY)) < 0) {
+    PLOG(ERROR) << "Error opening " << file;
+  } else if (!CallFlockOnFileWithFlag(fd, LOCK_EX)) {
+    close(fd);
+    fd = -1;
+  }
+
+  return fd;
+}
+
+bool UnlockAndCloseFile(const int fd) {
+  if (!CallFlockOnFileWithFlag(fd, LOCK_UN))
+    return false;
+  return !close(fd);
+}
+
+}  // namespace chromecast
diff --git a/chromecast/base/file_utils.h b/chromecast/base/file_utils.h
@@ -0,0 +1,24 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMECAST_BASE_FILE_UTILS_H_
+#define CHROMECAST_BASE_FILE_UTILS_H_
+
+#include "base/files/file_path.h"
+
+namespace chromecast {
+
+// Lock a file (blocking request) with filepath |path|. Returns the file
+// descriptor on success, an invalid file descriptor (-1) on failure. Failure
+// indicates that |path| does not exist, or the locking procedure failed.
+// A true |write| value opens the file in RW mode, false opens in R-only.
+int OpenAndLockFile(const base::FilePath& path, bool write);
+
+// Unlock a file (blocking request) denoted by file descriptor |fd|. Returns
+// true on success, false on failure.
+bool UnlockAndCloseFile(const int fd);
+
+}  // namespace chromecast
+
+#endif  // CHROMECAST_BASE_FILE_UTILS_H_
diff --git a/chromecast/crash/linux/crash_testing_utils.cc b/chromecast/crash/linux/crash_testing_utils.cc
@@ -143,7 +143,8 @@ bool AppendLockFile(const std::string& lockfile_path,
   return WriteLockFile(lockfile_path, contents.get()) == 0;
 }
 
-bool SetRatelimitPeriodStart(const std::string& metadata_path, time_t start) {
+bool SetRatelimitPeriodStart(const std::string& metadata_path,
+                             const base::Time& start) {
   std::unique_ptr<base::Value> contents = ParseMetadataFile(metadata_path);
 
   base::DictionaryValue* dict;
@@ -153,10 +154,7 @@ bool SetRatelimitPeriodStart(const std::string& metadata_path, time_t start) {
     return false;
   }
 
-  std::string period_start_str =
-      base::StringPrintf("%lld", static_cast<long long>(start));
-  ratelimit_params->SetString(kRatelimitPeriodStartKey, period_start_str);
-
+  ratelimit_params->SetDouble(kRatelimitPeriodStartKey, start.ToDoubleT());
   return WriteMetadataFile(metadata_path, contents.get()) == 0;
 }
 

diff --git a/chromecast/crash/linux/crash_testing_utils.h b/chromecast/crash/linux/crash_testing_utils.h
@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/memory/scoped_vector.h"
+#include "base/time/time.h"
 
 namespace chromecast {
 
@@ -42,7 +43,8 @@ bool AppendLockFile(const std::string& lockfile_path,
 
 // Set the ratelimit period start in the metadata file at |metadata_path| to
 // |start|. Returns true on success, false on error.
-bool SetRatelimitPeriodStart(const std::string& metadata_path, time_t start);
+bool SetRatelimitPeriodStart(const std::string& metadata_path,
+                             const base::Time& start);
 
 }  // namespace chromecast
 

diff --git a/chromecast/crash/linux/crash_util.cc b/chromecast/crash/linux/crash_util.cc
@@ -67,7 +67,6 @@ bool CrashUtil::RequestUploadCrashDump(
         new MinidumpWriter(&minidump_generator, filename.value(), params));
   }
   bool success = false;
-  writer->set_non_blocking(false);
   success = (0 == writer->Write());  // error already logged.
 
   // In case the file is still in $TEMP, remove it. Note that DeleteFile() will

diff --git a/chromecast/crash/linux/dump_info.cc b/chromecast/crash/linux/dump_info.cc
@@ -9,14 +9,15 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 
 namespace chromecast {
 
 namespace {
 
-const char kDumpTimeFormat[] = "%Y-%m-%d %H:%M:%S";
-const unsigned kDumpTimeMaxLen = 255;
+// "%Y-%m-%d %H:%M:%S";
+const char kDumpTimeFormat[] = "%04d-%02d-%02d %02d:%02d:%02d";
 
 const int kNumRequiredParams = 5;
 
@@ -40,25 +41,13 @@ DumpInfo::DumpInfo(const base::Value* entry) : valid_(ParseEntry(entry)) {
 
 DumpInfo::DumpInfo(const std::string& crashed_process_dump,
                    const std::string& logfile,
-                   const time_t& dump_time,
+                   const base::Time& dump_time,
                    const MinidumpParams& params)
     : crashed_process_dump_(crashed_process_dump),
       logfile_(logfile),
       dump_time_(dump_time),
       params_(params),
-      valid_(false) {
-
-  // Validate the time passed in.
-  struct tm* tm = gmtime(&dump_time);
-  char buf[kDumpTimeMaxLen];
-  int n = strftime(buf, kDumpTimeMaxLen, kDumpTimeFormat, tm);
-  if (n <= 0) {
-    LOG(INFO) << "strftime failed";
-    return;
-  }
-
-  valid_ = true;
-}
+      valid_(true) {}
 
 DumpInfo::~DumpInfo() {
 }
@@ -70,11 +59,11 @@ std::unique_ptr<base::Value> DumpInfo::GetAsValue() const {
   result->GetAsDictionary(&entry);
   entry->SetString(kNameKey, params_.process_name);
 
-  struct tm* tm = gmtime(&dump_time_);
-  char buf[kDumpTimeMaxLen];
-  int n = strftime(buf, kDumpTimeMaxLen, kDumpTimeFormat, tm);
-  DCHECK_GT(n, 0);
-  std::string dump_time(buf);
+  base::Time::Exploded ex;
+  dump_time_.LocalExplode(&ex);
+  std::string dump_time =
+      base::StringPrintf(kDumpTimeFormat, ex.year, ex.month, ex.day_of_month,
+                         ex.hour, ex.minute, ex.second);
   entry->SetString(kDumpTimeKey, dump_time);
 
   entry->SetString(kDumpKey, crashed_process_dump_);
@@ -152,13 +141,14 @@ bool DumpInfo::ParseEntry(const base::Value* entry) {
 }
 
 bool DumpInfo::SetDumpTimeFromString(const std::string& timestr) {
-  struct tm tm = {0};
-  char* text = strptime(timestr.c_str(), kDumpTimeFormat, &tm);
-  dump_time_ = mktime(&tm);
-  if (!text || dump_time_ < 0) {
+  base::Time::Exploded ex = {0};
+  if (sscanf(timestr.c_str(), kDumpTimeFormat, &ex.year, &ex.month,
+             &ex.day_of_month, &ex.hour, &ex.minute, &ex.second) < 6) {
     LOG(INFO) << "Failed to convert dump time invalid";
     return false;
   }
+
+  dump_time_ = base::Time::FromLocalExploded(ex);
   return true;
 }
 

diff --git a/chromecast/crash/linux/dump_info.h b/chromecast/crash/linux/dump_info.h
@@ -5,11 +5,11 @@
 #ifndef CHROMECAST_CRASH_LINUX_DUMP_INFO_H_
 #define CHROMECAST_CRASH_LINUX_DUMP_INFO_H_
 
-#include <ctime>
 #include <memory>
 #include <string>
 
 #include "base/macros.h"
+#include "base/time/time.h"
 #include "chromecast/crash/linux/minidump_params.h"
 
 namespace base {
@@ -34,7 +34,7 @@ class DumpInfo {
   // -params: a structure containing other useful crash information
   DumpInfo(const std::string& crashed_process_dump,
            const std::string& crashed_process_logfile,
-           const time_t& dump_time,
+           const base::Time& dump_time,
            const MinidumpParams& params);
 
   ~DumpInfo();
@@ -43,7 +43,7 @@ class DumpInfo {
     return crashed_process_dump_;
   }
   const std::string& logfile() const { return logfile_; }
-  const time_t& dump_time() const { return dump_time_; }
+  const base::Time& dump_time() const { return dump_time_; }
 
   // Return a deep copy of the entry's JSON representation.
   // The format is:
@@ -73,7 +73,7 @@ class DumpInfo {
 
   std::string crashed_process_dump_;
   std::string logfile_;
-  time_t dump_time_;
+  base::Time dump_time_;
   MinidumpParams params_;
   bool valid_;
 

diff --git a/chromecast/crash/linux/dump_info_unittest.cc b/chromecast/crash/linux/dump_info_unittest.cc
@@ -47,15 +47,14 @@ TEST(DumpInfoTest, AllRequiredFieldsIsValid) {
                      "\"uptime\": \"123456789\","
                      "\"logfile\": \"logfile.log\""
                      "}"));
-  struct tm tm = {0};
-  tm.tm_isdst = 0;
-  tm.tm_sec = 1;
-  tm.tm_min = 31;
-  tm.tm_hour = 18;
-  tm.tm_mday = 12;
-  tm.tm_mon = 10;
-  tm.tm_year = 101;
-  time_t dump_time = mktime(&tm);
+  base::Time::Exploded ex = {0};
+  ex.second = 1;
+  ex.minute = 31;
+  ex.hour = 18;
+  ex.day_of_month = 12;
+  ex.month = 11;
+  ex.year = 2001;
+  base::Time dump_time = base::Time::FromLocalExploded(ex);
 
   ASSERT_TRUE(info->valid());
   ASSERT_EQ("name", info->params().process_name);
@@ -86,15 +85,14 @@ TEST(DumpInfoTest, SomeRequiredFieldsEmptyIsValid) {
                      "\"uptime\": \"\","
                      "\"logfile\": \"\""
                      "}"));
-  struct tm tm = {0};
-  tm.tm_isdst = 0;
-  tm.tm_sec = 1;
-  tm.tm_min = 31;
-  tm.tm_hour = 18;
-  tm.tm_mday = 12;
-  tm.tm_mon = 10;
-  tm.tm_year = 101;
-  time_t dump_time = mktime(&tm);
+  base::Time::Exploded ex = {0};
+  ex.second = 1;
+  ex.minute = 31;
+  ex.hour = 18;
+  ex.day_of_month = 12;
+  ex.month = 11;
+  ex.year = 2001;
+  base::Time dump_time = base::Time::FromLocalExploded(ex);
 
   ASSERT_TRUE(info->valid());
   ASSERT_EQ("name", info->params().process_name);
@@ -120,15 +118,14 @@ TEST(DumpInfoTest, AllOptionalFieldsIsValid) {
                      "\"build_number\": \"BUILD_NUMBER\","
                      "\"reason\": \"foo\""
                      "}"));
-  struct tm tm = {0};
-  tm.tm_isdst = 0;
-  tm.tm_sec = 1;
-  tm.tm_min = 31;
-  tm.tm_hour = 18;
-  tm.tm_mday = 12;
-  tm.tm_mon = 10;
-  tm.tm_year = 101;
-  time_t dump_time = mktime(&tm);
+  base::Time::Exploded ex = {0};
+  ex.second = 1;
+  ex.minute = 31;
+  ex.hour = 18;
+  ex.day_of_month = 12;
+  ex.month = 11;
+  ex.year = 2001;
+  base::Time dump_time = base::Time::FromLocalExploded(ex);
 
   ASSERT_TRUE(info->valid());
   ASSERT_EQ("name", info->params().process_name);
@@ -155,15 +152,14 @@ TEST(DumpInfoTest, SomeOptionalFieldsIsValid) {
                      "\"suffix\": \"suffix\","
                      "\"prev_app_name\": \"previous_app\""
                      "}"));
-  struct tm tm = {0};
-  tm.tm_isdst = 0;
-  tm.tm_sec = 1;
-  tm.tm_min = 31;
-  tm.tm_hour = 18;
-  tm.tm_mday = 12;
-  tm.tm_mon = 10;
-  tm.tm_year = 101;
-  time_t dump_time = mktime(&tm);
+  base::Time::Exploded ex = {0};
+  ex.second = 1;
+  ex.minute = 31;
+  ex.hour = 18;
+  ex.day_of_month = 12;
+  ex.month = 11;
+  ex.year = 2001;
+  base::Time dump_time = base::Time::FromLocalExploded(ex);
 
   ASSERT_TRUE(info->valid());
   ASSERT_EQ("name", info->params().process_name);

diff --git a/chromecast/crash/linux/minidump_writer.cc b/chromecast/crash/linux/minidump_writer.cc
@@ -61,7 +61,7 @@ MinidumpWriter::MinidumpWriter(MinidumpGenerator* minidump_generator,
 MinidumpWriter::~MinidumpWriter() {
 }
 
-int MinidumpWriter::DoWork() {
+bool MinidumpWriter::DoWork() {
   // If path is not absolute, append it to |dump_path_|.
   if (!minidump_path_.value().empty() && minidump_path_.value()[0] != '/')
     minidump_path_ = dump_path_.Append(minidump_path_);
@@ -70,33 +70,32 @@ int MinidumpWriter::DoWork() {
   if (dump_path_ != minidump_path_.DirName()) {
     LOG(INFO) << "The absolute path: " << minidump_path_.value() << " is not"
               << "in the correct directory: " << dump_path_.value();
-    return -1;
+    return false;
   }
 
   // Generate a minidump at the specified |minidump_path_|.
   if (!minidump_generator_->Generate(minidump_path_.value())) {
     LOG(ERROR) << "Generate minidump failed " << minidump_path_.value();
-    return -1;
+    return false;
   }
 
   // Run the dumpstate callback.
   DCHECK(!dump_state_cb_.is_null());
   if (dump_state_cb_.Run(minidump_path_.value()) < 0) {
     LOG(ERROR) << "DumpState callback failed.";
-    return -1;
+    return false;
   }
 
   // Add this entry to the lockfile.
   const DumpInfo info(minidump_path_.value(),
                       minidump_path_.value() + kDumpStateSuffix,
-                      time(nullptr),
-                      params_);
-  if (AddEntryToLockFile(info) < 0) {
+                      base::Time::Now(), params_);
+  if (!AddEntryToLockFile(info)) {
     LOG(ERROR) << "lockfile logging failed";
-    return -1;
+    return false;
   }
 
-  return 0;
+  return true;
 }
 
 }  // namespace crash_manager