Collect and send kernel crash diagnostics

Collect and send kernel crash diagnostics

BUG=1512, 1914

[kmixter1, 2010-08-17 06:40:35]

hughd: the code in kernel_collector.cc may be of interest to you.

[petkov, 2010-08-17 18:13:47]

I think you're not sending to UMA the raw kernel crash counts. Nothing
significant otherwise.

http://codereview.chromium.org/3179006/diff/2001/3001
File Makefile (right):

http://codereview.chromium.org/3179006/diff/2001/3001#newcode31
Makefile:31: crash_collector_test: crash_collector_test.o $(TEST_OBJS)
You could probably combine the three test targets under a common rule. Something
like (not sure about the exact syntax):

%_test: %_test.o $(TEST_OBJS)

http://codereview.chromium.org/3179006/diff/2001/3002
File crash_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3002#newcode11
crash_collector.cc:11: #include "crash-reporter/crash_collector.h"
this should be the top include

http://codereview.chromium.org/3179006/diff/2001/3003
File crash_collector.h (right):

http://codereview.chromium.org/3179006/diff/2001/3003#newcode37
crash_collector.h:37: // For testing, set the directory always returned by
s/set/replace/ ?

http://codereview.chromium.org/3179006/diff/2001/3004
File crash_collector_test.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3004#newcode9
crash_collector_test.cc:9: #include "crash-reporter/crash_collector.h"
sort?

actually, this should be the top include.

http://codereview.chromium.org/3179006/diff/2001/3005
File crash_reporter.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3005#newcode11
crash_reporter.cc:11: #include "crash-reporter/kernel_collector.h"
shouldn't these be abc sorted?

http://codereview.chromium.org/3179006/diff/2001/3005#newcode27
crash_reporter.cc:27: static const char kKernelCrashSignal[] =
Do you need this constant?

http://codereview.chromium.org/3179006/diff/2001/3005#newcode70
crash_reporter.cc:70: return file_util::WriteFile(file_path, kEmpty, 0) == 0;
use TouchFile?

