Start invoking core2md to implement full system crash reporting

Start invoking core2md to implement full system crash reporting

BUG=4741

[petkov, 2010-07-22 05:35:51]

One pass... One potential issue with scoped_ptr/scoped_array, mostly style nits
otherwise.

http://codereview.chromium.org/3040013/diff/1/3
File crash_reporter.cc (right):

http://codereview.chromium.org/3040013/diff/1/3#newcode112
crash_reporter.cc:112: true);
Maybe add  // generate_diagnostics at the end to document "true".

http://codereview.chromium.org/3040013/diff/1/5
File user_collector.cc (right):

http://codereview.chromium.org/3040013/diff/1/5#newcode14
user_collector.cc:14: #include "base/time.h"
Unused?

http://codereview.chromium.org/3040013/diff/1/5#newcode96
user_collector.cc:96: #include <errno.h>
Do you need this include? If so, move to the top?

http://codereview.chromium.org/3040013/diff/1/5#newcode100
user_collector.cc:100: scoped_ptr<char> buffer;
You have to use scoped_array -- this will use delete rather than delete[] for
deallocation. I'm not sure why this works for you.

http://codereview.chromium.org/3040013/diff/1/5#newcode101
user_collector.cc:101: while(true) {
space after while

http://codereview.chromium.org/3040013/diff/1/5#newcode107
user_collector.cc:107: buffer.get()[size] = 0;
Remove .get() once you switch to scoped_array.

http://codereview.chromium.org/3040013/diff/1/5#newcode109
user_collector.cc:109: if (max_size * 2 > max_size) {
Is this an overflow check? Add comment.

http://codereview.chromium.org/3040013/diff/1/5#newcode133
user_collector.cc:133: UserCollector::IdKind kind,
No need for UserCollector:: ?

http://codereview.chromium.org/3040013/diff/1/5#newcode142
user_collector.cc:142: int ids[4];
Maybe add a kIdKindCount as the last enum value and use it here? Up to you...

http://codereview.chromium.org/3040013/diff/1/5#newcode147
user_collector.cc:147: for (unsigned i = 0; i < sizeof(ids)/sizeof(ids[0]); ++i)
{
Use either kIdKindCount or arraysize(ids) from base/basictypes.h.

http://codereview.chromium.org/3040013/diff/1/5#newcode148
user_collector.cc:148: if (status_contents.at(found) != '\t')
Are you intentionally not using SplitString from string_util.h here?

http://codereview.chromium.org/3040013/diff/1/5#newcode196
user_collector.cc:196: for (unsigned i = 0; i <
sizeof(proc_files)/sizeof(proc_files[0]); ++i) {
arraysize

http://codereview.chromium.org/3040013/diff/1/5#newcode206
user_collector.cc:206: FilePath UserCollector::GetCrashFilePathInfo(
It's a bit confusing here and before if CrashFilePath refers to a path to a file
or a directory. It seems it's a directory. Does it make sense to remove "File"
and "file" from a few of the method and variable names or replace with "dump" or
something?

http://codereview.chromium.org/3040013/diff/1/5#newcode277
user_collector.cc:277: bool UserCollector::GenerateDiagnostics(pid_t pid,
Can you either add a summary, or split this method into separate parts (i.e.,
copy process files, store core, core2md)?

http://codereview.chromium.org/3040013/diff/1/6
File user_collector.h (right):

http://codereview.chromium.org/3040013/diff/1/6#newcode14
user_collector.h:14: class Time;
Unused?

http://codereview.chromium.org/3040013/diff/1/6#newcode31
user_collector.h:31: // called.
Document generate_diagnostics.

http://codereview.chromium.org/3040013/diff/1/6#newcode46
user_collector.h:46: // Handle a specific user crash.
Returns true on success?

http://codereview.chromium.org/3040013/diff/1/6#newcode47
user_collector.h:47: bool HandleCrash(int signal, int pid, const char
*force_exec = NULL);
Default arguments are not allowed.

http://codereview.chromium.org/3040013/diff/1/6#newcode69
user_collector.h:69: ID_REAL = 0,  // uid and gid
Style prefers kEnumValueName.

http://codereview.chromium.org/3040013/diff/1/6#newcode84
user_collector.h:84: UserCollector::IdKind kind,
You don't need UserCollector:: here, do you?

http://codereview.chromium.org/3040013/diff/1/7
File user_collector_test.cc (right):

http://codereview.chromium.org/3040013/diff/1/7#newcode8
user_collector_test.cc:8: #include "base/time.h"
Unused?

http://codereview.chromium.org/3040013/diff/1/7#newcode127
user_collector_test.cc:127: ASSERT_STREQ(this_link.c_str(),
result.value().c_str());
does it work to ASSERT_EQ the actual strings (no c_str())?

http://codereview.chromium.org/3040013/diff/1/7#newcode279
user_collector_test.cc:279: base::Time timestamp;
unused?

[kmixter1, 2010-07-22 23:12:55]

PTAL - addressed all comments.

http://codereview.chromium.org/3040013/diff/1/3
File crash_reporter.cc (right):

http://codereview.chromium.org/3040013/diff/1/3#newcode112
crash_reporter.cc:112: true);
On 2010/07/22 05:35:51, petkov wrote:
> Maybe add  // generate_diagnostics at the end to document "true".
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5
File user_collector.cc (right):

http://codereview.chromium.org/3040013/diff/1/5#newcode14
user_collector.cc:14: #include "base/time.h"
On 2010/07/22 05:35:51, petkov wrote:
> Unused?
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode96
user_collector.cc:96: #include <errno.h>
On 2010/07/22 05:35:51, petkov wrote:
> Do you need this include? If so, move to the top?

No, just for debugging. Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode100
user_collector.cc:100: scoped_ptr<char> buffer;
On 2010/07/22 05:35:51, petkov wrote:
> You have to use scoped_array -- this will use delete rather than delete[] for
> deallocation. I'm not sure why this works for you.

Pretty amazing that it didn't crash during unit tests.  Thanks for catching
this.

http://codereview.chromium.org/3040013/diff/1/5#newcode101
user_collector.cc:101: while(true) {
On 2010/07/22 05:35:51, petkov wrote:
> space after while

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode107
user_collector.cc:107: buffer.get()[size] = 0;
On 2010/07/22 05:35:51, petkov wrote:
> Remove .get() once you switch to scoped_array.
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode109
user_collector.cc:109: if (max_size * 2 > max_size) {
On 2010/07/22 05:35:51, petkov wrote:
> Is this an overflow check? Add comment.
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode133
user_collector.cc:133: UserCollector::IdKind kind,
On 2010/07/22 05:35:51, petkov wrote:
> No need for UserCollector:: ?
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode142
user_collector.cc:142: int ids[4];
On 2010/07/22 05:35:51, petkov wrote:
> Maybe add a kIdKindCount as the last enum value and use it here? Up to you...
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode147
user_collector.cc:147: for (unsigned i = 0; i < sizeof(ids)/sizeof(ids[0]); ++i)
{
On 2010/07/22 05:35:51, petkov wrote:
> Use either kIdKindCount or arraysize(ids) from base/basictypes.h.
> 

Done.

http://codereview.chromium.org/3040013/diff/1/5#newcode148
user_collector.cc:148: if (status_contents.at(found) != '\t')
On 2010/07/22 05:35:51, petkov wrote:
> Are you intentionally not using SplitString from string_util.h here?
> 

No - I looked for something like that but missed it. Rewritten to be slower but
prettier. :)

http://codereview.chromium.org/3040013/diff/1/5#newcode196
user_collector.cc:196: for (unsigned i = 0; i <
sizeof(proc_files)/sizeof(proc_files[0]); ++i) {
On 2010/07/22 05:35:51, petkov wrote:
> arraysize
> 

Don't know where that's coming from, but seems to work.

http://codereview.chromium.org/3040013/diff/1/5#newcode206
user_collector.cc:206: FilePath UserCollector::GetCrashFilePathInfo(
On 2010/07/22 05:35:51, petkov wrote:
> It's a bit confusing here and before if CrashFilePath refers to a path to a
file
> or a directory. It seems it's a directory. Does it make sense to remove "File"
> and "file" from a few of the method and variable names or replace with "dump"
or
> something?

Agreed - FilePath was referring to the named type, but parsing these function
names is confusing.  Changed FilePath to Directory.

http://codereview.chromium.org/3040013/diff/1/5#newcode277
user_collector.cc:277: bool UserCollector::GenerateDiagnostics(pid_t pid,
On 2010/07/22 05:35:51, petkov wrote:
> Can you either add a summary, or split this method into separate parts (i.e.,
> copy process files, store core, core2md)?

Refactored.

http://codereview.chromium.org/3040013/diff/1/6
File user_collector.h (right):

http://codereview.chromium.org/3040013/diff/1/6#newcode14
user_collector.h:14: class Time;
On 2010/07/22 05:35:51, petkov wrote:
> Unused?
> 

Done.

http://codereview.chromium.org/3040013/diff/1/6#newcode31
user_collector.h:31: // called.
On 2010/07/22 05:35:51, petkov wrote:
> Document generate_diagnostics.
> 

Done.

http://codereview.chromium.org/3040013/diff/1/6#newcode46
user_collector.h:46: // Handle a specific user crash.
On 2010/07/22 05:35:51, petkov wrote:
> Returns true on success?
> 

Done.

http://codereview.chromium.org/3040013/diff/1/6#newcode47
user_collector.h:47: bool HandleCrash(int signal, int pid, const char
*force_exec = NULL);
On 2010/07/22 05:35:51, petkov wrote:
> Default arguments are not allowed.
> 

Done.

http://codereview.chromium.org/3040013/diff/1/6#newcode69
user_collector.h:69: ID_REAL = 0,  // uid and gid
On 2010/07/22 05:35:51, petkov wrote:
> Style prefers kEnumValueName.
> 

The style guide has changed out from underneath me.  Guess I'll go with the new
style.

http://codereview.chromium.org/3040013/diff/1/6#newcode84
user_collector.h:84: UserCollector::IdKind kind,
On 2010/07/22 05:35:51, petkov wrote:
> You don't need UserCollector:: here, do you?
> 

Done.

http://codereview.chromium.org/3040013/diff/1/7
File user_collector_test.cc (right):

http://codereview.chromium.org/3040013/diff/1/7#newcode8
user_collector_test.cc:8: #include "base/time.h"
On 2010/07/22 05:35:51, petkov wrote:
> Unused?
> 

Done.

http://codereview.chromium.org/3040013/diff/1/7#newcode127
user_collector_test.cc:127: ASSERT_STREQ(this_link.c_str(),
result.value().c_str());
On 2010/07/22 05:35:51, petkov wrote:
> does it work to ASSERT_EQ the actual strings (no c_str())?
> 

It does, thanks.

http://codereview.chromium.org/3040013/diff/1/7#newcode279
user_collector_test.cc:279: base::Time timestamp;
On 2010/07/22 05:35:51, petkov wrote:
> unused?
> 

Done.

[petkov, 2010-07-22 23:57:33]

LGTM

http://codereview.chromium.org/3040013/diff/4002/10008
File user_collector_test.cc (right):

http://codereview.chromium.org/3040013/diff/4002/10008#newcode105
user_collector_test.cc:105: ASSERT_STREQ("/proc/100", path.value().c_str());
I think that ASSERT_EQ would work in this case too (no c_str()).