Add MD_OS_ANDROID definition.

Add MD_OS_ANDROID definition.

In order to better distinguish Android and Linux minidumps, introduce
a new MD_OS_ANDROID definition, and modify related source code accordingly.

Also append the build-fingerprint to the minidump location descriptor.
This gives more information about the system image the device runs on.
Committed: https://code.google.com/p/google-breakpad/source/detail?r=981

[digit]

For the record, I only tested that:

1/ This doesn't break the Android client library build
2/ This doesn't break the Linux x86/64 builds and unit tests.

I still need to check that it doesn't break our internal unit tests. However,
feel free to comment on the current patch.

Thanks.

[Mark Mentovai]

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1274: #define SEPARATOR " "
Huh?

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1276: size_t separator_len =
sizeof(SEPARATOR)-1;
Huh? The compiler optimizes strlen for const char[].

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1295: // On Android, try to
get the build fingerprint as well from system
Put this inside the #ifdef.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1299: // Get the
ro.build.fingerprint system property. Note that we reserve
Don’t say “we” in comments. Again on line 1309. And the same with “let's” on
line 1306.

Who are “we?” The code? The compiler? The team? Which team? It detracts from the
point of the comment. Just saying “Reserve one initial byte to store a space
separator” is direct, clear, and unambiguous.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1301: char  fingerprint[1 +
PROP_VALUE_MAX];
Don’t align variable names like this. We never do this.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1301: char  fingerprint[1 +
PROP_VALUE_MAX];
PROP_VALUE_MAX + 1 is more conventional.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1305: fingerprint+1);
Spaces around operators like +. Same with fingerprint_len on lines 1310 and
1312.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1306: // system property
values are always zero-terminated, but let's
This is a sentence, so begin it with a capital letter.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1309: // We have a build
fingerprint, append it to our buffer.
Fix the indentation here and on line 1131.

https://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/m...
src/client/linux/minidump_writer/minidump_writer.cc:1311: strcat(buf,
fingerprint);
If you were being paranoid, you’d use strlcat, or strncat plus an explicit '\0'
termination, since you probably don’t have strlcat.

[digit]

http://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/mi...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1276: size_t separator_len =
sizeof(SEPARATOR)-1;
On 2012/06/28 14:31:45, Mark Mentovai wrote:
> Huh? The compiler optimizes strlen for const char[].

Really. I didn't know that, will remove this.

http://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1295: // On Android, try to
get the build fingerprint as well from system
On 2012/06/28 14:31:45, Mark Mentovai wrote:
> Put this inside the #ifdef.
will do

http://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1299: // Get the
ro.build.fingerprint system property. Note that we reserve
On 2012/06/28 14:31:45, Mark Mentovai wrote:
> Don’t say “we” in comments. Again on line 1309. And the same with
“let's” on
> line 1306.
> 
> Who are “we?” The code? The compiler? The team? Which team? It detracts
from the
> point of the comment. Just saying “Reserve one initial byte to store a space
> separator” is direct, clear, and unambiguous.

Point taken. Will do.

http://breakpad.appspot.com/405002/diff/1/src/client/linux/minidump_writer/mi...
src/client/linux/minidump_writer/minidump_writer.cc:1311: strcat(buf,
fingerprint);
On 2012/06/28 14:31:45, Mark Mentovai wrote:
> If you were being paranoid, you’d use strlcat, or strncat plus an explicit
'\0'
> termination, since you probably don’t have strlcat.
Good point. Android does have strlcat() so we can use it in this #ifdef block
(unlike the code above). Will do.

[digit]

All points fixed in path set 3. thanks.

[Mark Mentovai]

http://breakpad.appspot.com/405002/diff/8001/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/405002/diff/8001/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1310:
strlcat(buf,fingerprint,buf_len);
Spaces after the commas.

http://breakpad.appspot.com/405002/diff/8001/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1311: space_left -=
fingerprint_len;
I don’t think that space_left is useful after line 1280.

[digit]

http://breakpad.appspot.com/405002/diff/8001/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/405002/diff/8001/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1310:
strlcat(buf,fingerprint,buf_len);
On 2012/06/29 16:15:36, Mark Mentovai wrote:
> Spaces after the commas.

Will do.

http://breakpad.appspot.com/405002/diff/8001/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1311: space_left -=
fingerprint_len;
On 2012/06/29 16:15:36, Mark Mentovai wrote:
> I don’t think that space_left is useful after line 1280.
I'll remove this.

[Mark Mentovai]

http://breakpad.appspot.com/405002/diff/8003/src/client/linux/minidump_writer...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/405002/diff/8003/src/client/linux/minidump_writer...
src/client/linux/minidump_writer/minidump_writer.cc:1307: space_left -=
separator_len;
space_left is still in here?

[digit]

http://breakpad.appspot.com/405002/diff/8003/src/client/linux/minidump_writer...
> src/client/linux/minidump_writer/minidump_writer.cc:1307: space_left -=
> separator_len;
> space_left is still in here?

Fixed in patch set 5

[Mark Mentovai]

LGTM otherwise

http://breakpad.appspot.com/405002/diff/13001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/405002/diff/13001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1303: static const char*
separator = " ";
While the compiler may be smart enough to figure this out, the “static” doesn’t
actually contribute anything here. I’d get rid of it because it’s excess
baggage.

[digit]

http://breakpad.appspot.com/405002/diff/13001/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

http://breakpad.appspot.com/405002/diff/13001/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1303: static const char*
separator = " ";
I agree, I was just trying to follow the style of the code above. I've fixed
that in the latest patch. Thanks.

[Mark Mentovai]

LGTM

[Ted Mielczarek]

https://breakpad.appspot.com/405002/diff/1008/src/client/linux/minidump_write...
File src/client/linux/minidump_writer/minidump_writer.cc (right):

https://breakpad.appspot.com/405002/diff/1008/src/client/linux/minidump_write...
src/client/linux/minidump_writer/minidump_writer.cc:1307: }
Late to the party, but I was hoping we'd be able to get the Android OS version
and stick it in the minidump version field in place of the kernel version, since
that's almost never useful.