http://codereview.chromium.org/3179006/diff/2001/3005#newcode78
crash_reporter.cc:78: static void TouchFile(const FilePath &file_path) {
return bool success/failure?

http://codereview.chromium.org/3179006/diff/2001/3005#newcode138
crash_reporter.cc:138: if (!kernel_crash) {
shouldn't you guard touching the file under IsMetricsCollectionAllowed for
consistency?

http://codereview.chromium.org/3179006/diff/2001/3005#newcode140
crash_reporter.cc:140: // an associated
finish the comment.

http://codereview.chromium.org/3179006/diff/2001/3007
File kernel_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3007#newcode8
kernel_collector.cc:8: #include "crash-reporter/kernel_collector.h"
move to top

http://codereview.chromium.org/3179006/diff/2001/3007#newcode9
kernel_collector.cc:9: #include "metrics/metrics_library.h"
not needed?

http://codereview.chromium.org/3179006/diff/2001/3007#newcode62
kernel_collector.cc:62: //int x = system("echo '' >
/sys/kernel/debug/preserved/kcrash");  x=x;
remove?

http://codereview.chromium.org/3179006/diff/2001/3007#newcode91
kernel_collector.cc:91: 
Do you need to call count_crash_function here?

http://codereview.chromium.org/3179006/diff/2001/3007#newcode95
kernel_collector.cc:95: kernel_dump.c_str(),
using .data() may, theoretically, be slightly better (or at least not any worse)
-- both from efficiency standpoint and in the event you have \0 in the middle of
the kernel dump.

http://codereview.chromium.org/3179006/diff/2001/3009
File kernel_collector_test.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3009#newcode15
kernel_collector_test.cc:15: int s_crashes = 0;
make these static?

http://codereview.chromium.org/3179006/diff/2001/3009#newcode38
kernel_collector_test.cc:38: mkdir("test/crash_directory", 0777);
make this a global constant?

http://codereview.chromium.org/3179006/diff/2001/3010
File user_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3010#newcode15
user_collector.cc:15: #include "crash-reporter/user_collector.h"
move to top?

[kmixter1, 2010-08-18 03:46:29]

PTAL. I split out an UncleanShutdownCollector to help improve testability. 
Otherwise, responded to all comments.

http://codereview.chromium.org/3179006/diff/2001/3001
File Makefile (right):

http://codereview.chromium.org/3179006/diff/2001/3001#newcode31
Makefile:31: crash_collector_test: crash_collector_test.o $(TEST_OBJS)
On 2010/08/17 18:13:47, petkov wrote:
> You could probably combine the three test targets under a common rule.
Something
> like (not sure about the exact syntax):
> 
> %_test: %_test.o $(TEST_OBJS)
> 

Good guess on the syntax. Done.

http://codereview.chromium.org/3179006/diff/2001/3002
File crash_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3002#newcode11
crash_collector.cc:11: #include "crash-reporter/crash_collector.h"
On 2010/08/17 18:13:47, petkov wrote:
> this should be the top include
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3003
File crash_collector.h (right):

http://codereview.chromium.org/3179006/diff/2001/3003#newcode37
crash_collector.h:37: // For testing, set the directory always returned by
On 2010/08/17 18:13:47, petkov wrote:
> s/set/replace/ ?
> 

Hm, set makes more sense to me.

http://codereview.chromium.org/3179006/diff/2001/3004
File crash_collector_test.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3004#newcode9
crash_collector_test.cc:9: #include "crash-reporter/crash_collector.h"
On 2010/08/17 18:13:47, petkov wrote:
> sort?
> 
> actually, this should be the top include.
> 

Put in abc order, but I think the top include would only belong to
crash_collector_test.h if it existed.  crash_collector.h is already the top
include in crash_collector.cc which is enough.

http://codereview.chromium.org/3179006/diff/2001/3005
File crash_reporter.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3005#newcode11
crash_reporter.cc:11: #include "crash-reporter/kernel_collector.h"
On 2010/08/17 18:13:47, petkov wrote:
> shouldn't these be abc sorted?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3005#newcode27
crash_reporter.cc:27: static const char kKernelCrashSignal[] =
On 2010/08/17 18:13:47, petkov wrote:
> Do you need this constant?
> 
No - thought I did half way through and never removed.

http://codereview.chromium.org/3179006/diff/2001/3005#newcode70
crash_reporter.cc:70: return file_util::WriteFile(file_path, kEmpty, 0) == 0;
On 2010/08/17 18:13:47, petkov wrote:
> use TouchFile?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3005#newcode78
crash_reporter.cc:78: static void TouchFile(const FilePath &file_path) {
On 2010/08/17 18:13:47, petkov wrote:
> return bool success/failure?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3005#newcode138
crash_reporter.cc:138: if (!kernel_crash) {
On 2010/08/17 18:13:47, petkov wrote:
> shouldn't you guard touching the file under IsMetricsCollectionAllowed for
> consistency?

As off-line, assuming metrics daemon will properly filter any stats if opted
out.

http://codereview.chromium.org/3179006/diff/2001/3005#newcode140
crash_reporter.cc:140: // an associated
On 2010/08/17 18:13:47, petkov wrote:
> finish the comment.
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3007
File kernel_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3007#newcode8
kernel_collector.cc:8: #include "crash-reporter/kernel_collector.h"
On 2010/08/17 18:13:47, petkov wrote:
> move to top
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3007#newcode9
kernel_collector.cc:9: #include "metrics/metrics_library.h"
On 2010/08/17 18:13:47, petkov wrote:
> not needed?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3007#newcode62
kernel_collector.cc:62: //int x = system("echo '' >
/sys/kernel/debug/preserved/kcrash");  x=x;
On 2010/08/17 18:13:47, petkov wrote:
> remove?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3007#newcode91
kernel_collector.cc:91: 
On 2010/08/17 18:13:47, petkov wrote:
> Do you need to call count_crash_function here?

Thanks!  I missed a few tests in kernel_collector.  I've added them in now
(making sure this gets called, making sure the opt-in function is respected).

http://codereview.chromium.org/3179006/diff/2001/3007#newcode95
kernel_collector.cc:95: kernel_dump.c_str(),
On 2010/08/17 18:13:47, petkov wrote:
> using .data() may, theoretically, be slightly better (or at least not any
worse)
> -- both from efficiency standpoint and in the event you have \0 in the middle
of
> the kernel dump.

I think .c_str() doesn't terminate at \0, but just tacks on an extra \0.  But
.data() seems reasonable, so I'll use it.

http://codereview.chromium.org/3179006/diff/2001/3009
File kernel_collector_test.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3009#newcode15
kernel_collector_test.cc:15: int s_crashes = 0;
On 2010/08/17 18:13:47, petkov wrote:
> make these static?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3009#newcode38
kernel_collector_test.cc:38: mkdir("test/crash_directory", 0777);
On 2010/08/17 18:13:47, petkov wrote:
> make this a global constant?
> 

Done.

http://codereview.chromium.org/3179006/diff/2001/3010
File user_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3010#newcode15
user_collector.cc:15: #include "crash-reporter/user_collector.h"
On 2010/08/17 18:13:47, petkov wrote:
> move to top?
> 

Done.

[petkov, 2010-08-18 17:37:51]

Some minor comments/suggestions.

LGTM

http://codereview.chromium.org/3179006/diff/2001/3007
File kernel_collector.cc (right):

http://codereview.chromium.org/3179006/diff/2001/3007#newcode95
kernel_collector.cc:95: kernel_dump.c_str(),
On 2010/08/18 03:46:30, kmixter1 wrote:
> On 2010/08/17 18:13:47, petkov wrote:
> > using .data() may, theoretically, be slightly better (or at least not any
> worse)
> > -- both from efficiency standpoint and in the event you have \0 in the
middle
> of
> > the kernel dump.
> 
> I think .c_str() doesn't terminate at \0, but just tacks on an extra \0.  But
> .data() seems reasonable, so I'll use it. 
> 
> 

You're right, \0 in the middle would not be an issue in this case.

http://codereview.chromium.org/3179006/diff/13001/14004
File crash_collector_test.cc (right):

http://codereview.chromium.org/3179006/diff/13001/14004#newcode14
crash_collector_test.cc:14: }
How about ASSERT_TRUE(false)? Or something like that.

http://codereview.chromium.org/3179006/diff/13001/14004#newcode17
crash_collector_test.cc:17: return false;
ASSERT_TRUE(false)?

http://codereview.chromium.org/3179006/diff/13001/14004#newcode25
crash_collector_test.cc:25: }
Assert that the crash_count_function and feedback_allowed function pointers are
set correctly?

http://codereview.chromium.org/3179006/diff/13001/14005
File crash_reporter.cc (right):

http://codereview.chromium.org/3179006/diff/13001/14005#newcode27
crash_reporter.cc:27: static const char kEmpty[] = "";
This has only a single use now. I suspect you may as well use "" directly in
TouchFile.

http://codereview.chromium.org/3179006/diff/13001/14005#newcode37
crash_reporter.cc:37: kCrashKindUser   = 2,
remove extra spaces before =

http://codereview.chromium.org/3179006/diff/13001/14005#newcode153
crash_reporter.cc:153: user_collector.Initialize(CountUserCrash,
Can you add some comments on why user and clean shutdown collectors are always
constructed and initialized while kernel is done only under an option? I think
it's clear from reading the code but some text would help.

http://codereview.chromium.org/3179006/diff/13001/14008
File kernel_collector.h (right):

http://codereview.chromium.org/3179006/diff/13001/14008#newcode38
kernel_collector.h:38: static const char kClearingSequence[];
Not sure but I think this should be after the friend typedefs.

[kmixter1, 2010-08-18 22:37:01]

Pushed

http://codereview.chromium.org/3179006/diff/13001/14004
File crash_collector_test.cc (right):

http://codereview.chromium.org/3179006/diff/13001/14004#newcode14
crash_collector_test.cc:14: }
On 2010/08/18 17:37:51, petkov wrote:
> How about ASSERT_TRUE(false)? Or something like that.
> 

Done.

http://codereview.chromium.org/3179006/diff/13001/14004#newcode17
crash_collector_test.cc:17: return false;
On 2010/08/18 17:37:51, petkov wrote:
> ASSERT_TRUE(false)?
> 

Done.

http://codereview.chromium.org/3179006/diff/13001/14004#newcode25
crash_collector_test.cc:25: }
On 2010/08/18 17:37:51, petkov wrote:
> Assert that the crash_count_function and feedback_allowed function pointers
are
> set correctly?
> 

Done.

http://codereview.chromium.org/3179006/diff/13001/14005
File crash_reporter.cc (right):

http://codereview.chromium.org/3179006/diff/13001/14005#newcode27
crash_reporter.cc:27: static const char kEmpty[] = "";
On 2010/08/18 17:37:51, petkov wrote:
> This has only a single use now. I suspect you may as well use "" directly in
> TouchFile.
> 

Done.

http://codereview.chromium.org/3179006/diff/13001/14005#newcode37
crash_reporter.cc:37: kCrashKindUser   = 2,
On 2010/08/18 17:37:51, petkov wrote:
> remove extra spaces before =
> 

Done.

http://codereview.chromium.org/3179006/diff/13001/14005#newcode153
crash_reporter.cc:153: user_collector.Initialize(CountUserCrash,
On 2010/08/18 17:37:51, petkov wrote:
> Can you add some comments on why user and clean shutdown collectors are always
> constructed and initialized while kernel is done only under an option? I think
> it's clear from reading the code but some text would help.

It was convenience over uniformity - so I switched it - now all three are
constructed at startup.

http://codereview.chromium.org/3179006/diff/13001/14008
File kernel_collector.h (right):

http://codereview.chromium.org/3179006/diff/13001/14008#newcode38
kernel_collector.h:38: static const char kClearingSequence[];
On 2010/08/18 17:37:51, petkov wrote:
> Not sure but I think this should be after the friend typedefs.
> 

Done.