crash-reporter: Use standard logging and new libchromeos Process code Change-Id: Ie085d61d4d79c0df38e51debaa044d701a860c89 BUG=none TEST=unit tests / UserCrash / CrashSender Review URL: http://codereview.chromium.org/6517001
diff --git a/Makefile b/Makefile index 0523c19..b649bea 100644 --- a/Makefile +++ b/Makefile
@@ -7,10 +7,9 @@ CRASH_OBJS = \ crash_collector.o \ kernel_collector.o \ - system_logging.o \ unclean_shutdown_collector.o \ user_collector.o -TEST_OBJS = $(CRASH_OBJS) system_logging_mock.o +TEST_OBJS = $(CRASH_OBJS) TEST_BINS = \ crash_collector_test \ kernel_collector_test \ @@ -20,7 +19,7 @@ LDCONFIG = $(shell $(PKG_CONFIG) --libs libpcrecpp) # -lglib-2.0 is needed by libbase.a now. -COMMON_LIBS = -lbase -lpthread -lglib-2.0 -lgflags -lrt $(LDCONFIG) +COMMON_LIBS = -lchromeos -lbase -lpthread -lglib-2.0 -lgflags -lrt $(LDCONFIG) REPORTER_LIBS = $(COMMON_LIBS) -lmetrics TEST_LIBS = $(COMMON_LIBS) -lgtest -lgmock
diff --git a/crash_collector.cc b/crash_collector.cc index afd72d1..9600b44 100644 --- a/crash_collector.cc +++ b/crash_collector.cc
@@ -17,7 +17,7 @@ #include "base/file_util.h" #include "base/logging.h" #include "base/string_util.h" -#include "crash-reporter/system_logging.h" +#include "chromeos/process.h" static const char kDefaultUserName[] = "chronos"; static const char kLsbRelease[] = "/etc/lsb-release"; @@ -54,15 +54,12 @@ void CrashCollector::Initialize( CrashCollector::CountCrashFunction count_crash_function, - CrashCollector::IsFeedbackAllowedFunction is_feedback_allowed_function, - SystemLogging *logger) { + CrashCollector::IsFeedbackAllowedFunction is_feedback_allowed_function) { CHECK(count_crash_function != NULL); CHECK(is_feedback_allowed_function != NULL); - CHECK(logger != NULL); count_crash_function_ = count_crash_function; is_feedback_allowed_function_ = is_feedback_allowed_function; - logger_ = logger; } int CrashCollector::WriteNewFile(const FilePath &filename, @@ -79,63 +76,6 @@ return rv; } -int CrashCollector::ForkExecAndPipe(std::vector<const char *> &arguments, - const char *output_file) { - // Copy off a writeable version of arguments. - scoped_array<char*> argv(new char *[arguments.size() + 1]); - int total_args_size = 0; - for (size_t i = 0; i < arguments.size(); ++i) { - if (arguments[i] == NULL) { - logger_->LogError("Bad parameter"); - return -1; - } - total_args_size += strlen(arguments[i]) + 1; - } - scoped_array<char> buffer(new char[total_args_size]); - char *buffer_pointer = &buffer[0]; - - for (size_t i = 0; i < arguments.size(); ++i) { - argv[i] = buffer_pointer; - strcpy(buffer_pointer, arguments[i]); - buffer_pointer += strlen(arguments[i]); - *buffer_pointer = '\0'; - ++buffer_pointer; - } - argv[arguments.size()] = NULL; - - int pid = fork(); - if (pid < 0) { - logger_->LogError("Fork failed: %d", errno); - return -1; - } - - if (pid == 0) { - int output_handle = HANDLE_EINTR( - open(output_file, O_CREAT | O_WRONLY | O_TRUNC | O_EXCL, 0666)); - if (output_handle < 0) { - logger_->LogError("Could not create %s: %d", output_file, errno); - // Avoid exit() to avoid atexit handlers from parent. - _exit(127); - } - dup2(output_handle, 1); - dup2(output_handle, 2); - execv(argv[0], &argv[0]); - logger_->LogError("Exec failed: %d", errno); - _exit(127); - } - - int status = 0; - if (HANDLE_EINTR(waitpid(pid, &status, 0)) < 0) { - logger_->LogError("Problem waiting for pid: %d", errno); - return -1; - } - if (!WIFEXITED(status)) { - logger_->LogError("Process did not exit normally: %d", status); - return -1; - } - return WEXITSTATUS(status); -} - std::string CrashCollector::Sanitize(const std::string &name) { std::string result = name; for (size_t i = 0; i < name.size(); ++i) { @@ -199,7 +139,7 @@ if (getpwnam_r(name.c_str(), &passwd_storage, storage, sizeof(storage), &passwd_result) != 0 || passwd_result == NULL) { - logger_->LogError("Cannot find user named %s", name.c_str()); + LOG(ERROR) << "Cannot find user named " << name; return false; } @@ -225,7 +165,7 @@ if (!GetUserInfoFromName(kDefaultUserName, &default_user_id, &default_user_group)) { - logger_->LogError("Could not find default user info"); + LOG(ERROR) << "Could not find default user info"; return false; } mode_t directory_mode; @@ -247,15 +187,15 @@ chown(crash_directory->value().c_str(), directory_owner, directory_group) < 0) { - logger_->LogError("Unable to create appropriate crash directory"); + LOG(ERROR) << "Unable to create appropriate crash directory"; return false; } umask(old_mask); } if (!file_util::PathExists(*crash_directory)) { - logger_->LogError("Unable to create crash directory %s", - crash_directory->value().c_str()); + LOG(ERROR) << "Unable to create crash directory " + << crash_directory->value().c_str(); return false; } @@ -297,10 +237,9 @@ basenames.insert(basename); if (basenames.size() >= static_cast<size_t>(kMaxCrashDirectorySize)) { - logger_->LogWarning( - "Crash directory %s already full with %d pending reports", - crash_directory.value().c_str(), - kMaxCrashDirectorySize); + LOG(WARNING) << "Crash directory " << crash_directory.value() + << " already full with " << kMaxCrashDirectorySize + << " pending reports"; full = true; break; } @@ -350,24 +289,24 @@ const FilePath &output_file) { std::map<std::string, std::string> log_commands; if (!ReadKeyValueFile(config_path, ':', &log_commands)) { - logger_->LogInfo("Unable to read log configuration file %s", - config_path.value().c_str()); + LOG(INFO) << "Unable to read log configuration file " + << config_path.value(); return false; } if (log_commands.find(exec_name) == log_commands.end()) return false; - std::vector<const char *> command; - command.push_back(kShellPath); - command.push_back("-c"); + chromeos::ProcessImpl diag_process; + diag_process.AddArg(kShellPath); std::string shell_command = log_commands[exec_name]; - command.push_back(shell_command.c_str()); + diag_process.AddStringOption("-c", shell_command); + diag_process.RedirectOutput(output_file.value()); - int fork_result = ForkExecAndPipe(command, output_file.value().c_str()); - if (fork_result != 0) { - logger_->LogInfo("Running shell command %s failed with: %d", - shell_command.c_str(), fork_result); + int result = diag_process.Run(); + if (result != 0) { + LOG(INFO) << "Running shell command " << shell_command << "failed with: " + << result; return false; } return true; @@ -383,7 +322,7 @@ const std::string &payload_path) { std::map<std::string, std::string> contents; if (!ReadKeyValueFile(FilePath(std::string(lsb_release_)), '=', &contents)) { - logger_->LogError("Problem parsing %s", lsb_release_); + LOG(ERROR) << "Problem parsing " << lsb_release_; // Even though there was some failure, take as much as we could read. } std::string version("unknown"); @@ -407,6 +346,6 @@ // do not want to write with root access to a symlink that an attacker // might have created. if (WriteNewFile(meta_path, meta_data.c_str(), meta_data.size()) < 0) { - logger_->LogError("Unable to write %s", meta_path.value().c_str()); + LOG(ERROR) << "Unable to write " << meta_path.value(); } }
diff --git a/crash_collector.h b/crash_collector.h index f3fcbe5..3c09e2f 100644 --- a/crash_collector.h +++ b/crash_collector.h
@@ -13,8 +13,6 @@ #include "base/file_path.h" #include "gtest/gtest_prod.h" // for FRIEND_TEST -class SystemLogging; - // User crash collector. class CrashCollector { public: @@ -26,11 +24,9 @@ virtual ~CrashCollector(); // Initialize the crash collector for detection of crashes, given a - // crash counting function, metrics collection enabled oracle, and - // system logger facility. + // crash counting function, and metrics collection enabled oracle. void Initialize(CountCrashFunction count_crash, - IsFeedbackAllowedFunction is_metrics_allowed, - SystemLogging *logger); + IsFeedbackAllowedFunction is_metrics_allowed); protected: friend class CrashCollectorTest; @@ -66,9 +62,6 @@ // Otherwise returns the number of bytes written. int WriteNewFile(const FilePath &filename, const char *data, int size); - int ForkExecAndPipe(std::vector<const char *> &arguments, - const char *output_file); - // Return a filename that has only [a-z0-1_] characters by mapping // all others into '_'. std::string Sanitize(const std::string &name); @@ -140,7 +133,6 @@ CountCrashFunction count_crash_function_; IsFeedbackAllowedFunction is_feedback_allowed_function_; - SystemLogging *logger_; std::string extra_metadata_; const char *forced_crash_directory_; const char *lsb_release_;
diff --git a/crash_collector_test.cc b/crash_collector_test.cc index 8100a5f..d98e1af 100644 --- a/crash_collector_test.cc +++ b/crash_collector_test.cc
@@ -6,9 +6,9 @@ #include "base/file_util.h" #include "base/string_util.h" +#include "chromeos/syslog_logging.h" +#include "chromeos/test_helpers.h" #include "crash-reporter/crash_collector.h" -#include "crash-reporter/system_logging_mock.h" -#include "crash-reporter/test_helpers.h" #include "gflags/gflags.h" #include "gtest/gtest.h" @@ -18,6 +18,8 @@ static const char kBinEcho[] = "/bin/echo"; static const char kBinFalse[] = "/bin/false"; +using chromeos::FindLog; + void CountCrash() { ADD_FAILURE(); } @@ -31,10 +33,10 @@ public: void SetUp() { collector_.Initialize(CountCrash, - IsMetrics, - &logging_); + IsMetrics); test_dir_ = FilePath("test"); file_util::CreateDirectory(test_dir_); + chromeos::ClearLog(); } void TearDown() { @@ -44,7 +46,6 @@ bool CheckHasCapacity(); protected: - SystemLoggingMock logging_; CrashCollector collector_; FilePath test_dir_; }; @@ -52,7 +53,6 @@ TEST_F(CrashCollectorTest, Initialize) { ASSERT_TRUE(CountCrash == collector_.count_crash_function_); ASSERT_TRUE(IsMetrics == collector_.is_feedback_allowed_function_); - ASSERT_TRUE(&logging_ == collector_.logger_); } TEST_F(CrashCollectorTest, WriteNewFile) { @@ -154,7 +154,7 @@ bool CrashCollectorTest::CheckHasCapacity() { static const char kFullMessage[] = "Crash directory test already full"; bool has_capacity = collector_.CheckHasCapacity(test_dir_); - bool has_message = (logging_.log().find(kFullMessage) != std::string::npos); + bool has_message = FindLog(kFullMessage); EXPECT_EQ(has_message, !has_capacity); return has_capacity; } @@ -298,24 +298,24 @@ symlink(kMetaFileBasename, meta_symlink_path.value().c_str())); ASSERT_TRUE(file_util::PathExists(meta_symlink_path)); - logging_.clear(); + chromeos::ClearLog(); collector_.WriteCrashMetaData(meta_symlink_path, "kernel", payload_file.value()); - // Target metadata contents sould have stayed the same. + // Target metadata contents should have stayed the same. contents.clear(); EXPECT_TRUE(file_util::ReadFileToString(meta_file, &contents)); EXPECT_EQ(kExpectedMeta, contents); - EXPECT_NE(std::string::npos, logging_.log().find("Unable to write")); + EXPECT_TRUE(FindLog("Unable to write")); // Test target of dangling symlink is not created. file_util::Delete(meta_file, false); ASSERT_FALSE(file_util::PathExists(meta_file)); - logging_.clear(); + chromeos::ClearLog(); collector_.WriteCrashMetaData(meta_symlink_path, "kernel", payload_file.value()); EXPECT_FALSE(file_util::PathExists(meta_file)); - EXPECT_NE(std::string::npos, logging_.log().find("Unable to write")); + EXPECT_TRUE(FindLog("Unable to write")); } TEST_F(CrashCollectorTest, GetLogContents) { @@ -341,87 +341,7 @@ EXPECT_EQ("hello world\n", contents); } -class ForkExecAndPipeTest : public CrashCollectorTest { - public: - void SetUp() { - CrashCollectorTest::SetUp(); - output_file_ = "test/fork_out"; - file_util::Delete(FilePath(output_file_), false); - } - - void TearDown() { - CrashCollectorTest::TearDown(); - } - - protected: - std::vector<const char *> args_; - const char *output_file_; -}; - -TEST_F(ForkExecAndPipeTest, Basic) { - args_.push_back(kBinEcho); - args_.push_back("hello world"); - EXPECT_EQ(0, collector_.ForkExecAndPipe(args_, output_file_)); - ExpectFileEquals("hello world\n", output_file_); - EXPECT_EQ("", logging_.log()); -} - -TEST_F(ForkExecAndPipeTest, NonZeroReturnValue) { - args_.push_back(kBinFalse); - EXPECT_EQ(1, collector_.ForkExecAndPipe(args_, output_file_)); - ExpectFileEquals("", output_file_); - EXPECT_EQ("", logging_.log()); -} - -TEST_F(ForkExecAndPipeTest, BadOutputFile) { - EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, "/bad/path")); -} - -TEST_F(ForkExecAndPipeTest, ExistingOutputFile) { - args_.push_back(kBinEcho); - args_.push_back("hello world"); - EXPECT_FALSE(file_util::PathExists(FilePath(output_file_))); - EXPECT_EQ(0, collector_.ForkExecAndPipe(args_, output_file_)); - EXPECT_TRUE(file_util::PathExists(FilePath(output_file_))); - EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, output_file_)); -} - -TEST_F(ForkExecAndPipeTest, BadExecutable) { - args_.push_back("false"); - EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, output_file_)); -} - -TEST_F(ForkExecAndPipeTest, StderrCaptured) { - std::string contents; - args_.push_back(kBinCp); - EXPECT_EQ(1, collector_.ForkExecAndPipe(args_, output_file_)); - EXPECT_TRUE(file_util::ReadFileToString(FilePath(output_file_), - &contents)); - EXPECT_NE(std::string::npos, contents.find("missing file operand")); - EXPECT_EQ("", logging_.log()); -} - -TEST_F(ForkExecAndPipeTest, NULLParam) { - args_.push_back(NULL); - EXPECT_EQ(-1, collector_.ForkExecAndPipe(args_, output_file_)); - EXPECT_NE(std::string::npos, - logging_.log().find("Bad parameter")); -} - -TEST_F(ForkExecAndPipeTest, NoParams) { - EXPECT_EQ(127, collector_.ForkExecAndPipe(args_, output_file_)); -} - -TEST_F(ForkExecAndPipeTest, SegFaultHandling) { - args_.push_back(kBinBash); - args_.push_back("-c"); - args_.push_back("kill -SEGV $$"); - EXPECT_EQ(-1, collector_.ForkExecAndPipe(args_, output_file_)); - EXPECT_NE(std::string::npos, - logging_.log().find("Process did not exit normally")); -} - int main(int argc, char **argv) { - ::testing::InitGoogleTest(&argc, argv); + SetUpTests(&argc, argv, false); return RUN_ALL_TESTS(); }
diff --git a/crash_reporter.cc b/crash_reporter.cc index ab4c5ba..42998e9 100644 --- a/crash_reporter.cc +++ b/crash_reporter.cc
@@ -2,14 +2,16 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <fcntl.h> // for open + #include <string> #include "base/file_util.h" #include "base/command_line.h" #include "base/logging.h" #include "base/string_util.h" +#include "chromeos/syslog_logging.h" #include "crash-reporter/kernel_collector.h" -#include "crash-reporter/system_logging.h" #include "crash-reporter/unclean_shutdown_collector.h" #include "crash-reporter/user_collector.h" #include "gflags/gflags.h" @@ -41,7 +43,6 @@ }; static MetricsLibrary s_metrics_lib; -static SystemLoggingImpl s_system_log; static bool IsFeedbackAllowed() { return s_metrics_lib.AreMetricsEnabled(); @@ -96,9 +97,7 @@ // to be restarted anyway. int status = system(command.c_str()); - if (status != 0) { - s_system_log.LogWarning("dbus-send running failed"); - } + LOG_IF(WARNING, status != 0) << "dbus-send running failed"; } static int Initialize(KernelCollector *kernel_collector, @@ -149,10 +148,10 @@ } // Accumulate logs to help in diagnosing failures during user collection. - s_system_log.set_accumulating(true); + chromeos::LogToString(true); // Handle the crash, get the name of the process from procfs. bool handled = user_collector->HandleCrash(FLAGS_user, NULL); - s_system_log.set_accumulating(false); + chromeos::LogToString(false); if (!handled) return 1; return 0; @@ -178,31 +177,45 @@ return 0; } +// Ensure stdout, stdin, and stderr are open file descriptors. If +// they are not, any code which writes to stderr/stdout may write out +// to files opened during execution. In particular, when +// crash_reporter is run by the kernel coredump pipe handler (via +// kthread_create/kernel_execve), it will not have file table entries +// 1 and 2 (stdout and stderr) populated. We populate them here. +static void OpenStandardFileDescriptors() { + int new_fd = -1; + // We open /dev/null to fill in any of the standard [0, 2] file + // descriptors. We leave these open for the duration of the + // process. This works because open returns the lowest numbered + // invalid fd. + do { + new_fd = open("/dev/null", 0); + CHECK(new_fd >= 0) << "Unable to open /dev/null"; + } while (new_fd >= 0 && new_fd <= 2); + close(new_fd); +} + int main(int argc, char *argv[]) { + OpenStandardFileDescriptors(); google::ParseCommandLineFlags(&argc, &argv, true); FilePath my_path(argv[0]); file_util::AbsolutePath(&my_path); s_metrics_lib.Init(); CommandLine::Init(argc, argv); - logging::InitLogging(NULL, - logging::LOG_ONLY_TO_SYSTEM_DEBUG_LOG, - logging::DONT_LOCK_LOG_FILE, - logging::DELETE_OLD_LOG_FILE); - s_system_log.Initialize(my_path.BaseName().value().c_str()); + chromeos::OpenLog(my_path.BaseName().value().c_str(), true); + chromeos::InitLog(chromeos::kLogToSyslog); KernelCollector kernel_collector; kernel_collector.Initialize(CountKernelCrash, - IsFeedbackAllowed, - &s_system_log); + IsFeedbackAllowed); UserCollector user_collector; user_collector.Initialize(CountUserCrash, my_path.value(), IsFeedbackAllowed, - &s_system_log, true); // generate_diagnostics UncleanShutdownCollector unclean_shutdown_collector; unclean_shutdown_collector.Initialize(CountUncleanShutdown, - IsFeedbackAllowed, - &s_system_log); + IsFeedbackAllowed); if (FLAGS_init) { return Initialize(&kernel_collector,
diff --git a/kernel_collector.cc b/kernel_collector.cc index 4fa51e2..652e7ff 100644 --- a/kernel_collector.cc +++ b/kernel_collector.cc
@@ -7,7 +7,6 @@ #include "base/file_util.h" #include "base/logging.h" #include "base/string_util.h" -#include "crash-reporter/system_logging.h" const char KernelCollector::kClearingSequence[] = " "; static const char kDefaultKernelStackSignature[] = @@ -41,8 +40,7 @@ // clear contents since ReadFileToString actually appends to the string. contents->clear(); if (!file_util::ReadFileToString(preserved_dump_path_, contents)) { - logger_->LogError("Unable to read %s", - preserved_dump_path_.value().c_str()); + LOG(ERROR) << "Unable to read " << preserved_dump_path_.value(); return false; } return true; @@ -50,13 +48,13 @@ bool KernelCollector::Enable() { if (!file_util::PathExists(preserved_dump_path_)) { - logger_->LogWarning("Kernel does not support crash dumping"); + LOG(WARNING) << "Kernel does not support crash dumping"; return false; } // To enable crashes, we will eventually need to set // the chnv bit in BIOS, but it does not yet work. - logger_->LogInfo("Enabling kernel crash handling"); + LOG(INFO) << "Enabling kernel crash handling"; is_enabled_ = true; return true; } @@ -68,10 +66,10 @@ preserved_dump_path_, kClearingSequence, strlen(kClearingSequence)) != strlen(kClearingSequence)) { - logger_->LogError("Failed to clear kernel crash dump"); + LOG(ERROR) << "Failed to clear kernel crash dump"; return false; } - logger_->LogInfo("Cleared kernel crash diagnostics"); + LOG(INFO) << "Cleared kernel crash diagnostics"; return true; } @@ -256,10 +254,9 @@ bool feedback = is_feedback_allowed_function_(); - logger_->LogInfo("Received prior crash notification from " - "kernel (signature %s) (%s)", - signature.c_str(), - feedback ? "handling" : "ignoring - no consent"); + LOG(INFO) << "Received prior crash notification from " + << "kernel (signature " << signature << ") (" + << (feedback ? "handling" : "ignoring - no consent") << ")"; if (feedback) { count_crash_function_(); @@ -284,8 +281,8 @@ kernel_dump.data(), kernel_dump.length()) != static_cast<int>(kernel_dump.length())) { - logger_->LogInfo("Failed to write kernel dump to %s", - kernel_crash_path.value().c_str()); + LOG(INFO) << "Failed to write kernel dump to " + << kernel_crash_path.value().c_str(); return true; } @@ -296,8 +293,7 @@ kKernelExecName, kernel_crash_path.value()); - logger_->LogInfo("Stored kcrash to %s", - kernel_crash_path.value().c_str()); + LOG(INFO) << "Stored kcrash to " << kernel_crash_path.value(); } if (!ClearPreservedDump()) { return false;
diff --git a/kernel_collector_test.cc b/kernel_collector_test.cc index 7b84229..f7c88f0 100644 --- a/kernel_collector_test.cc +++ b/kernel_collector_test.cc
@@ -6,8 +6,9 @@ #include "base/file_util.h" #include "base/string_util.h" +#include "chromeos/syslog_logging.h" +#include "chromeos/test_helpers.h" #include "crash-reporter/kernel_collector.h" -#include "crash-reporter/system_logging_mock.h" #include "gflags/gflags.h" #include "gtest/gtest.h" @@ -17,6 +18,8 @@ static const char kTestKCrash[] = "test/kcrash"; static const char kTestCrashDirectory[] = "test/crash_directory"; +using chromeos::FindLog; + void CountCrash() { ++s_crashes; } @@ -30,13 +33,13 @@ s_crashes = 0; s_metrics = true; collector_.Initialize(CountCrash, - IsMetrics, - &logging_); + IsMetrics); mkdir("test", 0777); test_kcrash_ = FilePath(kTestKCrash); collector_.OverridePreservedDumpPath(test_kcrash_); unlink(kTestKCrash); mkdir(kTestCrashDirectory, 0777); + chromeos::ClearLog(); } protected: void WriteStringToFile(const FilePath &file_path, @@ -48,7 +51,6 @@ void SetUpSuccessfulCollect(); void CheckPreservedDumpClear(); - SystemLoggingMock logging_; KernelCollector collector_; FilePath test_kcrash_; }; @@ -68,10 +70,8 @@ TEST_F(KernelCollectorTest, EnableMissingKernel) { ASSERT_FALSE(collector_.Enable()); ASSERT_FALSE(collector_.IsEnabled()); - ASSERT_EQ(std::string::npos, - logging_.log().find("Enabling kernel crash handling")); - ASSERT_NE(std::string::npos, - logging_.log().find("Kernel does not support crash dumping")); + ASSERT_TRUE(FindLog( + "Kernel does not support crash dumping")); ASSERT_EQ(s_crashes, 0); } @@ -79,8 +79,7 @@ WriteStringToFile(test_kcrash_, ""); ASSERT_TRUE(collector_.Enable()); ASSERT_TRUE(collector_.IsEnabled()); - ASSERT_NE(std::string::npos, - logging_.log().find("Enabling kernel crash handling")); + ASSERT_TRUE(FindLog("Enabling kernel crash handling")); ASSERT_EQ(s_crashes, 0); } @@ -97,24 +96,22 @@ TEST_F(KernelCollectorTest, CollectPreservedFileMissing) { ASSERT_FALSE(collector_.Collect()); - ASSERT_NE(logging_.log().find("Unable to read test/kcrash"), - std::string::npos); + ASSERT_TRUE(FindLog("Unable to read test/kcrash")); ASSERT_EQ(0, s_crashes); } TEST_F(KernelCollectorTest, CollectNoCrash) { WriteStringToFile(test_kcrash_, ""); ASSERT_FALSE(collector_.Collect()); - ASSERT_EQ(logging_.log().find("Collected kernel crash"), - std::string::npos); + ASSERT_FALSE(FindLog("Collected kernel crash")); ASSERT_EQ(0, s_crashes); } TEST_F(KernelCollectorTest, CollectBadDirectory) { WriteStringToFile(test_kcrash_, "something"); ASSERT_TRUE(collector_.Collect()); - ASSERT_NE(logging_.log().find( - "Unable to create appropriate crash directory"), std::string::npos); + ASSERT_TRUE(FindLog( + "Unable to create appropriate crash directory")); ASSERT_EQ(1, s_crashes); } @@ -135,7 +132,7 @@ SetUpSuccessfulCollect(); s_metrics = false; ASSERT_TRUE(collector_.Collect()); - ASSERT_NE(std::string::npos, logging_.log().find("(ignoring - no consent)")); + ASSERT_TRUE(FindLog("(ignoring - no consent)")); ASSERT_EQ(0, s_crashes); CheckPreservedDumpClear(); @@ -145,12 +142,13 @@ SetUpSuccessfulCollect(); ASSERT_TRUE(collector_.Collect()); ASSERT_EQ(1, s_crashes); - ASSERT_NE(std::string::npos, logging_.log().find("(handling)")); + ASSERT_TRUE(FindLog("(handling)")); static const char kNamePrefix[] = "Stored kcrash to "; - size_t pos = logging_.log().find(kNamePrefix); + std::string log = chromeos::GetLog(); + size_t pos = log.find(kNamePrefix); ASSERT_NE(std::string::npos, pos); pos += strlen(kNamePrefix); - std::string filename = logging_.log().substr(pos, std::string::npos); + std::string filename = log.substr(pos, std::string::npos); // Take the name up until \n size_t end_pos = filename.find_first_of("\n"); ASSERT_NE(std::string::npos, end_pos); @@ -272,6 +270,6 @@ } int main(int argc, char **argv) { - ::testing::InitGoogleTest(&argc, argv); + SetUpTests(&argc, argv, false); return RUN_ALL_TESTS(); }
diff --git a/system_logging.cc b/system_logging.cc deleted file mode 100644 index 04bb356..0000000 --- a/system_logging.cc +++ /dev/null
@@ -1,55 +0,0 @@ -// Copyright (c) 2010 The Chromium OS 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 "crash-reporter/system_logging.h" - -#include <syslog.h> - -#include "base/stringprintf.h" - -std::string SystemLoggingImpl::identity_; - -SystemLoggingImpl::SystemLoggingImpl() : is_accumulating_(false) { -} - -SystemLoggingImpl::~SystemLoggingImpl() { -} - -void SystemLoggingImpl::Initialize(const char *ident) { - // Man page does not specify if openlog copies its string or assumes - // the pointer is always valid, so make its scope global. - identity_ = ident; - openlog(identity_.c_str(), LOG_PID, LOG_USER); -} - -void SystemLoggingImpl::LogWithLevel(int level, const char *format, - va_list arg_list) { - std::string message = StringPrintV(format, arg_list); - syslog(level, "%s", message.c_str()); - if (is_accumulating_) { - accumulator_.append(message); - accumulator_.push_back('\n'); - } -} - -void SystemLoggingImpl::LogInfo(const char *format, ...) { - va_list vl; - va_start(vl, format); - LogWithLevel(LOG_INFO, format, vl); - va_end(vl); -} - -void SystemLoggingImpl::LogWarning(const char *format, ...) { - va_list vl; - va_start(vl, format); - LogWithLevel(LOG_WARNING, format, vl); - va_end(vl); -} - -void SystemLoggingImpl::LogError(const char *format, ...) { - va_list vl; - va_start(vl, format); - LogWithLevel(LOG_ERR, format, vl); - va_end(vl); -}
diff --git a/system_logging.h b/system_logging.h deleted file mode 100644 index d1f72a5..0000000 --- a/system_logging.h +++ /dev/null
@@ -1,45 +0,0 @@ -// Copyright (c) 2010 The Chromium OS 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 CRASH_REPORTER_SYSTEM_LOGGING_H_ -#define CRASH_REPORTER_SYSTEM_LOGGING_H_ - -#include <stdarg.h> -#include <string> - -class SystemLogging { - public: - virtual void Initialize(const char *ident) = 0; - virtual void LogInfo(const char *format, ...) = 0; - virtual void LogWarning(const char *format, ...) = 0; - virtual void LogError(const char *format, ...) = 0; - virtual void set_accumulating(bool value) = 0; - virtual std::string get_accumulator() = 0; -}; - -// SystemLoggingImpl implements SystemLogging but adds the -// capability of accumulating the log to a STL string. -class SystemLoggingImpl : public SystemLogging { - public: - SystemLoggingImpl(); - virtual ~SystemLoggingImpl(); - virtual void Initialize(const char *ident); - virtual void LogInfo(const char *format, ...); - virtual void LogWarning(const char *format, ...); - virtual void LogError(const char *format, ...); - virtual void set_accumulating(bool value) { - is_accumulating_ = value; - } - virtual std::string get_accumulator() { - return accumulator_; - } - private: - static std::string identity_; - std::string accumulator_; - bool is_accumulating_; - void LogWithLevel(int level, const char *format, - va_list arg_list); -}; - -#endif // CRASH_REPORTER_SYSTEM_LOGGING_H_
diff --git a/system_logging_mock.cc b/system_logging_mock.cc deleted file mode 100644 index 04ce853..0000000 --- a/system_logging_mock.cc +++ /dev/null
@@ -1,42 +0,0 @@ -// Copyright (c) 2010 The Chromium OS 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 <stdarg.h> - -#include "base/string_util.h" -#include "crash-reporter/system_logging_mock.h" - -void SystemLoggingMock::LogInfo(const char *format, ...) { - va_list vl; - va_start(vl, format); - log_ += ident_ + "info: "; - StringAppendV(&log_, format, vl); - log_ += "\n"; - va_end(vl); -} - -void SystemLoggingMock::LogWarning(const char *format, ...) { - va_list vl; - va_start(vl, format); - log_ += ident_ + "warning: "; - StringAppendV(&log_, format, vl); - log_ += "\n"; - va_end(vl); -} - -void SystemLoggingMock::LogError(const char *format, ...) { - va_list vl; - va_start(vl, format); - log_ += ident_ + "error: "; - StringAppendV(&log_, format, vl); - log_ += "\n"; - va_end(vl); -} - -void SystemLoggingMock::set_accumulating(bool value) { -} - -std::string SystemLoggingMock::get_accumulator() { - return ""; -}
diff --git a/system_logging_mock.h b/system_logging_mock.h deleted file mode 100644 index 56c6e58..0000000 --- a/system_logging_mock.h +++ /dev/null
@@ -1,31 +0,0 @@ -// Copyright (c) 2010 The Chromium OS 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 CRASH_REPORTER_SYSTEM_LOGGING_MOCK_H_ -#define CRASH_REPORTER_SYSTEM_LOGGING_MOCK_H_ - -#include <string> - -#include "crash-reporter/system_logging.h" - -class SystemLoggingMock : public SystemLogging { - public: - void Initialize(const char *ident) {} - virtual void LogInfo(const char *format, ...); - virtual void LogWarning(const char *format, ...); - virtual void LogError(const char *format, ...); - virtual void set_accumulating(bool value); - virtual std::string get_accumulator(); - - const std::string &log() { return log_; } - - void clear() { log_.clear(); } - - private: - static std::string identity_; - std::string log_; - std::string ident_; -}; - -#endif // CRASH_REPORTER_SYSTEM_LOGGING_H_
diff --git a/test_helpers.h b/test_helpers.h deleted file mode 100644 index b515901..0000000 --- a/test_helpers.h +++ /dev/null
@@ -1,18 +0,0 @@ -// Copyright (c) 2010 The Chromium OS 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 _CRASH_REPORTER_TEST_HELPERS_H_ -#define _CRASH_REPORTER_TEST_HELPERS_H_ - -#include "gtest/gtest.h" - -inline void ExpectFileEquals(const char *golden, - const char *file_path) { - std::string contents; - EXPECT_TRUE(file_util::ReadFileToString(FilePath(file_path), - &contents)); - EXPECT_EQ(golden, contents); -} - -#endif // _CRASH_REPORTER_TEST_HELPERS_H_
diff --git a/unclean_shutdown_collector.cc b/unclean_shutdown_collector.cc index 1e1c784..ad7386f 100644 --- a/unclean_shutdown_collector.cc +++ b/unclean_shutdown_collector.cc
@@ -6,7 +6,6 @@ #include "base/file_util.h" #include "base/logging.h" -#include "crash-reporter/system_logging.h" static const char kUncleanShutdownFile[] = "/var/lib/crash_reporter/pending_clean_shutdown"; @@ -32,7 +31,7 @@ FilePath file_path(unclean_shutdown_file_); file_util::CreateDirectory(file_path.DirName()); if (file_util::WriteFile(file_path, "", 0) != 0) { - logger_->LogError("Unable to create shutdown check file"); + LOG(ERROR) << "Unable to create shutdown check file"; return false; } return true; @@ -40,8 +39,8 @@ bool UncleanShutdownCollector::DeleteUncleanShutdownFiles() { if (!file_util::Delete(FilePath(unclean_shutdown_file_), false)) { - logger_->LogError("Failed to delete unclean shutdown file %s", - unclean_shutdown_file_); + LOG(ERROR) << "Failed to delete unclean shutdown file " + << unclean_shutdown_file_; return false; } // Delete power manager trace files if they exist. @@ -55,7 +54,7 @@ if (!file_util::PathExists(unclean_file_path)) { return false; } - logger_->LogWarning("Last shutdown was not clean"); + LOG(WARNING) << "Last shutdown was not clean"; if (DeadBatteryCausedUncleanShutdown()) { DeleteUncleanShutdownFiles(); return false; @@ -69,7 +68,7 @@ } bool UncleanShutdownCollector::Disable() { - logger_->LogInfo("Clean shutdown signalled"); + LOG(INFO) << "Clean shutdown signalled"; return DeleteUncleanShutdownFiles(); } @@ -77,16 +76,16 @@ { // Check for case of battery running out while suspended. if (file_util::PathExists(powerd_suspended_file_)) { - logger_->LogInfo("Unclean shutdown occurred while suspended. Not counting " - "toward unclean shutdown statistic."); + LOG(INFO) << "Unclean shutdown occurred while suspended. Not counting " + << "toward unclean shutdown statistic."; return true; } // Check for case of battery running out after resuming from a low-battery // suspend. if (file_util::PathExists(powerd_low_battery_file_)) { - logger_->LogInfo("Unclean shutdown occurred while running with battery " - "critically low. Not counting toward unclean shutdown " - "statistic."); + LOG(INFO) << "Unclean shutdown occurred while running with battery " + << "critically low. Not counting toward unclean shutdown " + << "statistic."; return true; } return false;
diff --git a/unclean_shutdown_collector_test.cc b/unclean_shutdown_collector_test.cc index 2cf9fb5..89b0ec9 100644 --- a/unclean_shutdown_collector_test.cc +++ b/unclean_shutdown_collector_test.cc
@@ -6,8 +6,9 @@ #include "base/file_util.h" #include "base/string_util.h" +#include "chromeos/syslog_logging.h" +#include "chromeos/test_helpers.h" #include "crash-reporter/unclean_shutdown_collector.h" -#include "crash-reporter/system_logging_mock.h" #include "gflags/gflags.h" #include "gtest/gtest.h" @@ -18,6 +19,8 @@ static const char kTestSuspended[] = "test/suspended"; static const char kTestUnclean[] = "test/unclean"; +using ::chromeos::FindLog; + void CountCrash() { ++s_crashes; } @@ -30,8 +33,7 @@ void SetUp() { s_crashes = 0; collector_.Initialize(CountCrash, - IsMetrics, - &logging_); + IsMetrics); rmdir("test"); test_unclean_ = FilePath(kTestUnclean); collector_.unclean_shutdown_file_ = kTestUnclean; @@ -39,6 +41,7 @@ // Set up alternate power manager tracing files as well collector_.powerd_suspended_file_ = FilePath(kTestSuspended); collector_.powerd_low_battery_file_ = FilePath(kTestLowBattery); + chromeos::ClearLog(); } protected: void WriteStringToFile(const FilePath &file_path, @@ -47,7 +50,6 @@ file_util::WriteFile(file_path, data, strlen(data))); } - SystemLoggingMock logging_; UncleanShutdownCollector collector_; FilePath test_unclean_; }; @@ -66,8 +68,7 @@ TEST_F(UncleanShutdownCollectorTest, EnableCannotWrite) { collector_.unclean_shutdown_file_ = "/bad/path"; ASSERT_FALSE(collector_.Enable()); - ASSERT_NE(std::string::npos, - logging_.log().find("Unable to create shutdown check file")); + ASSERT_TRUE(FindLog("Unable to create shutdown check file")); } TEST_F(UncleanShutdownCollectorTest, CollectTrue) { @@ -76,8 +77,7 @@ ASSERT_TRUE(collector_.Collect()); ASSERT_FALSE(file_util::PathExists(test_unclean_)); ASSERT_EQ(1, s_crashes); - ASSERT_NE(std::string::npos, - logging_.log().find("Last shutdown was not clean")); + ASSERT_TRUE(FindLog("Last shutdown was not clean")); } TEST_F(UncleanShutdownCollectorTest, CollectFalse) { @@ -93,9 +93,8 @@ ASSERT_FALSE(file_util::PathExists(test_unclean_)); ASSERT_FALSE(file_util::PathExists(collector_.powerd_low_battery_file_)); ASSERT_EQ(0, s_crashes); - ASSERT_NE(std::string::npos, - logging_.log().find("Unclean shutdown occurred while running with " - "battery critically low.")); + ASSERT_TRUE(FindLog( + "Unclean shutdown occurred while running with battery critically low.")); } TEST_F(UncleanShutdownCollectorTest, CollectDeadBatterySuspended) { @@ -106,8 +105,7 @@ ASSERT_FALSE(file_util::PathExists(test_unclean_)); ASSERT_FALSE(file_util::PathExists(collector_.powerd_suspended_file_)); ASSERT_EQ(0, s_crashes); - ASSERT_NE(std::string::npos, - logging_.log().find("Unclean shutdown occurred while suspended.")); + ASSERT_TRUE(FindLog("Unclean shutdown occurred while suspended.")); } TEST_F(UncleanShutdownCollectorTest, Disable) { @@ -130,6 +128,6 @@ } int main(int argc, char **argv) { - ::testing::InitGoogleTest(&argc, argv); + SetUpTests(&argc, argv, false); return RUN_ALL_TESTS(); }
diff --git a/user_collector.cc b/user_collector.cc index 6e6c765..bfaaddf 100644 --- a/user_collector.cc +++ b/user_collector.cc
@@ -16,7 +16,8 @@ #include "base/file_util.h" #include "base/logging.h" #include "base/string_util.h" -#include "crash-reporter/system_logging.h" +#include "chromeos/process.h" +#include "chromeos/syslog_logging.h" #include "gflags/gflags.h" #pragma GCC diagnostic ignored "-Wstrict-aliasing" @@ -55,11 +56,9 @@ UserCollector::CountCrashFunction count_crash_function, const std::string &our_path, UserCollector::IsFeedbackAllowedFunction is_feedback_allowed_function, - SystemLogging *logger, bool generate_diagnostics) { CrashCollector::Initialize(count_crash_function, - is_feedback_allowed_function, - logger); + is_feedback_allowed_function); our_path_ = our_path; initialized_ = true; generate_diagnostics_ = generate_diagnostics; @@ -82,13 +81,13 @@ bool UserCollector::SetUpInternal(bool enabled) { CHECK(initialized_); - logger_->LogInfo("%s user crash handling", - enabled ? "Enabling" : "Disabling"); + LOG(INFO) << (enabled ? "Enabling" : "Disabling") << " user crash handling"; + if (file_util::WriteFile(FilePath(core_pipe_limit_file_), kCorePipeLimit, strlen(kCorePipeLimit)) != static_cast<int>(strlen(kCorePipeLimit))) { - logger_->LogError("Unable to write %s", core_pipe_limit_file_.c_str()); + LOG(ERROR) << "Unable to write " << core_pipe_limit_file_; return false; } std::string pattern = GetPattern(enabled); @@ -96,7 +95,7 @@ pattern.c_str(), pattern.length()) != static_cast<int>(pattern.length())) { - logger_->LogError("Unable to write %s", core_pattern_file_.c_str()); + LOG(ERROR) << "Unable to write " << core_pattern_file_; return false; } return true; @@ -115,8 +114,8 @@ ssize_t size = readlink(symlink.value().c_str(), buffer.get(), max_size); if (size < 0) { int saved_errno = errno; - logger_->LogError("Readlink failed on %s with %d", - symlink.value().c_str(), saved_errno); + LOG(ERROR) << "Readlink failed on " << symlink.value() << " with " + << saved_errno; return false; } buffer[size] = 0; @@ -142,19 +141,19 @@ FilePath process_path = GetProcessPath(pid); FilePath exe_path = process_path.Append("exe"); if (!GetSymlinkTarget(exe_path, &target)) { - logger_->LogInfo("GetSymlinkTarget failed - Path %s DirectoryExists: %d", - process_path.value().c_str(), - file_util::DirectoryExists(process_path)); + LOG(INFO) << "GetSymlinkTarget failed - Path " << process_path.value() + << " DirectoryExists: " + << file_util::DirectoryExists(process_path); // Try to further diagnose exe readlink failure cause. struct stat buf; int stat_result = stat(exe_path.value().c_str(), &buf); int saved_errno = errno; if (stat_result < 0) { - logger_->LogInfo("stat %s failed: %d %d", exe_path.value().c_str(), - stat_result, saved_errno); + LOG(INFO) << "stat " << exe_path.value() << " failed: " << stat_result + << " " << saved_errno; } else { - logger_->LogInfo("stat %s succeeded: st_mode=%d", - exe_path.value().c_str(), buf.st_mode); + LOG(INFO) << "stat " << exe_path.value() << " succeeded: st_mode=" + << buf.st_mode; } return false; } @@ -198,13 +197,13 @@ void UserCollector::EnqueueCollectionErrorLog(pid_t pid, const std::string &exec) { FilePath crash_path; - logger_->LogInfo("Writing conversion problems as separate crash report."); + LOG(INFO) << "Writing conversion problems as separate crash report."; if (!GetCreatedCrashDirectoryByEuid(0, &crash_path, NULL)) { - logger_->LogError("Could not even get log directory; out of space?"); + LOG(ERROR) << "Could not even get log directory; out of space?"; return; } std::string dump_basename = FormatDumpBasename(exec, time(NULL), pid); - std::string error_log = logger_->get_accumulator(); + std::string error_log = chromeos::GetLog(); FilePath diag_log_path = GetCrashPath(crash_path, dump_basename, "diaglog"); if (GetLogContents(FilePath(kDefaultLogConfig), kCollectionErrorSignature, diag_log_path)) { @@ -229,13 +228,12 @@ bool UserCollector::CopyOffProcFiles(pid_t pid, const FilePath &container_dir) { if (!file_util::CreateDirectory(container_dir)) { - logger_->LogError("Could not create %s", - container_dir.value().c_str()); + LOG(ERROR) << "Could not create " << container_dir.value().c_str(); return false; } FilePath process_path = GetProcessPath(pid); if (!file_util::PathExists(process_path)) { - logger_->LogError("Path %s does not exist", process_path.value().c_str()); + LOG(ERROR) << "Path " << process_path.value() << " does not exist"; return false; } static const char *proc_files[] = { @@ -248,7 +246,7 @@ for (unsigned i = 0; i < arraysize(proc_files); ++i) { if (!file_util::CopyFile(process_path.Append(proc_files[i]), container_dir.Append(proc_files[i]))) { - logger_->LogError("Could not copy %s file", proc_files[i]); + LOG(ERROR) << "Could not copy " << proc_files[i] << " file"; return false; } } @@ -261,26 +259,25 @@ FilePath process_path = GetProcessPath(pid); std::string status; if (FLAGS_directory_failure) { - logger_->LogError("Purposefully failing to create spool directory"); + LOG(ERROR) << "Purposefully failing to create spool directory"; return false; } if (!file_util::ReadFileToString(process_path.Append("status"), &status)) { - logger_->LogError("Could not read status file"); - logger_->LogInfo("Path %s DirectoryExists: %d", - process_path.value().c_str(), - file_util::DirectoryExists(process_path)); + LOG(ERROR) << "Could not read status file"; + LOG(INFO) << "Path " << process_path.value() << " DirectoryExists: " + << file_util::DirectoryExists(process_path); return false; } int process_euid; if (!GetIdFromStatus(kUserId, kIdEffective, status, &process_euid)) { - logger_->LogError("Could not find euid in status file"); + LOG(ERROR) << "Could not find euid in status file"; return false; } if (!GetCreatedCrashDirectoryByEuid(process_euid, crash_file_path, out_of_capacity)) { - logger_->LogError("Could not create crash directory"); + LOG(ERROR) << "Could not create crash directory"; return false; } return true; @@ -293,7 +290,7 @@ return true; } - logger_->LogError("Could not write core file"); + LOG(ERROR) << "Could not write core file"; // If the file system was full, make sure we remove any remnants. file_util::Delete(core_path, false); return false; @@ -304,34 +301,32 @@ const FilePath &minidump_path, const FilePath &temp_directory) { FilePath output_path = temp_directory.Append("output"); - std::vector<const char *> core2md_arguments; - core2md_arguments.push_back(kCoreToMinidumpConverterPath); - core2md_arguments.push_back(core_path.value().c_str()); - core2md_arguments.push_back(procfs_directory.value().c_str()); - core2md_arguments.push_back(minidump_path.value().c_str()); + chromeos::ProcessImpl core2md; + core2md.RedirectOutput(output_path.value()); + core2md.AddArg(kCoreToMinidumpConverterPath); + core2md.AddArg(core_path.value()); + core2md.AddArg(procfs_directory.value()); - if (FLAGS_core2md_failure) { + if (!FLAGS_core2md_failure) { + core2md.AddArg(minidump_path.value()); + } else { // To test how core2md errors are propagaged, cause an error // by forgetting a required argument. - core2md_arguments.pop_back(); } - int errorlevel = ForkExecAndPipe(core2md_arguments, - output_path.value().c_str()); + int errorlevel = core2md.Run(); std::string output; file_util::ReadFileToString(output_path, &output); if (errorlevel != 0) { - logger_->LogError("Problem during %s [result=%d]: %s", - kCoreToMinidumpConverterPath, - errorlevel, - output.c_str()); + LOG(ERROR) << "Problem during " << kCoreToMinidumpConverterPath + << " [result=" << errorlevel << "]: " << output; return false; } if (!file_util::PathExists(minidump_path)) { - logger_->LogError("Minidump file %s was not created", - minidump_path.value().c_str()); + LOG(ERROR) << "Minidump file " << minidump_path.value() + << " was not created"; return false; } return true; @@ -356,7 +351,7 @@ container_dir); // temporary directory if (conversion_result) { - logger_->LogInfo("Stored minidump to %s", minidump_path.value().c_str()); + LOG(INFO) << "Stored minidump to " << minidump_path.value(); } return conversion_result; @@ -367,7 +362,7 @@ bool *out_of_capacity) { FilePath crash_path; if (!GetCreatedCrashDirectory(pid, &crash_path, out_of_capacity)) { - logger_->LogError("Unable to find/create process-specific crash path"); + LOG(ERROR) << "Unable to find/create process-specific crash path"; return false; } @@ -390,8 +385,8 @@ if (!ConvertCoreToMinidump(pid, container_dir, core_path, minidump_path)) { - logger_->LogInfo("Leaving core file at %s due to conversion error", - core_path.value().c_str()); + LOG(INFO) << "Leaving core file at " << core_path.value() + << " due to conversion error"; return false; } @@ -405,8 +400,8 @@ if (!file_util::PathExists(FilePath(kLeaveCoreFile))) { file_util::Delete(core_path, false); } else { - logger_->LogInfo("Leaving core file at %s due to developer image", - core_path.value().c_str()); + LOG(INFO) << "Leaving core file at " << core_path.value() + << " due to developer image"; } file_util::Delete(container_dir, true); @@ -429,7 +424,7 @@ if (!ParseCrashAttributes(crash_attributes, &pid, &signal, &kernel_supplied_name)) { - logger_->LogError("Invalid parameter: --user=%s", crash_attributes.c_str()); + LOG(ERROR) << "Invalid parameter: --user=" << crash_attributes; return false; } @@ -450,8 +445,8 @@ FLAGS_filter_in != exec)) { // We use a different format message to make it more obvious in tests // which crashes are test generated and which are real. - logger_->LogWarning("Ignoring crash from %s[%d] while filter_in=%s.", - exec.c_str(), pid, FLAGS_filter_in.c_str()); + LOG(WARNING) << "Ignoring crash from " << exec << "[" << pid << "] while " + << "filter_in=" << FLAGS_filter_in << "."; return true; } @@ -469,8 +464,8 @@ handling_string = "ignoring - chrome crash"; } - logger_->LogWarning("Received crash notification for %s[%d] sig %d (%s)", - exec.c_str(), pid, signal, handling_string); + LOG(WARNING) << "Received crash notification for " << exec << "[" << pid + << "] sig " << signal << " (" << handling_string << ")"; if (feedback) { count_crash_function_();
diff --git a/user_collector.h b/user_collector.h index 21ef14b..e46382c 100644 --- a/user_collector.h +++ b/user_collector.h
@@ -28,7 +28,6 @@ void Initialize(CountCrashFunction count_crash, const std::string &our_path, IsFeedbackAllowedFunction is_metrics_allowed, - SystemLogging *logger, bool generate_diagnostics); virtual ~UserCollector();
diff --git a/user_collector_test.cc b/user_collector_test.cc index 9a1d687..93a76ad 100644 --- a/user_collector_test.cc +++ b/user_collector_test.cc
@@ -5,9 +5,9 @@ #include <unistd.h> #include "base/file_util.h" -#include "crash-reporter/system_logging_mock.h" +#include "chromeos/syslog_logging.h" +#include "chromeos/test_helpers.h" #include "crash-reporter/user_collector.h" -#include "crash-reporter/test_helpers.h" #include "gflags/gflags.h" #include "gtest/gtest.h" @@ -16,6 +16,8 @@ static const char kFilePath[] = "/my/path"; +using chromeos::FindLog; + void CountCrash() { ++s_crashes; } @@ -30,13 +32,13 @@ collector_.Initialize(CountCrash, kFilePath, IsMetrics, - &logging_, false); file_util::Delete(FilePath("test"), true); mkdir("test", 0777); collector_.set_core_pattern_file("test/core_pattern"); collector_.set_core_pipe_limit_file("test/core_pipe_limit"); pid_ = getpid(); + chromeos::ClearLog(); } protected: void ExpectFileEquals(const char *golden, @@ -47,7 +49,6 @@ EXPECT_EQ(golden, contents); } - SystemLoggingMock logging_; UserCollector collector_; pid_t pid_; }; @@ -57,18 +58,15 @@ ExpectFileEquals("|/my/path --user=%p:%s:%e", "test/core_pattern"); ExpectFileEquals("4", "test/core_pipe_limit"); ASSERT_EQ(s_crashes, 0); - ASSERT_NE(logging_.log().find("Enabling user crash handling"), - std::string::npos); + EXPECT_TRUE(FindLog("Enabling user crash handling")); } TEST_F(UserCollectorTest, EnableNoPatternFileAccess) { collector_.set_core_pattern_file("/does_not_exist"); ASSERT_FALSE(collector_.Enable()); ASSERT_EQ(s_crashes, 0); - ASSERT_NE(logging_.log().find("Enabling user crash handling"), - std::string::npos); - ASSERT_NE(logging_.log().find("Unable to write /does_not_exist"), - std::string::npos); + EXPECT_TRUE(FindLog("Enabling user crash handling")); + EXPECT_TRUE(FindLog("Unable to write /does_not_exist")); } TEST_F(UserCollectorTest, EnableNoPipeLimitFileAccess) { @@ -78,28 +76,23 @@ // Core pattern should not be written if we cannot access the pipe limit // or otherwise we may set a pattern that results in infinite recursion. ASSERT_FALSE(file_util::PathExists(FilePath("test/core_pattern"))); - ASSERT_NE(logging_.log().find("Enabling user crash handling"), - std::string::npos); - ASSERT_NE(logging_.log().find("Unable to write /does_not_exist"), - std::string::npos); + EXPECT_TRUE(FindLog("Enabling user crash handling")); + EXPECT_TRUE(FindLog("Unable to write /does_not_exist")); } TEST_F(UserCollectorTest, DisableOK) { ASSERT_TRUE(collector_.Disable()); ExpectFileEquals("core", "test/core_pattern"); ASSERT_EQ(s_crashes, 0); - ASSERT_NE(logging_.log().find("Disabling user crash handling"), - std::string::npos); + EXPECT_TRUE(FindLog("Disabling user crash handling")); } TEST_F(UserCollectorTest, DisableNoFileAccess) { collector_.set_core_pattern_file("/does_not_exist"); ASSERT_FALSE(collector_.Disable()); ASSERT_EQ(s_crashes, 0); - ASSERT_NE(logging_.log().find("Disabling user crash handling"), - std::string::npos); - ASSERT_NE(logging_.log().find("Unable to write /does_not_exist"), - std::string::npos); + EXPECT_TRUE(FindLog("Disabling user crash handling")); + EXPECT_TRUE(FindLog("Unable to write /does_not_exist")); } TEST_F(UserCollectorTest, ParseCrashAttributes) { @@ -132,40 +125,34 @@ TEST_F(UserCollectorTest, HandleCrashWithoutMetrics) { s_metrics = false; collector_.HandleCrash("20:10:ignored", "foobar"); - ASSERT_NE(std::string::npos, - logging_.log().find( - "Received crash notification for foobar[20] sig 10")); + EXPECT_TRUE(FindLog( + "Received crash notification for foobar[20] sig 10")); ASSERT_EQ(s_crashes, 0); } TEST_F(UserCollectorTest, HandleNonChromeCrashWithMetrics) { s_metrics = true; collector_.HandleCrash("5:2:ignored", "chromeos-wm"); - ASSERT_NE(std::string::npos, - logging_.log().find( - "Received crash notification for chromeos-wm[5] sig 2")); + EXPECT_TRUE(FindLog( + "Received crash notification for chromeos-wm[5] sig 2")); ASSERT_EQ(s_crashes, 1); } TEST_F(UserCollectorTest, HandleChromeCrashWithMetrics) { s_metrics = true; collector_.HandleCrash("5:2:ignored", "chrome"); - ASSERT_NE(std::string::npos, - logging_.log().find( - "Received crash notification for chrome[5] sig 2")); - ASSERT_NE(std::string::npos, - logging_.log().find("(ignoring - chrome crash)")); + EXPECT_TRUE(FindLog( + "Received crash notification for chrome[5] sig 2")); + EXPECT_TRUE(FindLog("(ignoring - chrome crash)")); ASSERT_EQ(s_crashes, 0); } TEST_F(UserCollectorTest, HandleSuppliedChromeCrashWithMetrics) { s_metrics = true; collector_.HandleCrash("0:2:chrome", NULL); - ASSERT_NE(std::string::npos, - logging_.log().find( - "Received crash notification for supplied_chrome[0] sig 2")); - ASSERT_NE(std::string::npos, - logging_.log().find("(ignoring - chrome crash)")); + EXPECT_TRUE(FindLog( + "Received crash notification for supplied_chrome[0] sig 2")); + EXPECT_TRUE(FindLog("(ignoring - chrome crash)")); ASSERT_EQ(s_crashes, 0); } @@ -178,9 +165,8 @@ FilePath result; ASSERT_FALSE(collector_.GetSymlinkTarget(FilePath("/does_not_exist"), &result)); - ASSERT_NE(std::string::npos, - logging_.log().find( - "Readlink failed on /does_not_exist with 2")); + ASSERT_TRUE(FindLog( + "Readlink failed on /does_not_exist with 2")); std::string long_link; for (int i = 0; i < 50; ++i) long_link += "0123456789"; @@ -201,23 +187,16 @@ TEST_F(UserCollectorTest, GetExecutableBaseNameFromPid) { std::string base_name; EXPECT_FALSE(collector_.GetExecutableBaseNameFromPid(0, &base_name)); - EXPECT_NE(std::string::npos, - logging_.log().find( - "Readlink failed on /proc/0/exe with 2")); - EXPECT_NE(std::string::npos, - logging_.log().find( - "GetSymlinkTarget failed - Path " - "/proc/0 DirectoryExists: 0")); - EXPECT_NE(std::string::npos, - logging_.log().find( - "stat /proc/0/exe failed: -1 2")); + EXPECT_TRUE(FindLog( + "Readlink failed on /proc/0/exe with 2")); + EXPECT_TRUE(FindLog( + "GetSymlinkTarget failed - Path /proc/0 DirectoryExists: 0")); + EXPECT_TRUE(FindLog("stat /proc/0/exe failed: -1 2")); - logging_.clear(); + chromeos::ClearLog(); pid_t my_pid = getpid(); EXPECT_TRUE(collector_.GetExecutableBaseNameFromPid(my_pid, &base_name)); - EXPECT_EQ(std::string::npos, - logging_.log().find( - "Readlink failed")); + EXPECT_FALSE(FindLog("Readlink failed")); EXPECT_EQ("user_collector_test", base_name); } @@ -303,24 +282,19 @@ TEST_F(UserCollectorTest, CopyOffProcFilesBadPath) { // Try a path that is not writable. ASSERT_FALSE(collector_.CopyOffProcFiles(pid_, FilePath("/bad/path"))); - ASSERT_NE(logging_.log().find( - "Could not create /bad/path"), - std::string::npos); + EXPECT_TRUE(FindLog("Could not create /bad/path")); } TEST_F(UserCollectorTest, CopyOffProcFilesBadPid) { FilePath container_path("test/container"); ASSERT_FALSE(collector_.CopyOffProcFiles(0, container_path)); - ASSERT_NE(logging_.log().find( - "Path /proc/0 does not exist"), - std::string::npos); + EXPECT_TRUE(FindLog("Path /proc/0 does not exist")); } TEST_F(UserCollectorTest, CopyOffProcFilesOK) { FilePath container_path("test/container"); ASSERT_TRUE(collector_.CopyOffProcFiles(pid_, container_path)); - ASSERT_EQ(logging_.log().find( - "Could not copy"), std::string::npos); + EXPECT_FALSE(FindLog("Could not copy")); static struct { const char *name; bool exists; @@ -342,6 +316,6 @@ } int main(int argc, char **argv) { - ::testing::InitGoogleTest(&argc, argv); + SetUpTests(&argc, argv, false); return RUN_ALL_TESTS(); }