crash-reporter: Use chromeos::KeyValueStore.
Make the crash reporter use libchromeos's KeyValueStore
class to read its log-collection config file and
/etc/lsb-release instead of its own parsing code.
Also update the log-collection config file to split long
commands across multiple lines and to use '=' instead of ':'
to separate executable names from commands.
BUG=chromium:452520
TEST=updated tests; also triggered powerd and chrome crashes
and checked that logs were attached
Change-Id: I4e2447712869608f32a4ae38f5d5cb9c6046af14
Reviewed-on: https://chromium-review.googlesource.com/244121
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
diff --git a/crash_reporter/crash-reporter.gyp b/crash_reporter/crash-reporter.gyp
index b80924b..b42657b 100644
--- a/crash_reporter/crash-reporter.gyp
+++ b/crash_reporter/crash-reporter.gyp
@@ -103,6 +103,7 @@
'chrome_collector_test.cc',
'crash_collector_test.cc',
'crash_collector_test.h',
+ 'crash_reporter_logs_test.cc',
'kernel_collector_test.cc',
'kernel_collector_test.h',
'testrunner.cc',
diff --git a/crash_reporter/crash_collector.cc b/crash_reporter/crash_collector.cc
index e009ee4..a53bff9 100644
--- a/crash_reporter/crash_collector.cc
+++ b/crash_reporter/crash_collector.cc
@@ -30,19 +30,26 @@
#include <chromeos/cryptohome.h>
#include <chromeos/dbus/dbus.h>
#include <chromeos/dbus/service_constants.h>
+#include <chromeos/key_value_store.h>
#include <chromeos/process.h>
-static const char kCollectChromeFile[] =
+namespace {
+
+const char kCollectChromeFile[] =
"/mnt/stateful_partition/etc/collect_chrome_crashes";
-static const char kCrashTestInProgressPath[] = "/tmp/crash-test-in-progress";
-static const char kDefaultLogConfig[] = "/etc/crash_reporter_logs.conf";
-static const char kDefaultUserName[] = "chronos";
-static const char kLeaveCoreFile[] = "/root/.leave_core";
-static const char kLsbRelease[] = "/etc/lsb-release";
-static const char kShellPath[] = "/bin/sh";
-static const char kSystemCrashPath[] = "/var/spool/crash";
-static const char kUploadVarPrefix[] = "upload_var_";
-static const char kUploadFilePrefix[] = "upload_file_";
+const char kCrashTestInProgressPath[] = "/tmp/crash-test-in-progress";
+const char kDefaultLogConfig[] = "/etc/crash_reporter_logs.conf";
+const char kDefaultUserName[] = "chronos";
+const char kLeaveCoreFile[] = "/root/.leave_core";
+const char kLsbRelease[] = "/etc/lsb-release";
+const char kShellPath[] = "/bin/sh";
+const char kSystemCrashPath[] = "/var/spool/crash";
+const char kUploadVarPrefix[] = "upload_var_";
+const char kUploadFilePrefix[] = "upload_file_";
+
+// Key of the lsb-release entry containing the OS version.
+const char kLsbVersionKey[] = "CHROMEOS_RELEASE_VERSION";
+
// Normally this path is not used. Unfortunately, there are a few edge cases
// where we need this. Any process that runs as kDefaultUserName that crashes
// is consider a "user crash". That includes the initial Chrome browser that
@@ -53,16 +60,18 @@
// This also comes up when running autotests. The GUI is sitting at the login
// screen while tests are sshing in, changing users, and triggering crashes as
// the user (purposefully).
-static const char kFallbackUserCrashPath[] = "/home/chronos/crash";
+const char kFallbackUserCrashPath[] = "/home/chronos/crash";
// Directory mode of the user crash spool directory.
-static const mode_t kUserCrashPathMode = 0755;
+const mode_t kUserCrashPathMode = 0755;
// Directory mode of the system crash spool directory.
-static const mode_t kSystemCrashPathMode = 01755;
+const mode_t kSystemCrashPathMode = 01755;
-static const uid_t kRootOwner = 0;
-static const uid_t kRootGroup = 0;
+const uid_t kRootOwner = 0;
+const uid_t kRootGroup = 0;
+
+} // namespace
// Maximum crash reports per crash spool directory. Note that this is
// a separate maximum from the maximum rate at which we upload these
@@ -416,65 +425,28 @@
return !full;
}
-bool CrashCollector::IsCommentLine(const std::string &line) {
- size_t found = line.find_first_not_of(" ");
- return found != std::string::npos && line[found] == '#';
-}
-
-bool CrashCollector::ReadKeyValueFile(
- const FilePath &path,
- const char separator,
- std::map<std::string, std::string> *dictionary) {
- std::string contents;
- if (!base::ReadFileToString(path, &contents)) {
- return false;
- }
- typedef std::vector<std::string> StringVector;
- StringVector lines;
- base::SplitString(contents, '\n', &lines);
- bool any_errors = false;
- for (StringVector::iterator line = lines.begin(); line != lines.end();
- ++line) {
- // Allow empty strings.
- if (line->empty())
- continue;
- // Allow comment lines.
- if (IsCommentLine(*line))
- continue;
- StringVector sides;
- base::SplitString(*line, separator, &sides);
- if (sides.size() != 2) {
- any_errors = true;
- continue;
- }
- dictionary->insert(std::pair<std::string, std::string>(sides[0], sides[1]));
- }
- return !any_errors;
-}
-
bool CrashCollector::GetLogContents(const FilePath &config_path,
const std::string &exec_name,
const FilePath &output_file) {
- std::map<std::string, std::string> log_commands;
- if (!ReadKeyValueFile(config_path, ':', &log_commands)) {
+ chromeos::KeyValueStore store;
+ if (!store.Load(config_path)) {
LOG(INFO) << "Unable to read log configuration file "
<< config_path.value();
return false;
}
- if (log_commands.find(exec_name) == log_commands.end())
+ std::string command;
+ if (!store.GetString(exec_name, &command))
return false;
chromeos::ProcessImpl diag_process;
diag_process.AddArg(kShellPath);
- std::string shell_command = log_commands[exec_name];
- diag_process.AddStringOption("-c", shell_command);
+ diag_process.AddStringOption("-c", command);
diag_process.RedirectOutput(output_file.value());
- int result = diag_process.Run();
+ const int result = diag_process.Run();
if (result != 0) {
- LOG(INFO) << "Running shell command " << shell_command << "failed with: "
- << result;
+ LOG(INFO) << "Log command \"" << command << "\" exited with " << result;
return false;
}
return true;
@@ -500,15 +472,16 @@
void CrashCollector::WriteCrashMetaData(const FilePath &meta_path,
const std::string &exec_name,
const std::string &payload_path) {
- std::map<std::string, std::string> contents;
- if (!ReadKeyValueFile(FilePath(lsb_release_), '=', &contents)) {
+ chromeos::KeyValueStore store;
+ if (!store.Load(FilePath(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");
- std::map<std::string, std::string>::iterator i;
- if ((i = contents.find("CHROMEOS_RELEASE_VERSION")) != contents.end()) {
- version = i->second;
+ if (!store.GetString(kLsbVersionKey, &version)) {
+ LOG(ERROR) << "Unable to read " << kLsbVersionKey << " from "
+ << lsb_release_;
}
int64_t payload_size = -1;
base::GetFileSize(FilePath(payload_path), &payload_size);
diff --git a/crash_reporter/crash_collector.h b/crash_reporter/crash_collector.h
index a1565e3..68806d9 100644
--- a/crash_reporter/crash_collector.h
+++ b/crash_reporter/crash_collector.h
@@ -43,10 +43,8 @@
FRIEND_TEST(CrashCollectorTest, ForkExecAndPipe);
FRIEND_TEST(CrashCollectorTest, FormatDumpBasename);
FRIEND_TEST(CrashCollectorTest, Initialize);
- FRIEND_TEST(CrashCollectorTest, IsCommentLine);
FRIEND_TEST(CrashCollectorTest, IsUserSpecificDirectoryEnabled);
FRIEND_TEST(CrashCollectorTest, MetaData);
- FRIEND_TEST(CrashCollectorTest, ReadKeyValueFile);
FRIEND_TEST(CrashCollectorTest, Sanitize);
FRIEND_TEST(CrashCollectorTest, WriteNewFile);
FRIEND_TEST(ForkExecAndPipeTest, Basic);
@@ -120,15 +118,6 @@
// crash.
bool CheckHasCapacity(const base::FilePath &crash_directory);
- // Checks if the line starts with '#' after optional whitespace.
- static bool IsCommentLine(const std::string &line);
-
- // Read the given file of form [<key><separator><value>\n...] and return
- // a map of its contents.
- bool ReadKeyValueFile(const base::FilePath &file,
- char separator,
- std::map<std::string, std::string> *dictionary);
-
// Write a log applicable to |exec_name| to |output_file| based on the
// log configuration file at |config_path|.
bool GetLogContents(const base::FilePath &config_path,
diff --git a/crash_reporter/crash_collector_test.cc b/crash_reporter/crash_collector_test.cc
index 1548d70..0ca3792 100644
--- a/crash_reporter/crash_collector_test.cc
+++ b/crash_reporter/crash_collector_test.cc
@@ -220,59 +220,6 @@
EXPECT_FALSE(CheckHasCapacity());
}
-TEST_F(CrashCollectorTest, IsCommentLine) {
- EXPECT_FALSE(CrashCollector::IsCommentLine(""));
- EXPECT_TRUE(CrashCollector::IsCommentLine("#"));
- EXPECT_TRUE(CrashCollector::IsCommentLine("#real comment"));
- EXPECT_TRUE(CrashCollector::IsCommentLine(" # real comment"));
- EXPECT_FALSE(CrashCollector::IsCommentLine("not comment"));
- EXPECT_FALSE(CrashCollector::IsCommentLine(" not comment"));
-}
-
-TEST_F(CrashCollectorTest, ReadKeyValueFile) {
- const char *contents = ("a=b\n"
- "\n"
- " c=d \n");
- FilePath path(test_dir_.Append("keyval"));
- std::map<std::string, std::string> dictionary;
- std::map<std::string, std::string>::iterator i;
-
- base::WriteFile(path, contents, strlen(contents));
-
- EXPECT_TRUE(collector_.ReadKeyValueFile(path, '=', &dictionary));
- i = dictionary.find("a");
- EXPECT_TRUE(i != dictionary.end() && i->second == "b");
- i = dictionary.find("c");
- EXPECT_TRUE(i != dictionary.end() && i->second == "d");
-
- dictionary.clear();
-
- contents = ("a=b c d\n"
- "e\n"
- " f g = h\n"
- "i=j\n"
- "=k\n"
- "#comment=0\n"
- "l=\n");
- base::WriteFile(path, contents, strlen(contents));
-
- EXPECT_FALSE(collector_.ReadKeyValueFile(path, '=', &dictionary));
- EXPECT_EQ(5, dictionary.size());
-
- i = dictionary.find("a");
- EXPECT_TRUE(i != dictionary.end() && i->second == "b c d");
- i = dictionary.find("e");
- EXPECT_TRUE(i == dictionary.end());
- i = dictionary.find("f g");
- EXPECT_TRUE(i != dictionary.end() && i->second == "h");
- i = dictionary.find("i");
- EXPECT_TRUE(i != dictionary.end() && i->second == "j");
- i = dictionary.find("");
- EXPECT_TRUE(i != dictionary.end() && i->second == "k");
- i = dictionary.find("l");
- EXPECT_TRUE(i != dictionary.end() && i->second == "");
-}
-
TEST_F(CrashCollectorTest, MetaData) {
const char kMetaFileBasename[] = "generated.meta";
FilePath meta_file = test_dir_.Append(kMetaFileBasename);
@@ -280,7 +227,10 @@
FilePath payload_file = test_dir_.Append("payload-file");
std::string contents;
collector_.lsb_release_ = lsb_release.value();
- const char kLsbContents[] = "CHROMEOS_RELEASE_VERSION=version\n";
+ const char kLsbContents[] =
+ "CHROMEOS_RELEASE_BOARD=lumpy\n"
+ "CHROMEOS_RELEASE_VERSION=6727.0.2015_01_26_0853\n"
+ "CHROMEOS_RELEASE_NAME=Chromium OS\n";
ASSERT_TRUE(base::WriteFile(lsb_release, kLsbContents, strlen(kLsbContents)));
const char kPayload[] = "foo";
ASSERT_TRUE(base::WriteFile(payload_file, kPayload, strlen(kPayload)));
@@ -290,7 +240,7 @@
const char kExpectedMeta[] =
"foo=bar\n"
"exec_name=kernel\n"
- "ver=version\n"
+ "ver=6727.0.2015_01_26_0853\n"
"payload=test/payload-file\n"
"payload_size=3\n"
"done=1\n";
@@ -328,7 +278,7 @@
FilePath config_file = test_dir_.Append("crash_config");
FilePath output_file = test_dir_.Append("crash_log");
const char kConfigContents[] =
- "foobar:echo hello there | sed -e \"s/there/world/\"";
+ "foobar=echo hello there | \\\n sed -e \"s/there/world/\"";
ASSERT_TRUE(
base::WriteFile(config_file, kConfigContents, strlen(kConfigContents)));
base::DeleteFile(FilePath(output_file), false);
diff --git a/crash_reporter/crash_reporter_logs.conf b/crash_reporter/crash_reporter_logs.conf
index 7f54368..1a69032 100644
--- a/crash_reporter/crash_reporter_logs.conf
+++ b/crash_reporter/crash_reporter_logs.conf
@@ -2,64 +2,102 @@
# Use of this source code is governed by a BSD-style license that can
# be found in the LICENSE file.
-# This file has the format:
-# <basename>:<shell command>\n
+# This file is parsed by chromeos::KeyValueStore. It has the format:
#
-# Where when any executable with the basename <basename> crashes, the
-# given <shell command> is executed and its standard output and
-# standard error is sent along with the crash report.
+# <basename>=<shell command>\n
#
-# Use caution in modifying this file. Only run common unix commands
-# here as these commands will be run when a crash has recently
-# occurred and we should avoid running anything that might cause
-# another crash. Similarly these command block the notification of
-# the crash to parent processes, so commands should execute quickly.
-# Commands cannot contain colons.
+# Commands may be split across multiple lines using trailing backslashes.
+#
+# When an executable named <basename> crashes, the corresponding command is
+# executed and its standard output and standard error are attached to the crash
+# report.
+#
+# Use caution in modifying this file. Only run common Unix commands here, as
+# these commands will be run when a crash has recently occurred and we should
+# avoid running anything that might cause another crash. Similarly, these
+# commands block notification of the crash to parent processes, so commands
+# should execute quickly.
-update_engine:cat $(ls -1tr /var/log/update_engine | tail -5 | sed s.^./var/log/update_engine/.) | tail -c 50000
+update_engine=cat $(ls -1tr /var/log/update_engine | tail -5 | \
+ sed s.^./var/log/update_engine/.) | tail -c 50000
# The cros_installer output is logged into the update engine log file,
# so it is handled in the same way as update_engine.
-cros_installer:cat $(ls -1tr /var/log/update_engine | tail -5 | sed s.^./var/log/update_engine/.) | tail -c 50000
+cros_installer=cat $(ls -1tr /var/log/update_engine | tail -5 | \
+ sed s.^./var/log/update_engine/.) | tail -c 50000
# Dump the last 20 lines of the last two files in Chrome's system and user log
# directories, along with the last 20 messages from the session manager.
-chrome:for f in $(ls -1rt /var/log/chrome/chrome_[0-9]* | tail -2) $(ls -1rt /home/chronos/u-*/log/chrome_[0-9]* 2>/dev/null | tail -2); do echo "===$f (tail)==="; tail -20 $f; echo EOF; echo; done; echo "===session_manager (tail)==="; awk '$3 ~ "^session_manager\[" { print }' /var/log/messages | tail -20; echo EOF
+chrome=\
+ for f in $(ls -1rt /var/log/chrome/chrome_[0-9]* | tail -2) \
+ $(ls -1rt /home/chronos/u-*/log/chrome_[0-9]* 2>/dev/null | tail -2); do \
+ echo "===$f (tail)==="; \
+ tail -20 $f; \
+ echo EOF; \
+ echo; \
+ done; \
+ echo "===session_manager (tail)==="; \
+ awk '$3 ~ "^session_manager\[" { print }' /var/log/messages | tail -20; \
+ echo EOF
# The following rule is used for generating additional diagnostics when
# collection of user crashes fails. This output should not be too large
# as it is stored in memory. The output format specified for 'ps' is the
# same as with the "u" ("user-oriented") option, except it doesn't show
# the commands' arguments (i.e. "comm" instead of "command").
-crash_reporter-user-collection:echo "===ps output==="; ps axw -o user,pid,%cpu,%mem,vsz,rss,tname,stat,start_time,bsdtime,comm | tail -c 25000; echo "===meminfo==="; cat /proc/meminfo
+crash_reporter-user-collection=\
+ echo "===ps output==="; \
+ ps axw -o user,pid,%cpu,%mem,vsz,rss,tname,stat,start_time,bsdtime,comm | \
+ tail -c 25000; \
+ echo "===meminfo==="; \
+ cat /proc/meminfo
# This rule is similar to the crash_reporter-user-collection rule, except it is
# run for kernel errors reported through udev events.
-crash_reporter-udev-collection-change-card0-drm:for dri in /sys/kernel/debug/dri/*; do echo "===$dri/i915_error_state==="; cat $dri/i915_error_state; done
+crash_reporter-udev-collection-change-card0-drm=\
+ for dri in /sys/kernel/debug/dri/*; do \
+ echo "===$dri/i915_error_state==="; \
+ cat $dri/i915_error_state; \
+ done
# When trackpad driver cyapa detects some abnormal behavior, we collect
# additional logs from kernel messages.
-crash_reporter-udev-collection-change--i2c-cyapa:/usr/sbin/kernel_log_collector.sh cyapa 30
-# When trackpad/touchscreen driver atmel_mxt_ts detects some abnormal behavior, we collect
-# additional logs from kernel messages.
-crash_reporter-udev-collection-change--i2c-atmel_mxt_ts:/usr/sbin/kernel_log_collector.sh atmel 30
-# When touch device noise are detected, we collect relevant logs. (crosbug.com/p/16788)
-crash_reporter-udev-collection---TouchNoise:cat /var/log/touch_noise.log
+crash_reporter-udev-collection-change--i2c-cyapa=\
+ /usr/sbin/kernel_log_collector.sh cyapa 30
+# When trackpad/touchscreen driver atmel_mxt_ts detects some abnormal behavior,
+# we collect additional logs from kernel messages.
+crash_reporter-udev-collection-change--i2c-atmel_mxt_ts=\
+ /usr/sbin/kernel_log_collector.sh atmel 30
+# When touch device noise are detected, we collect relevant logs.
+# (crosbug.com/p/16788)
+crash_reporter-udev-collection---TouchNoise=cat /var/log/touch_noise.log
# Periodically collect touch event log for debugging (crosbug.com/p/17244)
-crash_reporter-udev-collection---TouchEvent:cat /var/log/touch_event.log
+crash_reporter-udev-collection---TouchEvent=cat /var/log/touch_event.log
# Dump the last 50 lines of the last two powerd log files -- if the job has
# already restarted, we want to see the end of the previous instance's logs.
-powerd:for f in $(ls -1tr /var/log/power_manager/powerd.[0-9]* | tail -2); do echo "===$(basename $f) (tail)==="; tail -50 $f; echo EOF; done
+powerd=\
+ for f in $(ls -1tr /var/log/power_manager/powerd.[0-9]* | tail -2); do \
+ echo "===$(basename $f) (tail)==="; \
+ tail -50 $f; \
+ echo EOF; \
+ done
# If power_supply_info aborts (due to e.g. a bad battery), its failure message
# could end up in various places depending on which process was running it.
# Attach the end of powerd's log since it might've also logged the underlying
# problem.
-power_supply_info:echo "===powerd.LATEST (tail)==="; tail -50 /var/log/power_manager/powerd.LATEST; echo EOF
+power_supply_info=\
+ echo "===powerd.LATEST (tail)==="; \
+ tail -50 /var/log/power_manager/powerd.LATEST; \
+ echo EOF
# powerd_setuid_helper gets run by powerd, so its stdout/stderr will be mixed in
# with powerd's stdout/stderr.
-powerd_setuid_helper:echo "===powerd.OUT (tail)==="; tail -50 /var/log/powerd.out; echo EOF
+powerd_setuid_helper=\
+ echo "===powerd.OUT (tail)==="; \
+ tail -50 /var/log/powerd.out; \
+ echo EOF
# The following rules are only for testing purposes.
-crash_log_test:echo hello world
-crash_log_recursion_test:sleep 1 && /usr/local/autotest/tests/crash_log_recursion_test
+crash_log_test=echo hello world
+crash_log_recursion_test=sleep 1 && \
+ /usr/local/autotest/tests/crash_log_recursion_test
diff --git a/crash_reporter/crash_reporter_logs_test.cc b/crash_reporter/crash_reporter_logs_test.cc
new file mode 100644
index 0000000..9879470
--- /dev/null
+++ b/crash_reporter/crash_reporter_logs_test.cc
@@ -0,0 +1,28 @@
+// Copyright 2015 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 <string>
+
+#include <base/files/file_path.h>
+#include <chromeos/key_value_store.h>
+#include <gtest/gtest.h>
+
+namespace {
+
+// Name of the checked-in configuration file containing log-collection commands.
+const char kConfigFile[] = "crash_reporter_logs.conf";
+
+// Executable name for Chrome. kConfigFile is expected to contain this entry.
+const char kChromeExecName[] = "chrome";
+
+} // namespace
+
+// Tests that the config file is parsable and that Chrome is listed.
+TEST(CrashReporterLogsTest, ReadConfig) {
+ chromeos::KeyValueStore store;
+ ASSERT_TRUE(store.Load(base::FilePath(kConfigFile)));
+ std::string command;
+ EXPECT_TRUE(store.GetString(kChromeExecName, &command));
+ EXPECT_FALSE(command.empty());
+}
diff --git a/crash_reporter/udev_collector_test.cc b/crash_reporter/udev_collector_test.cc
index 66daaf1..f41b06f 100644
--- a/crash_reporter/udev_collector_test.cc
+++ b/crash_reporter/udev_collector_test.cc
@@ -19,9 +19,9 @@
// 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";
+ "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() {}