Chromium Code Reviews
Help | Chromium Project | Sign in
(693)

Issue 6297004: crash-reporter: Add diagnostics to help diagnose failures in the wild (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 3 months ago by kmixter1
Modified:
2 years, 11 months ago
Reviewers:
petkov
CC:
chromium-os-reviews_chromium.org
Visibility:
Public.

Description

crash-reporter: Add diagnostics to help diagnose failures in the wild

We add logging messages that are generated during invocation of core2md, but we also enable sending arbitrary system info based on the configuration file. In this case we add the list of running processes, meminfo, and dmesg.

Change-Id: Ifdf29b89dd60d56349fa39095d2aa07f6b5e2de2

BUG=chromium-os:6299, chromium-os:9345
TEST=UserCrash, unit tests

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=fed58af

Patch Set 1 #

Patch Set 2 : and now fix that unit test #

Patch Set 3 : clarify #

Total comments: 3

Patch Set 4 : Fix security issue, reduce commandline size, add more diagnostics #

Patch Set 5 : log bad params instead of check failing #

Total comments: 8

Patch Set 6 : respond to review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+255 lines, -119 lines) Lint Patch
M crash_collector.h View 1 2 3 1 chunk +9 lines, -0 lines 0 comments 0 errors Download
M crash_collector.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments ? errors Download
M crash_collector_test.cc View 1 2 3 3 chunks +82 lines, -63 lines 0 comments 0 errors Download
M crash_reporter.cc View 1 2 3 3 chunks +3 lines, -5 lines 0 comments 0 errors Download
M crash_reporter_logs.conf View 1 2 3 1 chunk +5 lines, -0 lines 0 comments ? errors Download
M system_logging.h View 1 2 3 3 chunks +15 lines, -0 lines 0 comments 0 errors Download
M system_logging.cc View 1 2 3 2 chunks +17 lines, -6 lines 0 comments 0 errors Download
M system_logging_mock.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments 0 errors Download
M system_logging_mock.cc View 1 2 3 1 chunk +7 lines, -0 lines 0 comments 1 errors Download
M user_collector.h View 1 2 3 4 chunks +6 lines, -4 lines 0 comments 1 errors Download
M user_collector.cc View 1 2 3 4 5 16 chunks +76 lines, -36 lines 0 comments 2 errors Download
M user_collector_test.cc View 1 2 3 4 chunks +31 lines, -4 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 5
kmixter1
3 years, 3 months ago #1
petkov
LGTM w/ a couple of nits. I assume somewhere you're including the crash reporter log ...
3 years, 3 months ago #2
kmixter1
PTAL, some significant changes.
3 years, 3 months ago #3
petkov
LGTM w/ a few nits. Btw, if you specify explicitly BUG=chromium-os:..., the hyperlink will take ...
3 years, 3 months ago #4
kmixter1
3 years, 2 months ago #5
pushing

http://codereview.chromium.org/6297004/diff/5001/user_collector.cc
File user_collector.cc (right):

http://codereview.chromium.org/6297004/diff/5001/user_collector.cc#newcode282
user_collector.cc:282: std::string errors_during_fork;
On 2011/01/18 19:18:20, petkov wrote:
> unused?

Done.

http://codereview.chromium.org/6297004/diff/23001/crash_collector.cc
File crash_collector.cc (right):

http://codereview.chromium.org/6297004/diff/23001/crash_collector.cc#newcode114
crash_collector.cc:114: open(output_file,O_CREAT | O_WRONLY | O_TRUNC | O_EXCL,
0666));
On 2011/01/24 18:36:04, petkov wrote:
> space after ,

Done.

http://codereview.chromium.org/6297004/diff/23001/user_collector.cc
File user_collector.cc (right):

http://codereview.chromium.org/6297004/diff/23001/user_collector.cc#newcode9
user_collector.cc:9: #include <pcrecpp.h>
On 2011/01/24 18:36:04, petkov wrote:
> sort

Done.

http://codereview.chromium.org/6297004/diff/23001/user_collector.cc#newcode72
user_collector.cc:72: return StringPrintf("|%s --user=%%p:%%s:%%e",
our_path_.c_str());
On 2011/01/24 18:36:04, petkov wrote:
> I guess no concern about %%e containing spaces?

It's annoying but the kernel doesn't support quoting parameters in core_pattern,
and %%e can contain spaces.  If this occurs, only the part before the space will
be used - and it's only used if the normal way of getting the command line fails
(via proc).  So I'm not too concerned.  Added a comment.

http://codereview.chromium.org/6297004/diff/23001/user_collector.cc#newcode72
user_collector.cc:72: return StringPrintf("|%s --user=%%p:%%s:%%e",
our_path_.c_str());
On 2011/01/24 18:36:04, petkov wrote:
> add a comment why you're combining the values into a single option?

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6