Enable dumping of the Linux extension streams.

Enable dumping of the Linux extension streams.

We now dump information about process's environment/command line/status, Linux
release, and CPU info.

[Mark Mentovai]

http://breakpad.appspot.com/238001/diff/1/2
File src/processor/minidump_dump.cc (right):

http://breakpad.appspot.com/238001/diff/1/2#newcode1
Line 1: // Copyright (c) 2006, Google Inc.
“Created a new changelist blah blah blah” doesn’t belong in your CL description.
That becomes the Subversion commit message, and none of that is really relevant
to the commit message.

http://breakpad.appspot.com/238001/diff/1/2#newcode55
Line 55: static bool LoadStreamContents(Minidump *minidump,
You can get rid of this now.

http://breakpad.appspot.com/238001/diff/1/2#newcode67
Line 67: std::vector<char> contents;
Don’t declare this until you need to use it, a few lines later.

You can actually use the vector::vector(size_t) constructor form at declaration
instead of having the vector::resize call.

http://breakpad.appspot.com/238001/diff/1/2#newcode68
Line 68: if (!minidump->SeekToStreamType(stream_type, &length) || length == 0) {
Maybe if length == 0, you actually do want to just print the "Stream %s\n" thing
but obviously not any data? Seems like we might wind up with a 0-length stream
at some point, and if we get to the point that SeekToStreamType succeeds, it’s
not really a missing stream, and we probably ought to print something here.

http://breakpad.appspot.com/238001/diff/1/2#newcode71
Line 71: contents.resize(length);
Blank line before, to break up the flow. (This line will probably change if you
take my previous suggestion, but it’ll still be good to break up the flow.)

http://breakpad.appspot.com/238001/diff/1/2#newcode73
Line 73: ++errors;
This doesn’t seem right. Don’t you mean ++*errors? Or this:

    if (errors)
      ++*errors;

You can probably get away with the simpler form since this is a static utility
method and none of the callers have a good reason to pass NULL for errors.

http://breakpad.appspot.com/238001/diff/1/2#newcode77
Line 77: printf("Stream %s:\n", stream_name);
Blank line before this one, too.

http://breakpad.appspot.com/238001/diff/1/2#newcode80
Line 80: int remaining = length - current_offset;
length and current_offset are both unsigned, and current_offset is guaranteed <
length. This doesn’t need to be a signed type, it should probably just be a
size_t.

http://breakpad.appspot.com/238001/diff/1/2#newcode81
Line 81: printf("%.*s", remaining, &contents[current_offset]);
Nice use of %.*s!

At some point we may want to enhance this to do friendly escaping of non-ASCII
data, but we can save that until it’s actually called for.

[kmixter]

PTAL

http://breakpad.appspot.com/238001/diff/1/2
File src/processor/minidump_dump.cc (right):

http://breakpad.appspot.com/238001/diff/1/2#newcode55
Line 55: static bool LoadStreamContents(Minidump *minidump,
On 2010/12/01 04:15:46, Mark Mentovai wrote:
> You can get rid of this now.

Doh - sorry about that.

http://breakpad.appspot.com/238001/diff/1/2#newcode67
Line 67: std::vector<char> contents;
On 2010/12/01 04:15:46, Mark Mentovai wrote:
> Don’t declare this until you need to use it, a few lines later.
> 
> You can actually use the vector::vector(size_t) constructor form at
declaration
> instead of having the vector::resize call.

Done.

http://breakpad.appspot.com/238001/diff/1/2#newcode68
Line 68: if (!minidump->SeekToStreamType(stream_type, &length) || length == 0) {
On 2010/12/01 04:15:46, Mark Mentovai wrote:
> Maybe if length == 0, you actually do want to just print the "Stream %s\n"
thing
> but obviously not any data? Seems like we might wind up with a 0-length stream
> at some point, and if we get to the point that SeekToStreamType succeeds, it’s
> not really a missing stream, and we probably ought to print something here.

Done.  Short-circuiting the rest of the function to not worry about creating
zero-length buffers, etc.

http://breakpad.appspot.com/238001/diff/1/2#newcode73
Line 73: ++errors;
On 2010/12/01 04:15:46, Mark Mentovai wrote:
> This doesn’t seem right. Don’t you mean ++*errors? Or this:
> 
>     if (errors)
>       ++*errors;
> 
> You can probably get away with the simpler form since this is a static utility
> method and none of the callers have a good reason to pass NULL for errors.

Done.

http://breakpad.appspot.com/238001/diff/1/2#newcode77
Line 77: printf("Stream %s:\n", stream_name);
On 2010/12/01 04:15:46, Mark Mentovai wrote:
> Blank line before this one, too.

Done.

http://breakpad.appspot.com/238001/diff/1/2#newcode80
Line 80: int remaining = length - current_offset;
On 2010/12/01 04:15:46, Mark Mentovai wrote:
> length and current_offset are both unsigned, and current_offset is guaranteed
<
> length. This doesn’t need to be a signed type, it should probably just be a
> size_t.

Done.

[Mark Mentovai]

LGTM