Create separate udev collector class
Previously added a udev collector to CrashCollector. Now it's time to
put it in its own class.
BUG=chromium-os:30268
TEST=See "TEST=" under https://gerrit.chromium.org/gerrit/21498
Change-Id: I6a62826cb84ef7ccd42a512b00e127af6de3280d
Signed-off-by: Simon Que <sque@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/25112
Reviewed-by: Michael Krebs <mkrebs@chromium.org>
diff --git a/crash_reporter/Makefile b/crash_reporter/Makefile
index d123a80..51f18e8 100644
--- a/crash_reporter/Makefile
+++ b/crash_reporter/Makefile
@@ -12,6 +12,7 @@
CRASH_OBJS = \
crash_collector.o \
kernel_collector.o \
+ udev_collector.o \
unclean_shutdown_collector.o \
user_collector.o
@@ -48,6 +49,7 @@
# Uncomment these to just run specific test(s).
#TEST_BINS += crash_collector_test
#TEST_BINS += kernel_collector_test
+#TEST_BINS += udev_collector_test
#TEST_BINS += unclean_shutdown_collector_test
#TEST_BINS += user_collector_test
diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc
index aaee3cb..35d2e68 100644
--- a/crash_reporter/crash_collector.cc
+++ b/crash_reporter/crash_collector.cc
@@ -26,7 +26,6 @@
static const char kCollectChromeFile[] =
"/mnt/stateful_partition/etc/collect_chrome_crashes";
-static const char kCollectUdevSignature[] = "crash_reporter-udev-collection";
static const char kCrashTestInProgressPath[] = "/tmp/crash-test-in-progress";
static const char kDefaultLogConfig[] = "/etc/crash_reporter_logs.conf";
static const char kDefaultUserName[] = "chronos";
@@ -34,10 +33,7 @@
static const char kLsbRelease[] = "/etc/lsb-release";
static const char kShellPath[] = "/bin/sh";
static const char kSystemCrashPath[] = "/var/spool/crash";
-static const char kUdevExecName[] = "udev";
-static const char kUdevSignatureKey[] = "sig";
static const char kUserCrashPath[] = "/home/chronos/user/crash";
-static const char kGzipPath[] = "/bin/gzip";
// Directory mode of the user crash spool directory.
static const mode_t kUserCrashPathMode = 0755;
@@ -60,7 +56,8 @@
CrashCollector::CrashCollector()
: forced_crash_directory_(NULL),
- lsb_release_(kLsbRelease) {
+ lsb_release_(kLsbRelease),
+ log_config_path_(kDefaultLogConfig) {
}
CrashCollector::~CrashCollector() {
@@ -76,71 +73,6 @@
is_feedback_allowed_function_ = is_feedback_allowed_function;
}
-bool CrashCollector::HandleUdevCrash(const std::string &udev_event) {
- // Process the udev event string.
- // The udev string should be formatted as follows:
- // "ACTION=[action]:KERNEL=[name]:SUBSYSTEM=[subsystem]"
- // The values don't have to be in any particular order.
-
- // First get all the key-value pairs.
- std::vector<std::pair<std::string, std::string> > udev_event_keyval;
- base::SplitStringIntoKeyValuePairs(udev_event, '=', ':', &udev_event_keyval);
- std::vector<std::pair<std::string, std::string> >::const_iterator iter;
- std::map<std::string, std::string> udev_event_map;
- for (iter = udev_event_keyval.begin();
- iter != udev_event_keyval.end();
- ++iter) {
- udev_event_map[iter->first] = iter->second;
- }
-
- // Construct the basename string for crash_reporter_logs.conf:
- // "crash_reporter-udev-collection-[action]-[name]-[subsystem]"
- // If a udev field is not provided, "" is used in its place, e.g.:
- // "crash_reporter-udev-collection-[action]--[subsystem]"
- // Hence, "" is used as a wildcard name string.
- std::string basename = udev_event_map["ACTION"] + "-" +
- udev_event_map["KERNEL"] + "-" +
- udev_event_map["SUBSYSTEM"];
- std::string udev_log_name = std::string(kCollectUdevSignature) + '-' +
- basename;
-
- // Make sure the crash directory exists, or create it if it doesn't.
- FilePath crash_directory;
- if (!GetCreatedCrashDirectoryByEuid(0, &crash_directory, NULL)) {
- LOG(ERROR) << "Could not get crash directory.";
- return false;
- }
- // Create the destination path.
- std::string log_file_name =
- FormatDumpBasename(basename, time(NULL), 0);
- FilePath crash_path = GetCrashPath(crash_directory, log_file_name, "log");
-
- // Handle the crash.
- bool result = GetLogContents(FilePath(kDefaultLogConfig), udev_log_name,
- crash_path);
- if (!result) {
- LOG(ERROR) << "Error reading udev log info " << udev_log_name;
- return false;
- }
-
- // Compress the output using gzip.
- chromeos::ProcessImpl gzip_process;
- gzip_process.AddArg(kGzipPath);
- gzip_process.AddArg(crash_path.value());
- int process_result = gzip_process.Run();
- FilePath crash_path_zipped = FilePath(crash_path.value() + ".gz");
- // If the zip file was not created, use the uncompressed file.
- if (process_result != 0 || !file_util::PathExists(crash_path_zipped))
- LOG(ERROR) << "Could not create zip file " << crash_path_zipped.value();
- else
- crash_path = crash_path_zipped;
-
- AddCrashMetaData(kUdevSignatureKey, kCollectUdevSignature);
- WriteCrashMetaData(GetCrashPath(crash_directory, log_file_name, "meta"),
- kUdevExecName, crash_path.value());
- return true;
-}
-
int CrashCollector::WriteNewFile(const FilePath &filename,
const char *data,
int size) {
diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h
index 77e2121..650f536 100644
--- a/crash_reporter/crash_collector.h
+++ b/crash_reporter/crash_collector.h
@@ -28,9 +28,6 @@
void Initialize(CountCrashFunction count_crash,
IsFeedbackAllowedFunction is_metrics_allowed);
- // TODO(crosbug.com/30268): refactor into separate class.
- bool HandleUdevCrash(const std::string &udev_event);
-
protected:
friend class CrashCollectorTest;
FRIEND_TEST(CrashCollectorTest, CheckHasCapacityCorrectBasename);
@@ -149,6 +146,7 @@
std::string extra_metadata_;
const char *forced_crash_directory_;
const char *lsb_release_;
+ FilePath log_config_path_;
};
#endif // _CRASH_REPORTER_CRASH_COLLECTOR_H_
diff --git a/crash_reporter/crash_reporter.cc b/crash_reporter/crash_reporter.cc
index e563b45..de67326 100644
--- a/crash_reporter/crash_reporter.cc
+++ b/crash_reporter/crash_reporter.cc
@@ -15,6 +15,7 @@
#include "base/stringprintf.h"
#include "chromeos/syslog_logging.h"
#include "crash-reporter/kernel_collector.h"
+#include "crash-reporter/udev_collector.h"
#include "crash-reporter/unclean_shutdown_collector.h"
#include "crash-reporter/user_collector.h"
#include "gflags/gflags.h"
@@ -46,6 +47,7 @@
kCrashKindUncleanShutdown = 1,
kCrashKindUser = 2,
kCrashKindKernel = 3,
+ kCrashKindUdev = 4,
kCrashKindMax
};
@@ -70,6 +72,10 @@
SendCrashMetrics(kCrashKindKernel, "kernel");
}
+static void CountUdevCrash() {
+ SendCrashMetrics(kCrashKindUdev, "udevcrash");
+}
+
static void CountUncleanShutdown() {
SendCrashMetrics(kCrashKindUncleanShutdown, "uncleanshutdown");
}
@@ -156,13 +162,13 @@
return 0;
}
-static int HandleUdevCrash(CrashCollector *udev_collector) {
+static int HandleUdevCrash(UdevCollector *udev_collector) {
// Handle a crash indicated by a udev event.
CHECK(!FLAGS_udev.empty()) << "--udev= must be set";
// Accumulate logs to help in diagnosing failures during user collection.
chromeos::LogToString(true);
- bool handled = udev_collector->HandleUdevCrash(FLAGS_udev);
+ bool handled = udev_collector->HandleCrash(FLAGS_udev);
chromeos::LogToString(false);
if (!handled)
return 1;
@@ -228,6 +234,8 @@
UncleanShutdownCollector unclean_shutdown_collector;
unclean_shutdown_collector.Initialize(CountUncleanShutdown,
IsFeedbackAllowed);
+ UdevCollector udev_collector;
+ udev_collector.Initialize(CountUdevCrash, IsFeedbackAllowed);
if (FLAGS_init) {
return Initialize(&kernel_collector,
@@ -246,7 +254,6 @@
}
if (!FLAGS_udev.empty()) {
- CrashCollector udev_collector;
return HandleUdevCrash(&udev_collector);
}
diff --git a/crash_reporter/udev_collector.cc b/crash_reporter/udev_collector.cc
new file mode 100644
index 0000000..0f7df3d
--- /dev/null
+++ b/crash_reporter/udev_collector.cc
@@ -0,0 +1,87 @@
+// Copyright (c) 2012 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/udev_collector.h"
+
+#include "base/basictypes.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/string_split.h"
+#include "base/string_util.h"
+#include "chromeos/process.h"
+
+static const char kCollectUdevSignature[] = "crash_reporter-udev-collection";
+static const char kGzipPath[] = "/bin/gzip";
+static const char kUdevExecName[] = "udev";
+static const char kUdevSignatureKey[] = "sig";
+
+UdevCollector::UdevCollector() {}
+
+UdevCollector::~UdevCollector() {}
+
+bool UdevCollector::HandleCrash(const std::string &udev_event) {
+ if (!is_feedback_allowed_function_()) {
+ LOG(ERROR) << "No consent given to collect crash info.";
+ return false;
+ }
+
+ // Process the udev event string.
+ // First get all the key-value pairs.
+ std::vector<std::pair<std::string, std::string> > udev_event_keyval;
+ base::SplitStringIntoKeyValuePairs(udev_event, '=', ':', &udev_event_keyval);
+ std::vector<std::pair<std::string, std::string> >::const_iterator iter;
+ std::map<std::string, std::string> udev_event_map;
+ for (iter = udev_event_keyval.begin();
+ iter != udev_event_keyval.end();
+ ++iter) {
+ udev_event_map[iter->first] = iter->second;
+ }
+
+ // Construct the basename string for crash_reporter_logs.conf:
+ // "crash_reporter-udev-collection-[action]-[name]-[subsystem]"
+ // If a udev field is not provided, "" is used in its place, e.g.:
+ // "crash_reporter-udev-collection-[action]--[subsystem]"
+ // Hence, "" is used as a wildcard name string.
+ // TODO(sque, crosbug.com/32238): Implement wildcard checking.
+ std::string basename = udev_event_map["ACTION"] + "-" +
+ udev_event_map["KERNEL"] + "-" +
+ udev_event_map["SUBSYSTEM"];
+ std::string udev_log_name = std::string(kCollectUdevSignature) + '-' +
+ basename;
+
+ // Make sure the crash directory exists, or create it if it doesn't.
+ FilePath crash_directory;
+ if (!GetCreatedCrashDirectoryByEuid(0, &crash_directory, NULL)) {
+ LOG(ERROR) << "Could not get crash directory.";
+ return false;
+ }
+ // Create the destination path.
+ std::string log_file_name =
+ FormatDumpBasename(basename, time(NULL), 0);
+ FilePath crash_path = GetCrashPath(crash_directory, log_file_name, "log");
+
+ // Handle the crash.
+ bool result = GetLogContents(log_config_path_, udev_log_name, crash_path);
+ if (!result) {
+ LOG(ERROR) << "Error reading udev log info " << udev_log_name;
+ return false;
+ }
+
+ // Compress the output using gzip.
+ chromeos::ProcessImpl gzip_process;
+ gzip_process.AddArg(kGzipPath);
+ gzip_process.AddArg(crash_path.value());
+ int process_result = gzip_process.Run();
+ FilePath crash_path_zipped = FilePath(crash_path.value() + ".gz");
+ // If the zip file was not created, use the uncompressed file.
+ if (process_result != 0 || !file_util::PathExists(crash_path_zipped))
+ LOG(ERROR) << "Could not create zip file " << crash_path_zipped.value();
+ else
+ crash_path = crash_path_zipped;
+
+ AddCrashMetaData(kUdevSignatureKey, kCollectUdevSignature);
+ WriteCrashMetaData(GetCrashPath(crash_directory, log_file_name, "meta"),
+ kUdevExecName, crash_path.value());
+ return true;
+}
diff --git a/crash_reporter/udev_collector.h b/crash_reporter/udev_collector.h
new file mode 100644
index 0000000..acb09cb
--- /dev/null
+++ b/crash_reporter/udev_collector.h
@@ -0,0 +1,37 @@
+// Copyright (c) 2012 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_UDEV_COLLECTOR_H_
+#define _CRASH_REPORTER_UDEV_COLLECTOR_H_
+
+#include <string>
+
+#include "crash-reporter/crash_collector.h"
+#include "gtest/gtest_prod.h" // for FRIEND_TEST
+
+class FilePath;
+
+// Udev crash collector.
+class UdevCollector : public CrashCollector {
+ public:
+ UdevCollector();
+
+ virtual ~UdevCollector();
+
+ // The udev event string should be formatted as follows:
+ // "ACTION=[action]:KERNEL=[name]:SUBSYSTEM=[subsystem]"
+ // The values don't have to be in any particular order. One or more of them
+ // could be omitted, in which case it would be treated as a wildcard (*).
+ bool HandleCrash(const std::string &udev_event);
+
+ private:
+ friend class UdevCollectorTest;
+
+ // Mutator for unit testing.
+ void set_log_config_path(const std::string& path) {
+ log_config_path_ = FilePath(path);
+ }
+};
+
+#endif // _CRASH_REPORTER_UDEV_COLLECTOR_H_
diff --git a/crash_reporter/udev_collector_test.cc b/crash_reporter/udev_collector_test.cc
new file mode 100644
index 0000000..b75abfd
--- /dev/null
+++ b/crash_reporter/udev_collector_test.cc
@@ -0,0 +1,105 @@
+// Copyright (c) 2012 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 "base/file_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/scoped_temp_dir.h"
+#include "chromeos/test_helpers.h"
+#include "crash-reporter/udev_collector.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+// Dummy log config file name.
+const char kLogConfigFileName[] = "log_config_file";
+
+// A bunch of random rules to put into the dummy log config file.
+const char kLogConfigFileContents[] =
+ "crash_reporter-udev-collection-change-card0-drm:echo change card0 drm\n"
+ "crash_reporter-udev-collection-add-state0-cpu:echo change state0 cpu\n"
+ "cros_installer:echo not for udev";
+
+void CountCrash() {}
+
+bool s_consent_given = true;
+
+bool IsMetrics() {
+ return s_consent_given;
+}
+
+// Returns the number of compressed crash log files found in the given path.
+int GetNumLogFiles(const FilePath& path) {
+ file_util::FileEnumerator enumerator(path, false,
+ file_util::FileEnumerator::FILES,
+ "*.log.gz");
+ int num_files = 0;
+ for (FilePath file_path = enumerator.Next();
+ !file_path.value().empty();
+ file_path = enumerator.Next()) {
+ num_files++;
+ }
+ return num_files;
+}
+
+} // namespace
+
+class UdevCollectorTest : public ::testing::Test {
+ void SetUp() {
+ s_consent_given = true;
+
+ collector_.reset(new UdevCollector);
+ collector_->Initialize(CountCrash, IsMetrics);
+
+ temp_dir_generator_.reset(new ScopedTempDir());
+ ASSERT_TRUE(temp_dir_generator_->CreateUniqueTempDir());
+ EXPECT_TRUE(temp_dir_generator_->IsValid());
+
+ FilePath log_config_path =
+ temp_dir_generator_->path().Append(kLogConfigFileName);
+ collector_->log_config_path_ = log_config_path;
+ collector_->ForceCrashDirectory(
+ temp_dir_generator_->path().value().c_str());
+
+ // Write to a dummy log config file.
+ ASSERT_EQ(strlen(kLogConfigFileContents),
+ file_util::WriteFile(log_config_path,
+ kLogConfigFileContents,
+ strlen(kLogConfigFileContents)));
+
+ chromeos::ClearLog();
+ }
+
+ protected:
+ scoped_ptr<UdevCollector> collector_;
+ scoped_ptr<ScopedTempDir> temp_dir_generator_;
+};
+
+TEST_F(UdevCollectorTest, TestNoConsent) {
+ s_consent_given = false;
+ collector_->HandleCrash("ACTION=change:KERNEL=card0:SUBSYSTEM=drm");
+ EXPECT_EQ(0, GetNumLogFiles(temp_dir_generator_->path()));
+}
+
+TEST_F(UdevCollectorTest, TestNoMatch) {
+ // No rule should match this.
+ collector_->HandleCrash("ACTION=change:KERNEL=foo:SUBSYSTEM=bar");
+ EXPECT_EQ(0, GetNumLogFiles(temp_dir_generator_->path()));
+}
+
+TEST_F(UdevCollectorTest, TestMatches) {
+ // Try multiple udev events in sequence. The number of log files generated
+ // should increase.
+ collector_->HandleCrash("ACTION=change:KERNEL=card0:SUBSYSTEM=drm");
+ EXPECT_EQ(1, GetNumLogFiles(temp_dir_generator_->path()));
+ collector_->HandleCrash("ACTION=add:KERNEL=state0:SUBSYSTEM=cpu");
+ EXPECT_EQ(2, GetNumLogFiles(temp_dir_generator_->path()));
+}
+
+// TODO(sque, crosbug.com/32238) - test wildcard cases, multiple identical udev
+// events.
+
+int main(int argc, char **argv) {
+ SetUpTests(&argc, argv, false);
+ return RUN_ALL_TESTS();
+}
diff --git a/crash_reporter/user_collector.cc b/crash_reporter/user_collector.cc
index b060bd6..fca2058 100644
--- a/crash_reporter/user_collector.cc
+++ b/crash_reporter/user_collector.cc
@@ -43,7 +43,6 @@
static const char kCorePipeLimit[] = "4";
static const char kCoreToMinidumpConverterPath[] = "/usr/bin/core2md";
-static const char kDefaultLogConfig[] = "/etc/crash_reporter_logs.conf";
static const char kStatePrefix[] = "State:\t";
const char *UserCollector::kUserId = "Uid:\t";
@@ -244,7 +243,7 @@
std::string dump_basename = FormatDumpBasename(exec, time(NULL), pid);
std::string error_log = chromeos::GetLog();
FilePath diag_log_path = GetCrashPath(crash_path, dump_basename, "diaglog");
- if (GetLogContents(FilePath(kDefaultLogConfig), kCollectionErrorSignature,
+ if (GetLogContents(FilePath(log_config_path_), kCollectionErrorSignature,
diag_log_path)) {
// We load the contents of diag_log into memory and append it to
// the error log. We cannot just append to files because we need
@@ -491,7 +490,7 @@
FilePath minidump_path = GetCrashPath(crash_path, dump_basename, "dmp");
FilePath log_path = GetCrashPath(crash_path, dump_basename, "log");
- if (GetLogContents(FilePath(kDefaultLogConfig), exec, log_path))
+ if (GetLogContents(FilePath(log_config_path_), exec, log_path))
AddCrashMetaData("log", log_path.value());
ErrorType error_type =